benjamn / recast

JavaScript syntax tree transformer, nondestructive pretty-printer, and automatic source map generator
MIT License
5.01k stars 350 forks source link

Incorrect print result when array disables ASI #394

Open not-an-aardvark opened 7 years ago

not-an-aardvark commented 7 years ago

Using recast 0.12.2 on the following code:

foo = 1
bar.concat([1, 2, 3])
const recast = require('recast');
const ast = recast.parse('foo = 1\nbar.concat([1, 2, 3])').program

// swap the array and `bar`
const barIdentifier = ast.body[1].expression.callee.object;
const arrayExpression = ast.body[1].expression.arguments[0];

ast.body[1].expression.callee.object = arrayExpression;
ast.body[1].expression.arguments[0] = barIdentifier;

recast.print(ast);

Output:

foo = 1
[1, 2, 3].concat(bar)

This is incorrect because [1, 2, 3] is now parsed as a MemberExpression access on 1, rather than an array in a new statement. I would expect recast to insert a semicolon somewhere to prevent this.

benjamn commented 7 years ago

This is definitely a bug, though it's not surprising (to me) because Recast currently makes no attempt to insert additional semicolons when AST nodes are transplanted to new positions. The closest related issue I can think of is https://github.com/benjamn/recast/pull/364.

This may be something we can address in the same way we ensure transplanted nodes have parentheses when necessary.

In the meantime, you have a couple options:

  1. If you're aware you may be creating a risky scenario like this, you can set ast.body[0].original = null to trigger reprinting of the foo = 1 statement, which will give it a semicolon.

  2. You could run your code through a tool that adds missing semicolons, though that may be more invasive than you'd like.

  3. Knowing that you're putting an array literal at the beginning of a line, you can guard it with an exclamation point (or any unary operator):

    var b = recast.types.builders;
    ast.body[1].expression =
    b.unaryExpression("!", ast.body[1].expression);

    which will give you

    foo = 1
    ![1, 2, 3].concat(bar)

In general, this guarding is necessary if a line begins with any of the following: +-[(/ (reference).

Just to be clear, I agree this is something Recast could help handle (more) automatically; I just want you to know your immediate options.

not-an-aardvark commented 7 years ago

Thanks for the response. Actually, I don't have any code like this -- I was trying this out because I've ended up implementing some similar logic (and creating similar bugs) when working on ESLint's autofix feature, and I wanted to see if recast had a better way to handle it.

In general, this guarding is necessary if a line begins with any of the following: +-[(/ (reference).

The page you linked is a bit out of date -- it's also necessary if a line begins with `:

foo
`bar` + baz; // this is parsed as a TaggedTemplateExpression, not a separate statement
benjamn commented 7 years ago

Cool. By the way, I have a long-term goal of making Recast understand all .eslintrc configuration options (#226), and I would be more than happy to help if you felt like implementing any of your auto-fix functionality using Recast.