Skip to content

Handle namepaths inside JSDoc type expressions a bit better #32563

Merged
orta merged 5 commits into
microsoft:masterfrom
orta:fix_31298
Aug 9, 2019
Merged

Handle namepaths inside JSDoc type expressions a bit better #32563
orta merged 5 commits into
microsoft:masterfrom
orta:fix_31298

Conversation

@orta
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread src/compiler/parser.ts Outdated
while (token() !== SyntaxKind.CloseBraceToken && token() !== SyntaxKind.EndOfFileToken) {
nextTokenJSDoc();
}
type = finishNode(moduleTag);
Copy link
Copy Markdown
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);

Comment thread src/compiler/parser.ts Outdated
scanner.setInJSDocType(false);
if (moduleSpecifier) {
const moduleTag = createNode(SyntaxKind.JSDocNamepathType, moduleSpecifier.pos) as JSDocNamepathType;
while (token() !== SyntaxKind.CloseBraceToken && token() !== SyntaxKind.EndOfFileToken) {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread src/compiler/parser.ts Outdated
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
Copy Markdown
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
Copy Markdown
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.

Comment thread src/compiler/parser.ts Outdated
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

orta commented Aug 8, 2019

Will fix that up tomorrow 👍

@orta orta merged commit 2a2866c into microsoft:master Aug 9, 2019
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
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.

Odd quickinfo result for jsdoc return tag using module

2 participants