Relax the requirement in WithChanges that changes be ordered #26798

sharwell · 2018-05-11T16:18:20Z

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/604150

/cc @minestarks

Customer scenario

A customer attempts to reformat an HTML document, and Visual Studio crashes.

Bugs this fixes

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

Workarounds, if any

None.

Risk

Low. This change relaxes the preconditions of an internal fallback method to produce expected results when the input is unordered but also unambiguous.

Performance impact

Performance is not changed in normal usage scenarios.

Is this a regression from a previous update?

Yes, introduced by #25558.

Root cause analysis

See #26339 for the primary RCA.

This change corrects an additional edge case for ITextBuffer instances created before VisualStudioWorkspace attaches event handlers to ITextBufferFactoryService and IProjectionBufferFactoryService. This case is only believed to occur in IDE initialization scenarios where the workspace load is triggered by opening a document.

How was the bug found?

Watson.

Test documentation updated?

N/A

CyrusNajmabadi · 2018-05-11T17:07:23Z

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

why the second clause?

➡️ TestChangedTextWithReplaceBeforeInsertSamePosition

CyrusNajmabadi · 2018-05-11T17:08:39Z

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

it's not clear ot me why this check is correct. why is it sufficient to only check the last change range?

I think the answer is because this is in a loop that goes through all of the changes.

In reply to: 187675569 [](ancestors = 187675569)

➡️ changeRanges is always sorted (a condition that was true both before and after this change).

gafter · 2018-05-11T17:15:50Z

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

WithChanges [](start = 31, length = 11)

Do we have tests that demonstrate the new behavior?

Do we have tests that demonstrate the new behavior?

No. Local install was broken due to NRT preview install so I had to fix that, and now am working on tests. Hence the PR for Personal Review Only label. 😄

Ah, sorry. The label is not in a visible location for me :( That's my bad for not checking. I'm a fan of "WIP" in teh title to help indicate this more obviously.

➡️ Tests are added

WIP in the title is a bad idea at the moment because of an Outlook threading bug that triggers when you change the title (e.g. to remove WIP later). I'm working with them to get that fixed.

gafter · 2018-05-11T17:16:10Z

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

ArgumentException [](start = 30, length = 17)

Are there no tests for the exceptional behavior?

➡️ Tests are included for both normal and exceptional behavior

Can you please point me to tests for the exceptional behavior? Are those existing tests?

In reply to: 187682548 [](ancestors = 187682548)

The only test I could find is one existing test TestChangedTextWithMultipleOverlappingChanges

In reply to: 188384892 [](ancestors = 188384892,187682548)

@gafter Yes, that is the test for the condition which is still a failure

minestarks · 2018-05-11T17:50:04Z

@sharwell thanks for the fix!
Does the order of application of the TextChanges matter? I thought the start/end indices of a change was based on the state of the text after the previous TextChanges were applied.

sharwell · 2018-05-11T19:08:39Z

Does the order of application of the TextChanges matter? I thought the start/end indices of a change was based on the state of the text after the previous TextChanges were applied.

All the changes are non-overlapping changes on the original text. The order only matters for sequences of insertions at the same position (empty old span, non-empty new text).

jaredpar · 2018-05-15T03:43:43Z

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

Other code in this method assumes that NewText can be null. Is the check to ensure the span isn't empty a sufficient guard against this case here?

jaredpar

jasonmalinowski

I worry that this is an interesting kind of breaking change: since this type is something we expect people hosting Roslyn to derive from, we've now changed their contract on their derived type which says they now must support unordered changes. If VS for Mac, for example, isn't expecting this, we might still be broken.

I would have guessed we would just fix this by changing whatever code is passing unordered spans and sorting before calling WithChanges. Did that have some other complexity I'm not imagining here?

jasonmalinowski · 2018-05-15T20:38:16Z

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

+                        changes = from c in changes
+                                  where !c.Span.IsEmpty || c.NewText?.Length > 0
+                                  orderby c.Span.Start, c.Span.End
+                                  select c;


Note that this isn't being realized, and the recursive call is going to do .Any() and then an enumeration over the changes again. Will that result in multiple sorts?

That's a good point. Sort requires a full iteration to resolve even Any. That will mean that we sort this at least twice: changes.Any and foreach (var change in changes)

In reply to: 188428290 [](ancestors = 188428290)

That will mean that we sort this at least twice: changes.Any and foreach (var change in changes)

Currently, yes. However, this is really a fallback handler for a condition we try to avoid in practice (a bad outcome of a race condition during package initialization). I could call ToList() if you prefer.

I don't consider is a "fallback handler due to package initialization" -- the compiler layer knows not what package initialization is. 😄 If I were you, I'd consider adding a .ToList(), but potentially at the start of the method. Even not in the fallback case, we enumerate this twice which is potentially bad, but potentially not.

If I were you, I'd consider adding a .ToList(), but potentially at the start of the method. Even not in the fallback case, we enumerate this twice which is potentially bad, but potentially not.

It seems risky to add to the beginning of the method. If most of the inputs are collections with low-overhead enumeration, then calling ToList every time could cause a performance regression. Currently this code change only touches a path that previously threw an exception. Any objection to just calling ToList on the result of the LINQ query?

That's fine too.

jasonmalinowski · 2018-05-15T20:40:46Z

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

+                    {
+                        changes = from c in changes
+                                  where !c.Span.IsEmpty || c.NewText?.Length > 0
+                                  orderby c.Span.Start, c.Span.End


Why the secondary sort?

Handles an incorrectly-ordered zero-length insertion. See #26798 (comment) which points to TestChangedTextWithReplaceBeforeInsertSamePosition.

If @CyrusNajmabadi and I both had to ask, please add a comment explaining why. I admit within a minute or two of thinking it's not obvious to me why not having this breaks that other test (I'd expect this to fail either way), so a comment would be good.

➡️ Fixed in af5afd7

jasonmalinowski · 2018-05-15T20:58:30Z

And to clarify my earlier comment: this is a change that makes the world a better place for consumers of the compiler type because yes, this really wasn't a requirement we had to have. So I'm for the change, but we might want to check with VS for Mac and VS Code first to see if they'll need any updates made.

gafter

sharwell · 2018-05-15T21:28:20Z

since this type is something we expect people hosting Roslyn to derive from, we've now changed their contract on their derived type which says they now must support unordered changes.

It's actually the other way around. One of the derived types already supported unordered changes, and consumers were depending on that behavior. However, we can no longer guarantee that derived implementation will be used, and in some edge cases this base implementation was called instead.

I would have guessed we would just fix this by changing whatever code is passing unordered spans and sorting before calling WithChanges. Did that have some other complexity I'm not imagining here?

Yes, this was one of the alternatives discussed. The offending input is in the TypeScript code, and we could still decide to resolve this issue by requesting the input be ordered.

sharwell added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 11, 2018

sharwell added this to the 15.7 milestone May 11, 2018

sharwell requested a review from a team as a code owner May 11, 2018 16:18

CyrusNajmabadi reviewed May 11, 2018

View reviewed changes

gafter reviewed May 11, 2018

View reviewed changes

sharwell force-pushed the unordered-changes branch from 0e9e405 to a744edd Compare May 11, 2018 17:28

sharwell added Area-Compilers and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels May 11, 2018

jaredpar reviewed May 15, 2018

View reviewed changes

Relax the requirement in WithChanges that changes be ordered

0ffe920

sharwell force-pushed the unordered-changes branch from a744edd to 0ffe920 Compare May 15, 2018 17:07

jaredpar approved these changes May 15, 2018

View reviewed changes

jinujoseph requested a review from jasonmalinowski May 15, 2018 18:54

jinujoseph added the Needs Shiproom Approval label May 15, 2018

jasonmalinowski reviewed May 15, 2018

View reviewed changes

gafter approved these changes May 15, 2018

View reviewed changes

Use TextSpan's IComparable implementation rather than duplicating it

af5afd7

jasonmalinowski approved these changes May 15, 2018

View reviewed changes

Use ToList() to avoid multiple-enumeration overhead

6de37e6

jinujoseph added Approved to merge and removed Needs Shiproom Approval labels May 15, 2018

agocke merged commit 20b7e22 into dotnet:dev15.7.x May 16, 2018

sharwell deleted the unordered-changes branch May 16, 2018 02:29

Relax the requirement in WithChanges that changes be ordered #26798

Relax the requirement in WithChanges that changes be ordered #26798

Uh oh!

Conversation

sharwell commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

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!

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!

Choose a reason for hiding this comment

Uh oh!

minestarks commented May 11, 2018

Uh oh!

sharwell commented May 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

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!

sharwell May 15, 2018 • 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!

sharwell May 15, 2018 • 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!

sharwell commented May 11, 2018 •

edited

Loading

sharwell May 15, 2018 •

edited

Loading

sharwell May 15, 2018 •

edited

Loading