multi-line comment formatting fix and handler #16385

aozgaa · 2017-06-09T00:20:46Z

Fixes an issue where indentation behavior in VS was poor in multi-line comments. Two changes:

in order for VS to recognize that it needs to add asterisks on newlines, the client needs to know that it is in a multi-line comment when a newline is typed. The new request provides precisely this information.
~~The level of indentation for lines in the middle of a multi-line comment corresponds to the position where the multi-line comment starts. ie:~~

function foo(){
    let x = 10;  /**
                  * some comment text
                  */

Instead*, we determine indentation in by looking at the first non-whitespace character on the previous line or start of the comment, whichever is later, and shifting one column left if there is an asterisk character. In the case of jsdoc-style comments, this result precisely coincides with the level of indentation of the start of the comment (provided the comment started on its own line). This behavior is consistent with what Roslyn does in CSharp.

* The original strategy was problematic because finding the number of columns to displace for the opening comment marker would require us to determine how many columns non-whitespace characters take up. That is, we would need to start splitting source text into graphemes, which we haven't done up to this point. To handle this, we would either need to (a) add a runtime dependency or (b) implement it ourselves. Since we don't currently have runtime dependencies, we are biased towards (b). But the added complexity of handling graphemes correctly is not worthwhile for what appears to be a rare use-case.

EDIT (8/16/2017): re-ordered items, and changed description of indentation strategy.

…Script into isInMultiLineComment

amcasey · 2017-06-09T00:31:01Z

src/harness/fourslash.ts

@@ -2500,6 +2500,16 @@ namespace FourSlash {
            }
        }

+        public verifyisInMultiLineCommentAtPosition(negative: boolean) {


"verifyIs"?

amcasey · 2017-06-09T00:32:15Z

src/harness/fourslash.ts

+            const fileName = this.activeFile.fileName;
+            const actual = this.languageService.getisInMultiLineCommentAtPosition(fileName, position);
+            if (expected !== actual) {
+                this.raiseError(`verifyIsInDocComment failed: at position '${position}' in '${fileName}', expected '${expected}'.`);


Copy-paste error?

amcasey · 2017-06-09T00:32:37Z

src/harness/fourslash.ts

+            const expected = !negative;
+            const position = this.currentCaretPosition;
+            const fileName = this.activeFile.fileName;
+            const actual = this.languageService.getisInMultiLineCommentAtPosition(fileName, position);


"getis" -> "is"?

e.g. isValidBraceCompletion

amcasey · 2017-06-09T00:39:25Z

src/services/formatting/formatting.ts

@@ -1117,6 +1117,27 @@ namespace ts.formatting {
        }
    }

+    /**
+     * @returns -1 iff the position is not in a multi-line comment.


It's common to simply say "a negative value" rather than "-1" to retain flexibility for future refactoring.

Also, what does it return if it is in a multi-line comment?

amcasey · 2017-06-09T00:40:29Z

src/services/formatting/formatting.ts

+        if (leadingCommentRanges) {
+            loop: for (const range of leadingCommentRanges) {
+                // We need to extend the range when in an unclosed multi-line comment.
+                if (range.pos < position && (position < range.end || position === range.end && position === sourceFile.getFullWidth())) {


Does range.pos < position mean that the opening slash doesn't count?

Is that important?

amcasey · 2017-06-09T00:49:05Z

src/services/formatting/smartIndenter.ts

                return 0;
            }

            const lineAtPosition = sourceFile.getLineAndCharacterOfPosition(position).line;

+            const indentationOfEnclosingMultiLineComment = getIndentationOfEnclosingMultiLineComment(sourceFile, position);
+            if (indentationOfEnclosingMultiLineComment !== -1) {


amcasey · 2017-06-09T00:51:29Z

src/services/shims.ts

@@ -835,6 +840,14 @@ namespace ts {
            );
        }

+        /// GET IS IN MULTI-LINE COMMENT


This comment isn't obviously helpful.

amcasey · 2017-06-09T00:53:01Z

tests/cases/fourslash/isInMultiLineComment.ts

+    verify.isInMultiLineCommentAtPosition();
+}
+
+for (let i = 0; i < 2; ++i) {


I think the loop makes this harder to read.

amcasey · 2017-06-09T00:55:40Z

tests/cases/fourslash/isInMultiLineComment.ts

@@ -0,0 +1,36 @@
+/// <reference path="fourslash.ts" />
+
+//// /* x */


Would it be interesting to test a comment that spans multiple lines?

amcasey · 2017-06-09T00:57:04Z

src/services/formatting/formatting.ts

+     * @returns -1 iff the position is not in a multi-line comment.
+     */
+    export function getIndentationOfEnclosingMultiLineComment(sourceFile: SourceFile, position: number): number {
+        const token = getTokenAtPosition(sourceFile, position, /*includeJsDocComment*/ false);


Is there a test that is affected by the value of includeJsDocComment?

billti · 2017-06-28T21:18:51Z

src/services/formatting/formatting.ts

+        const leadingCommentRangesOfNextToken = getLeadingCommentRangesOfNode(getTokenAtPosition(sourceFile, position, /*includeJsDocComment*/ false), sourceFile);
+        const commentRanges = trailingRangesOfPreviousToken && leadingCommentRangesOfNextToken ?
+            trailingRangesOfPreviousToken.concat(leadingCommentRangesOfNextToken) :
+            trailingRangesOfPreviousToken || leadingCommentRangesOfNextToken;


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.

Added above.

billti · 2017-06-28T21:28:40Z

src/services/formatting/smartIndenter.ts

                return 0;
            }

            const lineAtPosition = sourceFile.getLineAndCharacterOfPosition(position).line;

+            const indentationOfEnclosingMultiLineComment = getIndentationOfEnclosingMultiLineComment(sourceFile, position, options);


utilities.ts already exports a brief isInComment function. It it worth enclosing this block in an if check on that to make it clearer?

aozgaa · 2017-08-07T23:04:21Z

This has been updated after sitting here for a while. Thoughts, @billti , @amcasey ?

billti · 2017-08-08T00:01:19Z

src/services/formatting/formatting.ts

+            const commentStart = range.pos;
+            const commentLineStart = getLineStartPositionForPosition(commentStart, sourceFile);
+            const { column, character } = SmartIndenter.findFirstNonWhitespaceCharacterAndColumn(commentLineStart, commentStart, sourceFile, options);
+            return column + /*length after whitespace ends*/ range.pos - (commentLineStart + character);


Can you clarify what the above logic is doing? Its a lot more involved than I would have expected, and I can't figure out the reason for some of it.

billti · 2017-08-08T00:11:18Z

Minor comment to clarify some logic, and make sure the builds/checks are passing before checking in. Thanks!

amcasey

Some question inline.

amcasey · 2017-06-26T21:20:23Z

src/services/formatting/formatting.ts

-            loop: for (const range of leadingCommentRanges) {
+    export function getIndentationOfEnclosingMultiLineComment(sourceFile: SourceFile, position: number, options: EditorSettings): number {
+        const range = getRangeOfEnclosingComment(sourceFile, position, /*onlyMultiLine*/ true);
+        if (range) {


Nit: if you return immediately after determining that range is undefined, the rest of the method reads as straight-line code.

Just in-lined the helper since it's only called once. Additionally, the logic has changed because I discovered an issue with determining the columns when we have text which is non-whitespace (because determining the number of graphemes in a string is non-trivial).

amcasey · 2017-08-09T00:49:08Z

src/harness/fourslash.ts

@@ -2508,6 +2508,20 @@ namespace FourSlash {
            }
        }

+        public verifySpanOfEnclosingComment(negative: boolean, onlyMultiLine: boolean) {


Why not just pass expected?

Methods in VerifyNegatable often call into the TestState by passing in negative. I'm merely following that idiom.

amcasey · 2017-08-09T00:54:22Z

tests/cases/fourslash/isInMultiLineComment.ts

+
+goTo.position(singleLineCommentStart + 1);
+verify.not.isInCommentAtPosition(/*onlyMultiLine*/ true);
+verify.isInCommentAtPosition(/*onlyMultiLine*/ false);


Presumably, the result for false matches the result for true in every case but this one?

Yes. I thought it felt a bit heavy-handed to query each case in the test that is checked in, but I'm open to checking more thoroughly if it seems worthwhile.

Maybe isInCommentAtPosition could check both unless either true or false is specified?

amcasey · 2017-08-09T00:59:16Z

src/server/protocol.ts

+        /**
+         * Requires that the enclosing span be a multi-line comment, or else the request returns undefined.
+         */
+        onlyMultiLine: boolean;


Is this just here for the generality of the API? We don't appear to actually use it.

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

I have mixed feeling about this but don't object as long as we have test coverage (which we do).

amcasey · 2017-08-09T20:12:59Z

src/services/formatting/formatting.ts

+    }
+
+    export function getRangeOfEnclosingComment(sourceFile: SourceFile, position: number, onlyMultiLine: boolean): CommentRange | undefined {
+        // Considering a fixed position,


Is this an arbitrary position or is it assumed to be at a token boundary?

amcasey · 2017-08-09T20:14:01Z

src/services/formatting/formatting.ts

+        // tokens contain all comments in a sourcefile disjointly.
+        const precedingToken = findPrecedingToken(position, sourceFile);
+        const trailingRangesOfPreviousToken = precedingToken && getTrailingCommentRanges(sourceFile.text, precedingToken.end);
+        const leadingCommentRangesOfNextToken = getLeadingCommentRangesOfNode(getTokenAtPosition(sourceFile, position, /*includeJsDocComment*/ false), sourceFile);


What is the impact of /*includeJsDocComment*/ false? Is there a test that depends on the value?

amcasey · 2017-08-09T20:16:00Z

src/services/formatting/formatting.ts

+            for (const range of commentRanges) {
+                // We need to extend the range when in an unclosed multi-line comment.
+                if (range.pos < position && position < range.end ||
+                    position === range.end && (range.kind === SyntaxKind.SingleLineCommentTrivia || position === sourceFile.getFullWidth())) {


What is sourceFile.getFullWidth()?

amcasey · 2017-08-09T20:17:42Z

src/services/formatting/formatting.ts

+            trailingRangesOfPreviousToken || leadingCommentRangesOfNextToken;
+        if (commentRanges) {
+            for (const range of commentRanges) {
+                // We need to extend the range when in an unclosed multi-line comment.


How does the check below detect unclosed multi-line comments?

amcasey · 2017-08-09T20:19:42Z

src/services/services.ts

-            else if (key === "\n") {
-                return formatting.formatOnEnter(position, sourceFile, getRuleProvider(settings), settings);
+
+            if (!isInComment(sourceFile, position)) {


Would it be worthwhile to test this change?

Adding a test to fourslash and will add an integration test on the managed side to make sure the experience works end-to-end.

billti · 2017-08-15T19:52:03Z

src/services/utilities.ts

+                // we need to find the last token in a previous child.
+                // 2) `position` is within the same span: we recurse on `child`.
+                // * JsxText is exceptional in that its tokens are (non-trivia) whitespace, which we do not want to return.
+                // TODO(arozga): shouldn't `findRightmost...` need to handle JsxText?


This comment seems like something you should do before check-in - especially as you have zero test-cases covering JSX.

Addressed and tests have been added.

billti

Please add more tests. You have one test file with static text (i.e. no editing occurring) and a well-formed comment. Please ensure behavior in unclosed comments, nested comments, etc.

amcasey · 2017-08-16T00:48:30Z

src/services/formatting/formatting.ts

+        sourceFile: SourceFile,
+        position: number,
+        onlyMultiLine: boolean,
+        precedingToken: Node | null | undefined = findPrecedingToken(position, sourceFile), // tslint:disable-line:no-null-keyword


findPrecedingToken || null?

It shouldn't matter in what follows, as the null value is only used to prevent the default initializer from being evaluated.

* When in a multi-line comment, we would have liked to use the start of the comment as a reference point for the indentation inside the comment, but determining the number of columns shifted for the comment start woudl require determining the length w/r/t graphemes, which we do not currently implement. We would like to avoid taking on a runtime dependency on a grapheme-parsing library. Instead, we look at the indentation level on the previoud line or start of the comment as a reference point, and correct shift for lines starting with an asterisk.

aozgaa · 2017-08-17T02:35:44Z

@amcasey and @billti , changes have been addressed, can you take another look?

@RyanCavanaugh , can you comment on whether the treatment of JsxText in src/compiler/utilities.ts and src/services/utilities.ts is sane?

@uniqueiniquity , thoughts?

amcasey · 2017-08-17T18:22:37Z

src/compiler/utilities.ts

@@ -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;


What's special about jsx? Does it apply to tsx as well?

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.

And that covers tsx as well?

amcasey · 2017-08-17T20:01:50Z

src/services/formatting/formatting.ts

+        // 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);


We should probably short-circuit if position is within tokenAtPosition.

amcasey · 2017-08-17T20:03:14Z

src/services/formatting/formatting.ts

+        precedingToken: Node | null | undefined = findPrecedingToken(position, sourceFile), // tslint:disable-line:no-null-keyword
+        tokenAtPosition = getTokenAtPosition(sourceFile, position, /*includeJsDocComment*/ false),
+        predicate?: (c: CommentRange) => boolean): CommentRange | undefined {
+        // Considering a fixed position,


I would simplify this comment to something like

// Between two consecutive tokens, all comments are either trailing on the former or leading on the latter (and none are in both lists).

amcasey

LGTM, modulo minor comments

billti

That's a lot of work to put an asterisk at the right indentation :-o Thanks!

aozgaa · 2017-08-18T19:43:34Z

All PR's from the last day appear to be failing Travis with node 4 due to a gulp-typescript dependency. See ivogabe/gulp-typescript#536 for the fix.

Going to ignore this and go ahead and merge.

aozgaa added 4 commits June 7, 2017 14:01

temp

35b16ab

Merge branch 'isInMultiLineComment' of https://github.com/aozgaa/Type…

6b4cd0b

…Script into isInMultiLineComment

isInMultiLineComment

a819e4e

indent block comments according to first line

8f28a02

aozgaa requested a review from mhegazy June 9, 2017 00:20

msftclas added the cla-not-required label Jun 9, 2017

amcasey suggested changes Jun 9, 2017

View reviewed changes

aozgaa added 3 commits June 9, 2017 14:43

make indent work with trailing comments

b02963b

request returns span

8fc3fd9

fix offsetting and tests

cc88091

billti reviewed Jun 28, 2017

View reviewed changes

Arthur Ozga and others added 4 commits August 4, 2017 14:26

Merge branch 'master' into isInMultiLineComment

efdbeba

implementation comment

777bc57

supressFormatOnKeyInComments

091376f

Merge branch 'master' into isInMultiLineComment

2f5b1d3

billti reviewed Aug 8, 2017

View reviewed changes

billti approved these changes Aug 8, 2017

View reviewed changes

Merge branch 'master' into isInMultiLineComment

7c402d5

amcasey reviewed Aug 9, 2017

View reviewed changes

aozgaa added 7 commits August 10, 2017 10:01

fix end-of-file assert failure

de92e98

cleanup

b2188ad

findPrevious changes

472ad9d

findPrecedingToken handles EOF child more gracefully

f3e0cbb

dont compute preceding token twice

a209db7

consolidate isInComment and getRangeOfEnclosingComment

a08d18a

add test

ad9c29b

billti reviewed Aug 15, 2017

View reviewed changes

billti suggested changes Aug 15, 2017

View reviewed changes

amcasey reviewed Aug 16, 2017

View reviewed changes

aozgaa added 9 commits August 16, 2017 15:28

JsxText has no leading comments

153b94a

update test

70e4f34

rename tests

4b9f5a0

add tests

62f16be

clarify JsxText handling

b7bc7d8

Merge branch 'master' into isInMultiLineComment

19e2fa6

cleanup

6029b5c

test if onlyMultiLine flag changes answer

760ef44

amcasey reviewed Aug 17, 2017

View reviewed changes

amcasey approved these changes Aug 17, 2017

View reviewed changes

billti approved these changes Aug 17, 2017

View reviewed changes

aozgaa added 2 commits August 17, 2017 20:06

remove duplicate verify

e7d2af0

respond to comments

e4e969a

aozgaa merged commit a3d113b into microsoft:master Aug 18, 2017

uniqueiniquity mentioned this pull request Aug 18, 2017

Correct outlining spans for object and array literals in array #17709

Merged

ghost mentioned this pull request Dec 11, 2017

Debug Failure. False expression. in findPrecedingToken during getQuickInfoAtPosition #20560

Closed

ghost mentioned this pull request Jan 22, 2018

Debug Failure. False expression: JsxText tokens should not be the first child of JsxElement | JsxSelfClosingElement in findRightmostChildNodeWithTokens during getQuickInfoAtPosition #20788

Closed

microsoft locked and limited conversation to collaborators Jun 14, 2018

		@@ -0,0 +1,36 @@
		/// <reference path="fourslash.ts" />

		//// /* x */

multi-line comment formatting fix and handler #16385

multi-line comment formatting fix and handler #16385

Conversation

aozgaa commented Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aozgaa commented Aug 7, 2017

Choose a reason for hiding this comment

billti commented Aug 8, 2017

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aozgaa Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aozgaa commented Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

billti left a comment

Choose a reason for hiding this comment

aozgaa commented Aug 18, 2017

aozgaa commented Jun 9, 2017 •

edited

Loading

aozgaa Aug 17, 2017 •

edited

Loading

aozgaa commented Aug 17, 2017 •

edited

Loading