Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ArrowFunctionExpression returning ObjectPattern prints without parens when a return type is removed #1067

Closed
conartist6 opened this issue Feb 24, 2022 · 3 comments · Fixed by #1068

Comments

@conartist6
Copy link
Contributor

conartist6 commented Feb 24, 2022

I've created a minimal repro for this behavior here: https://github.com/conartist6/recast-arrow-object-pattern-repro

The input is:

(): Type => ({ prop: true });

and the output is invalid, as it is missing parens:

() => { prop: true };
@conartist6
Copy link
Contributor Author

I made the initial PR to add support for node.extra.parenthesized as a way to declare parens, but I'm thinking I missed a case. I notice a few things.

First: fastPath.needsParens will always return false because node.extra.parenthesized.

Second: I implemented the logic to check node.extra.parenthesized and print parens in genericPrint. But the ObjectPattern node is not printed with genericPrint. It has a reprinter:

recast/lib/printer.ts

Lines 107 to 126 in cb7ff9a

const reprinter = getReprinter(path);
const lines = reprinter
? // Since the print function that we pass to the reprinter will
// be used to print "new" nodes, it's tempting to think we
// should pass printRootGenerically instead of print, to avoid
// calling maybeReprint again, but that would be a mistake
// because the new nodes might not be entirely new, but merely
// moved from elsewhere in the AST. The print function is the
// right choice because it gives us the opportunity to reprint
// such nodes using their original source.
reprinter(print)
: genericPrint(
path,
config,
options,
makePrintFunctionWith(options, {
includeComments: true,
avoidRootParens: false,
}),
);

@conartist6
Copy link
Contributor Author

conartist6 commented Feb 24, 2022

Third: the patcher seems to rely only on fastPath.needsParens:

recast/lib/patcher.ts

Lines 234 to 236 in cb7ff9a

if (path.needsParens()) {
return linesModule.concat(["(", patchedLines, ")"]);
}

@conartist6
Copy link
Contributor Author

So I guess I understand how it's broken now. How should it work?

I clearly made a faulty assumption when I made the initial PR, that extra.parenthesized and ParenthesizedExpression could be handled exactly the same way, but they're not quite the same thing. One has its own node, and one does not.

While it is true that needsParens is always false for a ParenthesizedExpression because that node prints itself as a set of parens, I think for the implementation to be correct it cannot possibly return false for a node with extra.parenthesized. A node which has that flag does always need parens, and it's presence also must not override the following logic, which dictates whether a node needs parens for any other reason. What I've actually done is short-circuited the logic that would say that an ObjectPattern which is the body of an ArrowFunctionExpression must ALWAYS get parens. Oops!!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant