From 90b22103e534eb2791288b510e20661a1c123542 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 21 Jun 2019 16:01:03 -0700 Subject: [PATCH 1/7] Learn from calls to Equals methods in NullableWalker --- .../Portable/FlowAnalysis/NullableWalker.cs | 144 +++- .../Semantics/NullableReferenceTypesTests.cs | 759 ++++++++++++++++++ src/Compilers/Core/Portable/SpecialMember.cs | 1 + src/Compilers/Core/Portable/SpecialMembers.cs | 10 + .../Core/Portable/WellKnownMember.cs | 2 + .../Core/Portable/WellKnownMembers.cs | 22 +- src/Compilers/Core/Portable/WellKnownTypes.cs | 6 +- 7 files changed, 934 insertions(+), 10 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 15d607d3cf6b0..91dbce6639402 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2813,8 +2813,21 @@ private void ReinferMethodAndVisitArguments(BoundCall node, TypeWithState receiv method = (MethodSymbol)AsMemberOfType(receiverType.Type, method); } - method = VisitArguments(node, node.Arguments, refKindsOpt, method.Parameters, node.ArgsToParamsOpt, - node.Expanded, node.InvokedAsExtensionMethod, method).method; + ImmutableArray results; + (method, results) = VisitArguments(node, node.Arguments, refKindsOpt, method.Parameters, node.ArgsToParamsOpt, + node.Expanded, node.InvokedAsExtensionMethod, method); + + var arguments = node.Arguments; + if (IsStaticEqualsMethod(method) || IsIEqualityComparerEqualsMethodOrImplementation(method)) + { + Debug.Assert(arguments.Length == 2); + LearnFromEqualsMethod(arguments[0], results[0].RValueType, arguments[1], results[1].RValueType); + } + else if (IsObjectEqualsMethodOrOverride(method) || IsIEquatableEqualsMethodOrImplementation(method)) + { + Debug.Assert(arguments.Length == 1); + LearnFromEqualsMethod(node.ReceiverOpt, receiverType, arguments[0], results[0].RValueType); + } if (method.MethodKind == MethodKind.LocalFunction) { @@ -2825,6 +2838,133 @@ private void ReinferMethodAndVisitArguments(BoundCall node, TypeWithState receiv SetResult(node, GetReturnTypeWithState(method), method.ReturnTypeWithAnnotations); } + private bool IsStaticEqualsMethod(MethodSymbol method) + { + if (compilation.GetSpecialTypeMember(SpecialMember.System_Object__EqualsObjectObject).Equals(method)) + { + return true; + } + else if (compilation.GetSpecialTypeMember(SpecialMember.System_Object__ReferenceEquals).Equals(method)) + { + return true; + } + else + { + return false; + } + } + + private bool IsObjectEqualsMethodOrOverride(MethodSymbol method) + { + if (method.ParameterCount != 1 + || method.ReturnType.SpecialType != SpecialType.System_Boolean + || method.Name != WellKnownMemberNames.ObjectEquals) + { + return false; + } + + var objectEqualsMethod = compilation.GetSpecialTypeMember(SpecialMember.System_Object__Equals); + return objectEqualsMethod.Equals(method.GetLeastOverriddenMethod(accessingTypeOpt: null)); + } + + + private bool IsIEquatableEqualsMethodOrImplementation(MethodSymbol method) + { + if (method.ParameterCount != 1 + || method.MethodKind != MethodKind.Ordinary + || method.ReturnType.SpecialType != SpecialType.System_Boolean + || method.Name != WellKnownMemberNames.ObjectEquals) + { + return false; + } + + var iEquatableType = compilation.GetWellKnownType(WellKnownType.System_IEquatable_T); + if (iEquatableType is null) + { + return false; + } + + var parameterType = method.Parameters[0].TypeWithAnnotations; + var constructedIEquatableType = iEquatableType.Construct(ImmutableArray.Create(parameterType)); + 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. + return false; + } + + var constructedIEquatableEqualsMethod = members[0]; + if (constructedIEquatableEqualsMethod.Equals(method)) + { + return true; + } + + var implementationMethod = method.ContainingType.FindImplementationForInterfaceMember(constructedIEquatableEqualsMethod); + return method.Equals(implementationMethod); + } + + private bool IsIEqualityComparerEqualsMethodOrImplementation(MethodSymbol method) + { + if (method.ParameterCount != 2 + || !method.Parameters[0].Type.Equals(method.Parameters[1].Type) + || method.MethodKind != MethodKind.Ordinary + || method.ReturnType.SpecialType != SpecialType.System_Boolean + || method.Name != WellKnownMemberNames.ObjectEquals) + { + return false; + } + + var iEqualityComparerType = compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IEqualityComparer_T); + if (iEqualityComparerType is null) + { + return false; + } + + var parameterType = method.Parameters[0].TypeWithAnnotations; + var constructedIEqualityComparerType = iEqualityComparerType.Construct(ImmutableArray.Create(parameterType)); + var members = constructedIEqualityComparerType.GetMembers(WellKnownMemberNames.ObjectEquals); + if (members.Length != 1) + { + // If the Equals method is missing on the interface definition, or it's overloaded, give up. + return false; + } + + var constructedIEqualityComparerEqualsMethod = members[0]; + if (constructedIEqualityComparerEqualsMethod.Equals(method)) + { + return true; + } + + var implementationMethod = method.ContainingType.FindImplementationForInterfaceMember(constructedIEqualityComparerEqualsMethod); + return method.Equals(implementationMethod); + } + + private void LearnFromEqualsMethod(BoundExpression left, TypeWithState leftType, BoundExpression right, TypeWithState rightType) + { + if (left.ConstantValue?.IsNull == true) + { + Split(); + LearnFromNullTest(right, ref StateWhenTrue); + LearnFromNonNullTest(right, ref StateWhenFalse); + } + else if (right.ConstantValue?.IsNull == true) + { + Split(); + LearnFromNullTest(left, ref StateWhenTrue); + LearnFromNonNullTest(left, ref StateWhenFalse); + } + else if (leftType.MayBeNull && rightType.IsNotNull) + { + Split(); + LearnFromNonNullTest(left, ref StateWhenTrue); + } + else if (rightType.MayBeNull && leftType.IsNotNull) + { + Split(); + LearnFromNonNullTest(right, ref StateWhenTrue); + } + } + private TypeWithState VisitCallReceiver(BoundCall node) { var receiverOpt = node.ReceiverOpt; diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index 06a76077578fc..042b0020392aa 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -109527,5 +109527,764 @@ static void M( var comp = CreateCompilation(source); comp.VerifyDiagnostics(); } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_NonNullExpr_LiteralNull_WarnsWhenTrue() + { + var source0 = @" +#nullable enable +class C +{ + static void M(string s) + { + if (object.Equals(s, null)) + { + _ = s.ToString(); // 1 + } + else + { + _ = s.ToString(); + } + } + + static void M2(string s) + { + if (object.Equals(null, s)) + { + _ = s.ToString(); // 2 + } + else + { + _ = s.ToString(); + } + } +}"; + verify(source0); + verify(source0.Replace("object.Equals", "object.ReferenceEquals")); + + void verify(string source) + { + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (9,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(9, 17), + // (21,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(21, 17)); + } + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_NonNullExpr_ConstantNull_WarnsWhenTrue() + { + var source0 = @" +#nullable enable +class C +{ + static void M1(string s) + { + const string? s2 = null; + if (object.Equals(s, s2)) + { + _ = s.ToString(); // 1 + } + else + { + _ = s.ToString(); + } + } + + static void M2(string s) + { + const string? s2 = null; + if (object.Equals(s2, s)) + { + _ = s.ToString(); // 2 + } + else + { + _ = s.ToString(); + } + } +}"; + verify(source0); + verify(source0.Replace("object.Equals", "object.ReferenceEquals")); + + void verify(string source) + { + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (10,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(10, 17), + // (23,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(23, 17)); + } + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_MaybeNullExpr_ConstantNull_NoWarningWhenFalse() + { + var source0 = @" +#nullable enable +class C +{ + static void M1(string? s) + { + const string? s2 = null; + if (object.Equals(s, s2)) + { + _ = s.ToString(); // 1 + } + else + { + _ = s.ToString(); + } + } + + static void M2(string? s) + { + const string? s2 = null; + if (object.Equals(s2, s)) + { + _ = s.ToString(); // 2 + } + else + { + _ = s.ToString(); + } + } +}"; + verify(source0); + verify(source0.Replace("object.Equals", "object.ReferenceEquals")); + + void verify(string source) + { + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (10,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(10, 17), + // (23,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(23, 17)); + } + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_NonNullExpr_MaybeNullExpr_NoWarning() + { + var source = @" +#nullable enable +class C +{ + static void M1(string s) + { + string? s2 = null; + if (object.Equals(s, s2)) + { + _ = s.ToString(); + } + else + { + _ = s.ToString(); + } + } + + static void M2(string s) + { + string? s2 = null; + if (object.Equals(s2, s)) + { + _ = s.ToString(); + } + else + { + _ = s.ToString(); + } + } +}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics(); + + comp = CreateCompilation(source.Replace("object.Equals", "object.ReferenceEquals")); + comp.VerifyDiagnostics(); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + { + var source0 = @" +#nullable enable +class C +{ + static void M1(string? s) + { + if (object.Equals(s, """")) + { + _ = s.ToString(); + } + else + { + _ = s.ToString(); // 1 + } + } + + static void M2(string? s) + { + if (object.Equals("""", s)) + { + _ = s.ToString(); + } + else + { + _ = s.ToString(); // 2 + } + } +}"; + verify(source0); + verify(source0.Replace("object.Equals", "object.ReferenceEquals")); + + void verify(string source) + { + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (13,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(13, 17), + // (25,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(25, 17)); + } + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod__NullableValueTypeExpr_ValueTypeExpr_NoWarningWhenTrue() + { + var source0 = @" +#nullable enable +class C +{ + int? i = 42; + static void M1(C? c) + { + if (object.Equals(c?.i, 42)) + { + _ = c.i.Value.ToString(); + } + else + { + _ = c.i.Value.ToString(); // 1 + } + } + + static void M2(C? c) + { + if (object.Equals(42, c?.i)) + { + _ = c.i.Value.ToString(); + } + else + { + _ = c.i.Value.ToString(); // 2 + } + } +}"; + verify(source0); + verify(source0.Replace("object.Equals", "object.ReferenceEquals")); + + void verify(string source) + { + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (14,17): warning CS8602: Dereference of a possibly null reference. + // _ = c.i.Value.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c").WithLocation(14, 17), + // (14,17): warning CS8629: Nullable value type may be null. + // _ = c.i.Value.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullableValueTypeMayBeNull, "c.i").WithLocation(14, 17), + // (26,17): warning CS8602: Dereference of a possibly null reference. + // _ = c.i.Value.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c").WithLocation(26, 17), + // (26,17): warning CS8629: Nullable value type may be null. + // _ = c.i.Value.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullableValueTypeMayBeNull, "c.i").WithLocation(26, 17)); + } + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void InstanceEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +#nullable enable +class C +{ + static void M1(string? s) + { + if ("""".Equals(s)) + { + _ = s.ToString(); + } + else + { + _ = s.ToString(); // 1 + } + } +}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (13,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(13, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void InstanceEqualsMethod_NonNullExpr_LiteralNull_NoWarningWhenTrue() + { + var source = @" +#nullable enable +class C +{ + static void M1(string s) + { + if (s.Equals(null)) + { + _ = s.ToString(); // 1 + } + else + { + _ = s.ToString(); + } + } +}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (9,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(9, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void InstanceEqualsMethod_IEquatableMissing_MaybeNullExpr_NonNullExpr_Warns() + { + var source = @" +#nullable enable +class C +{ + static void M1(string? s) + { + if ("""".Equals(s)) + { + _ = s.ToString(); // 1 + } + else + { + _ = s.ToString(); // 2 + } + } +}"; + var comp = CreateCompilation(source); + comp.MakeTypeMissing(WellKnownType.System_IEquatable_T); + comp.VerifyDiagnostics( + // (9,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(9, 17), + // (13,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(13, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void IEquatableEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +using System; + +#nullable enable +class C +{ + static void M1(IEquatable equatable, string? s) + { + if (equatable.Equals(s)) + { + _ = s.ToString(); + } + else + { + _ = s.ToString(); // 1 + } + } +}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (15,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(15, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void IEquatableEqualsMethod_NullableVariance_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +using System; + +#nullable enable +class C : IEquatable +{ + public bool Equals(string? s) => throw null!; + + static void M1(C c, string? s) + { + if (c.Equals(s)) + { + _ = s.ToString(); + } + else + { + _ = s.ToString(); // 1 + } + } +}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (17,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(17, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void InstanceEqualsMethod_NullableVariance_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +using System; + +#nullable enable +class C : IEquatable +{ + public bool Equals(C? other) => throw null!; + + static void M1(C? c2) + { + C c1 = new C(); + if (c1.Equals(c2)) + { + _ = c2.ToString(); + } + else + { + _ = c2.ToString(); // 1 + } + } +}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (18,17): warning CS8602: Dereference of a possibly null reference. + // _ = c2.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c2").WithLocation(18, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void InstanceObjectEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +#nullable enable +class C +{ + static void M1(object? o) + { + if ("""".Equals(o)) + { + _ = o.ToString(); + } + else + { + _ = o.ToString(); // 1 + } + } +}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (13,17): warning CS8602: Dereference of a possibly null reference. + // _ = o.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "o").WithLocation(13, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void InstanceObjectEqualsMethod_MaybeNullExprReferenceConversion_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +#nullable enable +class C +{ + static void M1(string? s) + { + if ("""".Equals((object?)s)) + { + _ = s.ToString(); + } + else + { + _ = s.ToString(); // 1 + } + } +}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (13,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(13, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void InstanceObjectEqualsMethod_ConditionalAccessExpr_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +#nullable enable +class C +{ + object? F = default; + + static void M1(C? c) + { + C c1 = new C(); + if (c1.Equals(c?.F)) + { + _ = c.F.ToString(); + } + else + { + _ = c.F.ToString(); // 1, 2 + } + } +}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (16,17): warning CS8602: Dereference of a possibly null reference. + // _ = c.F.ToString(); // 1, 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c").WithLocation(16, 17), + // (16,17): warning CS8602: Dereference of a possibly null reference. + // _ = c.F.ToString(); // 1, 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c.F").WithLocation(16, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void IEqualityComparerEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +using System.Collections.Generic; + +#nullable enable +class C +{ + static void M1(IEqualityComparer comparer, string? s) + { + if (comparer.Equals("""", s)) + { + _ = s.ToString(); + } + else + { + _ = s.ToString(); // 1 + } + } +}"; + + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (15,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(15, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void EqualityComparerEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +using System.Collections.Generic; + +#nullable enable +class C +{ + static void M1(EqualityComparer comparer, string? s) + { + if (comparer.Equals("""", s)) + { + _ = s.ToString(); + } + else + { + _ = s.ToString(); // 1 + } + } +}"; + + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (15,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(15, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void EqualityComparerEqualsMethod_IEqualityComparerMissing_MaybeNullExpr_NonNullExpr_Warns() + { + var source = @" +using System.Collections.Generic; + +#nullable enable +class C +{ + static void M1(EqualityComparer comparer, string? s) + { + if (comparer.Equals("""", s)) + { + _ = s.ToString(); // 1 + } + else + { + _ = s.ToString(); // 2 + } + } +}"; + + var comp = CreateCompilation(source); + comp.MakeTypeMissing(WellKnownType.System_Collections_Generic_IEqualityComparer_T); + comp.VerifyDiagnostics( + // (11,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(11, 17), + // (15,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(15, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void IEqualityComparer_SubInterfaceEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +using System.Collections.Generic; + +#nullable enable + +interface MyEqualityComparer : IEqualityComparer { } + +class C +{ + static void M1(MyEqualityComparer comparer, string? s) + { + if (comparer.Equals("""", s)) + { + _ = s.ToString(); + } + else + { + _ = s.ToString(); // 1 + } + } +}"; + + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (18,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(18, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void IEqualityComparer_SubSubInterfaceEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +using System.Collections.Generic; + +#nullable enable + +interface MyEqualityComparer : IEqualityComparer { } +interface MySubEqualityComparer : MyEqualityComparer { } + +class C +{ + static void M1(MySubEqualityComparer comparer, string? s) + { + if (comparer.Equals("""", s)) + { + _ = s.ToString(); + } + else + { + _ = s.ToString(); // 1 + } + } +}"; + + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (19,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(19, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void OverrideEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +#nullable enable +class C +{ + public override bool Equals(object? other) => throw null!; + public override int GetHashCode() => throw null!; + + static void M1(C? c1) + { + C c2 = new C(); + if (c2.Equals(c1)) + { + _ = c1.ToString(); + } + else + { + _ = c1.ToString(); // 1 + } + } +}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (17,17): warning CS8602: Dereference of a possibly null reference. + // _ = c1.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c1").WithLocation(17, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void NotImplementingEqualsMethod_MaybeNullExpr_NonNullExpr_Warns() + { + var source = @" +#nullable enable +class C +{ + public bool Equals(C? other) => throw null!; + + static void M1(C? c1) + { + C c2 = new C(); + if (c2.Equals(c1)) + { + _ = c1.ToString(); // 1 + } + else + { + _ = c1.ToString(); // 2 + } + } +}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (12,17): warning CS8602: Dereference of a possibly null reference. + // _ = c1.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c1").WithLocation(12, 17), + // (16,17): warning CS8602: Dereference of a possibly null reference. + // _ = c1.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c1").WithLocation(16, 17)); + } } } diff --git a/src/Compilers/Core/Portable/SpecialMember.cs b/src/Compilers/Core/Portable/SpecialMember.cs index 94d8f6119b93b..d182c41200c81 100644 --- a/src/Compilers/Core/Portable/SpecialMember.cs +++ b/src/Compilers/Core/Portable/SpecialMember.cs @@ -122,6 +122,7 @@ internal enum SpecialMember System_Object__GetHashCode, System_Object__Equals, + System_Object__EqualsObjectObject, System_Object__ToString, System_Object__ReferenceEquals, diff --git a/src/Compilers/Core/Portable/SpecialMembers.cs b/src/Compilers/Core/Portable/SpecialMembers.cs index bf31714bd22c8..7c69bce1125cb 100644 --- a/src/Compilers/Core/Portable/SpecialMembers.cs +++ b/src/Compilers/Core/Portable/SpecialMembers.cs @@ -834,6 +834,15 @@ static SpecialMembers() (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Boolean, (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Object, + // System_Object__EqualsObjectObject + (byte)(MemberFlags.Method | MemberFlags.Static), // Flags + (byte)SpecialType.System_Object, // DeclaringTypeId + 0, // Arity + 2, // Method Signature + (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Boolean, + (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Object, + (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Object, + // System_Object__ToString (byte)(MemberFlags.Method | MemberFlags.Virtual), // Flags (byte)SpecialType.System_Object, // DeclaringTypeId @@ -1099,6 +1108,7 @@ static SpecialMembers() "GetUpperBound", // System_Array__GetUpperBound "GetHashCode", // System_Object__GetHashCode "Equals", // System_Object__Equals + "Equals", // System_Object__EqualsObjectObject "ToString", // System_Object__ToString "ReferenceEquals", // System_Object__ReferenceEquals "op_Explicit", // System_IntPtr__op_Explicit_ToPointer diff --git a/src/Compilers/Core/Portable/WellKnownMember.cs b/src/Compilers/Core/Portable/WellKnownMember.cs index d0514fd21cf10..c382803b83a20 100644 --- a/src/Compilers/Core/Portable/WellKnownMember.cs +++ b/src/Compilers/Core/Portable/WellKnownMember.cs @@ -68,6 +68,8 @@ internal enum WellKnownMember System_IEquatable_T__Equals, + System_Collections_Generic_IEqualityComparer_T__Equals, + System_Collections_Generic_EqualityComparer_T__Equals, System_Collections_Generic_EqualityComparer_T__GetHashCode, System_Collections_Generic_EqualityComparer_T__get_Default, diff --git a/src/Compilers/Core/Portable/WellKnownMembers.cs b/src/Compilers/Core/Portable/WellKnownMembers.cs index 1168db951e2e2..8ae3204532414 100644 --- a/src/Compilers/Core/Portable/WellKnownMembers.cs +++ b/src/Compilers/Core/Portable/WellKnownMembers.cs @@ -448,6 +448,15 @@ static WellKnownMembers() (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Boolean, // Return Type (byte)SignatureTypeCode.GenericTypeParameter, 0, + // System_Collections_Generic_IEqualityComparer_T__Equals + (byte)(MemberFlags.Method | MemberFlags.Virtual), // Flags + (byte)WellKnownType.System_Collections_Generic_IEqualityComparer_T, // DeclaringTypeId + 0, // Arity + 2, // Method Signature + (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Boolean, // Return Type + (byte)SignatureTypeCode.GenericTypeParameter, 0, + (byte)SignatureTypeCode.GenericTypeParameter, 0, + // System_Collections_Generic_EqualityComparer_T__Equals (byte)(MemberFlags.Method | MemberFlags.Virtual), // Flags (byte)WellKnownType.System_Collections_Generic_EqualityComparer_T, // DeclaringTypeId @@ -2610,31 +2619,31 @@ static WellKnownMembers() // System_ValueTuple_T5__Item1 (byte)MemberFlags.Field, // Flags - (byte)WellKnownType.System_ValueTuple_T5, // DeclaringTypeId + (byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_ValueTuple_T5 - WellKnownType.ExtSentinel), // DeclaringTypeId 0, // Arity (byte)SignatureTypeCode.GenericTypeParameter, 0, // Field Signature // System_ValueTuple_T5__Item2 (byte)MemberFlags.Field, // Flags - (byte)WellKnownType.System_ValueTuple_T5, // DeclaringTypeId + (byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_ValueTuple_T5 - WellKnownType.ExtSentinel), // DeclaringTypeId 0, // Arity (byte)SignatureTypeCode.GenericTypeParameter, 1, // Field Signature // System_ValueTuple_T5__Item3 (byte)MemberFlags.Field, // Flags - (byte)WellKnownType.System_ValueTuple_T5, // DeclaringTypeId + (byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_ValueTuple_T5 - WellKnownType.ExtSentinel), // DeclaringTypeId 0, // Arity (byte)SignatureTypeCode.GenericTypeParameter, 2, // Field Signature // System_ValueTuple_T5__Item4 (byte)MemberFlags.Field, // Flags - (byte)WellKnownType.System_ValueTuple_T5, // DeclaringTypeId + (byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_ValueTuple_T5 - WellKnownType.ExtSentinel), // DeclaringTypeId 0, // Arity (byte)SignatureTypeCode.GenericTypeParameter, 3, // Field Signature // System_ValueTuple_T5__Item5 (byte)MemberFlags.Field, // Flags - (byte)WellKnownType.System_ValueTuple_T5, // DeclaringTypeId + (byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_ValueTuple_T5 - WellKnownType.ExtSentinel), // DeclaringTypeId 0, // Arity (byte)SignatureTypeCode.GenericTypeParameter, 4, // Field Signature @@ -2804,7 +2813,7 @@ static WellKnownMembers() // System_ValueTuple_T_T2_T3_T4_T5__ctor (byte)MemberFlags.Constructor, // Flags - (byte)WellKnownType.System_ValueTuple_T5, // DeclaringTypeId + (byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_ValueTuple_T5 - WellKnownType.ExtSentinel), // DeclaringTypeId 0, // Arity 5, // Method Signature (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Void, // Return Type @@ -3484,6 +3493,7 @@ static WellKnownMembers() "GetFieldFromHandle", // System_Reflection_FieldInfo__GetFieldFromHandle2 "Value", // System_Reflection_Missing__Value "Equals", // System_IEquatable_T__Equals + "Equals", // System_Collections_Generic_IEqualityComparer_T__Equals "Equals", // System_Collections_Generic_EqualityComparer_T__Equals "GetHashCode", // System_Collections_Generic_EqualityComparer_T__GetHashCode "get_Default", // System_Collections_Generic_EqualityComparer_T__get_Default diff --git a/src/Compilers/Core/Portable/WellKnownTypes.cs b/src/Compilers/Core/Portable/WellKnownTypes.cs index 9b8c212fb3236..a3e1d9591c9d6 100644 --- a/src/Compilers/Core/Portable/WellKnownTypes.cs +++ b/src/Compilers/Core/Portable/WellKnownTypes.cs @@ -189,6 +189,7 @@ internal enum WellKnownType System_Collections_IList, System_Collections_ICollection, + System_Collections_Generic_IEqualityComparer_T, System_Collections_Generic_EqualityComparer_T, System_Collections_Generic_List_T, System_Collections_Generic_IDictionary_KV, @@ -252,10 +253,10 @@ internal enum WellKnownType System_ValueTuple_T2, System_ValueTuple_T3, System_ValueTuple_T4, - System_ValueTuple_T5, ExtSentinel, // Not a real type, just a marker for types above 255 and strictly below 512 + System_ValueTuple_T5, System_ValueTuple_T6, System_ValueTuple_T7, System_ValueTuple_TRest, @@ -488,6 +489,7 @@ internal static class WellKnownTypes "System.Collections.IList", "System.Collections.ICollection", + "System.Collections.Generic.IEqualityComparer`1", "System.Collections.Generic.EqualityComparer`1", "System.Collections.Generic.List`1", "System.Collections.Generic.IDictionary`2", @@ -550,10 +552,10 @@ internal static class WellKnownTypes "System.ValueTuple`2", "System.ValueTuple`3", "System.ValueTuple`4", - "System.ValueTuple`5", "", // extension marker + "System.ValueTuple`5", "System.ValueTuple`6", "System.ValueTuple`7", "System.ValueTuple`8", From 45e0cd8a2526648f0b8f76ee5f8c1013c311f341 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 24 Jun 2019 17:08:11 -0700 Subject: [PATCH 2/7] Document which Equals methods affect nullability analysis --- docs/features/nullable-reference-types.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/features/nullable-reference-types.md b/docs/features/nullable-reference-types.md index bdf189e22b40e..62fc7da0a445b 100644 --- a/docs/features/nullable-reference-types.md +++ b/docs/features/nullable-reference-types.md @@ -39,6 +39,12 @@ If the analysis determines that a null check always (or never) passes, a hidden A number of null checks affect the flow state when tested for: - comparisons to `null`: `x == null` and `x != null` - `is` operator: `x is null`, `x is K` (where `K` is a constant), `x is string`, `x is string s` +- calls to well-known equality methods, including: + - `static bool object.Equals(object, object)` + - `static bool object.ReferenceEquals(object, object)` + - `bool object.Equals(object)` and overrides + - `bool IEquatable(T)` and implementations + - `bool IEqualityComparer(T, T)` and implementations Invocation of methods annotated with the following attributes will also affect flow analysis: - simple pre-conditions: `[AllowNull]` and `[DisallowNull]` From 24e504eb038a716657d819fffe79d95771395cf3 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Tue, 25 Jun 2019 12:25:48 -0700 Subject: [PATCH 3/7] Fix some NREs in tests --- .../CSharp/Portable/FlowAnalysis/NullableWalker.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 91dbce6639402..f49470bdb9513 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2840,11 +2840,11 @@ private void ReinferMethodAndVisitArguments(BoundCall node, TypeWithState receiv private bool IsStaticEqualsMethod(MethodSymbol method) { - if (compilation.GetSpecialTypeMember(SpecialMember.System_Object__EqualsObjectObject).Equals(method)) + if (method.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__EqualsObjectObject))) { return true; } - else if (compilation.GetSpecialTypeMember(SpecialMember.System_Object__ReferenceEquals).Equals(method)) + else if (method.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__ReferenceEquals))) { return true; } @@ -2864,7 +2864,7 @@ private bool IsObjectEqualsMethodOrOverride(MethodSymbol method) } var objectEqualsMethod = compilation.GetSpecialTypeMember(SpecialMember.System_Object__Equals); - return objectEqualsMethod.Equals(method.GetLeastOverriddenMethod(accessingTypeOpt: null)); + return method.GetLeastOverriddenMethod(accessingTypeOpt: null).Equals(objectEqualsMethod); } From f361dda76222b4656ec4bc7b11c9e86442b4aa1c Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Tue, 25 Jun 2019 17:29:39 -0700 Subject: [PATCH 4/7] Respond to feedback and fix MissingSpecialMember test --- .../Portable/FlowAnalysis/NullableWalker.cs | 188 ++++++-------- .../Semantics/NullableReferenceTypesTests.cs | 243 +++++++++++++++++- .../Symbol/Symbols/MissingSpecialMember.cs | 5 +- 3 files changed, 313 insertions(+), 123 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index f49470bdb9513..956adf07ea56a 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2164,8 +2164,6 @@ protected override void AfterLeftChildHasBeenVisited(BoundBinaryOperator binary) if (operandComparedToNull != null) { - operandComparedToNull = SkipReferenceConversions(operandComparedToNull); - // Set all nested conditional slots. For example in a?.b?.c we'll set a, b, and c. bool nonNullCase = op != BinaryOperatorKind.Equal; // true represents WhenTrue splitAndLearnFromNonNullTest(operandComparedToNull, whenTrue: nonNullCase); @@ -2817,17 +2815,7 @@ private void ReinferMethodAndVisitArguments(BoundCall node, TypeWithState receiv (method, results) = VisitArguments(node, node.Arguments, refKindsOpt, method.Parameters, node.ArgsToParamsOpt, node.Expanded, node.InvokedAsExtensionMethod, method); - var arguments = node.Arguments; - if (IsStaticEqualsMethod(method) || IsIEqualityComparerEqualsMethodOrImplementation(method)) - { - Debug.Assert(arguments.Length == 2); - LearnFromEqualsMethod(arguments[0], results[0].RValueType, arguments[1], results[1].RValueType); - } - else if (IsObjectEqualsMethodOrOverride(method) || IsIEquatableEqualsMethodOrImplementation(method)) - { - Debug.Assert(arguments.Length == 1); - LearnFromEqualsMethod(node.ReceiverOpt, receiverType, arguments[0], results[0].RValueType); - } + LearnFromEqualsMethod(method, node, receiverType, results); if (method.MethodKind == MethodKind.LocalFunction) { @@ -2838,130 +2826,100 @@ private void ReinferMethodAndVisitArguments(BoundCall node, TypeWithState receiv SetResult(node, GetReturnTypeWithState(method), method.ReturnTypeWithAnnotations); } - private bool IsStaticEqualsMethod(MethodSymbol method) - { - if (method.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__EqualsObjectObject))) - { - return true; - } - else if (method.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__ReferenceEquals))) - { - return true; - } - else - { - return false; - } - } - - private bool IsObjectEqualsMethodOrOverride(MethodSymbol method) + private void LearnFromEqualsMethod(MethodSymbol method, BoundCall node, TypeWithState receiverType, ImmutableArray results) { - if (method.ParameterCount != 1 + // easy out + var parameterCount = method.ParameterCount; + if ((parameterCount != 1 && parameterCount != 2) + || method.MethodKind != MethodKind.Ordinary || method.ReturnType.SpecialType != SpecialType.System_Boolean - || method.Name != WellKnownMemberNames.ObjectEquals) + || (method.Name != SpecialMembers.GetDescriptor(SpecialMember.System_Object__Equals).Name + && method.Name != SpecialMembers.GetDescriptor(SpecialMember.System_Object__ReferenceEquals).Name)) { - return false; + return; } - var objectEqualsMethod = compilation.GetSpecialTypeMember(SpecialMember.System_Object__Equals); - return method.GetLeastOverriddenMethod(accessingTypeOpt: null).Equals(objectEqualsMethod); - } - + var arguments = node.Arguments; - private bool IsIEquatableEqualsMethodOrImplementation(MethodSymbol method) - { - if (method.ParameterCount != 1 - || method.MethodKind != MethodKind.Ordinary - || method.ReturnType.SpecialType != SpecialType.System_Boolean - || method.Name != WellKnownMemberNames.ObjectEquals) + var isStaticEqualsMethod = method.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__EqualsObjectObject)) + || method.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__ReferenceEquals)); + if (isStaticEqualsMethod || isWellKnownEqualityMethodOrImplementation(method, WellKnownMember.System_Collections_Generic_IEqualityComparer_T__Equals)) { - return false; + Debug.Assert(arguments.Length == 2); + learnFromEqualsMethodArguments(arguments[0], results[0].RValueType, arguments[1], results[1].RValueType); + return; } - var iEquatableType = compilation.GetWellKnownType(WellKnownType.System_IEquatable_T); - if (iEquatableType is null) + var isObjectEqualsMethodOrOverride = method.GetLeastOverriddenMethod(accessingTypeOpt: null) + .Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__Equals)); + if (isObjectEqualsMethodOrOverride || isWellKnownEqualityMethodOrImplementation(method, WellKnownMember.System_IEquatable_T__Equals)) { - return false; + Debug.Assert(arguments.Length == 1); + learnFromEqualsMethodArguments(node.ReceiverOpt, receiverType, arguments[0], results[0].RValueType); + return; } - var parameterType = method.Parameters[0].TypeWithAnnotations; - var constructedIEquatableType = iEquatableType.Construct(ImmutableArray.Create(parameterType)); - var members = constructedIEquatableType.GetMembers(WellKnownMemberNames.ObjectEquals); - if (members.Length != 1) + bool isWellKnownEqualityMethodOrImplementation(MethodSymbol method, WellKnownMember wellKnownMember) { - // If the Equals method is missing on the interface definition, or it's overloaded, give up. - return false; - } + var wellKnownMethod = compilation.GetWellKnownTypeMember(wellKnownMember); + if (wellKnownMethod is null) + { + return false; + } - var constructedIEquatableEqualsMethod = members[0]; - if (constructedIEquatableEqualsMethod.Equals(method)) - { - return true; - } + var wellKnownType = wellKnownMethod.ContainingType; + var parameterType = method.Parameters[0].TypeWithAnnotations; + var constructedType = wellKnownType.Construct(ImmutableArray.Create(parameterType)); - var implementationMethod = method.ContainingType.FindImplementationForInterfaceMember(constructedIEquatableEqualsMethod); - return method.Equals(implementationMethod); - } + Symbol constructedMethod = null; + foreach (var member in constructedType.GetMembers(WellKnownMemberNames.ObjectEquals)) + { + if (member.OriginalDefinition.Equals(wellKnownMethod)) + { + constructedMethod = member; + break; + } + } - private bool IsIEqualityComparerEqualsMethodOrImplementation(MethodSymbol method) - { - if (method.ParameterCount != 2 - || !method.Parameters[0].Type.Equals(method.Parameters[1].Type) - || method.MethodKind != MethodKind.Ordinary - || method.ReturnType.SpecialType != SpecialType.System_Boolean - || method.Name != WellKnownMemberNames.ObjectEquals) - { - return false; - } + Debug.Assert(constructedMethod != null, "the original definition is present but the constructed method isn't present"); - var iEqualityComparerType = compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IEqualityComparer_T); - if (iEqualityComparerType is null) - { - return false; - } - var parameterType = method.Parameters[0].TypeWithAnnotations; - var constructedIEqualityComparerType = iEqualityComparerType.Construct(ImmutableArray.Create(parameterType)); - var members = constructedIEqualityComparerType.GetMembers(WellKnownMemberNames.ObjectEquals); - if (members.Length != 1) - { - // If the Equals method is missing on the interface definition, or it's overloaded, give up. - return false; - } + // FindImplementationForInterfaceMember doesn't check if this method is itself the interface method we're looking for + if (constructedMethod.Equals(method)) + { + return true; + } - var constructedIEqualityComparerEqualsMethod = members[0]; - if (constructedIEqualityComparerEqualsMethod.Equals(method)) - { - return true; + var implementationMethod = method.ContainingType.FindImplementationForInterfaceMember(constructedMethod); + return method.Equals(implementationMethod); } - var implementationMethod = method.ContainingType.FindImplementationForInterfaceMember(constructedIEqualityComparerEqualsMethod); - return method.Equals(implementationMethod); - } - - private void LearnFromEqualsMethod(BoundExpression left, TypeWithState leftType, BoundExpression right, TypeWithState rightType) - { - if (left.ConstantValue?.IsNull == true) - { - Split(); - LearnFromNullTest(right, ref StateWhenTrue); - LearnFromNonNullTest(right, ref StateWhenFalse); - } - else if (right.ConstantValue?.IsNull == true) + void learnFromEqualsMethodArguments(BoundExpression left, TypeWithState leftType, BoundExpression right, TypeWithState rightType) { - Split(); - LearnFromNullTest(left, ref StateWhenTrue); - LearnFromNonNullTest(left, ref StateWhenFalse); - } - else if (leftType.MayBeNull && rightType.IsNotNull) - { - Split(); - LearnFromNonNullTest(left, ref StateWhenTrue); - } - else if (rightType.MayBeNull && leftType.IsNotNull) - { - Split(); - LearnFromNonNullTest(right, ref StateWhenTrue); + // comparing anything to a null literal gives maybe-null when true and not-null when false + // comparing a maybe-null to a not-null gives us not-null when true, nothing learned when false + if (left.ConstantValue?.IsNull == true) + { + Split(); + LearnFromNullTest(right, ref StateWhenTrue); + LearnFromNonNullTest(right, ref StateWhenFalse); + } + else if (right.ConstantValue?.IsNull == true) + { + Split(); + LearnFromNullTest(left, ref StateWhenTrue); + LearnFromNonNullTest(left, ref StateWhenFalse); + } + else if (leftType.MayBeNull && rightType.IsNotNull) + { + Split(); + LearnFromNonNullTest(left, ref StateWhenTrue); + } + else if (rightType.MayBeNull && leftType.IsNotNull) + { + Split(); + LearnFromNonNullTest(right, ref StateWhenTrue); + } } } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index 042b0020392aa..b0605be190a54 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -109575,6 +109575,120 @@ void verify(string source) } } + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_NonNullExpr_NotNullExpr_NoWarning() + { + var source0 = @" +#nullable enable +class C +{ + static void M(string s1, string s2) + { + if (object.Equals(s1, s2)) + { + _ = s1.ToString(); + _ = s2.ToString(); + } + else + { + _ = s1.ToString(); + _ = s2.ToString(); + } + } + + static void M2(string s1, string s2) + { + if (object.Equals(s2, s1)) + { + _ = s1.ToString(); + _ = s2.ToString(); + } + else + { + _ = s1.ToString(); + _ = s2.ToString(); + } + } +}"; + verify(source0); + verify(source0.Replace("object.Equals", "object.ReferenceEquals")); + + void verify(string source) + { + var comp = CreateCompilation(source); + comp.VerifyDiagnostics(); + } + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_MaybeNullExpr_MaybeNullExpr_Warns() + { + var source0 = @" +#nullable enable +class C +{ + static void M(string? s1, string? s2) + { + if (object.Equals(s1, s2)) + { + _ = s1.ToString(); // 1 + _ = s2.ToString(); // 2 + } + else + { + _ = s1.ToString(); // 3 + _ = s2.ToString(); // 4 + } + } + + static void M2(string? s1, string? s2) + { + if (object.Equals(s2, s1)) + { + _ = s1.ToString(); // 5 + _ = s2.ToString(); // 6 + } + else + { + _ = s1.ToString(); // 7 + _ = s2.ToString(); // 8 + } + } +}"; + verify(source0); + verify(source0.Replace("object.Equals", "object.ReferenceEquals")); + + void verify(string source) + { + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (9,17): warning CS8602: Dereference of a possibly null reference. + // _ = s1.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s1").WithLocation(9, 17), + // (10,17): warning CS8602: Dereference of a possibly null reference. + // _ = s2.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(10, 17), + // (14,17): warning CS8602: Dereference of a possibly null reference. + // _ = s1.ToString(); // 3 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s1").WithLocation(14, 17), + // (15,17): warning CS8602: Dereference of a possibly null reference. + // _ = s2.ToString(); // 4 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(15, 17), + // (23,17): warning CS8602: Dereference of a possibly null reference. + // _ = s1.ToString(); // 5 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s1").WithLocation(23, 17), + // (24,17): warning CS8602: Dereference of a possibly null reference. + // _ = s2.ToString(); // 6 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(24, 17), + // (28,17): warning CS8602: Dereference of a possibly null reference. + // _ = s1.ToString(); // 7 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s1").WithLocation(28, 17), + // (29,17): warning CS8602: Dereference of a possibly null reference. + // _ = s2.ToString(); // 8 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(29, 17)); + } + } + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] public void StaticEqualsMethod_NonNullExpr_ConstantNull_WarnsWhenTrue() { @@ -109885,15 +109999,104 @@ static void M1(string? s) } } }"; + var expected = new[] + { + // (9,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(9, 17), + // (13,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(13, 17) + }; var comp = CreateCompilation(source); comp.MakeTypeMissing(WellKnownType.System_IEquatable_T); + comp.VerifyDiagnostics(expected); + + comp = CreateCompilation(source); + comp.MakeMemberMissing(WellKnownMember.System_IEquatable_T__Equals); + comp.VerifyDiagnostics(expected); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void InstanceEqualsMethod_IEquatableEqualsOverloaded_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +#pragma warning disable CS0436 +namespace System +{ + interface IEquatable + { + bool Equals(T t, int compareFlags); + bool Equals(T t); + } +} + +#nullable enable +class C : System.IEquatable +{ + public bool Equals(C? other) => throw null!; + public bool Equals(C? other, int compareFlags) => throw null!; + + static void M1(C? c) + { + C c1 = new C(); + if (c1.Equals(c)) + { + _ = c.ToString(); + } + else + { + _ = c.ToString(); // 1 + } + } +}"; + var comp = CreateCompilation(source); comp.VerifyDiagnostics( - // (9,17): warning CS8602: Dereference of a possibly null reference. - // _ = s.ToString(); // 1 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(9, 17), - // (13,17): warning CS8602: Dereference of a possibly null reference. - // _ = s.ToString(); // 2 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(13, 17)); + // (27,17): warning CS8602: Dereference of a possibly null reference. + // _ = c.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c").WithLocation(27, 17)); + } + + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void InstanceEqualsMethod_IEquatableEqualsBadSignature_MaybeNullExpr_NonNullExpr_Warns() + { + var source = @" +#pragma warning disable CS0436 +namespace System +{ + interface IEquatable + { + bool Equals(T t, int compareFlags); + } +} + +#nullable enable +class C : System.IEquatable +{ + public bool Equals(C? c) => throw null!; + public bool Equals(C? c, int compareFlags) => throw null!; + + static void M1(C? c) + { + C c1 = new C(); + if (c1.Equals(c)) + { + _ = c.ToString(); // 1 + } + else + { + _ = c.ToString(); // 2 + } + } +}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (22,17): warning CS8602: Dereference of a possibly null reference. + // _ = c.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c").WithLocation(22, 17), + // (26,17): warning CS8602: Dereference of a possibly null reference. + // _ = c.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c").WithLocation(26, 17)); } [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] @@ -109924,6 +110127,34 @@ static void M1(IEquatable equatable, string? s) Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(15, 17)); } + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void IEquatableEqualsMethod_OpenType_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + { + var source = @" +using System; + +#nullable enable +class C +{ + static void M1(IEquatable equatable, T? t) where T : class + { + if (equatable.Equals(t)) + { + _ = t.ToString(); + } + else + { + _ = t.ToString(); // 1 + } + } +}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (15,17): warning CS8602: Dereference of a possibly null reference. + // _ = t.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "t").WithLocation(15, 17)); + } + [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] public void IEquatableEqualsMethod_NullableVariance_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() { diff --git a/src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs b/src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs index bd253e6013321..33695a0d8395e 100644 --- a/src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs +++ b/src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs @@ -803,6 +803,7 @@ public void AllWellKnownTypesBeforeCSharp7() WellKnownType.System_Collections_IList, WellKnownType.System_Collections_ICollection, + WellKnownType.System_Collections_Generic_IEqualityComparer_T, WellKnownType.System_Collections_Generic_EqualityComparer_T, WellKnownType.System_Collections_Generic_List_T, WellKnownType.System_Collections_Generic_IDictionary_KV, @@ -864,8 +865,8 @@ public void AllWellKnownTypesBeforeCSharp7() Assert.True(type <= WellKnownType.CSharp7Sentinel); } - // 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 + Assert.Equal(205, (int)(WellKnownType.CSharp7Sentinel - WellKnownType.First)); } [Fact] From cdadb79ef8ddb369fe165c7f7c3c9e73603449a6 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Thu, 27 Jun 2019 15:16:19 -0700 Subject: [PATCH 5/7] Allow GetInterfaceLocation to return a null Location for an implementingType from metadata --- src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs index 1efba690bbaa9..5bf429b1752ba 100644 --- a/src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs @@ -1705,7 +1705,7 @@ private static Location GetInterfaceLocation(Symbol interfaceMember, TypeSymbol snt = implementingType as SourceMemberContainerTypeSymbol; } - return snt?.GetImplementsLocation(@interface) ?? implementingType.Locations[0]; + return snt?.GetImplementsLocation(@interface) ?? implementingType.Locations.FirstOrDefault(); } private static bool ReportAnyMismatchedConstraints(MethodSymbol interfaceMethod, TypeSymbol implementingType, MethodSymbol implicitImpl, DiagnosticBag diagnostics) From 3b565c0c4c67cf384f9447c72add37c7f4972814 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Thu, 27 Jun 2019 16:24:40 -0700 Subject: [PATCH 6/7] Address review feedback --- .../CSharp/Portable/FlowAnalysis/NullableWalker.cs | 8 +++++--- src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs | 2 +- .../Semantic/Semantics/NullableReferenceTypesTests.cs | 3 +-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index a39d072498d8d..2c46d2d23baab 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2843,7 +2843,8 @@ private void LearnFromEqualsMethod(MethodSymbol method, BoundCall node, TypeWith var isStaticEqualsMethod = method.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__EqualsObjectObject)) || method.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__ReferenceEquals)); - if (isStaticEqualsMethod || isWellKnownEqualityMethodOrImplementation(method, WellKnownMember.System_Collections_Generic_IEqualityComparer_T__Equals)) + if (isStaticEqualsMethod || + isWellKnownEqualityMethodOrImplementation(compilation, method, WellKnownMember.System_Collections_Generic_IEqualityComparer_T__Equals)) { Debug.Assert(arguments.Length == 2); learnFromEqualsMethodArguments(arguments[0], results[0].RValueType, arguments[1], results[1].RValueType); @@ -2852,14 +2853,15 @@ private void LearnFromEqualsMethod(MethodSymbol method, BoundCall node, TypeWith var isObjectEqualsMethodOrOverride = method.GetLeastOverriddenMethod(accessingTypeOpt: null) .Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__Equals)); - if (isObjectEqualsMethodOrOverride || isWellKnownEqualityMethodOrImplementation(method, WellKnownMember.System_IEquatable_T__Equals)) + if (isObjectEqualsMethodOrOverride || + isWellKnownEqualityMethodOrImplementation(compilation, method, WellKnownMember.System_IEquatable_T__Equals)) { Debug.Assert(arguments.Length == 1); learnFromEqualsMethodArguments(node.ReceiverOpt, receiverType, arguments[0], results[0].RValueType); return; } - bool isWellKnownEqualityMethodOrImplementation(MethodSymbol method, WellKnownMember wellKnownMember) + static bool isWellKnownEqualityMethodOrImplementation(CSharpCompilation compilation, MethodSymbol method, WellKnownMember wellKnownMember) { var wellKnownMethod = compilation.GetWellKnownTypeMember(wellKnownMember); if (wellKnownMethod is null) diff --git a/src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs index 5bf429b1752ba..e95e076601021 100644 --- a/src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs @@ -1705,7 +1705,7 @@ private static Location GetInterfaceLocation(Symbol interfaceMember, TypeSymbol snt = implementingType as SourceMemberContainerTypeSymbol; } - return snt?.GetImplementsLocation(@interface) ?? implementingType.Locations.FirstOrDefault(); + return snt?.GetImplementsLocation(@interface) ?? implementingType.Locations.FirstOrNone(); } private static bool ReportAnyMismatchedConstraints(MethodSymbol interfaceMethod, TypeSymbol implementingType, MethodSymbol implicitImpl, DiagnosticBag diagnostics) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index 7f29e3207e884..c6d0e4ff58556 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -109783,7 +109783,7 @@ void verify(string source) } [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] - public void StaticEqualsMethod_NonNullExpr_NotNullExpr_NoWarning() + public void StaticEqualsMethod_NonNullExpr_NonNullExpr_NoWarning() { var source0 = @" #nullable enable @@ -110747,7 +110747,6 @@ void M(object? o1) Assert.True(comp.ShouldRunNullableWalker); } - [Theory] [InlineData("")] [InlineData("annotations")] From afa064785637127ab037a17bd8a28d9f463db90f Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 1 Jul 2019 14:35:56 -0700 Subject: [PATCH 7/7] Address review feedback --- .../Portable/FlowAnalysis/NullableWalker.cs | 1 - .../Semantics/NullableReferenceTypesTests.cs | 521 +++++++++--------- .../Symbol/Symbols/MissingSpecialMember.cs | 5 +- .../Core/Portable/WellKnownMembers.cs | 14 +- src/Compilers/Core/Portable/WellKnownTypes.cs | 10 +- 5 files changed, 264 insertions(+), 287 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 2c46d2d23baab..34bc69c3ef9c0 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2885,7 +2885,6 @@ static bool isWellKnownEqualityMethodOrImplementation(CSharpCompilation compilat Debug.Assert(constructedMethod != null, "the original definition is present but the constructed method isn't present"); - // FindImplementationForInterfaceMember doesn't check if this method is itself the interface method we're looking for if (constructedMethod.Equals(method)) { diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index c6d0e4ff58556..f1da3c6b94822 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -109735,404 +109735,383 @@ static void M( comp.VerifyDiagnostics(); } - [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] - public void StaticEqualsMethod_NonNullExpr_LiteralNull_WarnsWhenTrue() + [Theory] + [InlineData("object.Equals")] + [InlineData("object.ReferenceEquals")] + [WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_NonNullExpr_LiteralNull_WarnsWhenTrue(string equalsMethodName) { - var source0 = @" + var source = $@" #nullable enable class C -{ +{{ static void M(string s) - { - if (object.Equals(s, null)) - { + {{ + if ({equalsMethodName}(s, null)) + {{ _ = s.ToString(); // 1 - } + }} else - { + {{ _ = s.ToString(); - } - } + }} + }} static void M2(string s) - { - if (object.Equals(null, s)) - { + {{ + if ({equalsMethodName}(null, s)) + {{ _ = s.ToString(); // 2 - } + }} else - { + {{ _ = s.ToString(); - } - } -}"; - verify(source0); - verify(source0.Replace("object.Equals", "object.ReferenceEquals")); - - void verify(string source) - { - var comp = CreateCompilation(source); - comp.VerifyDiagnostics( - // (9,17): warning CS8602: Dereference of a possibly null reference. - // _ = s.ToString(); // 1 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(9, 17), - // (21,17): warning CS8602: Dereference of a possibly null reference. - // _ = s.ToString(); // 2 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(21, 17)); - } + }} + }} +}}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (9,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(9, 17), + // (21,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(21, 17)); } - [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] - public void StaticEqualsMethod_NonNullExpr_NonNullExpr_NoWarning() + [Theory] + [InlineData("object.Equals")] + [InlineData("object.ReferenceEquals")] + [WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_NonNullExpr_NonNullExpr_NoWarning(string equalsMethodName) { - var source0 = @" + var source = $@" #nullable enable class C -{ +{{ static void M(string s1, string s2) - { - if (object.Equals(s1, s2)) - { + {{ + if ({equalsMethodName}(s1, s2)) + {{ _ = s1.ToString(); _ = s2.ToString(); - } + }} else - { + {{ _ = s1.ToString(); _ = s2.ToString(); - } - } + }} + }} static void M2(string s1, string s2) - { - if (object.Equals(s2, s1)) - { + {{ + if ({equalsMethodName}(s2, s1)) + {{ _ = s1.ToString(); _ = s2.ToString(); - } + }} else - { + {{ _ = s1.ToString(); _ = s2.ToString(); - } - } -}"; - verify(source0); - verify(source0.Replace("object.Equals", "object.ReferenceEquals")); - - void verify(string source) - { - var comp = CreateCompilation(source); - comp.VerifyDiagnostics(); - } + }} + }} +}}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics(); } - [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] - public void StaticEqualsMethod_MaybeNullExpr_MaybeNullExpr_Warns() + [Theory] + [InlineData("object.Equals")] + [InlineData("object.ReferenceEquals")] + [WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_MaybeNullExpr_MaybeNullExpr_Warns(string equalsMethodName) { - var source0 = @" + var source = $@" #nullable enable class C -{ +{{ static void M(string? s1, string? s2) - { - if (object.Equals(s1, s2)) - { + {{ + if ({equalsMethodName}(s1, s2)) + {{ _ = s1.ToString(); // 1 _ = s2.ToString(); // 2 - } + }} else - { + {{ _ = s1.ToString(); // 3 _ = s2.ToString(); // 4 - } - } + }} + }} static void M2(string? s1, string? s2) - { - if (object.Equals(s2, s1)) - { + {{ + if ({equalsMethodName}(s2, s1)) + {{ _ = s1.ToString(); // 5 _ = s2.ToString(); // 6 - } + }} else - { + {{ _ = s1.ToString(); // 7 _ = s2.ToString(); // 8 - } - } -}"; - verify(source0); - verify(source0.Replace("object.Equals", "object.ReferenceEquals")); - - void verify(string source) - { - var comp = CreateCompilation(source); - comp.VerifyDiagnostics( - // (9,17): warning CS8602: Dereference of a possibly null reference. - // _ = s1.ToString(); // 1 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s1").WithLocation(9, 17), - // (10,17): warning CS8602: Dereference of a possibly null reference. - // _ = s2.ToString(); // 2 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(10, 17), - // (14,17): warning CS8602: Dereference of a possibly null reference. - // _ = s1.ToString(); // 3 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s1").WithLocation(14, 17), - // (15,17): warning CS8602: Dereference of a possibly null reference. - // _ = s2.ToString(); // 4 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(15, 17), - // (23,17): warning CS8602: Dereference of a possibly null reference. - // _ = s1.ToString(); // 5 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s1").WithLocation(23, 17), - // (24,17): warning CS8602: Dereference of a possibly null reference. - // _ = s2.ToString(); // 6 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(24, 17), - // (28,17): warning CS8602: Dereference of a possibly null reference. - // _ = s1.ToString(); // 7 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s1").WithLocation(28, 17), - // (29,17): warning CS8602: Dereference of a possibly null reference. - // _ = s2.ToString(); // 8 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(29, 17)); - } + }} + }} +}}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (9,17): warning CS8602: Dereference of a possibly null reference. + // _ = s1.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s1").WithLocation(9, 17), + // (10,17): warning CS8602: Dereference of a possibly null reference. + // _ = s2.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(10, 17), + // (14,17): warning CS8602: Dereference of a possibly null reference. + // _ = s1.ToString(); // 3 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s1").WithLocation(14, 17), + // (15,17): warning CS8602: Dereference of a possibly null reference. + // _ = s2.ToString(); // 4 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(15, 17), + // (23,17): warning CS8602: Dereference of a possibly null reference. + // _ = s1.ToString(); // 5 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s1").WithLocation(23, 17), + // (24,17): warning CS8602: Dereference of a possibly null reference. + // _ = s2.ToString(); // 6 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(24, 17), + // (28,17): warning CS8602: Dereference of a possibly null reference. + // _ = s1.ToString(); // 7 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s1").WithLocation(28, 17), + // (29,17): warning CS8602: Dereference of a possibly null reference. + // _ = s2.ToString(); // 8 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(29, 17)); } - [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] - public void StaticEqualsMethod_NonNullExpr_ConstantNull_WarnsWhenTrue() + [Theory] + [InlineData("object.Equals")] + [InlineData("object.ReferenceEquals")] + [WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_NonNullExpr_ConstantNull_WarnsWhenTrue(string equalsMethodName) { - var source0 = @" + var source = $@" #nullable enable class C -{ +{{ static void M1(string s) - { + {{ const string? s2 = null; - if (object.Equals(s, s2)) - { + if ({equalsMethodName}(s, s2)) + {{ _ = s.ToString(); // 1 - } + }} else - { + {{ _ = s.ToString(); - } - } + }} + }} static void M2(string s) - { + {{ const string? s2 = null; - if (object.Equals(s2, s)) - { + if ({equalsMethodName}(s2, s)) + {{ _ = s.ToString(); // 2 - } + }} else - { + {{ _ = s.ToString(); - } - } -}"; - verify(source0); - verify(source0.Replace("object.Equals", "object.ReferenceEquals")); - - void verify(string source) - { - var comp = CreateCompilation(source); - comp.VerifyDiagnostics( - // (10,17): warning CS8602: Dereference of a possibly null reference. - // _ = s.ToString(); // 1 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(10, 17), - // (23,17): warning CS8602: Dereference of a possibly null reference. - // _ = s.ToString(); // 2 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(23, 17)); - } + }} + }} +}}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (10,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(10, 17), + // (23,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(23, 17)); } - [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] - public void StaticEqualsMethod_MaybeNullExpr_ConstantNull_NoWarningWhenFalse() + [Theory] + [InlineData("object.Equals")] + [InlineData("object.ReferenceEquals")] + [WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_MaybeNullExpr_ConstantNull_NoWarningWhenFalse(string equalsMethodName) { - var source0 = @" + var source = $@" #nullable enable class C -{ +{{ static void M1(string? s) - { + {{ const string? s2 = null; - if (object.Equals(s, s2)) - { + if ({equalsMethodName}(s, s2)) + {{ _ = s.ToString(); // 1 - } + }} else - { + {{ _ = s.ToString(); - } - } + }} + }} static void M2(string? s) - { + {{ const string? s2 = null; - if (object.Equals(s2, s)) - { + if ({equalsMethodName}(s2, s)) + {{ _ = s.ToString(); // 2 - } + }} else - { + {{ _ = s.ToString(); - } - } -}"; - verify(source0); - verify(source0.Replace("object.Equals", "object.ReferenceEquals")); - - void verify(string source) - { - var comp = CreateCompilation(source); - comp.VerifyDiagnostics( - // (10,17): warning CS8602: Dereference of a possibly null reference. - // _ = s.ToString(); // 1 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(10, 17), - // (23,17): warning CS8602: Dereference of a possibly null reference. - // _ = s.ToString(); // 2 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(23, 17)); - } + }} + }} +}}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (10,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(10, 17), + // (23,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(23, 17)); } - [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] - public void StaticEqualsMethod_NonNullExpr_MaybeNullExpr_NoWarning() + [Theory] + [InlineData("object.Equals")] + [InlineData("object.ReferenceEquals")] + [WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_NonNullExpr_MaybeNullExpr_NoWarning(string equalsMethodName) { - var source = @" + var source = $@" #nullable enable class C -{ +{{ static void M1(string s) - { + {{ string? s2 = null; - if (object.Equals(s, s2)) - { + if ({equalsMethodName}(s, s2)) + {{ _ = s.ToString(); - } + }} else - { + {{ _ = s.ToString(); - } - } + }} + }} static void M2(string s) - { + {{ string? s2 = null; - if (object.Equals(s2, s)) - { + if ({equalsMethodName}(s2, s)) + {{ _ = s.ToString(); - } + }} else - { + {{ _ = s.ToString(); - } - } -}"; + }} + }} +}}"; var comp = CreateCompilation(source); comp.VerifyDiagnostics(); - - comp = CreateCompilation(source.Replace("object.Equals", "object.ReferenceEquals")); - comp.VerifyDiagnostics(); } - [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] - public void StaticEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue() + [Theory] + [InlineData("object.Equals")] + [InlineData("object.ReferenceEquals")] + [WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod_MaybeNullExpr_NonNullExpr_NoWarningWhenTrue(string equalsMethodName) { - var source0 = @" + var source = $@" #nullable enable class C -{ +{{ static void M1(string? s) - { - if (object.Equals(s, """")) - { + {{ + if ({equalsMethodName}(s, """")) + {{ _ = s.ToString(); - } + }} else - { + {{ _ = s.ToString(); // 1 - } - } + }} + }} static void M2(string? s) - { - if (object.Equals("""", s)) - { + {{ + if ({equalsMethodName}("""", s)) + {{ _ = s.ToString(); - } + }} else - { + {{ _ = s.ToString(); // 2 - } - } -}"; - verify(source0); - verify(source0.Replace("object.Equals", "object.ReferenceEquals")); - - void verify(string source) - { - var comp = CreateCompilation(source); - comp.VerifyDiagnostics( - // (13,17): warning CS8602: Dereference of a possibly null reference. - // _ = s.ToString(); // 1 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(13, 17), - // (25,17): warning CS8602: Dereference of a possibly null reference. - // _ = s.ToString(); // 2 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(25, 17)); - } + }} + }} +}}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (13,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(13, 17), + // (25,17): warning CS8602: Dereference of a possibly null reference. + // _ = s.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(25, 17)); } - [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] - public void StaticEqualsMethod__NullableValueTypeExpr_ValueTypeExpr_NoWarningWhenTrue() + [Theory] + [InlineData("object.Equals")] + [InlineData("object.ReferenceEquals")] + [WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] + public void StaticEqualsMethod__NullableValueTypeExpr_ValueTypeExpr_NoWarningWhenTrue(string equalsMethodName) { - var source0 = @" + var source = $@" #nullable enable class C -{ +{{ int? i = 42; static void M1(C? c) - { - if (object.Equals(c?.i, 42)) - { + {{ + if ({equalsMethodName}(c?.i, 42)) + {{ _ = c.i.Value.ToString(); - } + }} else - { + {{ _ = c.i.Value.ToString(); // 1 - } - } + }} + }} static void M2(C? c) - { - if (object.Equals(42, c?.i)) - { + {{ + if ({equalsMethodName}(42, c?.i)) + {{ _ = c.i.Value.ToString(); - } + }} else - { + {{ _ = c.i.Value.ToString(); // 2 - } - } -}"; - verify(source0); - verify(source0.Replace("object.Equals", "object.ReferenceEquals")); - - void verify(string source) - { - var comp = CreateCompilation(source); - comp.VerifyDiagnostics( - // (14,17): warning CS8602: Dereference of a possibly null reference. - // _ = c.i.Value.ToString(); // 1 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c").WithLocation(14, 17), - // (14,17): warning CS8629: Nullable value type may be null. - // _ = c.i.Value.ToString(); // 1 - Diagnostic(ErrorCode.WRN_NullableValueTypeMayBeNull, "c.i").WithLocation(14, 17), - // (26,17): warning CS8602: Dereference of a possibly null reference. - // _ = c.i.Value.ToString(); // 2 - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c").WithLocation(26, 17), - // (26,17): warning CS8629: Nullable value type may be null. - // _ = c.i.Value.ToString(); // 2 - Diagnostic(ErrorCode.WRN_NullableValueTypeMayBeNull, "c.i").WithLocation(26, 17)); - } + }} + }} +}}"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics( + // (14,17): warning CS8602: Dereference of a possibly null reference. + // _ = c.i.Value.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c").WithLocation(14, 17), + // (14,17): warning CS8629: Nullable value type may be null. + // _ = c.i.Value.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullableValueTypeMayBeNull, "c.i").WithLocation(14, 17), + // (26,17): warning CS8602: Dereference of a possibly null reference. + // _ = c.i.Value.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c").WithLocation(26, 17), + // (26,17): warning CS8629: Nullable value type may be null. + // _ = c.i.Value.ToString(); // 2 + Diagnostic(ErrorCode.WRN_NullableValueTypeMayBeNull, "c.i").WithLocation(26, 17)); } [Fact, WorkItem(35816, "https://github.com/dotnet/roslyn/issues/35816")] diff --git a/src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs b/src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs index 7feafea671554..2c84852163e5e 100644 --- a/src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs +++ b/src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs @@ -804,7 +804,6 @@ public void AllWellKnownTypesBeforeCSharp7() WellKnownType.System_Collections_IList, WellKnownType.System_Collections_ICollection, - WellKnownType.System_Collections_Generic_IEqualityComparer_T, WellKnownType.System_Collections_Generic_EqualityComparer_T, WellKnownType.System_Collections_Generic_List_T, WellKnownType.System_Collections_Generic_IDictionary_KV, @@ -866,8 +865,8 @@ public void AllWellKnownTypesBeforeCSharp7() Assert.True(type <= WellKnownType.CSharp7Sentinel); } - // There were 205 well-known types prior to CSharp7 - Assert.Equal(205, (int)(WellKnownType.CSharp7Sentinel - WellKnownType.First)); + // There were 204 well-known types prior to CSharp7 + Assert.Equal(204, (int)(WellKnownType.CSharp7Sentinel - WellKnownType.First)); } [Fact] diff --git a/src/Compilers/Core/Portable/WellKnownMembers.cs b/src/Compilers/Core/Portable/WellKnownMembers.cs index f24255234b87a..9901beb80a1e3 100644 --- a/src/Compilers/Core/Portable/WellKnownMembers.cs +++ b/src/Compilers/Core/Portable/WellKnownMembers.cs @@ -450,7 +450,7 @@ static WellKnownMembers() // System_Collections_Generic_IEqualityComparer_T__Equals (byte)(MemberFlags.Method | MemberFlags.Virtual), // Flags - (byte)WellKnownType.System_Collections_Generic_IEqualityComparer_T, // DeclaringTypeId + (byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_Collections_Generic_IEqualityComparer_T - WellKnownType.ExtSentinel), // DeclaringTypeId 0, // Arity 2, // Method Signature (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Boolean, // Return Type @@ -2619,31 +2619,31 @@ static WellKnownMembers() // System_ValueTuple_T5__Item1 (byte)MemberFlags.Field, // Flags - (byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_ValueTuple_T5 - WellKnownType.ExtSentinel), // DeclaringTypeId + (byte)WellKnownType.System_ValueTuple_T5, // DeclaringTypeId 0, // Arity (byte)SignatureTypeCode.GenericTypeParameter, 0, // Field Signature // System_ValueTuple_T5__Item2 (byte)MemberFlags.Field, // Flags - (byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_ValueTuple_T5 - WellKnownType.ExtSentinel), // DeclaringTypeId + (byte)WellKnownType.System_ValueTuple_T5, // DeclaringTypeId 0, // Arity (byte)SignatureTypeCode.GenericTypeParameter, 1, // Field Signature // System_ValueTuple_T5__Item3 (byte)MemberFlags.Field, // Flags - (byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_ValueTuple_T5 - WellKnownType.ExtSentinel), // DeclaringTypeId + (byte)WellKnownType.System_ValueTuple_T5, // DeclaringTypeId 0, // Arity (byte)SignatureTypeCode.GenericTypeParameter, 2, // Field Signature // System_ValueTuple_T5__Item4 (byte)MemberFlags.Field, // Flags - (byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_ValueTuple_T5 - WellKnownType.ExtSentinel), // DeclaringTypeId + (byte)WellKnownType.System_ValueTuple_T5, // DeclaringTypeId 0, // Arity (byte)SignatureTypeCode.GenericTypeParameter, 3, // Field Signature // System_ValueTuple_T5__Item5 (byte)MemberFlags.Field, // Flags - (byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_ValueTuple_T5 - WellKnownType.ExtSentinel), // DeclaringTypeId + (byte)WellKnownType.System_ValueTuple_T5, // DeclaringTypeId 0, // Arity (byte)SignatureTypeCode.GenericTypeParameter, 4, // Field Signature @@ -2813,7 +2813,7 @@ static WellKnownMembers() // System_ValueTuple_T_T2_T3_T4_T5__ctor (byte)MemberFlags.Constructor, // Flags - (byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_ValueTuple_T5 - WellKnownType.ExtSentinel), // DeclaringTypeId + (byte)WellKnownType.System_ValueTuple_T5, // DeclaringTypeId 0, // Arity 5, // Method Signature (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Void, // Return Type diff --git a/src/Compilers/Core/Portable/WellKnownTypes.cs b/src/Compilers/Core/Portable/WellKnownTypes.cs index 48335a95ebf87..30990f2dc688d 100644 --- a/src/Compilers/Core/Portable/WellKnownTypes.cs +++ b/src/Compilers/Core/Portable/WellKnownTypes.cs @@ -189,7 +189,6 @@ internal enum WellKnownType System_Collections_IList, System_Collections_ICollection, - System_Collections_Generic_IEqualityComparer_T, System_Collections_Generic_EqualityComparer_T, System_Collections_Generic_List_T, System_Collections_Generic_IDictionary_KV, @@ -253,10 +252,10 @@ internal enum WellKnownType System_ValueTuple_T2, System_ValueTuple_T3, System_ValueTuple_T4, + System_ValueTuple_T5, ExtSentinel, // Not a real type, just a marker for types above 255 and strictly below 512 - System_ValueTuple_T5, System_ValueTuple_T6, System_ValueTuple_T7, System_ValueTuple_TRest, @@ -304,6 +303,7 @@ internal enum WellKnownType System_InvalidOperationException, System_Runtime_CompilerServices_SwitchExpressionException, + System_Collections_Generic_IEqualityComparer_T, NextAvailable, @@ -490,7 +490,6 @@ internal static class WellKnownTypes "System.Collections.IList", "System.Collections.ICollection", - "System.Collections.Generic.IEqualityComparer`1", "System.Collections.Generic.EqualityComparer`1", "System.Collections.Generic.List`1", "System.Collections.Generic.IDictionary`2", @@ -553,10 +552,10 @@ internal static class WellKnownTypes "System.ValueTuple`2", "System.ValueTuple`3", "System.ValueTuple`4", + "System.ValueTuple`5", "", // extension marker - "System.ValueTuple`5", "System.ValueTuple`6", "System.ValueTuple`7", "System.ValueTuple`8", @@ -604,7 +603,8 @@ internal static class WellKnownTypes "System.Threading.CancellationTokenSource", "System.InvalidOperationException", - "System.Runtime.CompilerServices.SwitchExpressionException" + "System.Runtime.CompilerServices.SwitchExpressionException", + "System.Collections.Generic.IEqualityComparer`1", }; private readonly static Dictionary s_nameToTypeIdMap = new Dictionary((int)Count);