Fix crash for Equals analysis of types with bad base clauses#42968
Fix crash for Equals analysis of types with bad base clauses#42968RikkiGibson merged 3 commits intodotnet:masterfrom
Conversation
| .Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__Equals)); | ||
| if (isObjectEqualsMethodOrOverride || | ||
| isWellKnownEqualityMethodOrImplementation(compilation, method, receiverType.Type, WellKnownMember.System_IEquatable_T__Equals)) | ||
| if (node.ReceiverOpt is BoundExpression receiver && |
There was a problem hiding this comment.
nit: Consider putting this additional check in the local function itself
| { | ||
| void M(C? other) | ||
| { | ||
| if (Equals(other)) { other.ToString(); } |
There was a problem hiding this comment.
Equals(other) [](start = 12, length = 13)
The relationship between the test scenario and the implementation is not obvious. The implementation checks if we have a receiver, what is the relationship with the base type and the lack of the receiver? #Closed
There was a problem hiding this comment.
I find it surprising as well. It seems like the erroneous base clause causes us to bind the call differently, with a null ReceiverOpt.
I did find that the crash was not produced when calling this.Equals(other). So maybe that would be illustrative to add to the test along with some comment. Or maybe we should go looking in the binder and decide whether the behavior that's happening now is a bug in binding.
There was a problem hiding this comment.
I find it surprising as well. It seems like the erroneous base clause causes us to bind the call differently, with a null ReceiverOpt.
It is quite possible that binder simply creates an erroneous call node in this case, which doesn't really represent anything that we can learn from (perhaps even the method symbol is an ErrorMethodSymbol). Is this the case? I think IOperation factory detects nodes like that and doesn't even surface them as calls. Consider if we should use the same kind of detection in the nullable walker and bail even earlier than the null check that you added.
In reply to: 401748792 [](ancestors = 401748792)
Closes #42960
Would like to make a pass over NullableWalker to make sure we don't have other bad usages of ReceiverOpt (maybe even enable nullable analysis in NullableWalker?)