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.

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

fix(47923): Delete unused JSDoc quick fix changes formatting on rest of JSDoc comment #48482

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(47923): Delete unused JSDoc quick fix changes formatting on rest of JSDoc comment #48482

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7660c27
fix(47923): Preserve newlines in comment
Mar 31, 2022
4e60e0d
fix(47923) - Update tests
Mar 31, 2022
904d0ed
fix(47923) - Add newline checking explaination
Apr 1, 2022
8c55f81
fix(47923): Using counter instead of states and update test description
Apr 8, 2022
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
21 changes: 19 additions & 2 deletions src/compiler/emitter.ts
View file
Original file line number Diff line number Diff line change
@@ -3929,8 +3929,25 @@ namespace ts {
function emitJSDocComment(comment: string | NodeArray<JSDocComment> | undefined) {
const text = getTextOfJSDocComment(comment);
if (text) {
writeSpace();
write(text);
const lines = text.split(/\r\n?|\n/g);
for (let lineIndex = 0, length = lines.length ; lineIndex < length ; ++ lineIndex) {
const line = lines[lineIndex];
if (lineIndex === 0) {
if (line.length > 0) {
writeSpace();
Copy link
Member

Choose a reason for hiding this comment

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

does this space make sense when the comment is the only thing in the jsdoc? eg

/**
 * commentary 1
 * commentary 2
 */
var x;

It looks like it might not emit the * as expected for the commentary 1 line, or else emit an extra space on the line right after the **.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks the same before and after my change
VS Code
image

VS Code with my changes
image

}
write(line);
}
else {
writeLine();
writeSpace();
writePunctuation("*");
if (line.length > 0) {
writeSpace();
}
write(line);
}
}
}
}

64 changes: 46 additions & 18 deletions src/compiler/parser.ts
View file
Original file line number Diff line number Diff line change
@@ -8077,7 +8077,9 @@ namespace ts {
let comments: string[] = [];
const parts: JSDocComment[] = [];
let linkEnd;
let state = JSDocState.BeginningOfLine;
let sameLineWithTag = true;
let numberOfStatesChanged = 0;
let state = JSDocState.BeginningOfLine as JSDocState;
let previousWhitespace = true;
let margin: number | undefined;
function pushComment(text: string) {
@@ -8087,6 +8089,10 @@ namespace ts {
comments.push(text);
indent += text.length;
}
function updateState(newState: JSDocState) {
state = newState;
numberOfStatesChanged++;
}
if (initialMargin !== undefined) {
// jump straight to saving comments if there is some initial indentation
if (initialMargin !== "") {
@@ -8098,9 +8104,10 @@ namespace ts {
loop: while (true) {
switch (tok) {
case SyntaxKind.NewLineTrivia:
state = JSDocState.BeginningOfLine;
updateState(JSDocState.BeginningOfLine);
// don't use pushComment here because we want to keep the margin unchanged
comments.push(scanner.getTokenText());
sameLineWithTag = false;
indent = 0;
break;
case SyntaxKind.AtToken:
@@ -8129,7 +8136,7 @@ namespace ts {
}
break;
case SyntaxKind.OpenBraceToken:
state = JSDocState.SavingComments;
updateState(JSDocState.SavingComments);
const commentEnd = scanner.getStartPos();
const linkStart = scanner.getTextPos() - 1;
const link = parseJSDocLink(linkStart);
@@ -8144,26 +8151,42 @@ namespace ts {
}
break;
case SyntaxKind.BacktickToken:
if (state === JSDocState.SavingBackticks) {
state = JSDocState.SavingComments;
}
else {
state = JSDocState.SavingBackticks;
}
updateState(state === JSDocState.SavingBackticks ? JSDocState.SavingComments : JSDocState.SavingBackticks);
pushComment(scanner.getTokenText());
break;
case SyntaxKind.AsteriskToken:
if (state === JSDocState.BeginningOfLine) {
// if there are two ** after a newline, considering it as a pure comment
if (state === JSDocState.BeginningOfLine && lookAhead(() => nextTokenJSDoc() !== SyntaxKind.AsteriskToken)) {
/*
* @param x
* in the first line
*/
// to check tag starting newline without comments like above
// it needs to actually encounter a newline
if (numberOfStatesChanged === 1 && lookAhead(() => nextTokenJSDoc() !== SyntaxKind.NewLineTrivia)) {
removeTrailingWhitespace(comments);
}
// leading asterisks start recording on the *next* (non-whitespace) token
state = JSDocState.SawAsterisk;
updateState(JSDocState.SawAsterisk);
indent += 1;
break;
}
// record the * as a comment
// falls through
default:
/*
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what the parser changes do and why they're needed?

From reading the code, it looks like states is added to know when to call removeTrailingWhitespace, and the change in tryParseTypeExpression now doesn't skip whitespace when a parse fails, but I don't understand the intent of shuffling whitespace parsing around.

Copy link
Contributor Author

@MQuy MQuy Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

like you mentioned

  • tryParseTypeExpression doesn't skip whitespace when a parse fails
/*
 * @param b  // <- there is no type expression, it rollback to this position
 * 
 * something here
 */
  • parseTagComments only remove whitespace for these two cases
/*
* @param x
* in the first line
*/

/*
* @param x
still in the first line
*/

we can not do it at the end like we are currently doing since it is too late because there is a case with {@link} will reset comments

I don't really get what you mean by I don't understand the intent of shuffling whitespace parsing around, can you explain about it? 😄

* @param x
still in the first line
*/
// by default state starts with BeginningOfLine
// to check tag starting newline without comments like above
// it needs to actually encounter a newline
// we cannot check `comments` since it is reset after {}
if (state === JSDocState.BeginningOfLine && !sameLineWithTag && numberOfStatesChanged === 1) {
removeTrailingWhitespace(comments);
}
if (state !== JSDocState.SavingBackticks) {
state = JSDocState.SavingComments; // leading identifiers start recording as well
updateState(JSDocState.SavingComments); // leading identifiers start recording as well
}
pushComment(scanner.getTokenText());
break;
@@ -8172,8 +8195,8 @@ namespace ts {
tok = nextTokenJSDoc();
}

removeLeadingNewlines(comments);
removeTrailingWhitespace(comments);

if (parts.length) {
if (comments.length) {
parts.push(finishNode(factory.createJSDocText(comments.join("")), linkEnd ?? commentsPos));
@@ -8253,8 +8276,15 @@ namespace ts {
}

function tryParseTypeExpression(): JSDocTypeExpression | undefined {
skipWhitespaceOrAsterisk();
return token() === SyntaxKind.OpenBraceToken ? parseJSDocTypeExpression() : undefined;
let expression;
tryParse(() => {
skipWhitespaceOrAsterisk();
if (token() === SyntaxKind.OpenBraceToken) {
expression = parseJSDocTypeExpression();
return true;
}
});
return expression;
}

function parseBracketNameInPropertyAndParamTag(): { name: EntityName, isBracketed: boolean } {
@@ -8299,13 +8329,11 @@ namespace ts {
skipWhitespaceOrAsterisk();

const { name, isBracketed } = parseBracketNameInPropertyAndParamTag();
const indentText = skipWhitespaceOrAsterisk();

if (isNameFirst && !lookAhead(parseJSDocLinkPrefix)) {
typeExpression = tryParseTypeExpression();
}

const comment = parseTrailingTagComments(start, getNodePos(), indent, indentText);
const comment = parseTrailingTagComments(start, getNodePos(), indent, "");

const nestedTypeLiteral = target !== PropertyLikeParse.CallbackParameter && parseNestedTypeLiteral(typeExpression, name, target, indent);
if (nestedTypeLiteral) {
8 changes: 4 additions & 4 deletions tests/baselines/reference/JSDocParsing/DocComments.parsesCorrectly.@link tags.json
View file
Original file line number Diff line number Diff line change
@@ -86,7 +86,7 @@
"comment": {
"0": {
"kind": "JSDocText",
"pos": 70,
"pos": 69,
"end": 79,
"modifierFlagsCache": 0,
"transformFlags": 0,
@@ -124,7 +124,7 @@
"text": ""
},
"length": 2,
"pos": 70,
"pos": 69,
"end": 102,
"hasTrailingComma": false,
"transformFlags": 0
@@ -157,7 +157,7 @@
"comment": {
"0": {
"kind": "JSDocText",
"pos": 113,
"pos": 112,
"end": 120,
"modifierFlagsCache": 0,
"transformFlags": 0,
@@ -346,7 +346,7 @@
"text": " * }, because of the intermediate asterisks."
},
"length": 15,
"pos": 113,
"pos": 112,
"end": 589,
"hasTrailingComma": false,
"transformFlags": 0
24 changes: 24 additions & 0 deletions tests/cases/fourslash/codeFixDeleteUnmatchedParameterJS5.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/// <reference path='fourslash.ts' />

// @allowJs: true
// @checkJs: true
// @filename: /a.js
/////**
//// * @param c
//// * @param b
//// *
//// * bla bla bla
//// */
////function add(a, b) { }

verify.codeFix({
description: [ts.Diagnostics.Delete_unused_param_tag_0.message, "c"],
index: 0,
newFileContent:
`/**
* @param b
*
* bla bla bla
*/
function add(a, b) { }`
});
14 changes: 7 additions & 7 deletions tests/cases/fourslash/commentsLinePreservation.ts
View file
Original file line number Diff line number Diff line change
@@ -63,7 +63,7 @@
//// * @random tag This should be third line
//// */
////function /*h*/h(param1: string) { /*5*/param1 = "hello"; }
/////**
/////**
//// * This is firstLine
//// * This is second Line
//// * @param param1
@@ -101,7 +101,7 @@
//// *
//// * @param param1
//// *
//// * blank line that shouldnt be shown when starting this
//// * blank line that should be shown when starting this
//// * second time information about the param again
//// */
////function /*l*/l(param1: string) { /*9*/param1 = "hello"; }
@@ -129,22 +129,22 @@ verify.quickInfos({
3: ["(parameter) param1: string", "first line of param\n\nparam information third line"],

g: ["function g(param1: string): void", "This is firstLine\nThis is second Line"],
4: ["(parameter) param1: string", " param information first line"],
Copy link
Member

Choose a reason for hiding this comment

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

I like the old output better; @param is supposed to set a margin so that the space is preserved in param information first line. Preserving the newlines too is OK, depending on how it looks when formatted in VS Code.

This is true of all the changes below.

Copy link
Contributor Author

@MQuy MQuy Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

current VSCode version
image
image
image

VS Code with my changes
image
image
image

For the the later looks better 😄

4: ["(parameter) param1: string", "\n\nparam information first line"],

h: ["function h(param1: string): void", "This is firstLine\nThis is second Line"],
5: ["(parameter) param1: string", " param information first line\n\n param information third line"],
5: ["(parameter) param1: string", "\n\nparam information first line\n\nparam information third line"],

i: ["function i(param1: string): void", "This is firstLine\nThis is second Line"],
6: ["(parameter) param1: string", " param information first line\n\n param information third line"],
6: ["(parameter) param1: string", "\n\nparam information first line\n\nparam information third line"],

j: ["function j(param1: string): void", "This is firstLine\nThis is second Line"],
7: ["(parameter) param1: string", " param information first line\n\n param information third line"],
7: ["(parameter) param1: string", "\n\nparam information first line\n\nparam information third line"],

k: ["function k(param1: string): void", "This is firstLine\nThis is second Line"],
8: ["(parameter) param1: string", "hello"],

l: ["function l(param1: string): void", "This is firstLine\nThis is second Line"],
9: ["(parameter) param1: string", "first Line text\nblank line that shouldnt be shown when starting this \nsecond time information about the param again"],
9: ["(parameter) param1: string", "first Line text\n\n\nblank line that should be shown when starting this \nsecond time information about the param again"],

m: ["function m(param1: string): void", "This is firstLine\nThis is second Line\n[1]: third * line"],
10: ["(parameter) param1: string", "first Line text\nsecond line text"]