Add null check for textSnapshot#24693
Conversation
|
@CyrusNajmabadi @sharwell @dpoeschl , for review please. Any reason a null check wouldn't be a good idea here? |
|
See #7566 for discussion of why to check for null |
| Contract.ThrowIfNull(textSnapshot); | ||
| if (textSnapshot == null) | ||
| { | ||
| return; |
There was a problem hiding this comment.
This should (minimally) be a continue, or otherwise we won't connect to the rest of the buffers.
jasonmalinowski
left a comment
There was a problem hiding this comment.
This could mean we don't hook up to other buffers which could be bad.
Changed from return to continue and added a non fatal exception
|
@jasonmalinowski , @dpoeschl , @sharwell for review. |
| namespace Microsoft.CodeAnalysis.Editor.Implementation.InlineRename | ||
| { | ||
| // Used to aid the investigation of https://github.com/dotnet/roslyn/issues/7364 | ||
| internal class NullTextBufferException : Exception |
There was a problem hiding this comment.
Nit: I'd move this as private class right beside InitializeOpenBuffers so somebody doesn't get the idea of depending on it.
| Contract.ThrowIfNull(textSnapshot); | ||
| if (textSnapshot == null) | ||
| { | ||
| FatalError.ReportWithoutCrash(new NullTextBufferException(document)); |
There was a problem hiding this comment.
I guess we should probably hold onto text as well?
Moved NullTextBufferException to InlineRenameSession and added SourceText.
|
we need to re-visit how buffer and snapshot got mapped each other. |
|
Approved to merge via link |
Fixes #7364
Customer scenario
In some situations, when customer renames a variable an
Unexpected nulldialog appears and the rename feature is broken until VS is restarted.Bugs this fixes
#7364
Workarounds, if any
Restart VS and try again.
Risk
Very low, just added a null check
Performance impact
None
Is this a regression from a previous update?
No
Root cause analysis
This code did not originally have a null check, but in looking at other uses of the same method it appears a null check is standard. Unable to add a test for this because it can't be predictably reproduced.
How was the bug found?
Customers and internal users
Test documentation updated?
n/a