for loops over implicit function calls crash #269

alangpierce · 2016-07-01T06:00:17Z

Here's a reduced case:

for a in b c
  d()

http://decaffeinate.github.io/decaffeinate/repl/#?evaluate=true&code=for%20a%20in%20b%20c%0A%20%20d()

It gets an "unexpected token" error due to generating invalid JS code. With some improved error handling set up, I can get the incorrect JS output:

an intermediate stage failed to parse: Unexpected token (2:39)
  1 | iterable = b(c)
> 2 | for (i = 0; i < iterable.length; i++) {)
    |                                        ^
  3 |   a = iterable[i];
  4 |   d();

So it looks like it's putting a close-paren at the end of the for loop line, and it's causing a syntax error.

The text was updated successfully, but these errors were encountered:

This is originally based on decaffeinate#222 , but I ended up changing the approach pretty significantly. Since the `PatchError` class already contains logic to pretty-print an error in a particular place in a file, with necessary context, I decided to reuse that. Unfortunately, it relied on having a `ParseContext` from `decaffeinate-parser`, even though none of the code is CoffeeScript-specific. The main purpose of the `ParseContext` over just the source code is for the `lineMap`, so for now I refactored `PatchError` to just take the source code and create its own `lineMap` by copying the function from `decaffeinate-parser`. But maybe a better solution is to just break the abstraction a little and expose the `lineColumnMapper` function from `decaffeinate-parser`? Or maybe there's some other clever way to reuse this code that I haven't thought of. Since it's just 20 lines, it seems like it's not worth worrying about that much, I think. This change has been really useful for me to track down the underlying cause of errors in files I'm trying to decaffeinate, and makes it much easier to change files to be decaffeinate-friendly. See decaffeinate#269, decaffeinate#270, and decaffeinate#271 for examples of the output produced after this change.

alangpierce · 2016-07-09T22:26:42Z

Ok, I've tracked down the problem and I think I have a good fix for it. Here's what's going wrong:

First, the function call b c is patched to become b(c), the close-paren is inserted using magic-string's insertLeft, which seems correct to me (although the decaffeinate code path is FunctionApplicationPatcher.patchAsExpression calling NodePatcher.insert calling NodePatcher.insertRight calling MagicString.insertLeft, so probably that could be cleaned up).
The code for the function is grabbed using magic-string's slice method. This respects the insertLeft so the close-paren is included when defining the iterable variable.
The for loop header is rewritten using magic-string's overwrite. However, unlike slice and move, overwrite does NOT consider the insertLefted close-paren to be part of the range, so the close-paren insert stays and shows up in the output, so the resulting JS has that extra paren that breaks things. It might be controversial, but my impression is that this is just a bug in magic-string, and that all magic-string functions that take a range should have the same behavior as far as what's included in that range.

I have a PR for magic-string that fixes this case and doesn't cause any decaffeinate test failures, so I'll send it out soon.

This changes the `overwrite` and `remove` behavior to be consistent with `move` and `slice`: the specified range includes any `insertRight`s on the left side and any `insertLeft`s on the right side. The code change ended up actually making the code simpler: `Chunk.edit` now overwrites all chunk content, including the intro and outro, so the `overwrite` code doesn't need to be careful about clearing the intro and outro for specific chunks. This fixes decaffeinate/decaffeinate#269 . See my comment in that bug for an explanation of what was going wrong, and why this case is important. In addition to this passing all of the magic-string tests, I also ran all of the decaffeinate tests with this change and they all pass. So at least with the decaffeinate project, the code wasn't depending on the previous behavior.

The code in decaffeinate#301 is actually parsed incorrectly by the CoffeeScript parser (even before decaffeinate-parser does anything interesting). It says that the function body ends at the end of the file, so decaffeinate was inserting a close-curly-brace at the end of the file, which was incorrect. However, the function application was correctly parsed, and explicitly putting parens for the function application causes the CoffeeScript parser to work again, so we can work around this issue by inserting parens for all implicit functions before the MainStage. I think this will also fix a bunch of other problems caused by implicit function calls. For example, it also happens to fix decaffeinate#269. In the codebase that I'm trying to decaffeinate, it fixes 49 out of the 104 files with decaffeinate failures that I hadn't categorized. I added the heuristic that we put the close-paren on the next line if the last arg is a multi-line expression. This fixed the formatting in almost every test, except the change introduced two test issues: * Object literal formatting sometimes had an excessive newline in non-implicit function calls ( http://decaffeinate-project.org/repl/#?evaluate=true&code=a(b%2C%0A%20%20c%3A%20d%0A%20%20e%3A%20f%0A%29 ) so adding parens exposed the issue. It's pretty minor, though. I think. * In another test, the added parens interfered with some other operations in the normalize step in a way that will be fixed by Rich-Harris/magic-string#89 Closes decaffeinate#301. Closes decaffeinate#269.

The code in #301 is actually parsed incorrectly by the CoffeeScript parser (even before decaffeinate-parser does anything interesting). It says that the function body ends at the end of the file, so decaffeinate was inserting a close-curly-brace at the end of the file, which was incorrect. However, the function application was correctly parsed, and explicitly putting parens for the function application causes the CoffeeScript parser to work again, so we can work around this issue by inserting parens for all implicit functions before the MainStage. I think this will also fix a bunch of other problems caused by implicit function calls. For example, it also happens to fix #269. In the codebase that I'm trying to decaffeinate, it fixes 49 out of the 104 files with decaffeinate failures that I hadn't categorized. I added the heuristic that we put the close-paren on the next line if the last arg is a multi-line expression. This fixed the formatting in almost every test, except the change introduced two test issues: * Object literal formatting sometimes had an excessive newline in non-implicit function calls ( http://decaffeinate-project.org/repl/#?evaluate=true&code=a(b%2C%0A%20%20c%3A%20d%0A%20%20e%3A%20f%0A%29 ) so adding parens exposed the issue. It's pretty minor, though. I think. * In another test, the added parens interfered with some other operations in the normalize step in a way that will be fixed by Rich-Harris/magic-string#89 Closes #301. Closes #269.

alangpierce mentioned this issue Jul 1, 2016

Improve error reporting when there is a parser error #272

Merged

alangpierce mentioned this issue Jul 9, 2016

Change overwrite and remove to remove interior inserts Rich-Harris/magic-string#89

Merged

alangpierce mentioned this issue Jul 11, 2016

fix: add implicit function call parens in the normalize step #302

Merged

eventualbuddha closed this as completed in #302 Jul 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

for loops over implicit function calls crash #269

for loops over implicit function calls crash #269

alangpierce commented Jul 1, 2016

alangpierce commented Jul 9, 2016

for loops over implicit function calls crash #269

for loops over implicit function calls crash #269

Comments

alangpierce commented Jul 1, 2016

alangpierce commented Jul 9, 2016