Fix SourceText.GetChanges exception on merging certain changes by ashmind · Pull Request #39258 · dotnet/roslyn

ashmind · 2019-10-13T00:22:34Z

See #22289 for full details on the bug.

Summary

After certain WithChanges calls, SourceText.GetChanges is no longer able to handle the SourceText state and throws ArgumentOutOfRangeException when called.

Notes

Three of the new tests are failing without my change, other tests added just in case.
I didn't follow "Hello World" pattern in tests, sorry. I found it a bit hard to remember the offsets when using actual words.
Not fully confident in the change even though the tests pass. The flow is quite complicated and I feel the current coverage might be insufficient.

… changes

dnfclas · 2019-10-13T00:22:47Z

All CLA requirements met. #Closed

CyrusNajmabadi · 2019-10-13T00:40:02Z

src/Compilers/Core/Portable/Text/ChangedText.cs

+                            AddRange(list, new TextChangeRange(oldChange.Span, oldChange.NewLength + newChange.NewLength - newChange.Span.Length));
+                            oldDelta = oldDelta - oldChange.Span.Length + oldChange.NewLength - newChange.Span.Length;
                            oldIndex++;
                            newIndex++;


not certain why this is safe/ok. if newChange and oldChange have different contents, then what's going on here?

It actually merges the list of TextChangeRange objects, not TextChange objects.
Those don't have contents, just old length and new length, so there is no difference in contents.

The ranges are mapped to the final text later, in GetTextChanges.

I don't know if this will clarify things or not, but here's the way I understand this method:

We have an original text t1 and call t1.WithTextChanges(oldChanges). This produces a ChangedText t2, storing the oldChanges and t1 in fields.

When we call t2.WithTextChanges(newChanges) it produces a new ChangedText t3, storing the newChanges and t2.

When we call t3.GetTextChanges(t1), we have to rewrite the newChanges to make them occur w.r.t. t1 instead of w.r.t. t2:

we have to split individual changes apart when old and new changes overlap with each other

we merge adjacent changes together as we go to simplify them.

In the end we get a TextChangeRange[] which describes the difference between the original text and the final text.

src/Compilers/Core/CodeAnalysisTest/Text/TextChangeTests.cs

RikkiGibson · 2019-10-14T21:44:42Z

src/Compilers/Core/Portable/Text/ChangedText.cs

-                        oldDelta = oldDelta - oldChange.Span.Length + oldChangeLeadingInsertion;
-                        oldChange = new TextChangeRange(new TextSpan(oldChange.Span.Start, 0), oldChange.NewLength - oldChangeLeadingInsertion);
-                        newChange = new TextChangeRange(new TextSpan(oldChange.Span.Start + oldDelta, newChange.Span.Length), newChange.NewLength);
+                        var oldChangeLeadingDeletion = Math.Min(oldChange.Span.Length, oldChangeLeadingInsertion);


OK, I'm talking myself through why this makes sense.

In this line we are figuring out how many characters of the old change occur before the new change. We assume that if the old change deletes characters then it will delete from the left at most as many as it is inserting on the left. When the old change's deletion is longer than the old change's leading insertion, the rest of its deletion has to be shifted over to remove the characters after the new change's insertion. #Resolved

RikkiGibson · 2019-10-14T21:44:56Z

src/Compilers/Core/CodeAnalysisTest/Text/TextChangeTests.cs

+            Assert.Equal("0aa4", change1.ToString());
+            Assert.Equal("0bba4", change2.ToString());
+            Assert.Equal(new[] { new TextChange(new TextSpan(1, 3), "bba") }, changes);
+        }


Are these all cases that would have thrown before your change?

BTW, this new form for the tests is significantly easier to reason about, thanks.

Only 3 of them would throw, but I no longer remember which ones. TestMergeChanges_Overlapping_NewInsideOld_AndOldHasDeletion for sure. We can revert the class itself on local and run tests to see.

I ended up pulling down the changes after I commented to see what would break, and it was pretty much as you describe :)

I actually needed to pull down the changes and run the whole test class with the debugger with breakpoints on areas you changed to be sure I understood what the code was doing.

Thanks for your time! Yep it was a bit hard to follow for me as well, existing tests were extremely useful.

src/Compilers/Core/Portable/Text/ChangedText.cs

RikkiGibson

LGTM, just had a few questions and suggestions

Co-Authored-By: Rikki Gibson <rikkigibson@gmail.com>

RikkiGibson · 2019-10-15T21:46:38Z

Could we please get a senior review on this? @dotnet/roslyn-compiler

RikkiGibson · 2019-10-18T20:26:30Z

Ping @dotnet/roslyn-compiler

cston · 2019-10-18T22:28:27Z

src/Compilers/Core/CodeAnalysisTest/Text/TextChangeTests.cs

            Assert.Equal("Cool ", changes[0].NewText);
        }

+        [Fact]


Fact [](start = 9, length = 4)

Please add a WorkItem attribute to each of these three tests and the last test.

Looks like the link might have been auto-formatted, did you mean something like:

[WorkItem(22289, "https://github.com/dotnet/roslyn/issues/22289")]

Sorry, it's a bit unclear which 3 we are talking about -- I'll add to all the ones that were failing before this fix.

cston · 2019-10-19T17:42:57Z

src/Compilers/Core/CodeAnalysisTest/Text/TextChangeTests.cs

+            Assert.Equal("0aa4", change1.ToString());
+            Assert.Equal("0bba4", change2.ToString());
+            Assert.Equal(new[] { new TextChange(new TextSpan(1, 3), "bba") }, changes);
+        }


} [](start = 8, length = 1)

Do we also need to handle the case where the new deletion is larger than the old? It looks like the following fails with or without this PR:

var original = SourceText.From("01234"); var change1 = original.WithChanges(new TextChange(new TextSpan(1, 3), "aa")); var change2 = change1.WithChanges(new TextChange(new TextSpan(1, 3), "bb")); var changes = change2.GetTextChanges(original); Assert.Equal("0aa4", change1.ToString()); Assert.Equal("0bb", change2.ToString());

We can log a bug for this case and handle it separately.

Thanks! I'll take a look in a few moments -- if it's easy to fix then maybe just fix it here, if hard (needs a lot of review) then I can do another PR.

Seems similar to what I am already doing in newChange.Span.Start > oldChange.Span.Start + oldDelta, but that means it's complicated enough for it's own review.

So I would prefer to do as a separate bug/PR just to make review easier (also maybe there is some reuse that can be done between those two cases).

RikkiGibson · 2019-10-20T02:24:43Z

Thanks so much for your contribution @ashmind!

[dotnet#22289] Fix SourceText.GetChanges exception on merging certain…

5cfc912

… changes

ashmind requested a review from a team as a code owner October 13, 2019 00:22

ashmind mentioned this pull request Oct 13, 2019

Fix WithChanges bug in Roslyn and submit fix as a Roslyn PR ashmind/SharpLab#436

Closed

CyrusNajmabadi reviewed Oct 13, 2019

View reviewed changes

RikkiGibson added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Oct 13, 2019

RikkiGibson added this to the 16.4.P3 milestone Oct 13, 2019

RikkiGibson self-requested a review October 14, 2019 20:20

RikkiGibson reviewed Oct 14, 2019

View reviewed changes

src/Compilers/Core/CodeAnalysisTest/Text/TextChangeTests.cs Show resolved Hide resolved

RikkiGibson reviewed Oct 14, 2019

View reviewed changes

src/Compilers/Core/Portable/Text/ChangedText.cs Outdated Show resolved Hide resolved

RikkiGibson approved these changes Oct 14, 2019

View reviewed changes

RikkiGibson self-assigned this Oct 14, 2019

RikkiGibson requested a review from a team October 14, 2019 22:49

ashmind and others added 2 commits October 15, 2019 14:39

Add extra test (leading deletion < leading insertion)

19be8c1

Co-Authored-By: Rikki Gibson <rikkigibson@gmail.com>

Improve comment wording

32b835c

Co-Authored-By: Rikki Gibson <rikkigibson@gmail.com>

RikkiGibson mentioned this pull request Oct 15, 2019

[WIP] Don't Debug.Assert in ChangedText.GetTextChanges #30651

Closed

cston reviewed Oct 18, 2019

View reviewed changes

cston reviewed Oct 19, 2019

View reviewed changes

cston approved these changes Oct 19, 2019

View reviewed changes

Add WorkItem attribute to all tests that reproduce the issue

e841095

RikkiGibson approved these changes Oct 20, 2019

View reviewed changes

RikkiGibson mentioned this pull request Oct 20, 2019

ChangedText.WithChanges crashes when new deletion larger than old #39405

Closed

RikkiGibson merged commit c9f142d into dotnet:master Oct 20, 2019

filipw mentioned this pull request Nov 15, 2019

updated to Roslyn 3.5.0-beta1-19564-02 OmniSharp/omnisharp-roslyn#1652

Merged

jasonmalinowski mentioned this pull request Feb 5, 2020

SourceText.GetTextChanges produces invalid edits after successive changes #41413

Closed

sharwell mentioned this pull request Aug 28, 2020

IncrementallyParse fails for certain text changes #47234

Closed

Conversation

ashmind commented Oct 13, 2019 • edited by RikkiGibson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Notes

Uh oh!

dnfclas commented Oct 13, 2019 • edited by RikkiGibson 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!

Uh oh!

RikkiGibson Oct 14, 2019 • 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!

RikkiGibson Oct 14, 2019 • 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!

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!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Oct 15, 2019

Uh oh!

RikkiGibson commented Oct 18, 2019

Uh oh!

cston Oct 18, 2019 • 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!

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!

RikkiGibson commented Oct 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ashmind commented Oct 13, 2019 •

edited by RikkiGibson

Loading

dnfclas commented Oct 13, 2019 •

edited by RikkiGibson

Loading

RikkiGibson Oct 14, 2019 •

edited

Loading

RikkiGibson Oct 14, 2019 •

edited

Loading

cston Oct 18, 2019 •

edited

Loading