identity transformation adds parens to IIFE

[sophiebits]

Given the source file

(function() {
}());

(with no parens around the function expression), recast outputs the file as

((function() {
})());

even if no changes are made (e.g., using examples/identity).

[sophiebits]

Apparently because needsParens returns true for the function. I don't understand why needsParens checks:

    if (n.FunctionExpression.check(node) &&
        this.firstInStatement())
        return true;

[benjamn]

I believe this happens because the hasParens function is dumb and gets confused. The reprinter knows the function expression needs parentheses, but when it goes to check whether it already has parentheses, it gets the answer wrong, because the first character after the closing } is a (.

If this theory is true, then

(function() {
})();

should print without the extra layer of parentheses (because hasParens gets the answer right when you move the invoking parens outside the parentheses wrapping the whole expression).

[sophiebits]

Yes, that already reprints correctly.

[benjamn]

For what it's worth, the function definitely does need parentheses, since the following code would be illegal:

function() {
}();

Also, NodePath.prototype.needsParens unfortunately only knows about the AST, so it's up to the Recast reprinter to second-guess needsParens when parentheses are already present in the surrounding code.

[sophiebits]

(2 + function() {
}());

also doesn't add extra parens.

[benjamn]

That'll be because the function expression is no longer firstInStatement.

By the way, these explanations are descriptive (of how things work currently, which is broken) rather than normative (I'm not trying to justify the way things work!).

[sophiebits]

Ahh, I see. firstInStatement isn't checking whether the expression is the first statement in the block but whether the function is the first thing in the statement. Sorry for my confusion.

[sophiebits]

After thinking about this a little bit, I think needsParens is conflating two concepts that should be separate (precedence and function/{ at the beginning of a statement). In these cases the parens must always be exactly where they are:

(2 + x) * 3
(7).toString()

but with the expression

(function() {})()

the parens can be removed as long as there's another leading parenthesis to distinguish it from a statement.

[benjamn]

Some thoughts: in the failing case we're talking about, the FunctionExpression shares the same .loc.start position as the parent CallExpression node, though their .loc.end positions differ.

The hasParens function returns false for the FunctionExpression because it encounters characters involved in the CallExpression before it finds the closing parenthesis, but hasParens would return true if passed the CallExpression instead of the FunctionExpression.

I'm not yet sure what algorithm is suggested by this observation, but there's a possibility of salvaging hasParens if we're willing to walk the parent path in cases like this.

[benjamn]

Just published v0.5.19 to NPM with a fix for this issue: https://www.npmjs.org/package/recast

Thanks for bringing my attention to this problem area, @spicyj!

[sophiebits]

Thanks for fixing!