Fix missing parens in reprinted output

[conartist6]

Fixes #1067

I am not sure the test is completely adequate, but I can verify that the bug is fixed in the minimal repro.

[conartist6]

Oh hmm, nope it's not quite right yet. I found a test that breaks things.

[conartist6]

I'm pretty confident that my fix is correct, but I'm seeing a problem in the typescript test.

[conartist6]

Oops, didn't mean to send that yet. The problem is that printer.printGenerically(parseExpression('(function () {})')) loses its outermost parens because FPp.needsParens returns false any time a node has no parent. This short-circuits both the extra.parenthesized logic and and the firstInStatement logic, either of which could indicate that the outermost parens surrounding the FunctionExpression are necessary. Interestingly only the typescript test happens to trigger this condition, but the underlying bug is not unique to typescript.

[conartist6]

It seems like I could just tweak the logic to ensure that neither of those is dependent on !node.parent. Both of those expressions already seem to make perfect sense when !node.parent.

[conartist6]

By the way this is only an issue because I've removed the extra.parenthesized check here as it was causing paren doubling when printing code parsed with the babylon parser.

[conartist6]

Though actually that line only handled the case when extra.parenthesized was true, which should not be a requirement for correct behavior.

[conartist6]

So I think I've fixed those problems.

I also renamed parens-babylon.ts to parens-extra to make it more clear what aspect of the code is under test. I've moved some assertions about that behavior from the typescript test to this one as well, and added coverage for reprinting.

[conartist6]

As it sits now this PR has one failing test that I'm not sure what to do about. The test case boils down to this:

    assert.strictEqual(
      new Printer().print(b.functionExpression(/*...*/)).code,
      "function a(b, c = 1, ...d) {}",
    );

which results in the following error:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '(function a(b, c = 1, ...d) {})'
- 'function a(b, c = 1, ...d) {}'

I definitely broke this by allowing the root node of an expression AST to need parens, and I read the result as malicious compliance. For *Expression nodes which have equivalent *Statement I do not see a downside to allowing an expression at the root of a printed AST to be interpreted as a statement as a special case.

[conartist6]

Ah ok, I'm putting it back exactly the way it was. All the tests pass again now. It looks a little different because it is still necessary to check extra.parenthesized even if there is no parent.

That ensures that both the formerly broken case and check("(function () {} ())"); pass.

[conartist6]

I strongly recommend reviewing this with the "Hide whitespace" option on as a large block was reindented.

[conartist6]

We delay this check so that we can reprint (function () {} ()), which otherwise will always have its outermost parens stripped since it has no parent.

[conartist6]

The problem with short circuiting to false as we did previously is that a node may still really need parens (that is, non-decoratively). I opted to make this code responsible for decorative parens which as far as I can tell works fine, though it isn't apparent to me yet if there is some reason the code was written the way it was. The test suite seems to indicate that there is not, or at least is no longer.

[conartist6]

d'oh

[conartist6]

I haven't yet been able to verify that this test fails if the behavior regresses.

[conartist6]

expected is first

[conartist6]

@coderaiser Sorry about that. The test cases for recast are really rather inadequate it seems. I'm looking into these issues.

[conartist6]

@coderaiser I'm sorry, at the moment I'm going to need some help from you to be able to understand how to investigate what's happening in your codebase. I've figured out how to isolate a specific test and hit breakpoints, but I'm confused. If I put a breakpoint at the beginning of the recast parse function (in node_modules/@putout/recast/lib/parse.js) I hit it no problem. If I put a breakpoint in print I don't hit it at all.

[conartist6]

Oh oops maybe I just focused the wrong test.

[conartist6]

I'm looking at the extra parens on JSX return first. The JSX node itself is node.extra.parenthesized, and the printer always adds parens when returning a multiline JSX node as a return statement. No mystery here really.

Unfortunately it's pretty tricky to see what the correct fix may be....

[conartist6]

Out of curiousity @coderaiser how do you feel about transitioning from recast to prettier if prettier were able to support transformations?

[coderaiser]

@conartist6 thanks for so fast reply! Actually this additional parens not a very big deal since all the code transformed by 🐊Putout then processed by ESLint with no-extra-parens rule. I just want to mention this while the merge is fresh and everybody remember what is going on here :).

Out of curiousity @coderaiser how do you feel about transitioning from recast to prettier if prettier were able to support transformations?

I will definitely try it, right now @putout/engine-parser supports all actual ESTree-based JavaScript parsers, I prefer Babel for rich API, ecosystem and fast support. And Recast really does it’s best to preserve source code in state it was before any transformation. Also ESLint helps to keep code in opinionated but human readable state with help of eslint-plugin-putout, so it’s not a compromise, this is what I think the code should look like (and the way configuration abilities should look like). So if prettier will give me the same code style I have in my codebase right now of course I’ll give it a try :).