Fixing VS Crash on invalid "MemberNotNull" #45563
Conversation
| @@ -120,6 +120,7 @@ protected virtual int GetOrCreateSlot(Symbol symbol, int containingSlot = 0, boo | |||
| { | |||
| Debug.Assert(containingSlot >= 0); | |||
|
|
|||
| if (symbol is null) return -1; | |||
There was a problem hiding this comment.
if (symbol is null) return -1 [](start = 12, length = 29)
Can we check for symbol is null in the caller? It feels like we might be hiding a bug if we allow null here. #Resolved
There was a problem hiding this comment.
Consider adding #nullable enable ... #nullable restore around this method and the override in NullableWalker to catch cases where symbol may be null.
Then it seems reasonable to check symbol is null here as well, for robustness, but it would force callers to consider null values as well.
In reply to: 447947315 [](ancestors = 447947315)
There was a problem hiding this comment.
Yeah, putting the check here doesn't seem like the right solution to me. We shouldn't be attempting to get a slot for a non-existant symbol.
In reply to: 447954354 [](ancestors = 447954354,447947315)
There was a problem hiding this comment.
Moved a check to the correct location, added an Assert here. (Commit 8).
In reply to: 447962405 [](ancestors = 447962405,447954354,447947315)
| @@ -9089,6 +9089,45 @@ public class C2 | |||
| Diagnostic(ErrorCode.ERR_BadAttributeArgument, "C.M(null)").WithLocation(20, 6)); | |||
| } | |||
|
|
|||
| [Fact] | |||
| [WorkItem(44049, "https://github.com/dotnet/roslyn/issues/44049")] | |||
| public void MemberNotNullAnnotationCrashes() | |||
There was a problem hiding this comment.
MemberNotNullAnnotationCrashes [](start = 20, length = 30)
Consider moving to NullableReferenceTypesTests.cs since most (all?) of the [MemberNotNull] tests are there.
Also, consider renaming since the test since the test should not crash. Perhaps something like MemberNotNull_InstanceMemberOnStaticMethod. #Resolved
| @@ -1268,14 +1268,16 @@ static MethodSymbol getTopLevelMethod(MethodSymbol method) | |||
| } | |||
| } | |||
|
|
|||
| #nullable enable | |||
| protected override int GetOrCreateSlot(Symbol symbol, int containingSlot = 0, bool forceSlotEvenIfEmpty = false) | |||
There was a problem hiding this comment.
If it's possible for symbol to be null then the type should change to Symbol?. #Resolved
There was a problem hiding this comment.
Shouldn't be possible, added an Assert to make sure in Commit 8.
In reply to: 447978511 [](ancestors = 447978511)
| protected override int GetOrCreateSlot(Symbol symbol, int containingSlot = 0, bool forceSlotEvenIfEmpty = false) | ||
| { | ||
|
|
||
| if (containingSlot > 0 && !IsSlotMember(containingSlot, symbol)) | ||
| if ((containingSlot > 0 && !IsSlotMember(containingSlot, symbol)) || symbol is null) |
There was a problem hiding this comment.
|| symbol is null [](start = 78, length = 17)
We should check symbol is null higher in the callstack where we know what the symbol represents in the codepath for the bug repro. (Sorry, I didn't realize the caller of the base method was the override when I suggested moving the check previously.) #Resolved
There was a problem hiding this comment.
Moved the check to the place where the null object is in Commit 8.
In reply to: 447985704 [](ancestors = 447985704)
kevinsun-dev
added
Area-Compilers
Bug
Feature - Nullable Reference Types
| @@ -715,6 +715,10 @@ int getSlotForFieldOrProperty(Symbol member) | |||
|
|
|||
| if (!isStatic) | |||
| { | |||
| if (MethodThisParameter is null) | |||
There was a problem hiding this comment.
What is the Kind of the member in this scenario? #Closed
| if (MethodThisParameter is null) | ||
| { | ||
| return -1; | ||
| } |
ghost
modified the milestones:
Fixes issue #44049.
Changelog:
NullPointerExceptionsituation