Skip to content

Relax the requirement in WithChanges that changes be ordered #26798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
agocke merged 3 commits into from
May 16, 2018
Merged

Relax the requirement in WithChanges that changes be ordered #26798

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0ffe920
Relax the requirement in WithChanges that changes be ordered
sharwell May 11, 2018
af5afd7
Use TextSpan's IComparable implementation rather than duplicating it
sharwell May 15, 2018
6de37e6
Use ToList() to avoid multiple-enumeration overhead
sharwell May 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/Compilers/Core/CodeAnalysisTest/Text/TextChangeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ public void TestChangedTextWithMultipleUnorderedChanges()
new TextChange(new TextSpan(0, 5), "Halo")
};

Assert.Throws<ArgumentException>(() => text.WithChanges(changes));
var newText = text.WithChanges(changes);
Assert.Equal("Halo Universe", newText.ToString());
}

[Fact]
Expand Down Expand Up @@ -138,8 +139,8 @@ public void TestChangedTextWithReplaceBeforeInsertSamePosition()
new TextChange(new TextSpan(6, 0), "Super ")
};

// this causes overlap
Assert.Throws<ArgumentException>(() => text.WithChanges(changes));
var newText = text.WithChanges(changes);
Assert.Equal("Hello Super Vurld", newText.ToString());
}

[Fact]
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/CodeAnalysisResources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/Core/Portable/CodeAnalysisResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,8 +413,8 @@
<data name="PEImageDoesntContainManagedMetadata" xml:space="preserve">
<value>PE image doesn't contain managed metadata.</value>
</data>
<data name="ChangesMustBeOrderedAndNotOverlapping" xml:space="preserve">
<value>The changes must be ordered and not overlapping.</value>
<data name="ChangesMustNotOverlap" xml:space="preserve">
<value>The changes must not overlap.</value>
</data>
<data name="DiagnosticIdCantBeNullOrWhitespace" xml:space="preserve">
<value>A DiagnosticDescriptor must have an Id that is neither null nor an empty string nor a string that only contains white space.</value>
Expand Down
14 changes: 13 additions & 1 deletion src/Compilers/Core/Portable/Text/SourceText.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,19 @@ public virtual SourceText WithChanges(IEnumerable<TextChange> changes)
// there can be no overlapping changes
if (change.Span.Start < position)
{
throw new ArgumentException(CodeAnalysisResources.ChangesMustBeOrderedAndNotOverlapping, nameof(changes));
Copy link
Member

@gafter gafter May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Are there no tests for the exceptional behavior?

Copy link
Contributor Author

@sharwell sharwell May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

➡️ Tests are included for both normal and exceptional behavior

Copy link
Member

@gafter gafter May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

@gafter gafter May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only test I could find is one existing test TestChangedTextWithMultipleOverlappingChanges

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

Copy link
Contributor Author

@sharwell sharwell May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Handle the case of unordered changes by sorting the input and retrying. This is inefficient, but
// downstream consumers have been known to hit this case in the past and we want to avoid crashes.
// https://github.com/dotnet/roslyn/pull/26339
if (change.Span.End <= changeRanges.Last().Span.Start)
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@gafter gafter May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

@sharwell sharwell May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
changes = from c in changes
where !c.Span.IsEmpty || c.NewText?.Length > 0
orderby c.Span
select c;
Copy link
Member

@jasonmalinowski jasonmalinowski May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@jaredpar jaredpar May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@sharwell sharwell May 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jasonmalinowski jasonmalinowski May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sharwell sharwell May 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@jasonmalinowski jasonmalinowski May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine too.

return WithChanges(changes);
Copy link
Member

@gafter gafter May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Do we have tests that demonstrate the new behavior?

Copy link
Contributor Author

@sharwell sharwell May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sharwell sharwell May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

➡️ Tests are added

Copy link
Member

@gafter gafter May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

throw new ArgumentException(CodeAnalysisResources.ChangesMustNotOverlap, nameof(changes));
}

var newTextLength = change.NewText?.Length ?? 0;
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/xlf/CodeAnalysisResources.cs.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@
<target state="translated">Image PE neobsahuje spravovaná metadata.</target>
<note />
</trans-unit>
<trans-unit id="ChangesMustBeOrderedAndNotOverlapping">
<source>The changes must be ordered and not overlapping.</source>
<target state="translated">Změny musí být seřazené a nesmí se překrývat.</target>
<trans-unit id="ChangesMustNotOverlap">
<source>The changes must not overlap.</source>
<target state="needs-review-translation">Změny musí být seřazené a nesmí se překrývat.</target>
<note />
</trans-unit>
<trans-unit id="DiagnosticIdCantBeNullOrWhitespace">
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/xlf/CodeAnalysisResources.de.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@
<target state="translated">PE-Abbild enthält keine verwalteten Metadaten.</target>
<note />
</trans-unit>
<trans-unit id="ChangesMustBeOrderedAndNotOverlapping">
<source>The changes must be ordered and not overlapping.</source>
<target state="translated">Die Änderungen müssen sortiert sein und dürfen sich nicht überschneiden.</target>
<trans-unit id="ChangesMustNotOverlap">
<source>The changes must not overlap.</source>
<target state="needs-review-translation">Die Änderungen müssen sortiert sein und dürfen sich nicht überschneiden.</target>
<note />
</trans-unit>
<trans-unit id="DiagnosticIdCantBeNullOrWhitespace">
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/xlf/CodeAnalysisResources.es.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@
<target state="translated">La imagen PE no contiene metadatos administrados.</target>
<note />
</trans-unit>
<trans-unit id="ChangesMustBeOrderedAndNotOverlapping">
<source>The changes must be ordered and not overlapping.</source>
<target state="translated">Los cambios deben estar ordenados y no superponerse.</target>
<trans-unit id="ChangesMustNotOverlap">
<source>The changes must not overlap.</source>
<target state="needs-review-translation">Los cambios deben estar ordenados y no superponerse.</target>
<note />
</trans-unit>
<trans-unit id="DiagnosticIdCantBeNullOrWhitespace">
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/xlf/CodeAnalysisResources.fr.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@
<target state="translated">L'image PE ne contient pas de métadonnées gérées.</target>
<note />
</trans-unit>
<trans-unit id="ChangesMustBeOrderedAndNotOverlapping">
<source>The changes must be ordered and not overlapping.</source>
<target state="translated">Les modifications doivent être ordonnées et ne pas se chevaucher.</target>
<trans-unit id="ChangesMustNotOverlap">
<source>The changes must not overlap.</source>
<target state="needs-review-translation">Les modifications doivent être ordonnées et ne pas se chevaucher.</target>
<note />
</trans-unit>
<trans-unit id="DiagnosticIdCantBeNullOrWhitespace">
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/xlf/CodeAnalysisResources.it.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@
<target state="translated">L'immagine PE non contiene metadati gestiti.</target>
<note />
</trans-unit>
<trans-unit id="ChangesMustBeOrderedAndNotOverlapping">
<source>The changes must be ordered and not overlapping.</source>
<target state="translated">Le modifiche devono essere ordinate e non sovrapposte.</target>
<trans-unit id="ChangesMustNotOverlap">
<source>The changes must not overlap.</source>
<target state="needs-review-translation">Le modifiche devono essere ordinate e non sovrapposte.</target>
<note />
</trans-unit>
<trans-unit id="DiagnosticIdCantBeNullOrWhitespace">
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/xlf/CodeAnalysisResources.ja.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@
<target state="translated">PE イメージには、管理されたメタデータが含まれていません。</target>
<note />
</trans-unit>
<trans-unit id="ChangesMustBeOrderedAndNotOverlapping">
<source>The changes must be ordered and not overlapping.</source>
<target state="translated">変更は順序付けする必要があり、重複は許可されません。</target>
<trans-unit id="ChangesMustNotOverlap">
<source>The changes must not overlap.</source>
<target state="needs-review-translation">変更は順序付けする必要があり、重複は許可されません。</target>
<note />
</trans-unit>
<trans-unit id="DiagnosticIdCantBeNullOrWhitespace">
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/xlf/CodeAnalysisResources.ko.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@
<target state="translated">PE 이미지에 관리된 메타데이터가 포함되어 있지 않습니다.</target>
<note />
</trans-unit>
<trans-unit id="ChangesMustBeOrderedAndNotOverlapping">
<source>The changes must be ordered and not overlapping.</source>
<target state="translated">변경 내용의 순서를 지정해야 하고 겹쳐서는 안 됩니다.</target>
<trans-unit id="ChangesMustNotOverlap">
<source>The changes must not overlap.</source>
<target state="needs-review-translation">변경 내용의 순서를 지정해야 하고 겹쳐서는 안 됩니다.</target>
<note />
</trans-unit>
<trans-unit id="DiagnosticIdCantBeNullOrWhitespace">
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/xlf/CodeAnalysisResources.pl.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@
<target state="translated">Obraz PE nie zawiera zarządzanych metadanych.</target>
<note />
</trans-unit>
<trans-unit id="ChangesMustBeOrderedAndNotOverlapping">
<source>The changes must be ordered and not overlapping.</source>
<target state="translated">Zmiany muszą być uporządkowane i nie mogą nakładać się na siebie.</target>
<trans-unit id="ChangesMustNotOverlap">
<source>The changes must not overlap.</source>
<target state="needs-review-translation">Zmiany muszą być uporządkowane i nie mogą nakładać się na siebie.</target>
<note />
</trans-unit>
<trans-unit id="DiagnosticIdCantBeNullOrWhitespace">
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/xlf/CodeAnalysisResources.pt-BR.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@
<target state="translated">Imagem PE não contém metadados gerenciados.</target>
<note />
</trans-unit>
<trans-unit id="ChangesMustBeOrderedAndNotOverlapping">
<source>The changes must be ordered and not overlapping.</source>
<target state="translated">As alterações devem ser ordenadas e não sobrepostas.</target>
<trans-unit id="ChangesMustNotOverlap">
<source>The changes must not overlap.</source>
<target state="needs-review-translation">As alterações devem ser ordenadas e não sobrepostas.</target>
<note />
</trans-unit>
<trans-unit id="DiagnosticIdCantBeNullOrWhitespace">
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/xlf/CodeAnalysisResources.ru.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@
<target state="translated">Образ среды предустановки не содержит управляемые метаданные.</target>
<note />
</trans-unit>
<trans-unit id="ChangesMustBeOrderedAndNotOverlapping">
<source>The changes must be ordered and not overlapping.</source>
<target state="translated">Изменения должны идти в строгом порядке и не накладываться.</target>
<trans-unit id="ChangesMustNotOverlap">
<source>The changes must not overlap.</source>
<target state="needs-review-translation">Изменения должны идти в строгом порядке и не накладываться.</target>
<note />
</trans-unit>
<trans-unit id="DiagnosticIdCantBeNullOrWhitespace">
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/xlf/CodeAnalysisResources.tr.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@
<target state="translated">PE görüntüsü yönetilen meta verileri içermiyor.</target>
<note />
</trans-unit>
<trans-unit id="ChangesMustBeOrderedAndNotOverlapping">
<source>The changes must be ordered and not overlapping.</source>
<target state="translated">Değişiklikler sıralanmalı ve çakışmamalıdır.</target>
<trans-unit id="ChangesMustNotOverlap">
<source>The changes must not overlap.</source>
<target state="needs-review-translation">Değişiklikler sıralanmalı ve çakışmamalıdır.</target>
<note />
</trans-unit>
<trans-unit id="DiagnosticIdCantBeNullOrWhitespace">
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/xlf/CodeAnalysisResources.zh-Hans.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@
<target state="translated">PE 映像不包含任何托管元数据。</target>
<note />
</trans-unit>
<trans-unit id="ChangesMustBeOrderedAndNotOverlapping">
<source>The changes must be ordered and not overlapping.</source>
<target state="translated">更改必须有序且不重叠。</target>
<trans-unit id="ChangesMustNotOverlap">
<source>The changes must not overlap.</source>
<target state="needs-review-translation">更改必须有序且不重叠。</target>
<note />
</trans-unit>
<trans-unit id="DiagnosticIdCantBeNullOrWhitespace">
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/xlf/CodeAnalysisResources.zh-Hant.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@
<target state="translated">PE 映像不包含 Managed 中繼資料。</target>
<note />
</trans-unit>
<trans-unit id="ChangesMustBeOrderedAndNotOverlapping">
<source>The changes must be ordered and not overlapping.</source>
<target state="translated">變更必須排序且不可重疊。</target>
<trans-unit id="ChangesMustNotOverlap">
<source>The changes must not overlap.</source>
<target state="needs-review-translation">變更必須排序且不可重疊。</target>
<note />
</trans-unit>
<trans-unit id="DiagnosticIdCantBeNullOrWhitespace">
Expand Down