Compute TextChange based on syntax instead of text #67120

genlu · 2023-03-01T04:04:27Z

and check for empty change range before merge.

An attempt to fix the error reported by @jasonmalinowski

    <description>System.ArgumentException: newChanges&#x000D;&#x000A;
    at Roslyn.Utilities.TextChangeRangeExtensions.Merge(ImmutableArray`1 oldChanges, ImmutableArray`1 newChanges)&#x000D;&#x000A;
    at Microsoft.CodeAnalysis.CSharp.BraceCompletion.AbstractCurlyBraceOrBracketCompletionService.&lt;GetTextChangeAfterReturn&gt;g__GetMergedChanges|5_2(TextChange newLineEdit, ImmutableArray`1 formattingChanges, SourceText formattedText)&#x000D;&#x000A;
    at Microsoft.CodeAnalysis.CSharp.BraceCompletion.AbstractCurlyBraceOrBracketCompletionService.GetTextChangeAfterReturn(BraceCompletionContext context, IndentationOptions options, CancellationToken cancellationToken)&#x000D;&#x000A;
    at Microsoft.CodeAnalysis.AutomaticCompletion.BraceCompletionSessionProvider.BraceCompletionSession.PostReturn()&#x000D;&#x000A;
    at Microsoft.VisualStudio.Text.Utilities.GuardedOperations.CallExtensionPoint(Object errorSource, Action call)</description>

https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1758005

genlu · 2023-03-02T01:04:57Z

@jasonmalinowski @CyrusNajmabadi Not sure how to trigger this exception in test. Any suggestion?

jasonmalinowski · 2023-03-02T02:04:14Z

@genlu Off the top of my head no idea -- although I hit it again today. :-/ Do we have dumps from telemetry? Those might let you look at the edit history of the buffer and see if there was a certain lead up of edits.

genlu · 2023-03-02T19:04:48Z

@jasonmalinowski ~~so far we don't have anything. Maybe we can take this fix with the IsEmpty check removed so we can take another look if we still have issue?~~

Looks like this is not a completely new issue. I saw same stacktrace from 17.2 during last week.

genlu · 2023-03-02T22:05:04Z

src/Features/CSharp/Portable/BraceCompletion/AbstractCurlyBraceOrBracketCompletionService.cs

            {
+                var formattingChangeRanges = formattingChanges.SelectAsArray(f => f.ToTextChangeRange());
+                if (formattingChangeRanges.IsEmpty)
+                    return ImmutableArray.Create(newLineEdit);


@jasonmalinowski tbh, I'm not sure how could we make the assumption that calling FormatTrackingSpan with a newline inserted would always return a non-empty change. Wouldn't it depend on user settings?

@genlu I'm not sure I follow the question.

We add a newline between brackets in some cases, and then pass the changed document to format the span between brackets. The assumption is the format change will never be empty as long as we inserted a newline, which I don't think it's guaranteed.

yeah, i don't expect we would have that guarantee. tbh, i really don't get what this code (including prior to your change) is trying to do and how it is going about trying to do it. Do you feel confident we have good test coverage here to catch issues?

OK, after a few attempts on tweaking the options, I finally get a test to repro the crash in both 17.5 branch (which is before #66575) and main

[WpfFact] public void Test() { var code = @"namespace NS1 $$"; var expected = @"namespace NS1 {}"; var expectedAfterReturn = @"namespace NS1 { }"; var globalOptions = new OptionsCollection(LanguageNames.CSharp) { { FormattingOptions2.SmartIndent, FormattingOptions2.IndentStyle.None }, { AutoFormattingOptionsStorage.FormatOnCloseBrace, false }, }; using var session = CreateSession(code, globalOptions); Assert.NotNull(session); CheckStart(session.Session); Assert.Equal(expected, session.Session.SubjectBuffer.CurrentSnapshot.GetText()); CheckReturn(session.Session, 0, expectedAfterReturn); }

[xUnit.net 00:00:05.25] Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.AutomaticCompletion.AutomaticBraceCompletionTests.Test [FAIL] [xUnit.net 00:00:05.25] System.ArgumentException : newChanges [xUnit.net 00:00:05.25] Stack Trace: [xUnit.net 00:00:05.25] src\Compilers\Core\Portable\InternalUtilities\TextChangeRangeExtensions.cs(129,0): at Roslyn.Utilities.TextChangeRangeExtensions.Merge(ImmutableArray`1 oldChanges, ImmutableArray`1 newChanges) [xUnit.net 00:00:05.25] src\Features\CSharp\Portable\BraceCompletion\AbstractCurlyBraceOrBracketCompletionService.cs(176,0): at Microsoft.CodeAnalysis.CSharp.BraceCompletion.AbstractCurlyBraceOrBracketCompletionService.<GetTextChangeAfterReturn>g__GetMergedChanges|5_2(TextChange newLineEdit, ImmutableArray`1 formattingChanges, SourceText formattedText) [xUnit.net 00:00:05.25] src\Features\CSharp\Portable\BraceCompletion\AbstractCurlyBraceOrBracketCompletionService.cs(153,0): at Microsoft.CodeAnalysis.CSharp.BraceCompletion.AbstractCurlyBraceOrBracketCompletionService.GetTextChangeAfterReturn(BraceCompletionContext context, IndentationOptions options, CancellationToken cancellationToken) [xUnit.net 00:00:05.25] src\EditorFeatures\Core\AutomaticCompletion\BraceCompletionSessionProvider.BraceCompletionSession.cs(290,0): at Microsoft.CodeAnalysis.AutomaticCompletion.BraceCompletionSessionProvider.BraceCompletionSession.PostReturn() [xUnit.net 00:00:05.25] src\EditorFeatures\TestUtilities\AutomaticCompletion\AbstractAutomaticBraceCompletionTests.cs(86,0): at Microsoft.CodeAnalysis.Editor.UnitTests.AutomaticCompletion.AbstractAutomaticBraceCompletionTests.CheckReturn(IBraceCompletionSession session, Int32 indentation, String result) [xUnit.net 00:00:05.25] src\EditorFeatures\CSharpTest\AutomaticCompletion\AutomaticBraceCompletionTests.cs(1694,0): at Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.AutomaticCompletion.AutomaticBraceCompletionTests.Test()

src/Features/CSharp/Portable/BraceCompletion/AbstractCurlyBraceOrBracketCompletionService.cs

CyrusNajmabadi · 2023-03-13T23:44:24Z

src/Features/CSharp/Portable/BraceCompletion/AbstractCurlyBraceOrBracketCompletionService.cs

-                var newLineString = options.FormattingOptions.NewLine;
-                newLineEdit = new TextChange(new TextSpan(closingToken.FullSpan.Start, 0), newLineString);
+                var closingToken = document.Root.FindToken(closingPoint - 1);
+                Debug.Assert(IsValidClosingBraceToken(closingToken));


i dont' love that thsi is only an assert. maybe bail out here since if we don't find the close token, everything is going to go bad?

I don't think these are actually needed. If we can't find the token here then something outside this method must be seriously wrong. In that case I guess we'd like this to crash so we can get dump?

i guess my question is: how do we know we'll have a close token here? I don't know this code well enough, so this is a question to you about what code ran prior to this that absolutely made us certain we have a good tree, and the tree will think there's a clsoe brace at this position :)

Ah, I see. From what I saw, the code prior to this handles the insertion of closing brace here. Only then this method would be called when 'return' is typed in between the braces. In other words, the close brace is added as apart of session start.

src/Features/CSharp/Portable/BraceCompletion/AbstractCurlyBraceOrBracketCompletionService.cs

genlu · 2023-03-14T19:53:57Z

@CyrusNajmabadi Any more comments?

CyrusNajmabadi · 2023-03-14T19:58:58Z

looking.

CyrusNajmabadi · 2023-03-14T19:59:23Z

src/EditorFeatures/CSharpTest/AutomaticCompletion/AutomaticBraceCompletionTests.cs

+            CheckStart(session.Session);
+            Assert.Equal(expected, session.Session.SubjectBuffer.CurrentSnapshot.GetText());
+
+            CheckReturn(session.Session, 0, expectedAfterReturn);


can you clarify what is was about this prior code that was causing the problem?

We previous assume calling formatter on the brace pair (including added newline in-between) would always cause text changes. But it's not true with certain option combination, which would cause us the throw.

CyrusNajmabadi · 2023-03-14T20:00:37Z

src/Features/CSharp/Portable/BraceCompletion/AbstractCurlyBraceOrBracketCompletionService.cs

-                document = document.WithChangedRoot(rootToFormat, cancellationToken);
+                var rootToFormat = document.Root.ReplaceToken(closingToken, newClosingToken);
+
+                newClosingToken = rootToFormat.FindToken(closingPoint - 1);


instead of this, use an annotation. that will be much safer and more correct. i really just do not trust thsi code.

Sure. Fixed

CyrusNajmabadi · 2023-03-14T20:03:42Z

src/Features/CSharp/Portable/BraceCompletion/AbstractCurlyBraceOrBracketCompletionService.cs

+                closingPoint = newClosingToken.Span.End;
+
+                var textChangeLength = newClosingToken.Span.End - closingToken.Span.End;
+                newLineEdit = new TextChange(new TextSpan(closingToken.FullSpan.Start, 0), newClosingToken.ToFullString()[..textChangeLength]);


ok. trying to reason about this. we're saying that we go to the start of where the close token was. then we figure out the distance the close token moved. Then we grab that distance from the start of teh new closing token?

I'm not really getting it. Can we be explicit about what text we're trying to grab here? It looks like it's just supposed to be the potential newlines at the start of the close token? Can we just grab this from it's trivia instead? Or, even better, can we just grab from the source-text?

The reason i don't like this is that ..TextChangeLength looks like it could grab random stuff (including the token contents itself).

src/Features/CSharp/Portable/BraceCompletion/AbstractCurlyBraceOrBracketCompletionService.cs

CyrusNajmabadi · 2023-03-15T19:39:15Z

src/Features/CSharp/Portable/BraceCompletion/AbstractCurlyBraceOrBracketCompletionService.cs

+                // Depending on options, we might not get any formatting change.
+                // In this case, the newline edit is the only change.
+                if (formattingChanges.IsEmpty)
+                    return ImmutableArray.Create(newLineEdit.Value);


consdier moving this into GetMergedChanges.

genlu requested a review from a team as a code owner March 1, 2023 04:04

ghost added the Area-IDE label Mar 1, 2023

genlu commented Mar 2, 2023

View reviewed changes

CyrusNajmabadi reviewed Mar 10, 2023

View reviewed changes

src/Features/CSharp/Portable/BraceCompletion/AbstractCurlyBraceOrBracketCompletionService.cs Outdated Show resolved Hide resolved

genlu requested review from a team, 333fred and JoeRobich as code owners March 13, 2023 23:07

genlu requested a review from a team March 13, 2023 23:07

genlu requested a review from a team as a code owner March 13, 2023 23:07

Compute TextChange based on syntax instead of text

9811ca5

genlu force-pushed the Fix branch from 0bdf583 to d66e850 Compare March 13, 2023 23:10

genlu removed request for a team, 333fred and JoeRobich March 13, 2023 23:10

Check for empty change range before merging

81e8ef8

genlu force-pushed the Fix branch from d66e850 to 81e8ef8 Compare March 13, 2023 23:15

CyrusNajmabadi reviewed Mar 13, 2023

View reviewed changes

src/Features/CSharp/Portable/BraceCompletion/AbstractCurlyBraceOrBracketCompletionService.cs Outdated Show resolved Hide resolved

Address review comment

463789f

CyrusNajmabadi reviewed Mar 14, 2023

View reviewed changes

Address comments

cbde16e

CyrusNajmabadi reviewed Mar 15, 2023

View reviewed changes

src/Features/CSharp/Portable/BraceCompletion/AbstractCurlyBraceOrBracketCompletionService.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Mar 15, 2023

View reviewed changes

src/Features/CSharp/Portable/BraceCompletion/AbstractCurlyBraceOrBracketCompletionService.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Mar 15, 2023

View reviewed changes

CyrusNajmabadi approved these changes Mar 15, 2023

View reviewed changes

Inline helper

acf6ad8

genlu enabled auto-merge (squash) March 15, 2023 20:22

genlu merged commit e7fe653 into dotnet:main Mar 15, 2023

ghost added this to the Next milestone Mar 15, 2023

genlu deleted the Fix branch March 22, 2023 22:09

genlu mentioned this pull request Mar 22, 2023

Please stop changing automatic bracket completion #67440

Closed

allisonchou modified the milestones: Next, 17.6 P3 Mar 28, 2023

Compute TextChange based on syntax instead of text #67120

Compute TextChange based on syntax instead of text #67120

Uh oh!

Conversation

genlu commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

genlu commented Mar 2, 2023

Uh oh!

jasonmalinowski commented Mar 2, 2023

Uh oh!

genlu commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genlu Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

genlu commented Mar 14, 2023

Uh oh!

CyrusNajmabadi commented Mar 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

genlu commented Mar 1, 2023 •

edited

Loading

genlu commented Mar 2, 2023 •

edited

Loading

genlu Mar 14, 2023 •

edited

Loading