feat(typescript-estree): add ParenthesizedExpression when surrounded by JSDoc type comment

[armano2]

This PR adds option to generate ParenthesizedExpression if they are surrounded by jsDoc comments

ParenthesizedExpression :< Node {
  type: 'ParenthesizedExpression';
  expression: Expression;
}

#1682
cc. @thorn0

[typescript-eslint]

Thanks for the PR, @armano2!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[armano2]

do we want to generate them in when comment is outside of node?

const x = /* test */ (2);
const x = /* test */(2);
const x = (2) /* test */;
const x = (2)/* test */;

[bradzacher]

This PR adds option to generate ParenthesizedExpression if they are surrounded by comments

For the specific usecase of a JSDoc type assertion - I think we should specifically handle that case with an AST node as part of the normal parse cycle (with no option required).

For other cases - I thought the intention was to indicate all parentheses via the AST?
eg (((1))) would generate ParenthesizedExpression > ParenthesizedExpression > ParenthesizedExpression > Literal.
Am I misunderstanding the request from prettier?

[armano2]

For the specific usecase of a JSDoc type assertion - I think we should specifically handle that case with an AST node as part of the normal parse cycle (with no option required).

this is breaking change, and will require major release

For other cases - I thought the intention was to indicate all parentheses via the AST?

if that's a case we can just remove condition

          hasComments(this.ast, node)

[codecov]

Codecov Report

Merging #3135 (a99cd94) into master (84fe328) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3135   +/-   ##
=======================================
  Coverage   92.88%   92.88%           
=======================================
  Files         315      315           
  Lines       10706    10708    +2     
  Branches     3025     3026    +1     
=======================================
+ Hits         9944     9946    +2     
  Misses        342      342           
  Partials      420      420           

Flag	Coverage Δ
unittest	92.88% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...pt-estree/src/ts-estree/estree-to-ts-node-types.ts	100.00% <ø> (ø)
packages/visitor-keys/src/visitor-keys.ts	100.00% <ø> (ø)
packages/typescript-estree/src/convert.ts	98.25% <100.00%> (+<0.01%)	⬆️

[thorn0]

Am I misunderstanding the request from prettier?

Yes, a little. When I was talking about nested parentheses, first and foremost I had nested JSDoc type assertions in mind. Prettier doesn't care about other parenthesized expressions.

Just like in TypeScript, you can sometimes see things like foo as any as MyType, people do the same thing with JSDoc.

[thorn0]

That said, as mentioned in estree/estree#194, seemingly redundant parentheses sometimes can have runtime consequences in JS, so probably it's another use case where it makes sense to represent them in the AST. Nestedness doesn't matter in that case though.

[armano2]

For cowpath-paving information, Acorn provides a preserveParens option that fixes exactly this:
estree/estree#194 (comment)

this option name seem better than mine

we can go both ways, i can generate them for all or when comments are present, i have no opinion on this.

[thorn0]

do we want to generate them in when comment is outside of node?

That's the only case when I personally would like to have them generated. Besides, the comment should be leading and contain @type {...}.

The syntax of JSDoc type assertions is simple: @type {...} comment followed by a parenthesized expression. Anything else is not a type assertion. Reference: Closure Compiler, TypeScript.

[bradzacher]

In that case I'd suggest that we just handle the specific case of a JSDoc type assertion and ignore all others as I mentioned here: #1682 (comment)

---

I don't think this is a breaking change - we don't usually treat adding a new AST node as a breaking change.
For example, when we introduced optional chaining it was a feature, when we introduced tuple types it was a feature.

In the absolute strictest sense: yes, a new node is a breaking change, and a new property value is too (eg nullish coalescing added ?? to LogicalExpression.operator). However if we used that logic then almost every single AST addition would be a breaking change.
For example - even #3112 added JSXNamespacedName, so that was technically breaking.

That is a bad line to follow and was what lead the old project to release 18 major versions in 4 months.

[armano2]

ok updated base on feedback, and i changed tests a little for this

[thorn0]

TypeScript expects the type to be enclosed in curly brackets, however Closure Compiler accepts types in parens and even without any delimiters at all. That's why Prettier searches comments only for starting with * and containing the substring "@type". I'm not insisting on taking this into account in this PR, especially if it complicates the implementation. JFYI.

upd: Looks like this has changed. TS is fine with types without delimiters, but still doesn't accept types in parens (/** @type (x) */). It still generates a JSDocTypeTag node for it though, so we are completely fine!

[armano2]

requested changes has been applied

[bradzacher]

all the code and tests LGTM.
One comment.

[bradzacher]

Because this case is specifically about JSDoc type assertions, and we have no immediate or future plans to support more parenthesized expressions - I think we should call this JSDocTypeAssertion.

[thorn0]

ParenthesizedExpression is an established thing in the ecosystem. Could we keep this type and add a boolean property jsDocTypeAnnotation: true instead? (The typeAnnotation property containing a type will do too, but it's out of scope for now I guess.)

[bradzacher]

Within ESTree its not an established practice. Most JS parsers don't emit it either. Babel does emit it all-or-nothing via a flag.

Adding a new node type is a much better API than using something generic because it's a special type of expression which has special meaning AND syntax to typescript and we should treat it akin to an as or angle bracket assertion. This isn't just a regular parenthesised expression where the parentheses control order of operations.

[armano2]

see babel/babel#12933 (review)

[thorn0]

Babel does emit it all-or-nothing via a flag.

I'd say that's what most JS parsers do. I was a bit worried about compatibility issues for various tools like recast. There is no evidence people use it with this parser though. If you think it's not a big deal, so be it.

[nicolo-ribaudo]

@armano2 Note that that PR doesn't change whether or not we create ParentesizedExpression nodes, it only ensures that we don't throw when we do emit them.

[thorn0]

I forgot to mention that, just like usual TS type assertions, this syntax can be the RHS of an assignment (see TS playground). This might be something that should be taken into account in this PR.