Merge pull request #37014 from RikkiGibson/nullable-compareexchange

 Make NullableWalker aware of calls to Interlocked.CompareExchange

docs/features/nullable-reference-types.md

@@ -53,6 +53,10 @@ Invocation of methods annotated with the following attributes will also affect f
 - `[AssertsTrue]` (e.g. `Debug.Assert`) and `[AssertsFalse]`
 See https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-05-15.md
 
+The `Interlocked.CompareExchange` methods have special handling in flow analysis instead of being annotated due to the complexity of their nullability semantics. The affected overloads include:
+- `static object? System.Threading.Interlocked.CompareExchange(ref object? location, object? value, object? comparand)`
+- `static T System.Threading.Interlocked.CompareExchange<T>(ref T location, T value, T comparand) where T : class?`
+
 ## `default`
 If `T` is a reference type, `default(T)` is `T?`.
 ```c#

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

@@ -2827,6 +2827,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;
@@ -2934,6 +2936,46 @@ void learnFromEqualsMethodArguments(BoundExpression left, TypeWithState leftType
             }
         }
 
+        private void LearnFromCompareExchangeMethod(MethodSymbol method, BoundCall node, ImmutableArray<VisitArgumentResult> results)
+        {
+            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 locationSlot = MakeSlot(arguments[0]);
+            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;

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

@@ -111706,5 +111706,380 @@ 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 isLocationNonNullAfterCall)
+        {
+            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
+{{
+    // locationNonNull is annotated, but its flow state is non-null
+    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(isLocationNonNullAfterCall
+                ? Array.Empty<DiagnosticDescription>()
+                : new[]
+                {
+                    // (19,13): warning CS8602: Dereference of a possibly null reference.
+                    //         _ = {location}.ToString();
+                    Diagnostic(ErrorCode.WRN_NullReferenceReceiver, location).WithLocation(19, 13)
+                });
+
+            source = $@"
+#pragma warning disable 0436
+using System.Threading;
+namespace System.Threading
+{{
+    public static class Interlocked
+    {{
+        public static T CompareExchange<T>(ref T location, T value, T comparand) where T : class? {{ return location; }}
+    }}
+}}
+
+class C
+{{
+    // locationNonNull is annotated, but its flow state is non-null
+    void M<T>(T? locationNonNull, T? locationMaybeNull, T comparandNonNull, T? comparandMaybeNull, T valueNonNull, T? valueMaybeNull) where T : class, new()
+    {{
+        locationNonNull = new T();
+        Interlocked.CompareExchange(ref {location}, {value}, {comparand});
+        _ = {location}.ToString();
+    }}
+}}
+    ";
+            comp = CreateCompilation(source, options: WithNonNullTypesTrue());
+            comp.VerifyDiagnostics(isLocationNonNullAfterCall
+                ? Array.Empty<DiagnosticDescription>()
+                : new[]
+                {
+                    // (19,13): warning CS8602: Dereference of a possibly null reference.
+                    //         _ = {location}.ToString();
+                    Diagnostic(ErrorCode.WRN_NullReferenceReceiver, location).WithLocation(19, 13)
+                });
+
+            source = $@"
+class C
+{{
+    // locationNonNull is annotated, but its flow state is non-null
+    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(isLocationNonNullAfterCall
+                ? Array.Empty<DiagnosticDescription>()
+                : new[]
+                {
+                    // (12,13): warning CS8602: Dereference of a possibly null reference.
+                    //         _ = {location}.ToString();
+                    Diagnostic(ErrorCode.WRN_NullReferenceReceiver, location).WithLocation(12, 13)
+                });
+        }
+
+        [Fact]
+        [WorkItem(36911, "https://github.com/dotnet/roslyn/issues/36911")]
+        public void CompareExchange_NoLocationSlot()
+        {
+            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()
+    {
+        var arr = new object?[1];
+        Interlocked.CompareExchange(ref arr[0], new object(), null);
+        _ = arr[0].ToString(); // 1
+    }
+}
+";
+            var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
+            comp.VerifyDiagnostics(
+                // (18,13): warning CS8602: Dereference of a possibly null reference.
+                //         _ = arr[0].ToString(); // 1
+                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "arr[0]").WithLocation(18, 13));
+        }
+
+        [Fact]
+        [WorkItem(36911, "https://github.com/dotnet/roslyn/issues/36911")]
+        public void CompareExchangeT_NotAnnotatedLocation_MaybeNullComparand()
+        {
+            var source = @"
+#pragma warning disable 0436
+using System.Threading;
+namespace System.Threading
+{
+    public static class Interlocked
+    {
+        public static T CompareExchange<T>(ref T location, T value, T comparand) where T : class? { return location; }
+    }
+}
+
+class C
+{
+    void M<T>(T location, T value, T? comparand) where T : class
+    {
+        Interlocked.CompareExchange(ref location, value, comparand); // 1
+        _ = location.ToString();
+    }
+}
+";
+            var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
+            comp.VerifyDiagnostics(
+                // (16,41): warning CS8600: Converting null literal or possible null value to non-nullable type.
+                //         Interlocked.CompareExchange(ref location, value, comparand); // 1
+                Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "location").WithLocation(16, 41));
+        }
+
+        [Fact]
+        [WorkItem(36911, "https://github.com/dotnet/roslyn/issues/36911")]
+        public void CompareExchangeT_NotAnnotatedLocation_NonNullComparand()
+        {
+            var source = @"
+#pragma warning disable 0436
+using System.Threading;
+namespace System.Threading
+{
+    public static class Interlocked
+    {
+        public static T CompareExchange<T>(ref T location, T value, T comparand) where T : class? { return location; }
+    }
+}
+
+class C
+{
+    void M<T>(T location, T value, T comparand) where T : class
+    {
+        Interlocked.CompareExchange(ref location, value, comparand);
+        _ = location.ToString();
+    }
+}
+";
+            var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
+            comp.VerifyDiagnostics();
+        }
+
+        [Fact]
+        [WorkItem(36911, "https://github.com/dotnet/roslyn/issues/36911")]
+        public void CompareExchangeT_NotAnnotatedLocation_NonNullReturnValue()
+        {
+            var source = @"
+#pragma warning disable 0436
+using System.Threading;
+namespace System.Threading
+{
+    public static class Interlocked
+    {
+        public static T CompareExchange<T>(ref T location, T value, T comparand) where T : class? { return location; }
+    }
+}
+
+class C
+{
+    void M<T>(T location, T value, T comparand) where T : class
+    {
+        var result = Interlocked.CompareExchange(ref location, value, comparand);
+        _ = location.ToString();
+        _ = result.ToString();
+    }
+}
+";
+            var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
+            comp.VerifyDiagnostics();
+        }
+
+        [Fact]
+        [WorkItem(36911, "https://github.com/dotnet/roslyn/issues/36911")]
+        public void CompareExchangeT_NonNullLocation_MaybeNullReturnValue()
+        {
+            var source = @"
+#pragma warning disable 0436
+using System.Threading;
+namespace System.Threading
+{
+    public static class Interlocked
+    {
+        public static T CompareExchange<T>(ref T location, T value, T comparand) where T : class? { return location; }
+    }
+}
+
+class C
+{
+    // location is annotated, but its flow state is non-null
+    void M<T>(T? location, T value, T comparand) where T : class, new()
+    {
+        location = new T();
+        var result = Interlocked.CompareExchange(ref location, value, comparand);
+        _ = location.ToString();
+        _ = result.ToString(); // 1
+    }
+}
+";
+            // Note: the return value of CompareExchange is the value in `location` before the call.
+            // Therefore it might make sense to give the return value a flow state equal to the flow state of `location` before the call.
+            // Tracked by https://github.com/dotnet/roslyn/issues/36911
+            var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
+            comp.VerifyDiagnostics(
+                // (20,13): warning CS8602: Dereference of a possibly null reference.
+                //         _ = result.ToString(); // 1
+                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "result").WithLocation(20, 13));
+        }
+
+        [Fact]
+        [WorkItem(36911, "https://github.com/dotnet/roslyn/issues/36911")]
+        public void CompareExchangeT_MaybeNullLocation_MaybeNullReturnValue()
+        {
+            var source = @"
+#pragma warning disable 0436
+using System.Threading;
+namespace System.Threading
+{
+    public static class Interlocked
+    {
+        public static T CompareExchange<T>(ref T location, T value, T comparand) where T : class? { return location; }
+    }
+}
+
+class C
+{
+    void M<T>(T? location, T value, T comparand) where T : class
+    {
+        var result = Interlocked.CompareExchange(ref location, value, comparand);
+        _ = location.ToString(); // 1
+        _ = result.ToString(); // 2
+    }
+}
+";
+            var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
+            comp.VerifyDiagnostics(
+                    // (17,13): warning CS8602: Dereference of a possibly null reference.
+                    //         _ = location.ToString(); // 1
+                    Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "location").WithLocation(17, 13),
+                    // (18,13): warning CS8602: Dereference of a possibly null reference.
+                    //         _ = result.ToString(); // 2
+                    Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "result").WithLocation(18, 13));
+        }
+
+        [Fact]
+        [WorkItem(36911, "https://github.com/dotnet/roslyn/issues/36911")]
+        public void CompareExchange_UnrecognizedOverload()
+        {
+            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_UnrecognizedOverload()
+        {
+            var source = @"
+#pragma warning disable 0436
+using System.Threading;
+namespace System.Threading
+{
+    public static class Interlocked
+    {
+        public static T CompareExchange<T>(ref T location, T value) where T : class? { return location; }
+        public static T CompareExchange<T>(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));
+        }
     }
 }

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)

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