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

uniqueiniquity · 2017-08-09T23:15:18Z

This change causes outlining regions for object and array literals in an array to no longer extend to the previous member of the array. This has the effect of no longer permitting single-line object or array literals to be folded.

msftclas · 2017-08-09T23:15:22Z

@uniqueiniquity,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

amcasey · 2017-08-10T00:22:06Z

tests/cases/fourslash/getOutliningForObjectsInArray.ts

@@ -0,0 +1,23 @@
+/// <reference path="fourslash.ts"/>
+
+////// objects in x should generate outlining spans that do not render in VS


Is this validated somehow? Also did this test fail before your change?

The particular point about the spans not rendering in VS is not validated, but the placement of the spans is. This test would have failed before my change.

amcasey · 2017-08-10T00:26:59Z

src/services/outliningElementsCollector.ts

+        function addOutliningForObjectLiteralsInArray(startElement: Node, endElement: Node, autoCollapse: boolean) {
+            if (startElement && endElement) {
+                const span: OutliningSpan = {
+                    textSpan: createTextSpanFromBounds(startElement.getStart(), endElement.end),


Extract a local for this repeated expression?

amcasey · 2017-08-10T00:35:45Z

src/services/outliningElementsCollector.ts

@@ -161,6 +173,10 @@ namespace ts.OutliningElementsCollector {
                case SyntaxKind.CaseBlock: {
                    const openBrace = findChildOfKind(n, SyntaxKind.OpenBraceToken, sourceFile);
                    const closeBrace = findChildOfKind(n, SyntaxKind.CloseBraceToken, sourceFile);
+                    if (n.kind === SyntaxKind.ObjectLiteralExpression && n.parent.kind === SyntaxKind.ArrayLiteralExpression) {


I'm afraid I don't understand why this case is special. It looks like the main things we're doing differently are calling getStart instead of pos and computing our own hintSpan. Why not make those changes to the general case? If this case is indeed different from the general case, why aren't there related cases (nested arrays, for example)?

amcasey · 2017-08-11T17:37:22Z

src/services/outliningElementsCollector.ts

-            if (startElement && endElement) {
-                const span: OutliningSpan = {
-                    textSpan: createTextSpanFromBounds(startElement.getStart(), endElement.end),
+                    textSpan: createTextSpanFromBounds(fullStart ? startElement.pos : startElement.getStart(), endElement.end),
                    hintSpan: createTextSpanFromBounds(startElement.getStart(), endElement.end),


Should fullStart (which I might call "useFullStart) apply to the hint span as well?

I don't think it should apply to the hint span, since it has always used the actual start (as opposed to the full start, which the textSpan previously always used).

amcasey · 2017-08-11T17:38:20Z

tests/cases/fourslash/getOutliningForObjectsInArray.ts

@@ -19,5 +19,24 @@
 ////         c: 2
 ////     }|]
 //// ]|];
+////
+////// same behavior for nested arrays


Would it be interesting to test an object in an array in an array?

amcasey

LGTM, modulo comments

aozgaa

Same questions as Andrew. In particular, could you come up with a succinct description for which parent-child matchings (eg: array literal containing object literal entry) the "next-line" phenomenon occurs?

aozgaa · 2017-08-11T21:09:13Z

tests/cases/fourslash/getOutliningForObjectsInArray.ts

@@ -0,0 +1,23 @@
+/// <reference path="fourslash.ts"/>
+
+////// objects in x should generate outlining spans that do not render in VS


Unless you want the comment to be part of the sourcefile, you can just use two slashes.

aozgaa · 2017-08-11T21:14:59Z

src/services/outliningElementsCollector.ts

@@ -16,6 +16,18 @@ namespace ts.OutliningElementsCollector {
            }
        }



Can you move the body of collectElements above to precede local functions? It's not strictly-speaking necessary for this change, but makes reading collectElements easier and is consistent with defining local functions last elsewhere.

uniqueiniquity · 2017-08-14T16:29:19Z

@aozgaa I added some comments characterizing the cases for which the change is needed, as well as explaining the new flag in addOutliningComments.

billti

Thanks!

aozgaa · 2017-08-17T23:39:14Z

src/services/outliningElementsCollector.ts

@@ -3,12 +3,18 @@ namespace ts.OutliningElementsCollector {
    export function collectElements(sourceFile: SourceFile, cancellationToken: CancellationToken): OutliningSpan[] {
        const elements: OutliningSpan[] = [];
        const collapseText = "...";
+        let depth = 0;
+        const maxDepth = 20;


Nit: you could move this outside the function body (don't export).

aozgaa

Use some internal helpers, but otherwise just grammatical and stylistic nit's. LGTM, thanks!

aozgaa · 2017-08-17T23:40:09Z

src/services/outliningElementsCollector.ts


-        function addOutliningSpan(hintSpanNode: Node, startElement: Node, endElement: Node, autoCollapse: boolean) {
+        // If useFullStart is true, then the collapsing span includes leading whitespace, including linebreaks.


Nit: make a JsDoc comment so the description shows up as part of quickinfo.

aozgaa · 2017-08-17T23:41:52Z

src/services/outliningElementsCollector.ts

-                    textSpan: createTextSpanFromBounds(startElement.pos, endElement.end),
-                    hintSpan: createTextSpanFromNode(hintSpanNode, sourceFile),
+                    textSpan: createTextSpanFromBounds(useFullStart ? startElement.pos : startElement.getStart(), endElement.end),
+                    hintSpan: createTextSpanFromBounds(startElement.getStart(), endElement.end),


use createTextSpanFromNode(...).

aozgaa · 2017-08-17T23:43:55Z

src/services/outliningElementsCollector.ts

            if (hintSpanNode && startElement && endElement) {
                const span: OutliningSpan = {
-                    textSpan: createTextSpanFromBounds(startElement.pos, endElement.end),
-                    hintSpan: createTextSpanFromNode(hintSpanNode, sourceFile),
+                    textSpan: createTextSpanFromBounds(useFullStart ? startElement.pos : startElement.getStart(), endElement.end),


Consider also calling createTextSpanFromNode and adding a boolean parameter to use full start that defaults to false.

Regardless of whether you do it inline here or by modifying the helper, use startElement.getFullStart(...) to get the full start.

I can't use createTextSpanFromNode everywhere because in most places, the start node and end node are different.
I'll change it back in the one relevant place though.

Also changing all .pos to .getFullStart (and similar for .end).

aozgaa · 2017-08-17T23:48:58Z

src/services/outliningElementsCollector.ts

                    break;
                }
+                // If the block has no leading keywords and is a member of an array literal,
+                // we again want to only collapse the span of the block.


Nit: Perhaps we again want to only should be we only want to

aozgaa · 2017-08-17T23:50:05Z

src/services/outliningElementsCollector.ts

                    break;
                }
+                // If the block has no leading keywords and is a member of an array literal,


Nit: and is a member of could be and is inside.

aozgaa · 2017-08-17T23:50:50Z

src/services/outliningElementsCollector.ts

@@ -82,8 +88,6 @@ namespace ts.OutliningElementsCollector {
            return isFunctionBlock(node) && node.parent.kind !== SyntaxKind.ArrowFunction;
        }

-        let depth = 0;
-        const maxDepth = 20;
        function walk(n: Node): void {


nit: consider renaming n to node.

aozgaa · 2017-08-17T23:51:59Z

src/services/outliningElementsCollector.ts

+                case SyntaxKind.ObjectLiteralExpression:
+                    const openBrace = findChildOfKind(n, SyntaxKind.OpenBraceToken, sourceFile);
+                    const closeBrace = findChildOfKind(n, SyntaxKind.CloseBraceToken, sourceFile);
+                    addOutliningSpan(n, openBrace, closeBrace, autoCollapse(n), /* fullStart */ n.parent.kind !== SyntaxKind.ArrayLiteralExpression);


use !isArrayLiteralExpression(n.parent).

aozgaa · 2017-08-17T23:52:19Z

src/services/outliningElementsCollector.ts

                case SyntaxKind.ArrayLiteralExpression:
                    const openBracket = findChildOfKind(n, SyntaxKind.OpenBracketToken, sourceFile);
                    const closeBracket = findChildOfKind(n, SyntaxKind.CloseBracketToken, sourceFile);
-                    addOutliningSpan(n, openBracket, closeBracket, autoCollapse(n));
+                    addOutliningSpan(n, openBracket, closeBracket, autoCollapse(n), /* fullStart */ n.parent.kind !== SyntaxKind.ArrayLiteralExpression);


ditto for isArrayLiteralExpression()

aozgaa · 2017-08-17T23:54:01Z

src/services/outliningElementsCollector.ts

@@ -3,12 +3,18 @@ namespace ts.OutliningElementsCollector {
    export function collectElements(sourceFile: SourceFile, cancellationToken: CancellationToken): OutliningSpan[] {
        const elements: OutliningSpan[] = [];
        const collapseText = "...";


nit: this could also be moved outside the function body (don't export).

aozgaa · 2017-08-17T23:54:12Z

src/services/outliningElementsCollector.ts

@@ -113,21 +117,21 @@ namespace ts.OutliningElementsCollector {
                            parent.kind === SyntaxKind.WithStatement ||
                            parent.kind === SyntaxKind.CatchClause) {

-                            addOutliningSpan(parent, openBrace, closeBrace, autoCollapse(n));
+                            addOutliningSpan(parent, openBrace, closeBrace, autoCollapse(n), /* fullStart */ true);


s/fullStart/getFullStart? Also, consider removing the spaces on either end of the comment to make it more compact. This should be the convention throughout the codebase -- it appears to be followed in the cases I saw within a cursory glance. Maybe a tslint rule could help here... @Andy-MS ?

To avoid this in the future, we could make renaming smarter, but deciding how to handle comments in general is not obvious (c.f. csharp).

Tried this out at #17893

uniqueiniquity · 2017-08-18T23:23:31Z

As from #16385,

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

uniqueiniquity added 2 commits August 9, 2017 15:11

Fix outlining of objects in array

1663d01

add fourslash test

df5e1a0

uniqueiniquity requested review from billti, aozgaa and amcasey August 9, 2017 23:15

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

uniqueiniquity changed the title ~~Objects in array~~ Correct outlining spans for object literals in array Aug 9, 2017

amcasey reviewed Aug 10, 2017

View reviewed changes

Generalize to nested arrays and refactor

c7d691d

uniqueiniquity changed the title ~~Correct outlining spans for object literals in array~~ Correct outlining spans for object and array literals in array Aug 10, 2017

amcasey reviewed Aug 11, 2017

View reviewed changes

amcasey approved these changes Aug 11, 2017

View reviewed changes

Cleans up and adds nested case to test

d6ccee6

aozgaa reviewed Aug 11, 2017

View reviewed changes

Add explanatory comments, consolidate main body

760812f

billti approved these changes Aug 14, 2017

View reviewed changes

aozgaa reviewed Aug 17, 2017

View reviewed changes

aozgaa approved these changes Aug 17, 2017

View reviewed changes

Style changes and cleanup

e6c1afb

uniqueiniquity merged commit 45c62ac into microsoft:master Aug 18, 2017

uniqueiniquity deleted the objectsInArray branch August 18, 2017 23:25

microsoft locked and limited conversation to collaborators Jun 14, 2018

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

		////// objects in x should generate outlining spans that do not render in VS

		@@ -16,6 +16,18 @@ namespace ts.OutliningElementsCollector {
		}
		}


		function addOutliningSpan(hintSpanNode: Node, startElement: Node, endElement: Node, autoCollapse: boolean) {
		// If useFullStart is true, then the collapsing span includes leading whitespace, including linebreaks.

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

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

Conversation

uniqueiniquity commented Aug 9, 2017 • edited Loading

msftclas commented Aug 9, 2017

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

amcasey left a comment

Choose a reason for hiding this comment

aozgaa left a 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

uniqueiniquity commented Aug 14, 2017

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 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uniqueiniquity commented Aug 18, 2017

uniqueiniquity commented Aug 9, 2017 •

edited

Loading