-
Notifications
You must be signed in to change notification settings - Fork 12.9k
multi-line comment formatting fix and handler #16385
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
Changes from 28 commits
35b16ab
6b4cd0b
a819e4e
8f28a02
b02963b
8fc3fd9
cc88091
efdbeba
777bc57
091376f
2f5b1d3
7c402d5
de92e98
b2188ad
472ad9d
f3e0cbb
a209db7
a08d18a
ad9c29b
153b94a
70e4f34
4b9f5a0
62f16be
23ca368
b7bc7d8
19e2fa6
6029b5c
760ef44
e7d2af0
e4e969a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ namespace ts.server.protocol { | |
/* @internal */ | ||
BraceFull = "brace-full", | ||
BraceCompletion = "braceCompletion", | ||
GetSpanOfEnclosingComment = "getSpanOfEnclosingComment", | ||
Change = "change", | ||
Close = "close", | ||
Completions = "completions", | ||
|
@@ -241,6 +242,21 @@ namespace ts.server.protocol { | |
body?: TodoComment[]; | ||
} | ||
|
||
/** | ||
* A request to determine if the caret is inside a comment. | ||
*/ | ||
export interface SpanOfEnclosingCommentRequest extends FileLocationRequest { | ||
command: CommandTypes.GetSpanOfEnclosingComment; | ||
arguments: SpanOfEnclosingCommentRequestArgs; | ||
} | ||
|
||
export interface SpanOfEnclosingCommentRequestArgs extends FileLocationRequestArgs { | ||
/** | ||
* Requires that the enclosing span be a multi-line comment, or else the request returns undefined. | ||
*/ | ||
onlyMultiLine: boolean; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this just here for the generality of the API? We don't appear to actually use it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only ever pass in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have mixed feeling about this but don't object as long as we have test coverage (which we do). |
||
} | ||
|
||
/** | ||
* Request to obtain outlining spans in file. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1152,6 +1152,53 @@ namespace ts.formatting { | |
} | ||
} | ||
|
||
/** | ||
* @param precedingToken pass `null` if preceding token was already computed and result was `undefined`. | ||
*/ | ||
export function getRangeOfEnclosingComment( | ||
sourceFile: SourceFile, | ||
position: number, | ||
onlyMultiLine: boolean, | ||
precedingToken: Node | null | undefined = findPrecedingToken(position, sourceFile), // tslint:disable-line:no-null-keyword | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It shouldn't matter in what follows, as the |
||
tokenAtPosition = getTokenAtPosition(sourceFile, position, /*includeJsDocComment*/ false), | ||
predicate?: (c: CommentRange) => boolean): CommentRange | undefined { | ||
// Considering a fixed position, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this an arbitrary position or is it assumed to be at a token boundary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Arbitrary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would simplify this comment to something like
|
||
// - trailing comments are those following and on the same line as the position. | ||
// - leading comments are those in the range [position, start of next non-trivia token) | ||
// that are not trailing comments of that position. | ||
// | ||
// Note, `node.start` is the start-position of the first comment following the previous | ||
// token that is not a trailing comment, so the leading and trailing comments of all | ||
// tokens contain all comments in a sourcefile disjointly. | ||
const trailingRangesOfPreviousToken = precedingToken && getTrailingCommentRanges(sourceFile.text, precedingToken.end); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably short-circuit if position is within |
||
const leadingCommentRangesOfNextToken = getLeadingCommentRangesOfNode(tokenAtPosition, sourceFile); | ||
const commentRanges = trailingRangesOfPreviousToken && leadingCommentRangesOfNextToken ? | ||
trailingRangesOfPreviousToken.concat(leadingCommentRangesOfNextToken) : | ||
trailingRangesOfPreviousToken || leadingCommentRangesOfNextToken; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add some explanatory comments for the logic in here? It assumes the reader knows a lot of detail about how we keep comments in the trees attached to tokens. And I'm guessing there's some edge cases or patterns here this logic needs to handle that makes it non-trivial. That knowledge would be good for future readers/maintains not to have to figure out again for themselves. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added above. |
||
if (commentRanges) { | ||
for (const range of commentRanges) { | ||
// The end marker of a single-line comment does not include the newline character. | ||
// With caret at `^`, in the following case, we are inside a comment (^ denotes the cursor position): | ||
// | ||
// // asdf ^\n | ||
// | ||
// But for closed multi-line comments, we don't want to be inside the comment in the following case: | ||
// | ||
// /* asdf */^ | ||
// | ||
// However, unterminated multi-line comments *do* contain their end. | ||
// | ||
// Internally, we represent the end of the comment at the newline and closing '/', respectively. | ||
// | ||
if ((range.pos < position && position < range.end || | ||
position === range.end && (range.kind === SyntaxKind.SingleLineCommentTrivia || position === sourceFile.getFullWidth()))) { | ||
return (range.kind === SyntaxKind.MultiLineCommentTrivia || !onlyMultiLine) && (!predicate || predicate(range)) ? range : undefined; | ||
} | ||
} | ||
} | ||
return undefined; | ||
} | ||
|
||
function getOpenTokenForList(node: Node, list: ReadonlyArray<Node>) { | ||
switch (node.kind) { | ||
case SyntaxKind.Constructor: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,12 +107,14 @@ namespace ts { | |
scanner.setTextPos(pos); | ||
while (pos < end) { | ||
const token = scanner.scan(); | ||
Debug.assert(token !== SyntaxKind.EndOfFileToken); // Else it would infinitely loop | ||
const textPos = scanner.getTextPos(); | ||
if (textPos <= end) { | ||
nodes.push(createNode(token, pos, textPos, this)); | ||
} | ||
pos = textPos; | ||
if (token === SyntaxKind.EndOfFileToken) { | ||
break; | ||
} | ||
} | ||
return pos; | ||
} | ||
|
@@ -1757,17 +1759,20 @@ namespace ts { | |
function getFormattingEditsAfterKeystroke(fileName: string, position: number, key: string, options: FormatCodeOptions | FormatCodeSettings): TextChange[] { | ||
const sourceFile = syntaxTreeCache.getCurrentSourceFile(fileName); | ||
const settings = toEditorSettings(options); | ||
if (key === "{") { | ||
return formatting.formatOnOpeningCurly(position, sourceFile, getRuleProvider(settings), settings); | ||
} | ||
else if (key === "}") { | ||
return formatting.formatOnClosingCurly(position, sourceFile, getRuleProvider(settings), settings); | ||
} | ||
else if (key === ";") { | ||
return formatting.formatOnSemicolon(position, sourceFile, getRuleProvider(settings), settings); | ||
} | ||
else if (key === "\n") { | ||
return formatting.formatOnEnter(position, sourceFile, getRuleProvider(settings), settings); | ||
|
||
if (!isInComment(sourceFile, position)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be worthwhile to test this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding a test to fourslash and will add an integration test on the managed side to make sure the experience works end-to-end. |
||
if (key === "{") { | ||
return formatting.formatOnOpeningCurly(position, sourceFile, getRuleProvider(settings), settings); | ||
} | ||
else if (key === "}") { | ||
return formatting.formatOnClosingCurly(position, sourceFile, getRuleProvider(settings), settings); | ||
} | ||
else if (key === ";") { | ||
return formatting.formatOnSemicolon(position, sourceFile, getRuleProvider(settings), settings); | ||
} | ||
else if (key === "\n") { | ||
return formatting.formatOnEnter(position, sourceFile, getRuleProvider(settings), settings); | ||
} | ||
} | ||
|
||
return []; | ||
|
@@ -1826,6 +1831,12 @@ namespace ts { | |
return true; | ||
} | ||
|
||
function getSpanOfEnclosingComment(fileName: string, position: number, onlyMultiLine: boolean) { | ||
const sourceFile = syntaxTreeCache.getCurrentSourceFile(fileName); | ||
const range = ts.formatting.getRangeOfEnclosingComment(sourceFile, position, onlyMultiLine); | ||
return range && createTextSpanFromRange(range); | ||
} | ||
|
||
function getTodoComments(fileName: string, descriptors: TodoCommentDescriptor[]): TodoComment[] { | ||
// Note: while getting todo comments seems like a syntactic operation, we actually | ||
// treat it as a semantic operation here. This is because we expect our host to call | ||
|
@@ -2050,6 +2061,7 @@ namespace ts { | |
getFormattingEditsAfterKeystroke, | ||
getDocCommentTemplateAtPosition, | ||
isValidBraceCompletionAtPosition, | ||
getSpanOfEnclosingComment, | ||
getCodeFixesAtPosition, | ||
getEmitOutput, | ||
getNonBoundSourceFile, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's special about jsx? Does it apply to tsx as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsxText
nodes are children ofJsxElement
s that are notJsxElements
themselves or curly-brace-delimited assignment expressions. In the following example, the underlined isJsxText
:We essentially treat them like string literals, though differences in the way they are delimited mean that
getLeadingCommentRanges
cannot handle them correctly without some information from the parser, whichgetLeadingCommentRanges
has been designed to avoid.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And that covers tsx as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.