Skip to content

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

Merged
merged 30 commits into from
Aug 18, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
35b16ab
temp
aozgaa Jun 7, 2017
6b4cd0b
Merge branch 'isInMultiLineComment' of https://github.com/aozgaa/Type…
aozgaa Jun 7, 2017
a819e4e
isInMultiLineComment
aozgaa Jun 7, 2017
8f28a02
indent block comments according to first line
aozgaa Jun 9, 2017
b02963b
make indent work with trailing comments
aozgaa Jun 9, 2017
8fc3fd9
request returns span
aozgaa Jun 10, 2017
cc88091
fix offsetting and tests
aozgaa Jun 10, 2017
efdbeba
Merge branch 'master' into isInMultiLineComment
Aug 4, 2017
777bc57
implementation comment
Aug 4, 2017
091376f
supressFormatOnKeyInComments
aozgaa Aug 7, 2017
2f5b1d3
Merge branch 'master' into isInMultiLineComment
aozgaa Aug 7, 2017
7c402d5
Merge branch 'master' into isInMultiLineComment
aozgaa Aug 9, 2017
de92e98
fix end-of-file assert failure
aozgaa Aug 10, 2017
b2188ad
cleanup
aozgaa Aug 14, 2017
472ad9d
findPrevious changes
aozgaa Aug 14, 2017
f3e0cbb
`findPrecedingToken` handles EOF child more gracefully
aozgaa Aug 15, 2017
a209db7
dont compute preceding token twice
aozgaa Aug 15, 2017
a08d18a
consolidate isInComment and getRangeOfEnclosingComment
aozgaa Aug 15, 2017
ad9c29b
add test
aozgaa Aug 15, 2017
153b94a
`JsxText` has no leading comments
aozgaa Aug 16, 2017
70e4f34
update test
aozgaa Aug 17, 2017
4b9f5a0
rename tests
aozgaa Aug 17, 2017
62f16be
add tests
aozgaa Aug 17, 2017
23ca368
Use simpler indentation for comments
aozgaa Aug 17, 2017
b7bc7d8
clarify `JsxText` handling
aozgaa Aug 17, 2017
19e2fa6
Merge branch 'master' into isInMultiLineComment
aozgaa Aug 17, 2017
6029b5c
cleanup
aozgaa Aug 17, 2017
760ef44
test if `onlyMultiLine` flag changes answer
aozgaa Aug 17, 2017
e7d2af0
remove duplicate verify
aozgaa Aug 18, 2017
e4e969a
respond to comments
aozgaa Aug 18, 2017
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
2 changes: 1 addition & 1 deletion src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,7 @@ namespace ts {
}

export function getLeadingCommentRangesOfNode(node: Node, sourceFileOfNode: SourceFile) {
return getLeadingCommentRanges(sourceFileOfNode.text, node.pos);
return node.kind !== SyntaxKind.JsxText ? getLeadingCommentRanges(sourceFileOfNode.text, node.pos) : undefined;
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JsxText nodes are children of JsxElements that are not JsxElements themselves or curly-brace-delimited assignment expressions. In the following example, the underlined is JsxText:

<div>Click me!<h1/></div>
     ~~~~~~~~~

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, which getLeadingCommentRanges has been designed to avoid.

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

}

export function getJSDocCommentRanges(node: Node, text: string) {
Expand Down
21 changes: 21 additions & 0 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2508,6 +2508,23 @@ namespace FourSlash {
}
}

public verifySpanOfEnclosingComment(negative: boolean, onlyMultiLineDiverges?: boolean) {
const expected = !negative;
const position = this.currentCaretPosition;
const fileName = this.activeFile.fileName;
const actual = !!this.languageService.getSpanOfEnclosingComment(fileName, position, /*onlyMultiLine*/ false);
const actualOnlyMultiLine = !!this.languageService.getSpanOfEnclosingComment(fileName, position, /*onlyMultiLine*/ true);
if (expected !== actual || onlyMultiLineDiverges === (actual === actualOnlyMultiLine)) {
this.raiseError(`verifySpanOfEnclosingComment failed:
position: '${position}'
fileName: '${fileName}'
onlyMultiLineDiverges: '${onlyMultiLineDiverges}'
actual: '${actual}'
actualOnlyMultiLine: '${actualOnlyMultiLine}'
expected: '${expected}'.`);
}
}

/*
Check number of navigationItems which match both searchValue and matchKind,
if a filename is passed in, limit the results to that file.
Expand Down Expand Up @@ -3648,6 +3665,10 @@ namespace FourSlashInterface {
this.state.verifyBraceCompletionAtPosition(this.negative, openingBrace);
}

public isInCommentAtPosition(onlyMultiLineDiverges?: boolean) {
this.state.verifySpanOfEnclosingComment(this.negative, onlyMultiLineDiverges);
}

public codeFixAvailable() {
this.state.verifyCodeFixAvailable(this.negative);
}
Expand Down
3 changes: 3 additions & 0 deletions src/harness/harnessLanguageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,9 @@ namespace Harness.LanguageService {
isValidBraceCompletionAtPosition(fileName: string, position: number, openingBrace: number): boolean {
return unwrapJSONCallResult(this.shim.isValidBraceCompletionAtPosition(fileName, position, openingBrace));
}
getSpanOfEnclosingComment(fileName: string, position: number, onlyMultiLine: boolean): ts.TextSpan {
return unwrapJSONCallResult(this.shim.getSpanOfEnclosingComment(fileName, position, onlyMultiLine));
}
getCodeFixesAtPosition(): ts.CodeAction[] {
throw new Error("Not supported on the shim.");
}
Expand Down
4 changes: 4 additions & 0 deletions src/server/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,10 @@ namespace ts.server {
return notImplemented();
}

getSpanOfEnclosingComment(_fileName: string, _position: number, _onlyMultiLine: boolean): TextSpan {
return notImplemented();
}

getCodeFixesAtPosition(file: string, start: number, end: number, errorCodes: number[]): CodeAction[] {
const args: protocol.CodeFixRequestArgs = { ...this.createFileRangeRequestArgs(file, start, end), errorCodes };

Expand Down
16 changes: 16 additions & 0 deletions src/server/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ namespace ts.server.protocol {
/* @internal */
BraceFull = "brace-full",
BraceCompletion = "braceCompletion",
GetSpanOfEnclosingComment = "getSpanOfEnclosingComment",
Change = "change",
Close = "close",
Completions = "completions",
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only ever pass in onlyMultiLine: true right now with the proposed change in the VS extension.

Copy link
Member

Choose a reason for hiding this comment

The 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.
*/
Expand Down
11 changes: 11 additions & 0 deletions src/server/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1025,6 +1025,14 @@ namespace ts.server {
return project.getLanguageService(/*ensureSynchronized*/ false).getDocCommentTemplateAtPosition(file, position);
}

private getSpanOfEnclosingComment(args: protocol.SpanOfEnclosingCommentRequestArgs) {
const { file, project } = this.getFileAndProjectWithoutRefreshingInferredProjects(args);
const scriptInfo = project.getScriptInfoForNormalizedPath(file);
const onlyMultiLine = args.onlyMultiLine;
const position = this.getPosition(args, scriptInfo);
return project.getLanguageService(/*ensureSynchronized*/ false).getSpanOfEnclosingComment(file, position, onlyMultiLine);
}

private getIndentation(args: protocol.IndentationRequestArgs) {
const { file, project } = this.getFileAndProjectWithoutRefreshingInferredProjects(args);
const position = this.getPosition(args, project.getScriptInfoForNormalizedPath(file));
Expand Down Expand Up @@ -1765,6 +1773,9 @@ namespace ts.server {
[CommandNames.DocCommentTemplate]: (request: protocol.DocCommentTemplateRequest) => {
return this.requiredResponse(this.getDocCommentTemplate(request.arguments));
},
[CommandNames.GetSpanOfEnclosingComment]: (request: protocol.SpanOfEnclosingCommentRequest) => {
return this.requiredResponse(this.getSpanOfEnclosingComment(request.arguments));
},
[CommandNames.Format]: (request: protocol.FormatRequest) => {
return this.requiredResponse(this.getFormattingEditsForRange(request.arguments));
},
Expand Down
50 changes: 50 additions & 0 deletions src/services/formatting/formatting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1152,6 +1152,56 @@ 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, // tslint:disable-line:no-null-keyword
tokenAtPosition = getTokenAtPosition(sourceFile, position, /*includeJsDocComment*/ false),
predicate?: (c: CommentRange) => boolean): CommentRange | undefined {
const tokenStart = tokenAtPosition.getStart(sourceFile);
if (tokenStart <= position && position < tokenAtPosition.getEnd()) {
return undefined;
}

if (precedingToken === undefined) {
precedingToken = findPrecedingToken(position, sourceFile);
}

// Between two consecutive tokens, all comments are either trailing on the former
// or leading on the latter (and none are in both lists).
const trailingRangesOfPreviousToken = precedingToken && getTrailingCommentRanges(sourceFile.text, precedingToken.end);
const leadingCommentRangesOfNextToken = getLeadingCommentRangesOfNode(tokenAtPosition, sourceFile);
const commentRanges = trailingRangesOfPreviousToken && leadingCommentRangesOfNextToken ?
trailingRangesOfPreviousToken.concat(leadingCommentRangesOfNextToken) :
trailingRangesOfPreviousToken || leadingCommentRangesOfNextToken;
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:
Expand Down
39 changes: 31 additions & 8 deletions src/services/formatting/smartIndenter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,36 @@ namespace ts.formatting {
}

const precedingToken = findPrecedingToken(position, sourceFile);

const enclosingCommentRange = getRangeOfEnclosingComment(sourceFile, position, /*onlyMultiLine*/ true, precedingToken || null); // tslint:disable-line:no-null-keyword
if (enclosingCommentRange) {
const previousLine = getLineAndCharacterOfPosition(sourceFile, position).line - 1;
const commentStartLine = getLineAndCharacterOfPosition(sourceFile, enclosingCommentRange.pos).line;

Debug.assert(commentStartLine >= 0);

if (previousLine <= commentStartLine) {
return findFirstNonWhitespaceColumn(getStartPositionOfLine(commentStartLine, sourceFile), position, sourceFile, options);
}

const startPostionOfLine = getStartPositionOfLine(previousLine, sourceFile);
const { column, character } = findFirstNonWhitespaceCharacterAndColumn(startPostionOfLine, position, sourceFile, options);

if (column === 0) {
return column;
}

const firstNonWhitespaceCharacterCode = sourceFile.text.charCodeAt(startPostionOfLine + character);
return firstNonWhitespaceCharacterCode === CharacterCodes.asterisk ? column - 1 : column;
}

if (!precedingToken) {
return getBaseIndentation(options);
}

// no indentation in string \regex\template literals
const precedingTokenIsLiteral = isStringOrRegularExpressionOrTemplateLiteral(precedingToken.kind);
if (precedingTokenIsLiteral && precedingToken.getStart(sourceFile) <= position && precedingToken.end > position) {
if (precedingTokenIsLiteral && precedingToken.getStart(sourceFile) <= position && position < precedingToken.end) {
return 0;
}

Expand Down Expand Up @@ -405,13 +428,13 @@ namespace ts.formatting {
return findFirstNonWhitespaceColumn(lineStart, lineStart + lineAndCharacter.character, sourceFile, options);
}

/*
Character is the actual index of the character since the beginning of the line.
Column - position of the character after expanding tabs to spaces
"0\t2$"
value of 'character' for '$' is 3
value of 'column' for '$' is 6 (assuming that tab size is 4)
*/
/**
* Character is the actual index of the character since the beginning of the line.
* Column - position of the character after expanding tabs to spaces.
* "0\t2$"
* value of 'character' for '$' is 3
* value of 'column' for '$' is 6 (assuming that tab size is 4)
*/
export function findFirstNonWhitespaceCharacterAndColumn(startPos: number, endPos: number, sourceFile: SourceFileLike, options: EditorSettings) {
let character = 0;
let column = 0;
Expand Down
36 changes: 24 additions & 12 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worthwhile to test this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 [];
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -2050,6 +2061,7 @@ namespace ts {
getFormattingEditsAfterKeystroke,
getDocCommentTemplateAtPosition,
isValidBraceCompletionAtPosition,
getSpanOfEnclosingComment,
getCodeFixesAtPosition,
getEmitOutput,
getNonBoundSourceFile,
Expand Down
12 changes: 12 additions & 0 deletions src/services/shims.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,11 @@ namespace ts {
*/
isValidBraceCompletionAtPosition(fileName: string, position: number, openingBrace: number): string;

/**
* Returns a JSON-encoded TextSpan | undefined indicating the range of the enclosing comment, if it exists.
*/
getSpanOfEnclosingComment(fileName: string, position: number, onlyMultiLine: boolean): string;

getEmitOutput(fileName: string): string;
getEmitOutputObject(fileName: string): EmitOutput;
}
Expand Down Expand Up @@ -815,6 +820,13 @@ namespace ts {
);
}

public getSpanOfEnclosingComment(fileName: string, position: number, onlyMultiLine: boolean): string {
return this.forwardJSONCall(
`getSpanOfEnclosingComment('${fileName}', ${position})`,
() => this.languageService.getSpanOfEnclosingComment(fileName, position, onlyMultiLine)
);
}

/// GET SMART INDENT
public getIndentationAtPosition(fileName: string, position: number, options: string /*Services.EditorOptions*/): string {
return this.forwardJSONCall(
Expand Down
2 changes: 2 additions & 0 deletions src/services/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,8 @@ namespace ts {

isValidBraceCompletionAtPosition(fileName: string, position: number, openingBrace: number): boolean;

getSpanOfEnclosingComment(fileName: string, position: number, onlyMultiLine: boolean): TextSpan;

getCodeFixesAtPosition(fileName: string, start: number, end: number, errorCodes: number[], formatOptions: FormatCodeSettings): CodeAction[];
getApplicableRefactors(fileName: string, positionOrRaneg: number | TextRange): ApplicableRefactorInfo[];
getEditsForRefactor(fileName: string, formatOptions: FormatCodeSettings, positionOrRange: number | TextRange, refactorName: string, actionName: string): RefactorEditInfo | undefined;
Expand Down
Loading