Skip to content

Handle namepaths inside JSDoc type expressions a bit better #32563

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

Merged
merged 5 commits into from
Aug 9, 2019

Conversation

orta
Copy link
Contributor

@orta orta commented Jul 25, 2019

Fixes #31298

Adds a way for the parser to move the scanner through a namepath inside JSDoc so that it can continue to parse the rest of the statement correctly and show up right in an editor.

@orta orta requested a review from sandersn July 25, 2019 21:13
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Couple of changes.

while (token() !== SyntaxKind.CloseBraceToken && token() !== SyntaxKind.EndOfFileToken) {
nextTokenJSDoc();
}
type = finishNode(moduleTag);
Copy link
Member

Choose a reason for hiding this comment

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

This whole thing should actually come first and be a completely separate code path. Here we call parseTypeOrTypePredicate (and allow prefix ...) in the namepath case, which is wrong.

Suggested change
type = finishNode(moduleTag);
return finishNode(moduleTag);

let type = parseTypeOrTypePredicate();
scanner.setInJSDocType(false);
if (moduleSpecifier) {
const moduleTag = createNode(SyntaxKind.JSDocNamepathType, moduleSpecifier.pos) as JSDocNamepathType;
while (token() !== SyntaxKind.CloseBraceToken && token() !== SyntaxKind.EndOfFileToken) {
Copy link
Member

@sandersn sandersn Jul 26, 2019

Choose a reason for hiding this comment

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

I realised that parseJSDocType is also called from parseJSDocParameter, not just single type expressions. That means its right terminators include , and ) See the ParsingContext.JSDocParameters entry in isListTerminator.

That also means that we need tests for jsdoc-style functions:

/** @type {function(module:xxxx, module:xxxx): module:xxxxx} */

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure whether jsdoc.app mentions whether namepaths can be used in function types, but I think we should support it even if it does not.

@orta
Copy link
Contributor Author

orta commented Jul 29, 2019

Hrm, these changes seem to affect the behavior of generators somehow - I'm gonna wait till Nathan's back and chat with him about it Figured it

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I would prefer a switch over an array created on each call to parseJSDocType.

const moduleSpecifier = parseOptionalToken(SyntaxKind.ModuleKeyword);
if (moduleSpecifier) {
const moduleTag = createNode(SyntaxKind.JSDocNamepathType, moduleSpecifier.pos) as JSDocNamepathType;
const terminators = [SyntaxKind.CloseBraceToken, SyntaxKind.EndOfFileToken, SyntaxKind.CommaToken, SyntaxKind.CloseParenToken, SyntaxKind.WhitespaceTrivia];
Copy link
Member

Choose a reason for hiding this comment

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

the new array+member approach feels like it creates unnecessary garbage in a path that may happen a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! Good point.

if (moduleSpecifier) {
const moduleTag = createNode(SyntaxKind.JSDocNamepathType, moduleSpecifier.pos) as JSDocNamepathType;
const terminators = [SyntaxKind.CloseBraceToken, SyntaxKind.EndOfFileToken, SyntaxKind.CommaToken, SyntaxKind.CloseParenToken, SyntaxKind.WhitespaceTrivia];
while (terminators.indexOf(token()) < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
while (terminators.indexOf(token()) < 0) {
terminate: while (true) {
switch(token()) {
case SyntaxKind.CloseBraceToken: // etc
break terminate;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I love it, this is the only project I've ever had PR suggestions where people recommend goto.

Copy link
Member

Choose a reason for hiding this comment

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

Gotta go fast!

@orta
Copy link
Contributor Author

orta commented Aug 8, 2019

Will fix that up tomorrow 👍

@orta orta merged commit 2a2866c into microsoft:master Aug 9, 2019
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 this pull request may close these issues.

Odd quickinfo result for jsdoc return tag using module
2 participants