Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rosetta): non-compiling snippets not reported on subsequent extracts #3260

Merged
merged 1 commit into from
Dec 16, 2021

Conversation

kaizencc
Copy link
Contributor

Picture this scenario: there is a non-compiling snippet in a README.md file.
I run yarn rosetta:extract --compile and it returns diagnostics as expected.
Behind the scenes, .jsii.tabl.json gets created as a cache, and this snippet
is inserted into that file with didCompile = false.

Without making any changes, I run yarn rosetta:extract --compile again.
This time, no diagnostics are returned, and it looks like my errors have
magically fixed themselves. However what is really happening is that extract
is finding a snippet in the cache that matches the offending snippet (since
I changed nothing). It is then filtering out that cached snippet, meaning we
do not actually try to compile the snippet again. This is bad; extract should
honor the --compile flag and return errors the second time around too.

There are two ways to solve this (that I can think of): we can return
diagnostics for cached snippets as well, or we can ignore non-compiling
cached snippets when --compile is set. I have opted for the second solution
for this reason: it is possible that I am intending to rerun the same snippet
with the expectation that I have changed something else that will result
in a successful compilation (for example, I add an import to the fixture).
Open to dialogue about this.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@kaizencc kaizencc requested a review from rix0rrr December 15, 2021 00:55
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Dec 15, 2021
@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 15, 2021

Good catch!

for example, I add an import to the fixture

Well yes, but if you added an import to the fixture, that means the sample has changed and it would in fact get recompiled (try it).

You're definitely right that this is unexpected. In what cases do you suppose this is a very bad error? If you didn't treat the exit code of the first extract as a problem, but you DO treat the exit code of the second extract as a problem?

Not opposed to the solution... just trying to reason through the implications...

@kaizencc
Copy link
Contributor Author

for example, I add an import to the fixture

Well yes, but if you added an import to the fixture, that means the sample has changed and it would in fact get recompiled (try it).

Welp, i was a bit iffy about whether this was true, should've tested it out first :).

The case I run into is when I am trying to get the README to compile, there are many errors at first. Say I work on a few of them and then run yarn rosetta:extract --compile again. I'm still expecting errors from the examples I didn't touch, but I just want to verify the few that I fixed are really fixed. However in this case the existing errors disappear, which is unexpected.

My solution is trying to take into account the experience of running rosetta extract without --compile. In this case we should want to match as much as possible from the cache regardless of whether they did compile. That makes me think we shouldn't be filtering when we first extract, since we don't know whether the second extract will come with a --compile flag or not. Does that make sense?

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 15, 2021

Say I work on a few of them and then run yarn rosetta:extract --compile again.

Ah, iterative workflow! Yes of course, you are completely correct. It doesn't make sense to NOT display the same errors again. For some reason I was only thinking of the release build workflow.

I 100% agree with you.

@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Dec 15, 2021
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional approval, I think we can do without the flag but otherwise I agree with the change.

@@ -83,14 +83,15 @@ export class RosettaTranslator {
*
* Will remove the cached snippets from the input array.
*/
public readFromCache(snippets: TypeScriptSnippet[], addToTablet = true): ReadFromCacheResults {
public readFromCache(snippets: TypeScriptSnippet[], addToTablet = true, compiledOnly = false): ReadFromCacheResults {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine making this the default. Let's not add switches we're not going to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think the flag is necessary here. We pass in options.includeCompilerDiagnostics into compiledOnly. Without it, rosetta:extract --no-compile will not take advantage of the cache. For example:

run rosetta:extract --no-compile. examples added to tablet and cache.
run rosetta:extract --no-compile again. we have cached examples, but they are not used because didCompile is undefined. So we get cache misses for all snippets.

Now I don't know why one would do this. But as long as we support --no-compile it feels iffy to say "btw, you won't get to use the cache anymore, haha".

Unless you simply mean you want to default to compiledOnly = true?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holy shit it breaks my brain 😅.

I think you are right though, running through some cases and the alternatives feel suboptimal. Shipping this!

@rix0rrr rix0rrr changed the title fix(rosetta): strict extract uses non-compiling cached snippets fix(rosetta): non-compiling snippets not reported on subsequent extracts Dec 15, 2021
@rix0rrr rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Dec 16, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2021

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Dec 16, 2021
@mergify mergify bot merged commit 771190b into main Dec 16, 2021
@mergify mergify bot deleted the conroy/extract-bug branch December 16, 2021 10:44
@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2021

Merging (with squash)...

@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants