Learn from calls to Equals methods in NullableWalker #36722

RikkiGibson · 2019-06-24T23:53:07Z

Resolves #36591

This changes the NullableWalker so that we can learn from calls to:

static bool object.Equals(object, object),
static bool object.ReferenceEquals(object, object)
bool object.Equals(object) and overrides
bool IEquatable<T>(T) and implementations
bool IEqualityComparer<T>(T, T) and implementations

Scenarios where we can learn new things about null state:

object.[Reference]Equals(maybeNull, notNull) // when true, changes maybeNull to not-null
object.[Reference]Equals(notNull, null) // when true, changes notNull to maybe-null
object.[Reference]Equals(maybeNull, null) // when false, changes notNull to maybe-null
notNull.Equals(maybeNull) // when true, changes maybeNull to not-null
notNull.Equals(maybeNull) // when true, changes maybeNull to not-null
notNull.Equals(null) // when true, changes nonNull to maybe-null

The notNull.Equals(null) scenario is a little silly (if you called Equals on a null, didn't you already throw?) But I think that this is just such an antipattern that it doesn't matter if we don't do the perfect thing here.

I tried to think of situations where we could get in trouble when using object.ReferenceEquals on nullable value types, but I couldn't think of any where we incorrectly rule out a diagnostic in a scenario that throws an exception at runtime.

jcouv · 2019-06-24T23:57:03Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

            comp.VerifyDiagnostics();
        }
+
+        [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")]


Fact [](start = 9, length = 4)

I'll review tomorrow, but one quick note: please update the speclet: https://github.com/dotnet/roslyn/blob/master/docs/features/nullable-reference-types.md#null-tests #Closed

jcouv · 2019-06-25T20:29:36Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            {
+                return true;
+            }
+            else if (method.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__ReferenceEquals)))


if (method.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__ReferenceEquals))) [](start = 17, length = 98)

nit: else if can be simplified to return method.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__ReferenceEquals)); #Closed

jcouv · 2019-06-25T20:30:33Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            (method, results) = VisitArguments(node, node.Arguments, refKindsOpt, method.Parameters, node.ArgsToParamsOpt,
+                node.Expanded, node.InvokedAsExtensionMethod, method);
+
+            var arguments = node.Arguments;


var [](start = 12, length = 3)

nit: Consider moving all the new logic (the following block) into a new method and turning the single-use helper methods (like IsStaticEqualsMethod) into local functions. #Closed

jcouv · 2019-06-25T20:35:33Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            var members = constructedIEquatableType.GetMembers(WellKnownMemberNames.ObjectEquals);
+            if (members.Length != 1)
+            {
+                // If the Equals method is missing on the interface definition, or it's overloaded, give up.


or it's overloaded, give up. [](start = 80, length = 28)

I don't imagine that the BCL is going to add an Equals overload on object, but it's technically possible for one to be there. ~~Can we pick the one with the object parameter type?~~

Update: Should we use GetWellKnownTypeMember+construct instead of GetWellKnownType+construct+selection? #Closed

The more "common" scenario is that bool IEquatable<T>.Equals(T) is not defined at all, but this is really just a "degrade gracefully in the face of bad metadata" behavior.

re: GetWellKnownTypeMember, I would like to, however I am stumped as to how to actually make the comparison between the method inside the original definition (IEquatable<T>.Equals(T)) and the method inside a constructed symbol (IEquatable<string?>.Equals(string?))

In reply to: 297381621 [](ancestors = 297381621)

jcouv · 2019-06-25T20:40:48Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            var constructedIEquatableEqualsMethod = members[0];
+            if (constructedIEquatableEqualsMethod.Equals(method))
+            {
+                return true;


return true; [](start = 16, length = 12)

This method has two code paths that could return true. I didn't understand why we need that. It seems like the path that uses FindImplementationForInterfaceMember would always work, no? #Closed

I found m1.FindImplementationForInterfaceMember(m2) returned null when m1 and m2 were both e.g. IEquatable<string?>.Equals(string?). I can double check on it.

In reply to: 297383790 [](ancestors = 297383790)

I double checked on this, removing this return breaks the following tests

IEqualityComparerEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue

IEqualityComparer_SubInterfaceEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue

IEqualityComparer_SubSubInterfaceEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue

IEquatableEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue

In reply to: 297392516 [](ancestors = 297392516,297383790)

jcouv · 2019-06-25T20:51:56Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            return method.Equals(implementationMethod);
+        }
+
+        private void LearnFromEqualsMethod(BoundExpression left, TypeWithState leftType, BoundExpression right, TypeWithState rightType)


LearnFromEqualsMethod [](start = 21, length = 21)

Probably deserves a comment for documentation:

a null argument tells us the other argument is maybe-null (ie. this is a pure null test)

equality to an expression that is not-null means the argument is not-null

I see that the call to LearnFromNullTest in AfterLeftChildHasBeenVisited is preceded by a call to SkipReferenceConversions. I wonder if we need to do the same here. It would be good to test IEquatable<B>.Equals(a, null) where a has implicit reference conversion to type B. Testing with user-defined conversions which flip nullability would be useful too.
We may have an issue tracking a problem in this area already.
#Closed

Should we expect InstanceObjectEqualsMethod_MaybeNullExprReferenceConversion_NonNullExpr_NoWarningWhenTrue to catch this scenario?

LearnFromNullTest also strips conversions internally (calls RemoveConversion on the expression ) so I'm also wondering if both steps are even necessary.

In reply to: 297388081 [](ancestors = 297388081)

Yes, InstanceObjectEqualsMethod_MaybeNullExprReferenceConversion_NonNullExpr_NoWarningWhenTrue is good.
I think we can probably remove SkipReferenceConversions from AfterLeftChildHasBeenVisited then. If a test is affected, then it can tell us what test to add for learning from Equals method.

In reply to: 297389419 [](ancestors = 297389419,297388081)

jcouv · 2019-06-25T20:58:23Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+                    // (16,17): warning CS8602: Dereference of a possibly null reference.
+                    //             _ = c1.ToString(); // 2
+                    Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c1").WithLocation(16, 17));
+        }


} [](start = 8, length = 1)

Consider adding tests that don't rely on string? or C? types, but rather also try int? or TOpen?.
Also, let's adding some tests for notNull1 compared to notNull2 (no learning, both remain not-null) and maybeNull1 compared to maybeNull2 (no learning, both remain maybe-null) #Closed

jcouv · 2019-06-25T20:59:05Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            if (members.Length != 1)
+            {
+                // If the Equals method is missing on the interface definition, or it's overloaded, give up.
+                return false;


return false; [](start = 16, length = 13)

do we have tests covering all the branches of these methods (such as missing or malformed types/members)? #Closed

jcouv

Done with review pass (iteration 3)

RikkiGibson · 2019-06-26T00:36:12Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs


                if (operandComparedToNull != null)
                {
-                    operandComparedToNull = SkipReferenceConversions(operandComparedToNull);


operandComparedToNull = SkipReferenceConversions(operandComparedToNull); [](start = 20, length = 72)

FYI @jcouv I found removing this conversion-stripping step did not affect anything. #Closed

RikkiGibson · 2019-06-26T00:46:54Z

@333fred I resolved some conflicts with your PR for ShouldRunNullableDirective. If you have a moment please take a glance at 86084e1 to make sure I didn't mangle anything. #Closed

RikkiGibson · 2019-06-26T17:56:33Z

Looks like bootstrap build is failing only on Mac and Linux. Not sure why yet, but my code is on the stack trace, so I'm pretty sure it's related to the changes I made. #Closed

…ingType from metadata

jcouv · 2019-06-27T22:38:04Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+                return;
+            }
+
+            bool isWellKnownEqualityMethodOrImplementation(MethodSymbol method, WellKnownMember wellKnownMember)


bool [](start = 12, length = 4)

static? #Closed

jcouv · 2019-06-27T22:45:26Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

            }

-            return snt?.GetImplementsLocation(@interface) ?? implementingType.Locations[0];
+            return snt?.GetImplementsLocation(@interface) ?? implementingType.Locations.FirstOrDefault();


FirstOrDefault [](start = 88, length = 14)

potentially returning null Location here seems risky. Let's return Location.None instead in the worst case #Closed

jcouv · 2019-06-27T22:47:57Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

nit: NonNull versus NotNull inconsistency #Closed

jcouv

Done with review pass (iteration 6)

jcouv · 2019-06-27T22:50:00Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

nit: two empty lines below #Closed

jcouv

LGTM Thanks (iteration 7)

333fred · 2019-06-28T20:46:40Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+    }
+}";
+            verify(source0);
+            verify(source0.Replace("object.Equals", "object.ReferenceEquals"));


Nit: consider using a Theory instead, and having the argument for the test be Equals and ReferenceEquals #Closed

Same suggestion for the test of the tests that do this style of multi-verification

In reply to: 298742762 [](ancestors = 298742762)

333fred · 2019-06-28T21:36:14Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+        string? s2 = null;
+        if (object.Equals(s2, s))
+        {
+            _ = s.ToString();


This feels wrong. If this line of code was reached, we will throw a NullReferenceException. #Closed

I understand that this isn't a pure null test, so we can't learn anything from the comparison. But it's a good example of a scenario we could have caught if we differentiated between "This could have null in it" and "This definitely has null in it", and vice versa for not null.

In reply to: 298755212 [](ancestors = 298755212)

Right. At the moment we have three categories ranging from highest confidence to lowest:

a null literal or constant

a not-null state

a maybe-null state

In this test, we're comparing two expressions: one has a not-null state and the other has a maybe-null state. We're more confident in the not-null state, so we leave it alone. #Closed

Will mark this resolved, let me know if there's action I should be taking related to this scenario (I do agree that it's questionable)

In reply to: 298757714 [](ancestors = 298757714)

No, there's nothing you can do, I'm just grouching.

In reply to: 298766107 [](ancestors = 298766107,298757714)

333fred · 2019-06-28T21:53:39Z

src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs


-            // There were 204 well-known types prior to CSharp7
-            Assert.Equal(204, (int)(WellKnownType.CSharp7Sentinel - WellKnownType.First));
+            // There were 205 well-known types prior to CSharp7


Is this comment correct? #Closed

We should change this assertion to check CSharp7Sentinel - First instead. Then it remains true when adding new special types.
The point of CSharp7Sentinel is that our rules for finding well-known types changed a bit in C# 7 with regards to dealing with ambiguities:

// well-known types introduced before CSharp7 allow lookup ambiguity and report a warning DiagnosticBag legacyWarnings = (type <= WellKnownType.CSharp7Sentinel) ? warnings : null;

In reply to: 298759023 [](ancestors = 298759023)

Should this new well-known type stay before the CSharp7Sentinel?

In reply to: 298769217 [](ancestors = 298769217,298759023)

after offline discussion it sounds like this should actually go after the sentinel so that we disallow multiple definitions of the type.

In reply to: 298770457 [](ancestors = 298770457,298769217,298759023)

333fred · 2019-06-28T21:59:45Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+                    }
+                }
+
+                Debug.Assert(constructedMethod != null, "the original definition is present but the constructed method isn't present");


Nit: empty line below #Resolved

333fred · 2019-06-28T22:02:03Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

            }

-            return snt?.GetImplementsLocation(@interface) ?? implementingType.Locations[0];
+            return snt?.GetImplementsLocation(@interface) ?? implementingType.Locations.FirstOrNone();


FirstOrNone [](start = 88, length = 11)

Is this Linq? #Resolved

Extension:

internal static Location FirstOrNone(this ImmutableArray<Location> items) { return items.IsEmpty ? Location.None : items[0]; } ``` #Resolved

333fred · 2019-06-28T22:10:01Z

Done review pass (commit 7). Some code cleanup comments. #Resolved

333fred

LGTM (commit 8)

Learn from calls to Equals methods in NullableWalker

90b2210

RikkiGibson added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jun 24, 2019

RikkiGibson added this to the 16.2.P4 milestone Jun 24, 2019

RikkiGibson requested a review from a team as a code owner June 24, 2019 23:53

jcouv reviewed Jun 24, 2019

View reviewed changes

jcouv self-assigned this Jun 24, 2019

RikkiGibson added 2 commits June 24, 2019 17:08

Document which Equals methods affect nullability analysis

45e0cd8

Fix some NREs in tests

24e504e

jcouv reviewed Jun 25, 2019

View reviewed changes

Respond to feedback and fix MissingSpecialMember test

f361dda

RikkiGibson commented Jun 26, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into nullable-equals

86084e1

RikkiGibson requested a review from jcouv June 26, 2019 00:47

jcouv modified the milestones: 16.2.P4, 16.3 Jun 26, 2019

Allow GetInterfaceLocation to return a null Location for an implement…

cdadb79

…ingType from metadata

jcouv reviewed Jun 27, 2019

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

Copy link

Member

jcouv Jun 27, 2019 •

edited

Loading

Choose a reason for hiding this comment

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

nit: two empty lines below #Closed

Address review feedback

3b565c0

RikkiGibson requested review from a team and jcouv June 27, 2019 23:26

jcouv approved these changes Jun 28, 2019

View reviewed changes

jcouv modified the milestones: 16.3, 16.3.P2 Jun 28, 2019

RikkiGibson requested a review from a team June 28, 2019 02:26

333fred reviewed Jun 28, 2019

View reviewed changes

Address review feedback

afa0647

RikkiGibson requested a review from 333fred July 1, 2019 21:38

333fred approved these changes Jul 1, 2019

View reviewed changes

RikkiGibson merged commit e4019b2 into dotnet:master Jul 1, 2019

RikkiGibson deleted the nullable-equals branch July 1, 2019 23:52

RikkiGibson mentioned this pull request Aug 15, 2019

preview8: Typing ReferenceEquals( crashes VS with IndexOutOfRangeException in NullableWalker.LearnFromEqualsMethod #38010

Closed

Learn from calls to Equals methods in NullableWalker #36722

Learn from calls to Equals methods in NullableWalker #36722

Uh oh!

Conversation

RikkiGibson commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jun 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

RikkiGibson commented Jun 24, 2019 •

edited

Loading

jcouv Jun 24, 2019 •

edited

Loading

jcouv Jun 25, 2019 •

edited

Loading

jcouv Jun 25, 2019 •

edited

Loading

jcouv Jun 25, 2019 •

edited

Loading

jcouv Jun 25, 2019 •

edited

Loading

RikkiGibson Jun 25, 2019 •

edited

Loading

jcouv Jun 25, 2019 •

edited

Loading

jcouv Jun 25, 2019 •

edited

Loading

jcouv Jun 25, 2019 •

edited

Loading

RikkiGibson Jun 26, 2019 •

edited

Loading

RikkiGibson commented Jun 26, 2019 •

edited

Loading

RikkiGibson commented Jun 26, 2019 •

edited

Loading

jcouv Jun 27, 2019 •

edited

Loading

jcouv Jun 27, 2019 •

edited

Loading

jcouv Jun 27, 2019 •

edited

Loading

jcouv Jun 27, 2019 •

edited

Loading

333fred Jun 28, 2019 •

edited

Loading

333fred Jun 28, 2019 •

edited

Loading

jcouv Jun 28, 2019 •

edited by 333fred

Loading

333fred Jun 28, 2019 •

edited

Loading

333fred Jun 28, 2019 •

edited by RikkiGibson

Loading

333fred Jun 28, 2019 •

edited by RikkiGibson

Loading

jcouv Jun 28, 2019 •

edited by RikkiGibson

Loading

333fred commented Jun 28, 2019 •

edited by RikkiGibson

Loading