fix: #322

[aleclarson]

This is a work-in-progress. Currently, it spits out an error.

Tried to finish processing tokens before reaching the end.

Fixes #322

[aleclarson]

I'll need some more context on how Sucrase works to move this further.

[alangpierce]

Thanks! Sigh, this is getting into some details...

If you take a look at RootTransformer.processBalancedCode, it's tracking all parentheses and braces so that it can stop when it sees a ) or } that it hasn't seen. For example, this is useful when walking classes and functions so we can stop at the correct } and know that we're at the end of the class/function. But that assumes that the this.processToken() call will never consume an unbalanced number of parens, and I guess your change is the first one to do that.

Two high-level options that I can think of:

• Change the processToken contract to return how many parens and braces of each type it consumed, so that processBalancedCode can update accordingly.
• Rather than directly consuming the close-parens and replacing the open-paren, your code could just "leave a note" in class state and then replace the open-paren in the later call to CJSImportTransformer.process.
• Rework the way of doing brace and paren depth. If it was kept in more of a shared place, it could maybe be updated by rootTransformer.processToken and others.
• Get rid of brace and paren accounting in the first place. It might be needed with some smarter info from the parser.

Option 2 seems like maybe the best one here. I'll also keep thinking if there are other options.

Oh, and this might be an argument for going back to the _id strategy, since that one is more self-contained syntactically.

[aleclarson]

your code could just "leave a note" in class state and then replace the open-paren in the later call to CJSImportTransformer.process.

Are there two calls to each transformer's process method every sucrase.transform call? Or how should I understand this?

[aleclarson]

Option 2 seems like maybe the best one here. I'll also keep thinking if there are other options.

Oh, and this might be an argument for going back to the _id strategy, since that one is more self-contained syntactically.

I think option 2 is the best... of those options. This is a good argument for conditionally replacing based on existence of closing parens, as I mentioned earlier.

[alangpierce]

Are there two calls to each transformer's process method every sucrase.transform call? Or how should I understand this?

Generally speaking, Transformer.process is how a transformer processes a single token, though it can decide to process multiple at once if it wants.

The implementation is at RootTransformer.processToken, which gets called over and over. Each time, it takes a look at the next token, asks every transformer to process the token if possible, and exits early if a transformer is able to process it. If no transformer is able to process the token, then it just copies the token, which should be the common case. It's a bit delicate because the transformer order defines precedence, so order matters.

What I'm saying is that CJSImportTransformer could keep state like shouldChangeNextOpenParenToCall or something, then later, RootTransformer.processToken will (hopefully) call CJSImportTransformer.process for the open-paren token, and CJSImportTransformer can do the ( -> .call(void 0, replacement then. That would allow processBalancedCode to do proper bookkeeping of parenDepth, since the first CJSImportTransformer.process call (processing an identifier) doesn't consume any open or close parens.

[alangpierce]

This is a good argument for conditionally replacing based on existence of closing parens, as I mentioned earlier.

I'm not totally sure, but my impression is that with the shouldChangeNextOpenParenToCall, it would still be cleaner and correct to just use the ( -> .call(void 0, strategy all the time, rather than having two strategies that we pick between. But happy to go either way.

[aleclarson]

Take 2 is ready. Apart from src/transformers/CJSImportTransformer.ts, all I had to do was add two new arguments to processBalancedCode so I can inject the initial state and recursively process the set of arguments associated with the imported function call.

[aleclarson]

I also have it default to (0, f)() if the previous token is an open brace, semicolon, or equal sign. There are probably other valid tokens, so maybe that check can be broken off into its own function.

[alangpierce]

Some comments. As you may have noticed, Sucrase can get pretty complex 😄. I want to be really cautious about adding more complexity, and I think some of your code adds complexity to make the output code a little cleaner, which I'd rather avoid unless there's a strong justification for it.

Also, looks like tests are failing because this breaks when used with constructors (new f()becoming new _myModule.f.call(), which isn't valid). Turns out that's an example of the case I was previously worrying about where the ( isn't a plain function call.

I'm still thinking the _id approach may be nicer since it has much less need to interpret the tokens around you. Worth considering.

I also realized that this code already doesn't handle TypeScript type arguments: https://sucrase.io/#code=%0Aimport%20f%20from%20'f'%3B%0A%0Af%3Cnumber%3E() . It would be great if this change either handled that or was built in a way that didn't make it harder to fix that in the future.

[aleclarson]

Take 3. Cleaner than ever. LGTM?

[alangpierce]

Cool, getting close, but looks like this still breaks the build because new expressions get transformed wrong. I think it's safe to just do the (0, f) strategy for those, and you can just look backward to see if there's a previous new token to detect that.

[alangpierce]

=== please. (This breaks lint, and you should be able to do yarn test locally to catch it.)

Also, maybe you could do this.tokens.currentIndex() + 1 === possibleOpenParenIndex?

I think you should be able to handle the new issues by just making sure the prior token is not new in this conditional.

[aleclarson]

Sorry, I live on the edge. Will fix it. (regarding yarn test: I've been using jest -i with a jest.config.js I setup, because I can't stand mocha)

Also, maybe you could do this.tokens.currentIndex() + 1 === possibleOpenParenIndex?

Nice! At this point, we could just merge it and you fix the nit picks. :P

[aleclarson]

Test case for new expression added.

Now using (0, f) for new expressions.

[alangpierce]

Looks like new parsing is busted 😳 . From https://sucrase.io/#compareWithBabel=false&showTokens=true&code=const%20a%20%3D%20new%20B()%3B , the new token is listed as an identifier token with name "new" when actually it should be a _new token.

Looks like this change to src/parser/traverser/expression.ts is a quick fix that doesn't break tests, and if you do that, it should hopefully fix your stuff.

 function parseNew(): void {
-  parseIdentifier();
+  expect(tt._new);
   if (eat(tt.dot)) {

[aleclarson]

Consider it done

[codecov-io]

Codecov Report

Merging #337 into master will increase coverage by 0.17%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
+ Coverage   77.47%   77.64%   +0.17%     
==========================================
  Files          41       42       +1     
  Lines        5305     5351      +46     
  Branches     1289     1304      +15     
==========================================
+ Hits         4110     4155      +45     
- Misses        827      829       +2     
+ Partials      368      367       -1

Impacted Files	Coverage Δ
src/parser/traverser/expression.ts	78.92% <100%> (ø)	⬆️
src/transformers/CJSImportTransformer.ts	92.05% <100%> (+0.49%)	⬆️
src/index.ts	82.85% <0%> (-2%)	⬇️
src/util/getNonTypeIdentifiers.ts	75% <0%> (ø)	⬆️
src/util/getTSImportedNames.ts	76.31% <0%> (ø)
src/parser/tokenizer/readWord.ts	72.24% <0%> (+0.42%)	⬆️
src/transformers/ESMImportTransformer.ts	75% <0%> (+6%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41476b0...049f9a1. Read the comment docs.

[alangpierce]

Cool, LGTM, thanks!

[alangpierce]

Technically this doesn't exercise your case, since it's surrounded by parens here, but no big deal.

[alangpierce]

@aleclarson published in 3.7.1! Thanks for working through all of the details!

[aleclarson]

Cheers!