Skip to content

Fix node.getStart() for nodes spanning multiline JSDoc comments #43854

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.

Sign up for GitHub

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 1 commit into from
Apr 28, 2021
Merged

Fix node.getStart() for nodes spanning multiline JSDoc comments #43854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/compiler/scanner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,11 +554,12 @@ namespace ts {
}

/* @internal */
export function skipTrivia(text: string, pos: number, stopAfterLineBreak?: boolean, stopAtComments = false): number {
export function skipTrivia(text: string, pos: number, stopAfterLineBreak?: boolean, stopAtComments?: boolean, inJSDoc?: boolean): number {
if (positionIsSynthesized(pos)) {
return pos;
}

let canConsumeStar = false;
// Keep in sync with couldStartTrivia
while (true) {
const ch = text.charCodeAt(pos);
Expand All @@ -573,6 +574,7 @@ namespace ts {
if (stopAfterLineBreak) {
return pos;
}
canConsumeStar = !!inJSDoc;
continue;
case CharacterCodes.tab:
case CharacterCodes.verticalTab:
Expand All @@ -592,6 +594,7 @@ namespace ts {
}
pos++;
}
canConsumeStar = false;
continue;
}
if (text.charCodeAt(pos + 1) === CharacterCodes.asterisk) {
Expand All @@ -603,6 +606,7 @@ namespace ts {
}
pos++;
}
canConsumeStar = false;
continue;
}
break;
Expand All @@ -613,13 +617,23 @@ namespace ts {
case CharacterCodes.greaterThan:
if (isConflictMarkerTrivia(text, pos)) {
pos = scanConflictMarkerTrivia(text, pos);
canConsumeStar = false;
continue;
}
break;

case CharacterCodes.hash:
if (pos === 0 && isShebangTrivia(text, pos)) {
pos = scanShebangTrivia(text, pos);
canConsumeStar = false;
continue;
}
break;

case CharacterCodes.asterisk:
if (canConsumeStar) {
pos++;
Copy link
Member

Choose a reason for hiding this comment

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

Dont you need to set canConsumeStar to false in the default case as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t think so—the default case is either whitespace (which doesn’t change whether we can consume a leading *) or it breaks and returns.

canConsumeStar = false;
continue;
}
break;
Expand Down
7 changes: 6 additions & 1 deletion src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,12 @@ namespace ts {
return getTokenPosOfNode((<SyntaxList>node)._children[0], sourceFile, includeJsDoc);
}

return skipTrivia((sourceFile || getSourceFileOfNode(node)).text, node.pos);
Copy link
Member

Choose a reason for hiding this comment

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

this looks like an OK fix, but I don't think skipTrivia is called on the inside of jsdoc anywhere else. Maybe there should be a completely separate code path?

I haven't thought about it deeply, so it's likely that skipTrivia is 98% right and just needed this tweak to work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it’s definitely true that I didn’t pass the parameter anywhere else. It occurred to me that I could accomplish the same thing by wrapping skipTrivia and calling it in a loop with stopAfterLineBreak, but I felt like it was a little less clear what was happening. But I can go back to that—I’m pretty ambivalent about it.

Copy link
Member

Choose a reason for hiding this comment

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

Ummm, me too. Neither one of these functions are specific to jsdoc, so jsdoc-specific code is surprise in both.

Probably means that this is good enough.

return skipTrivia(
(sourceFile || getSourceFileOfNode(node)).text,
node.pos,
/*stopAfterLineBreak*/ false,
/*stopAtComments*/ false,
isInJSDoc(node));
}

export function getNonDecoratorTokenPosOfNode(node: Node, sourceFile?: SourceFileLike): number {
Expand Down
20 changes: 20 additions & 0 deletions tests/cases/fourslash/documentHighlightJSDocTypedef.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/// <reference path="fourslash.ts" />

// @allowJs: true
// @checkJs: true

// @Filename: index.js
//// /**
//// * @typedef {{
//// * [|foo|]: string;
//// * [|bar|]: number;
//// * }} Foo
//// */
////
//// /** @type {Foo} */
//// const x = {
//// [|foo|]: "",
//// [|bar|]: 42,
//// };

verify.rangesWithSameTextAreDocumentHighlights();