Fixed a formatting crash on comments inside blocks starting on first line#2793
Fixed a formatting crash on comments inside blocks starting on first line#2793Andarist wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a panic in the LSP formatter that occurred when formatting JavaScript/TypeScript files containing comments inside blocks that start on the first line (e.g., arrow function bodies). The panic was caused by passing a negative indentation value (-1) to strings.Repeat().
Changes:
- Added a check to prevent passing -1 indentation values to
indentTriviaItems - Added regression tests for both multiline JSDoc comments and single-line comments in the problematic scenario
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/format/span.go | Added guard to check if commentIndentation is -1 before calling indentTriviaItems, preventing the panic |
| internal/format/comment_test.go | Added two regression tests verifying that formatting doesn't panic for comments inside blocks starting on first line |
| indentNextTokenOrTrivia := true | ||
| if len(currentTokenInfo.leadingTrivia) > 0 { | ||
| commentIndentation := dynamicIndenation.getIndentationForComment(currentTokenInfo.token.Kind, tokenIndentation, container) | ||
| indentNextTokenOrTrivia = w.indentTriviaItems(currentTokenInfo.leadingTrivia, commentIndentation, indentNextTokenOrTrivia, func(item TextRangeWithKind) { | ||
| w.insertIndentation(item.Loc.Pos(), commentIndentation, false) | ||
| }) | ||
| if commentIndentation != -1 { | ||
| indentNextTokenOrTrivia = w.indentTriviaItems(currentTokenInfo.leadingTrivia, commentIndentation, indentNextTokenOrTrivia, func(item TextRangeWithKind) { | ||
| w.insertIndentation(item.Loc.Pos(), commentIndentation, false) | ||
| }) | ||
| } |
There was a problem hiding this comment.
But why? The original code said:
let indentNextTokenOrTrivia = true;
if (currentTokenInfo.leadingTrivia) {
const commentIndentation = dynamicIndentation.getIndentationForComment(currentTokenInfo.token.kind, tokenIndentation, container);
indentNextTokenOrTrivia = indentTriviaItems(currentTokenInfo.leadingTrivia, commentIndentation, indentNextTokenOrTrivia, item => insertIndentation(item.pos, commentIndentation, /*lineAdded*/ false));
}which to me means the bug is elsewhere
There was a problem hiding this comment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/format/comment_test.go:233
- This subtest only checks len(formatted) > 0, which will always be true since originalText is non-empty—even if the formatter returns no edits or later regresses indentation. Per the project’s bugfix testing guidance, please assert something that demonstrates the fix (e.g., that the JSDoc text is preserved and that the comment line is not moved to column 0 / remains indented).
edits := format.FormatDocument(ctx, sourceFile)
formatted := applyBulkEdits(originalText, edits)
assert.Check(t, len(formatted) > 0, "formatted text should not be empty")
})
| edits := format.FormatDocument(ctx, sourceFile) | ||
| formatted := applyBulkEdits(originalText, edits) | ||
| assert.Check(t, len(formatted) > 0, "formatted text should not be empty") | ||
| }) |
There was a problem hiding this comment.
This subtest also uses only len(formatted) > 0, which is effectively a no-op because originalText is non-empty. Please add an assertion that demonstrates the intended behavior (e.g., the single-line comment remains indented and is not reformatted to column 0), so the test will catch regressions beyond “did not panic”.
This issue also appears on line 230 of the same file.
| commentIndentation := dynamicIndenation.getIndentationForComment(currentTokenInfo.token.Kind, tokenIndentation, container) | ||
| indentNextTokenOrTrivia = w.indentTriviaItems(currentTokenInfo.leadingTrivia, commentIndentation, indentNextTokenOrTrivia, func(item TextRangeWithKind) { | ||
| w.insertIndentation(item.Loc.Pos(), commentIndentation, false) | ||
| }) | ||
| if commentIndentation != -1 { | ||
| indentNextTokenOrTrivia = w.indentTriviaItems(currentTokenInfo.leadingTrivia, commentIndentation, indentNextTokenOrTrivia, func(item TextRangeWithKind) { | ||
| w.insertIndentation(item.Loc.Pos(), commentIndentation, false) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The new guard avoids passing the sentinel indentation (-1) into indent/insertIndentation (and thus avoids the strings.Repeat panic), but it also means leading comments won’t be indented at all in this scenario. The underlying source of commentIndentation == -1 appears to be that computeIndentation can return w.indentationOnLastIndentedLine (initialized to -1) when startLine == w.lastIndentedLine (which is never initialized and defaults to 0), which is exactly the “block opens on first line” case. Consider initializing w.lastIndentedLine to -1 alongside indentationOnLastIndentedLine (or otherwise ensuring indentation is never computed as -1 for in-range tokens), so comment indentation can be computed rather than skipped.
3 participants
fixes #2649