Correctly apply anchor indentation adjustments in the presence of trivia #59654

sharwell · 2022-02-18T22:20:50Z

davidwengier

Thank you <3

CyrusNajmabadi · 2022-02-22T19:01:18Z

src/Workspaces/CSharpTest/Formatting/FormattingTests.cs

-                            2 +
-                            3)
+                                2 +
+                                3)


i'm curious what happened here.

These lines previously anchored to from, but now anchor to the first token on the line containing from (var). Since var didn't move, these lines were not altered.

Note that anchoring is different from relative indentation. The latter applies to the select keyword on the next line.

hrmmmmmm.

i am worried about this. are we sure we don't want this to be anchored to the 'from'. While i genuinely think the anchoring change makes sense for many cases, queries are so special that it seems like we would want them them to follow that. (lambdas may be similar in that regard).

Also note that the new behavior is only problematic when all of the following hold:

The original anchor token is not the first token on the line

The first token on the line with the anchor token is in the correct location

The anchor token is not in the correct location (i.e. spaces need to be added or removed to correct the position of the anchor token)

The now position of the anchor token continues to be on the same line

The anchored token on a subsequent line is correctly aligned with the mispositioned anchor token, and needs to retain its position after the anchor adjustment

Of all the tests that needed updating, this is the only one which could be argued to meet all these conditions. However, it's also easy to argue that this case does not meet condition (5), and therefore the new behavior is equally acceptable.

➡️ Testing greatly increased by adding an idempotency check for format document tests

CyrusNajmabadi · 2022-02-22T19:01:54Z

...aredUtilitiesAndExtensions/Compiler/CSharp/Formatting/Engine/Trivia/CSharpTriviaFormatter.cs

+                    if (existingWhitespaceBetween.Spaces != this.Spaces)
+                        return LineColumnRule.PreserveWithGivenSpaces(spaces: this.Spaces);
+                    else
+                        return LineColumnRule.PreserveLinesWithDefaultIndentation(lines: 0);


i would doc this line. i think i get it, but it would be nice to ahve confirmation of intent.

➡️ This block now eliminated

CyrusNajmabadi · 2022-02-22T19:02:33Z

...orkspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Context/FormattingContext.cs

@@ -403,8 +403,9 @@ public void AddAnchorIndentationOperation(AnchorIndentationOperation operation)
                return;
            }

-            var originalSpace = _tokenStream.GetOriginalColumn(operation.StartToken);
-            var data = new AnchorData(operation, originalSpace);
+            var anchorToken = _tokenStream.FirstTokenOfBaseTokenLine(operation.AnchorToken);


➡️ Now documented

great doc, thanks.

CyrusNajmabadi · 2022-02-22T19:02:57Z

src/Workspaces/VisualBasicTest/Formatting/FormattingTests.vb

@@ -491,7 +491,7 @@ End Class</Code>
            Dim expected = <Code>Class C
    Sub Method()
        Dim a = From q In
-                   {1, 3, 5}
+                                                {1, 3, 5}


this does not not seem good. what happened here?

ok. based on your previous comment, this is explained. that said, this is so weird too. why did this value end up on the next line? that feels like a bug itself.

This was already on its own line, which is a strange situation from the start.

CyrusNajmabadi · 2022-02-23T16:53:40Z

src/Workspaces/VisualBasicTest/Formatting/FormattingTests.vb

+                Where q > 10
+                Select q
+    End Sub
+End Class</Code>


i'll reiterate that this seems not good. we could try taking this PR though and seeing if people have problems with it.

That said, i medium-strongly feel that within a query we should be keeping hte anchor attached to the start of hte query, not the start of hte line.

Is there a way we could have the best of both worlds? Your approach for most anchors, but allow an achor to override that with a flag that says "no, do not attach to start of line, keep me attached to this actual node".

Is there a way we could have the best of both worlds?

There is, possibly. In the original form, I had a new flag to control the anchor selection. All of them used "first token on line", and when I came back to query expressions I was worried about the following situation:

User has a correctly formatted expression:

Dim a = From q In {1, 3, 5} Where q > 10 Select q

User accidentally hits tab between = and From:

Dim a = From q In {1, 3, 5} Where q > 10 Select q

User presses Ctrl+K, Ctrl+D to format document¹. Do to movement of the anchor, a change is made to a line that was already correct:

Dim a = From q In {1, 3, 5} Where q > 10 Select q

Eventually I concluded that it's more likely for an entire block to be indented incorrectly than for a vertically-aligned subsection to be indented incorrectly, and kept the anchor pointing to the first token on the line.

¹ Moving to a different line and/or saving the document alone is not sufficient to trigger the problematic behavior, because these actions instruct the formatter to only touch "dirty" lines, and that doesn't include the line with the inline array.

Apply test trait to containing type

6206f0d

sharwell requested a review from a team as a code owner February 18, 2022 22:20

dotnet-issue-labeler bot added the Area-IDE label Feb 18, 2022

davidwengier approved these changes Feb 18, 2022

View reviewed changes

sharwell force-pushed the formatting-anchors branch from 1a99eb9 to b41f0e9 Compare February 18, 2022 23:55

Correctly apply anchor indentation adjustments in the presence of trivia

1a45d64

Fixes dotnet#57465

sharwell force-pushed the formatting-anchors branch from b41f0e9 to 1a45d64 Compare February 22, 2022 18:59

CyrusNajmabadi reviewed Feb 22, 2022

View reviewed changes

sharwell added 3 commits February 22, 2022 11:47

Add coverage for additional anchor scenarios

e87c20b

Document anchor token application

6bc46bb

Further simplify anchor indentation updates

e94072f

CyrusNajmabadi reviewed Feb 23, 2022

View reviewed changes

CyrusNajmabadi approved these changes Feb 23, 2022

View reviewed changes

sharwell merged commit e0f5df7 into dotnet:main Feb 23, 2022

ghost added this to the Next milestone Feb 23, 2022

sharwell deleted the formatting-anchors branch February 23, 2022 18:23

RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022

sharwell mentioned this pull request May 11, 2022

Yet Another Formatting Bug - Lambda within Object Initializer #61206

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly apply anchor indentation adjustments in the presence of trivia #59654

Correctly apply anchor indentation adjustments in the presence of trivia #59654

sharwell commented Feb 18, 2022

davidwengier left a comment

CyrusNajmabadi Feb 22, 2022

sharwell Feb 22, 2022

sharwell Feb 22, 2022

CyrusNajmabadi Feb 22, 2022

sharwell Feb 22, 2022

sharwell Feb 22, 2022

CyrusNajmabadi Feb 22, 2022

sharwell Feb 23, 2022

CyrusNajmabadi Feb 22, 2022

sharwell Feb 23, 2022

CyrusNajmabadi Feb 23, 2022

CyrusNajmabadi Feb 22, 2022

CyrusNajmabadi Feb 22, 2022

sharwell Feb 22, 2022

CyrusNajmabadi Feb 23, 2022

sharwell Feb 23, 2022 •

edited

Loading

-+
-)
++
+)

Correctly apply anchor indentation adjustments in the presence of trivia #59654

Correctly apply anchor indentation adjustments in the presence of trivia #59654

Conversation

sharwell commented Feb 18, 2022

davidwengier 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

sharwell Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

sharwell Feb 23, 2022 •

edited

Loading