Refactor ChangedText.Merge and add fuzz testing#48420
Merged
RikkiGibson merged 19 commits intodotnet:masterfrom Oct 12, 2020
Merged
Refactor ChangedText.Merge and add fuzz testing#48420RikkiGibson merged 19 commits intodotnet:masterfrom
RikkiGibson merged 19 commits intodotnet:masterfrom
Conversation
cston
reviewed
Oct 8, 2020
jaredpar
reviewed
Oct 8, 2020
RikkiGibson
commented
Oct 8, 2020
cston
reviewed
Oct 8, 2020
cston
reviewed
Oct 8, 2020
cston
reviewed
Oct 8, 2020
cston
reviewed
Oct 8, 2020
cston
reviewed
Oct 8, 2020
jcouv
reviewed
Oct 8, 2020
jcouv
reviewed
Oct 8, 2020
jcouv
reviewed
Oct 8, 2020
cston
reviewed
Oct 9, 2020
CyrusNajmabadi
approved these changes
Oct 10, 2020
Contributor
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Yup. This helped a lot!
Member
Author
|
Could I please get a second review from @dotnet/roslyn-compiler? |
Member
|
Looking |
jaredpar
reviewed
Oct 12, 2020
|
|
||
| nextOldChange: | ||
| if (oldIndex < oldChanges.Length) | ||
| public UnadjustedNewChange(TextChangeRange range) |
Member
There was a problem hiding this comment.
Suggested change
| public UnadjustedNewChange(TextChangeRange range) | |
| public UnadjustedNewChange(in TextChangeRange range) |
Given the size of TextChangeRange consider passing by in
Member
Author
There was a problem hiding this comment.
Proper utilization of this change also requires changing use sites to use .ItemRef()--maybe let's do it in a follow-up PR
| if (tryGetNextOldChange()) continue; | ||
| else break; | ||
| } | ||
| else if (newChange.SpanLength == 0 && newChange.NewLength == 0) |
Member
There was a problem hiding this comment.
The else is unnecessary here correct?
Member
Author
There was a problem hiding this comment.
Yes, but I don't think it's worth changing unless you have a strong opinion here.
| static void addAndAdjustOldDelta(ArrayBuilder<TextChangeRange> builder, ref int oldDelta, TextChangeRange oldChange) | ||
| { | ||
| // modify oldDelta to reflect characters deleted and inserted by an old change | ||
| oldDelta = oldDelta - oldChange.Span.Length + oldChange.NewLength; |
Member
There was a problem hiding this comment.
Did u consider under / overflow scenarios here?
Member
Author
There was a problem hiding this comment.
I haven't but I'm curious what thoughts you may have on how to mitigate such problems
Member
There was a problem hiding this comment.
Don't have any great ideas here. More wanted to see if we had considered them sufficiently. This is a bit corner case though. Don't want to block the current fix for extreme cases here.
Member
Author
There was a problem hiding this comment.
Well, for example, since both Span.Length and NewLength are required to be positive, does it help prevent overflow if we do oldDelta = oldDelta + (oldChange.NewLength - oldChange.Span.Length)? (not going to worry about it for this PR)
jcouv
reviewed
Oct 12, 2020
jcouv
approved these changes
Oct 12, 2020
Member
jcouv
left a comment
There was a problem hiding this comment.
LGTM Thanks (iteration 16). The added comments are greatly useful!
333fred
added a commit
that referenced
this pull request
Oct 13, 2020
…features/interpolated-string-constants * upstream/master: (295 commits) Update F1 Keywords to differentiate between semantics of default keyword (#48500) Default constructor suggestion between members (#48318) (#48503) Adjust ERR_PartialMisplaced diagnostic message (#48524) Refactor ChangedText.Merge and add fuzz testing (#48420) Apply suggestions from code review Do not crash on unexpected exception (#48367) Reference the contributing doc in 'Analyzer Suggestion' issue template Apply suggestions from code review Hardcode skipped Regex diagnostic ID as it is not available in CodeStyle layer Add using Skip help link for Regex diagnostic analyzer Add contributing doc for IDE code style analyzer documentation Make db lock static to investigate issue. Update dependencies from https://github.com/dotnet/roslyn build 20201012.2 (#48513) Hook up help link even for AbstractCodeQualityDiagnosticAnalyzer Add destructor intellisense test for record (#48297) Remove unused method (#48429) Fix bug Update src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTag.cs Add more test ...
333fred
added a commit
to 333fred/roslyn
that referenced
this pull request
Oct 13, 2020
* upstream/master: (68 commits) Update F1 Keywords to differentiate between semantics of default keyword (dotnet#48500) Default constructor suggestion between members (dotnet#48318) (dotnet#48503) Adjust ERR_PartialMisplaced diagnostic message (dotnet#48524) Refactor ChangedText.Merge and add fuzz testing (dotnet#48420) Apply suggestions from code review Do not crash on unexpected exception (dotnet#48367) Reference the contributing doc in 'Analyzer Suggestion' issue template Apply suggestions from code review Hardcode skipped Regex diagnostic ID as it is not available in CodeStyle layer Add using Skip help link for Regex diagnostic analyzer Add contributing doc for IDE code style analyzer documentation Make db lock static to investigate issue. Update dependencies from https://github.com/dotnet/roslyn build 20201012.2 (dotnet#48513) Hook up help link even for AbstractCodeQualityDiagnosticAnalyzer Add destructor intellisense test for record (dotnet#48297) Remove unused method (dotnet#48429) Fix bug Update src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTag.cs Add more test ...
RikkiGibson
added a commit
to RikkiGibson/roslyn
that referenced
this pull request
Nov 13, 2020
RikkiGibson
added a commit
that referenced
this pull request
Nov 13, 2020
333fred
added a commit
to 333fred/roslyn
that referenced
this pull request
Nov 16, 2020
* upstream/master: (148 commits) Remove unnecessary ArrayBuilder in MakeCallWithNoExplicitArgument (dotnet#49377) Revert "Skip binary for determinism checking" Use deterministic metadata names for emitted anonymous type fields. (dotnet#49370) Reuse LSP solutions if they don't need re-forking (dotnet#49305) Small nullability fixes (dotnet#49279) Create default arguments during binding (dotnet#49186) Clean nits Backport dotnet#48420 to release/dev16.8 (dotnet#49357) Rewrite AnalyzeFileReference.GetSupportedLanguages without LINQ (dotnet#49337) Use .Any extension of ImmutableArray(Of Symbol) (dotnet#48980) fix 'remove unnecessary cast' when doing bitwise ops on unsigned values. Harden, document, cross-link XunitDisposeHook Simplify x86 hook Fix split comment exporting for VB. Code style fix Overwrite xunit's app domain handling to not call AppDomain.Unload Revert pragma suppression Remove blank line Revert file Move block structure code back to Features layer ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #47234
Fixes #41413
Fixes #39405
I examined the crash dump for a while and I felt like there were multiple possible reasons that we could have gotten into the bad code path that we got into. I wrote a fuzz tester for various combinations of change sets and worked out the stability and correctness issues that it revealed.
I found the implementation was a little goto-heavy and it was making it difficult to solve one of the lingering issues I had found via fuzz testing. I refactored it a bit to use a loop instead and hopefully provide a little more clarity and less brittleness. I find the algorithm is somewhat challenging to reason through, so if you review this you may want to consider running some of the new tests in the debugger or even working through some of the new test cases with pen and paper.
Also note that repro of the original bug(s) in the linked issue is difficult because in most cases the repro steps are "just copy/paste things around and make edits for a while". I can sometimes repro the problem in VS 16.8-preview3 fairly quickly, and sometimes I can muddle around for quite a while without reproing. Therefore it is difficult to just open up the IDE with this change included, edit some code for a while, and be sure that we have completely addressed the root of the problem.
Please take a look @cston @dotnet/roslyn-compiler.