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

Proposal: Add Closure Compiler Syntax Plugin #7661

Closed
arv opened this issue Apr 3, 2018 · 13 comments · Fixed by #8025
Closed

Proposal: Add Closure Compiler Syntax Plugin #7661

arv opened this issue Apr 3, 2018 · 13 comments · Fixed by #8025
Labels
i: enhancement outdated A closed issue/PR that is archived due to age. Recommended to make a new issue

Comments

@arv
Copy link
Contributor

arv commented Apr 3, 2018

Choose one: Feature request

Input Code

// Closure Compiler input/output
/** @return {T} */
function f() {
   return /** @type {T} */ (someExpression);
}

The parentheses are important to Closure Compiler and it defines a Cast expression

https://github.com/google/closure-compiler/wiki/Types-in-the-Closure-Type-System#type-casts

https://github.com/google/closure-compiler/blob/79deb15554ae0903b21fa9dc94746a879ae8ef12/src/com/google/javascript/jscomp/parsing/IRFactory.java#L783-L788

Expected Behavior

Keep the parens on a paren expression after a JSDoc comment.

Current Behavior

The ParenExpression is "transformed" to an Expression. (Not really transformed but Babel does not output unnecessary parens).

Possible Solution

Introduce a syntax plugin that detects the case where we have a paren expression preceded by a JSDoc comment with @type in it and create a CcCastExpression instead. Have the generator for the CcCastExpression output the comment, parens and the expression.

Context

Cannot use Babel to do JSX transformation because we use Closure Compiler for type checking and minimization.

At this time I'm not suggesting adding any other Closure Compiler features. I was not planning on parsing the actual type in the JSDoc but future improvements to this plugin could do that.

@babel-bot
Copy link
Collaborator

Hey @arv! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@arv
Copy link
Contributor Author

arv commented Apr 3, 2018

@loganfsmyth

I'll probably write this in my own repo to start with but my end goal is to have this work with @babel/standalone

@loganfsmyth
Copy link
Member

Yeah basically I see two options for us, if we actually wanted to support this:

  • Add an option for the parser that would tell it to generate ParenthesizedExpression nodes
  • Go the Babylon plugin route

I go back and forth on it. The ParenthesizedExpression approach seems easiest to me if we honestly do not anticipate adding any other special cases for CC. If there are any other CC constructs where there are issues, there's where I lean more toward the Babylon plugin / CC-specific AST Node types.

I'd probably be open to ParenthesizedExpression as an option on Babylon if others agree. We're doing our best not to unnecessarily break existing plugins, so I don't think I'd want that AST type to be generated automatically, which is why I'm in favor of it being opt-in. Curious to hear what others think.

@suchipi
Copy link
Member

suchipi commented Apr 8, 2018

I'd be interested in writing a PR to add ParenthesizedExpression to Babylon behind an includeParenExpressions option- does that sound like the right approach?

@suchipi
Copy link
Member

suchipi commented Apr 8, 2018

For reference, luaparse's solution to this problem is to add "inParens" properties to every node, rather than adding a new node type.

@loganfsmyth
Copy link
Member

@suchipi Out of curiosity, is your usecase also ClosureCompiler, or something else?

@nicolo-ribaudo
Copy link
Member

Doesn't Babylon already add node.extra.parenthesized?

@suchipi
Copy link
Member

suchipi commented Apr 8, 2018

I'm a maintainer of prettier, and we could use paren information in the AST to ensure we reprint closure compiler and flowtype comments correctly. Currently we would have to check the source text. But I work with Babylon a lot in general (babel plugins, codemods, etc) and have found a lack of paren information in the AST understandable but occasionally inconvenient.

@suchipi
Copy link
Member

suchipi commented Apr 8, 2018

@nicolo-ribaudo if it does, that's great news- I can use that

@suchipi
Copy link
Member

suchipi commented Apr 8, 2018

It totally does, I can't believe I never noticed 😅 thanks!

@arv
Copy link
Contributor Author

arv commented Apr 10, 2018

node.extra.parenthesized is not sufficient for Closure Compiler since transformations generally do not carry the node.extra.parenthesized. That is what #7015 is trying to fix.

@koba04

@koba04
Copy link
Contributor

koba04 commented Apr 11, 2018

I guess Babylon adds node.extra.parenthesizes properly but babel-generator doesn't print the parens in some cases. #7075 is a PR to fix it with retainExtraParens option.

Are there any advantages to adding ParenthesizedExpression as a new AST as optional over just using node.extra.parenthesizes?
If Babylon adds ParenthesizedExpression with an option, does babel-generator have to care about the option to print parens?

@arv
Copy link
Contributor Author

arv commented May 24, 2018

Are there any advantages to adding ParenthesizedExpression as a new AST as optional over just using node.extra.parenthesizes?

Having an AST node for it ensures that transformers do not accidentally strip parens. We used ParenExpression in Traceur and it was never an issue there.

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 25, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i: enhancement outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants