Skip to content
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.

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

Refactor ChangedText.Merge and add fuzz testing #48420

Merged
merged 19 commits into from
Oct 12, 2020

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Oct 8, 2020

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.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Yup. This helped a lot!

@RikkiGibson
Copy link
Contributor Author

RikkiGibson commented Oct 12, 2020

Could I please get a second review from @dotnet/roslyn-compiler?

@jaredpar
Copy link
Member

Looking

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

nextOldChange:
if (oldIndex < oldChanges.Length)
public UnadjustedNewChange(TextChangeRange range)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public UnadjustedNewChange(TextChangeRange range)
public UnadjustedNewChange(in TextChangeRange range)

Given the size of TextChangeRange consider passing by in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

The else is unnecessary here correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I don't think it's worth changing unless you have a strong opinion here.

src/Compilers/Core/Portable/Text/ChangedText.cs Outdated Show resolved Hide resolved
src/Compilers/Core/Portable/Text/ChangedText.cs Outdated Show resolved Hide resolved
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;
Copy link
Member

Choose a reason for hiding this comment

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

Did u consider under / overflow scenarios here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't but I'm curious what thoughts you may have on how to mitigate such problems

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 16). The added comments are greatly useful!

@RikkiGibson RikkiGibson changed the base branch from release/dev16.8 to master October 12, 2020 21:31
@RikkiGibson RikkiGibson merged commit ed1ed1d into dotnet:master Oct 12, 2020
@ghost ghost added this to the Next milestone Oct 12, 2020
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 RikkiGibson deleted the fix-47234 branch October 15, 2020 18:15
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
  ...
@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants