Skip to content

Parse JSDoc ...T and T= only at top-level JSDoc#22661

Merged
sandersn merged 3 commits into
masterfrom
parse-jsdoc-variadic-and-optional-toplevel-only
Mar 16, 2018
Merged

Parse JSDoc ...T and T= only at top-level JSDoc#22661
sandersn merged 3 commits into
masterfrom
parse-jsdoc-variadic-and-optional-toplevel-only

Conversation

@sandersn
Copy link
Copy Markdown
Member

@sandersn sandersn commented Mar 16, 2018

...T and T= should only be legal at the top level of a type, and only in JSDoc, since at least T= is ambiguous elsewhere. This PR changes parsing to make that happen. The resulting parse tree is now simpler, allowing me to get rid of some code I had to add in the checker.

Originated from @Andy-MS's comments on #22646 and discussion with @weswigham.

@sandersn sandersn requested review from a user and weswigham March 16, 2018 21:59
@ghost
Copy link
Copy Markdown

ghost commented Mar 16, 2018

allowing me to get rid of some code I had to add in the checker.

Should include that in this PR

@sandersn
Copy link
Copy Markdown
Member Author

sandersn commented Mar 16, 2018

It is included, but I included the commits adding it in the diff, and then asked to merge to master. So it's not displaying. I'm not sure how to fix it short of retargeting the PR somehow.

Comment thread src/compiler/parser.ts Outdated
type = finishNode(variadic);
}
if (token() === SyntaxKind.EqualsToken) {
type = createJSDocPostfixType(SyntaxKind.JSDocOptionalType, type);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: would just return here (or use conditional expression)

@ghost
Copy link
Copy Markdown

ghost commented Mar 16, 2018

I checked out the branch and still see skipJSDocPrefixTypes?

...T and T= should only be legal at the top level of a type, and only in
JSDoc, since at least T= is ambiguous elsewhere. This PR changes parsing
to make that happen. The resulting parse tree is now simpler, allowing
me to get rid of some code I had to add in the checker.
@sandersn sandersn force-pushed the parse-jsdoc-variadic-and-optional-toplevel-only branch from 69a4128 to 3088bc3 Compare March 16, 2018 22:19
Copy link
Copy Markdown
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now 😄

@sandersn
Copy link
Copy Markdown
Member Author

Should be fixed now after some shenanigans followed by a force push.

@sandersn sandersn merged commit bb23e96 into master Mar 16, 2018
@sandersn sandersn deleted the parse-jsdoc-variadic-and-optional-toplevel-only branch March 16, 2018 23:08
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants