From bcce63634e9122016267821e474b8599153c0be9 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Wed, 3 Jul 2019 15:17:45 -0700 Subject: [PATCH] Make NullableWalker aware of calls to Interlocked.CompareExchange --- .../Portable/FlowAnalysis/NullableWalker.cs | 53 ++++++ .../Semantics/NullableReferenceTypesTests.cs | 179 ++++++++++++++++++ .../Core/Portable/WellKnownMember.cs | 1 + .../Core/Portable/WellKnownMembers.cs | 11 ++ 4 files changed, 244 insertions(+) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 99a239f455c..db9e6562cd2 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2821,6 +2821,8 @@ private void ReinferMethodAndVisitArguments(BoundCall node, TypeWithState receiv LearnFromEqualsMethod(method, node, receiverType, results); + LearnFromCompareExchangeMethod(method, node, results); + if (method.MethodKind == MethodKind.LocalFunction) { var localFunc = (LocalFunctionSymbol)method.OriginalDefinition; @@ -2928,6 +2930,57 @@ void learnFromEqualsMethodArguments(BoundExpression left, TypeWithState leftType } } + private void LearnFromCompareExchangeMethod(MethodSymbol method, BoundCall node, ImmutableArray results) + { + // easy out + var parameterCount = method.ParameterCount; + if (parameterCount != 3 + || method.MethodKind != MethodKind.Ordinary + || !method.IsStatic + || method.Name != WellKnownMembers.GetDescriptor(WellKnownMember.System_Threading_Interlocked__CompareExchange).Name) + { + return; + } + + var isCompareExchangeMethod = method.Equals(compilation.GetWellKnownTypeMember(WellKnownMember.System_Threading_Interlocked__CompareExchange)) + || method.OriginalDefinition.Equals(compilation.GetWellKnownTypeMember(WellKnownMember.System_Threading_Interlocked__CompareExchange_T)); + if (!isCompareExchangeMethod) + { + return; + } + + // In general a call to CompareExchange of the form: + // + // Interlocked.CompareExchange(ref location, value, comparand); + // + // will be analyzed similarly to the following: + // + // if (location == comparand) + // { + // location = value; + // } + + var arguments = node.Arguments; + var location = arguments[0]; + var locationSlot = MakeSlot(location); + if (locationSlot != -1) + { + var comparand = arguments[2]; + var valueFlowState = results[1].RValueType.State; + if (comparand.ConstantValue?.IsNull == true) + { + // If location contained a null, then the write `location = value` definitely occurred + State[locationSlot] = valueFlowState; + } + else + { + var locationFlowState = results[0].RValueType.State; + // A write may have occurred + State[locationSlot] = valueFlowState.Join(locationFlowState); + } + } + } + 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 ba64e8c5686..046870778b8 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -111685,5 +111685,184 @@ void M(object? o1) Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "o1").WithLocation(8, 21)); Assert.True(comp.ShouldRunNullableWalker); } + + [Theory] + // 2 states * 3 states * 3 states = 18 combinations + [InlineData("locationNonNull", "valueNonNull", "comparandNonNull", true)] + [InlineData("locationNonNull", "valueNonNull", "comparandMaybeNull", true)] + [InlineData("locationNonNull", "valueNonNull", "null", true)] + [InlineData("locationNonNull", "valueMaybeNull", "comparandNonNull", false)] + [InlineData("locationNonNull", "valueMaybeNull", "comparandMaybeNull", false)] + [InlineData("locationNonNull", "valueMaybeNull", "null", false)] + [InlineData("locationNonNull", "null", "comparandNonNull", false)] + [InlineData("locationNonNull", "null", "comparandMaybeNull", false)] + [InlineData("locationNonNull", "null", "null", false)] + [InlineData("locationMaybeNull", "valueNonNull", "comparandNonNull", false)] + [InlineData("locationMaybeNull", "valueNonNull", "comparandMaybeNull", false)] + [InlineData("locationMaybeNull", "valueNonNull", "null", true)] + [InlineData("locationMaybeNull", "valueMaybeNull", "comparandNonNull", false)] + [InlineData("locationMaybeNull", "valueMaybeNull", "comparandMaybeNull", false)] + [InlineData("locationMaybeNull", "valueMaybeNull", "null", false)] + [InlineData("locationMaybeNull", "null", "comparandNonNull", false)] + [InlineData("locationMaybeNull", "null", "comparandMaybeNull", false)] + [InlineData("locationMaybeNull", "null", "null", false)] + [WorkItem(36911, "https://github.com/dotnet/roslyn/issues/36911")] + public void CompareExchange_LocationNullState(string location, string value, string comparand, bool isLocationNonNull) + { + var source = $@" +#pragma warning disable 0436 +using System.Threading; +namespace System.Threading +{{ + public static class Interlocked + {{ + public static object? CompareExchange(ref object? location, object? value, object? comparand) {{ return location; }} + }} +}} + +class C +{{ + void M(object? locationNonNull, object? locationMaybeNull, object comparandNonNull, object? comparandMaybeNull, object valueNonNull, object? valueMaybeNull) + {{ + locationNonNull = new object(); + Interlocked.CompareExchange(ref {location}, {value}, {comparand}); + _ = {location}.ToString(); + }} +}} + "; + var comp = CreateCompilation(source, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics(isLocationNonNull + ? Array.Empty() + : new[] + { + // (18,13): warning CS8602: Dereference of a possibly null reference. + // _ = {location}.ToString(); + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, location).WithLocation(18, 13) + }); + + source = $@" +#pragma warning disable 0436 +using System.Threading; +namespace System.Threading +{{ + public static class Interlocked + {{ + public static T CompareExchange(ref T location, T value, T comparand) where T : class? {{ return location; }} + }} +}} + +class C +{{ + void M(string? locationNonNull, string? locationMaybeNull, string comparandNonNull, string? comparandMaybeNull, string valueNonNull, string? valueMaybeNull) + {{ + locationNonNull = ""hello""; + Interlocked.CompareExchange(ref {location}, {value}, {comparand}); + _ = {location}.ToString(); + }} +}} + "; + comp = CreateCompilation(source, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics(isLocationNonNull + ? Array.Empty() + : new[] + { + // (18,13): warning CS8602: Dereference of a possibly null reference. + // _ = {location}.ToString(); + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, location).WithLocation(18, 13) + }); + + source = $@" +class C +{{ + void M(object? locationNonNull, object? locationMaybeNull, object comparandNonNull, object? comparandMaybeNull, object valueNonNull, object? valueMaybeNull) + {{ + locationNonNull = new object(); + if ({location} == {comparand}) + {{ + {location} = {value}; + }} + _ = {location}.ToString(); + }} +}} + "; + comp = CreateCompilation(source, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics(isLocationNonNull + ? Array.Empty() + : new[] + { + // (11,13): warning CS8602: Dereference of a possibly null reference. + // _ = {location}.ToString(); + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, location).WithLocation(11, 13) + }); + } + + [Fact] + [WorkItem(36911, "https://github.com/dotnet/roslyn/issues/36911")] + public void CompareExchange_BadOverload() + { + var source = @" +#pragma warning disable 0436 +using System.Threading; +namespace System.Threading +{ + public static class Interlocked + { + public static object? CompareExchange(ref object? location, object? value) { return location; } + public static object? CompareExchange(ref object? location, object? value, object? comparand) { return location; } + } +} + +class C +{ + void M() + { + object? location = null; + Interlocked.CompareExchange(ref location, new object()); + _ = location.ToString(); // 1 + Interlocked.CompareExchange(ref location, new object(), null); + _ = location.ToString(); + } +} + "; + var comp = CreateCompilation(source, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (19,13): warning CS8602: Dereference of a possibly null reference. + // _ = location.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "location").WithLocation(19, 13)); + } + [Fact] + [WorkItem(36911, "https://github.com/dotnet/roslyn/issues/36911")] + public void CompareExchangeT_BadOverload() + { + var source = @" +#pragma warning disable 0436 +using System.Threading; +namespace System.Threading +{ + public static class Interlocked + { + public static T CompareExchange(ref T location, T value) where T : class? { return location; } + public static T CompareExchange(ref T location, T value, T comparand) where T : class? { return location; } + } +} + +class C +{ + void M() + { + string? location = null; + Interlocked.CompareExchange(ref location, ""hello""); + _ = location.ToString(); // 1 + Interlocked.CompareExchange(ref location, ""hello"", null); + _ = location.ToString(); + } +} + "; + var comp = CreateCompilation(source, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (19,13): warning CS8602: Dereference of a possibly null reference. + // _ = location.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "location").WithLocation(19, 13)); + } } } diff --git a/src/Compilers/Core/Portable/WellKnownMember.cs b/src/Compilers/Core/Portable/WellKnownMember.cs index 4019248afdb..a8ac7605a21 100644 --- a/src/Compilers/Core/Portable/WellKnownMember.cs +++ b/src/Compilers/Core/Portable/WellKnownMember.cs @@ -159,6 +159,7 @@ internal enum WellKnownMember System_Activator__CreateInstance, System_Activator__CreateInstance_T, + System_Threading_Interlocked__CompareExchange, System_Threading_Interlocked__CompareExchange_T, System_Threading_Monitor__Enter, //Monitor.Enter(object) diff --git a/src/Compilers/Core/Portable/WellKnownMembers.cs b/src/Compilers/Core/Portable/WellKnownMembers.cs index 9901beb80a1..a2a4c4e2317 100644 --- a/src/Compilers/Core/Portable/WellKnownMembers.cs +++ b/src/Compilers/Core/Portable/WellKnownMembers.cs @@ -1072,6 +1072,16 @@ static WellKnownMembers() 0, // Method Signature (byte)SignatureTypeCode.GenericMethodParameter, 0, // Return Type + // System_Threading_Interlocked__CompareExchange + (byte)(MemberFlags.Method | MemberFlags.Static), // Flags + (byte)WellKnownType.System_Threading_Interlocked, // DeclaringTypeId + 0, // Arity + 3, // Method Signature + (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Object, // Return Type + (byte)SignatureTypeCode.ByReference, (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Object, + (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Object, + (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Object, + // System_Threading_Interlocked__CompareExchange_T (byte)(MemberFlags.Method | MemberFlags.Static), // Flags (byte)WellKnownType.System_Threading_Interlocked, // DeclaringTypeId @@ -3580,6 +3590,7 @@ static WellKnownMembers() "SkipVerification", // System_Security_Permissions_SecurityPermissionAttribute__SkipVerification "CreateInstance", // System_Activator__CreateInstance "CreateInstance", // System_Activator__CreateInstance_T + "CompareExchange", // System_Threading_Interlocked__CompareExchange "CompareExchange", // System_Threading_Interlocked__CompareExchange_T "Enter", // System_Threading_Monitor__Enter "Enter", // System_Threading_Monitor__Enter2 -- GitLab