From 40adddcf0cac72eaa32170ac07817c6c42c55841 Mon Sep 17 00:00:00 2001 From: Yair Halberstadt Date: Tue, 22 Sep 2020 01:42:00 +0300 Subject: [PATCH] NullableWalker correctly handles positional arguments for Valuetuple and Interlocked (#47523) --- .../Portable/FlowAnalysis/NullableWalker.cs | 49 +++-- .../Semantics/NullableReferenceTypesTests.cs | 177 ++++++++++++++++++ 2 files changed, 215 insertions(+), 11 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index e26d61f87b9..07f670322f8 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2867,7 +2867,8 @@ protected override void VisitStatement(BoundStatement statement) slot = GetOrCreatePlaceholderSlot(node); if (slot > 0) { - var constructor = (node as BoundObjectCreationExpression)?.Constructor; + var boundObjectCreationExpression = node as BoundObjectCreationExpression; + var constructor = boundObjectCreationExpression?.Constructor; bool isDefaultValueTypeConstructor = constructor?.IsDefaultValueTypeConstructor() == true; if (EmptyStructTypeCache.IsTrackableStructType(type)) @@ -2876,7 +2877,7 @@ protected override void VisitStatement(BoundStatement statement) if (containingType?.IsTupleType == true && !isDefaultValueTypeConstructor) { // new System.ValueTuple(e1, ..., eN) - TrackNullableStateOfTupleElements(slot, containingType, arguments, argumentTypes, useRestField: true); + TrackNullableStateOfTupleElements(slot, containingType, arguments, argumentTypes, boundObjectCreationExpression!.ArgsToParamsOpt, useRestField: true); } else { @@ -4552,11 +4553,13 @@ private bool IsCompareExchangeMethod(MethodSymbol? method) { public readonly ImmutableArray Arguments; public readonly ImmutableArray Results; + public readonly ImmutableArray ArgsToParamsOpt; - public CompareExchangeInfo(ImmutableArray arguments, ImmutableArray results) + public CompareExchangeInfo(ImmutableArray arguments, ImmutableArray results, ImmutableArray argsToParamsOpt) { Arguments = arguments; Results = results; + ArgsToParamsOpt = argsToParamsOpt; } public bool IsDefault => Arguments.IsDefault || Results.IsDefault; @@ -4577,15 +4580,29 @@ private NullableFlowState LearnFromCompareExchangeMethod(in CompareExchangeInfo // location = value; // } - var comparand = compareExchangeInfo.Arguments[2]; - var valueFlowState = compareExchangeInfo.Results[1].RValueType.State; + if (compareExchangeInfo.Arguments.Length != 3) + { + // This can occur if CompareExchange has optional arguments. + // Since none of the main runtimes have optional arguments, + // we bail to avoid an exception but don't bother actually calculating the FlowState. + return NullableFlowState.NotNull; + } + + var argsToParamsOpt = compareExchangeInfo.ArgsToParamsOpt; + Debug.Assert(argsToParamsOpt is { IsDefault: true } or { Length: 3 }); + var (comparandIndex, valueIndex, locationIndex) = argsToParamsOpt.IsDefault + ? (2, 1, 0) + : (argsToParamsOpt.IndexOf(2), argsToParamsOpt.IndexOf(1), argsToParamsOpt.IndexOf(0)); + + var comparand = compareExchangeInfo.Arguments[comparandIndex]; + var valueFlowState = compareExchangeInfo.Results[valueIndex].RValueType.State; if (comparand.ConstantValue?.IsNull == true) { // If location contained a null, then the write `location = value` definitely occurred } else { - var locationFlowState = compareExchangeInfo.Results[0].RValueType.State; + var locationFlowState = compareExchangeInfo.Results[locationIndex].RValueType.State; // A write may have occurred valueFlowState = valueFlowState.Join(locationFlowState); } @@ -4872,7 +4889,7 @@ syntax switch if (!node.HasErrors && !parametersOpt.IsDefault) { // For CompareExchange method we need more context to determine the state of outbound assignment - CompareExchangeInfo compareExchangeInfo = IsCompareExchangeMethod(method) ? new CompareExchangeInfo(arguments, results) : default; + CompareExchangeInfo compareExchangeInfo = IsCompareExchangeMethod(method) ? new CompareExchangeInfo(arguments, results, argsToParamsOpt) : default; // Visit outbound assignments and post-conditions // Note: the state may get split in this step @@ -4893,7 +4910,7 @@ syntax switch parameterAnnotations, results[i], notNullParametersBuilder, - (!compareExchangeInfo.IsDefault && i == 0) ? compareExchangeInfo : default); + (!compareExchangeInfo.IsDefault && parameter.Ordinal == 0) ? compareExchangeInfo : default); } } else @@ -6146,7 +6163,7 @@ private void VisitTupleExpression(BoundTupleExpression node) if (slot > 0) { this.State[slot] = NullableFlowState.NotNull; - TrackNullableStateOfTupleElements(slot, tupleOpt, arguments, elementTypes, useRestField: false); + TrackNullableStateOfTupleElements(slot, tupleOpt, arguments, elementTypes, argsToParamsOpt: default, useRestField: false); } tupleOpt = tupleOpt.WithElementTypes(elementTypesWithAnnotations); @@ -6170,6 +6187,7 @@ private void VisitTupleExpression(BoundTupleExpression node) NamedTypeSymbol tupleType, ImmutableArray values, ImmutableArray types, + ImmutableArray argsToParamsOpt, bool useRestField) { Debug.Assert(tupleType.IsTupleType); @@ -6186,13 +6204,15 @@ private void VisitTupleExpression(BoundTupleExpression node) } for (int i = 0; i < n; i++) { - trackState(values[i], tupleElements[i], types[i]); + var argOrdinal = GetArgumentOrdinalFromParameterOrdinal(i); + trackState(values[argOrdinal], tupleElements[i], types[argOrdinal]); } if (useRestField && values.Length == NamedTypeSymbol.ValueTupleRestPosition && tupleType.GetMembers(NamedTypeSymbol.ValueTupleRestFieldName).FirstOrDefault() is FieldSymbol restField) { - trackState(values.Last(), restField, types.Last()); + var argOrdinal = GetArgumentOrdinalFromParameterOrdinal(NamedTypeSymbol.ValueTupleRestPosition - 1); + trackState(values[argOrdinal], restField, types[argOrdinal]); } } @@ -6201,6 +6221,13 @@ void trackState(BoundExpression value, FieldSymbol field, TypeWithState valueTyp int targetSlot = GetOrCreateSlot(field, slot); TrackNullableStateForAssignment(value, field.TypeWithAnnotations, targetSlot, valueType, MakeSlot(value)); } + + int GetArgumentOrdinalFromParameterOrdinal(int parameterOrdinal) + { + var index = argsToParamsOpt.IsDefault ? parameterOrdinal : argsToParamsOpt.IndexOf(parameterOrdinal); + Debug.Assert(index != -1); + return index; + } } private void TrackNullableStateOfNullableValue(int containingSlot, TypeSymbol containingType, BoundExpression? value, TypeWithState valueType, int valueSlot) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index 6d57c3c2d14..b1dd1915203 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -70187,6 +70187,109 @@ static void F(object? x, object y) Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "u.y").WithLocation(14, 9)); } + [Fact] + [WorkItem(46206, "https://github.com/dotnet/roslyn/issues/46206")] + public void Tuple_Assignment_25() + { + var source = +@"using System; +class Program +{ + static void F(object x, string y) + { + (object, string?) t = new ValueTuple(item2: """", item1: null) { Item1 = x }; + t.Item1.ToString(); + t.Item2.ToString(); + } +}"; + var comp = CreateCompilation(source, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (6,31): warning CS8619: Nullability of reference types in value of type '(object?, string)' doesn't match target type '(object, string?)'. + // (object, string?) t = new ValueTuple(item2: "", item1: null) { Item1 = x }; + Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, @"new ValueTuple(item2: """", item1: null) { Item1 = x }").WithArguments("(object?, string)", "(object, string?)").WithLocation(6, 31)); + } + + [Fact] + [WorkItem(46206, "https://github.com/dotnet/roslyn/issues/46206")] + public void Tuple_Assignment_26() + { + var source = +@"using System; +class Program +{ + static void F(bool b, object x, object? y) + { + var t = new ValueTuple, object, ValueTuple>( + x, + x, + x, + b ? y : x, // 1 + y, + new ValueTuple(b ? y : x, x), // 2, + rest: new ValueTuple(x, b ? y : x, b ? y : x), // 3 + item7: y)/*T:(object?, object!, object?, object!, object?, (object!, object?), object!, object!, object?, object!)*/; // 4 + t.Item1.ToString(); + t.Item2.ToString(); + t.Item3.ToString(); + t.Item4.ToString(); // 5 + t.Item5.ToString(); // 6 + t.Item6.Item1.ToString(); // 7 + t.Item6.Item2.ToString(); + t.Item7.ToString(); // 8 + if (b) + { + t.Item8.ToString(); + t.Item9.ToString(); // 9 + t.Item10.ToString(); // 10 + } + else + { + t.Rest.Item1.ToString(); + t.Rest.Item2.ToString(); // 11 + t.Rest.Item3.ToString(); // 12 + } + } +}"; + var comp = CreateCompilation(source, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (10,13): warning CS8604: Possible null reference argument for parameter 'item4' in '(object?, object, object?, object, object?, (object, object?), object, object, object?, object).ValueTuple(object? item1, object item2, object? item3, object item4, object? item5, (object, object?) item6, object item7, (object, object?, object) rest)'. + // b ? y : x, // 1 + Diagnostic(ErrorCode.WRN_NullReferenceArgument, "b ? y : x").WithArguments("item4", "(object?, object, object?, object, object?, (object, object?), object, object, object?, object).ValueTuple(object? item1, object item2, object? item3, object item4, object? item5, (object, object?) item6, object item7, (object, object?, object) rest)").WithLocation(10, 13), + // (12,45): warning CS8604: Possible null reference argument for parameter 'item1' in '(object, object?).ValueTuple(object item1, object? item2)'. + // new ValueTuple(b ? y : x, x), // 2, + Diagnostic(ErrorCode.WRN_NullReferenceArgument, "b ? y : x").WithArguments("item1", "(object, object?).ValueTuple(object item1, object? item2)").WithLocation(12, 45), + // (13,73): warning CS8604: Possible null reference argument for parameter 'item3' in '(object, object?, object).ValueTuple(object item1, object? item2, object item3)'. + // rest: new ValueTuple(x, b ? y : x, b ? y : x), // 3 + Diagnostic(ErrorCode.WRN_NullReferenceArgument, "b ? y : x").WithArguments("item3", "(object, object?, object).ValueTuple(object item1, object? item2, object item3)").WithLocation(13, 73), + // (14,20): warning CS8604: Possible null reference argument for parameter 'item7' in '(object?, object, object?, object, object?, (object, object?), object, object, object?, object).ValueTuple(object? item1, object item2, object? item3, object item4, object? item5, (object, object?) item6, object item7, (object, object?, object) rest)'. + // item7: y)/*T:(object?, object!, object?, object!, object?, (object!, object?), object!, object!, object?, object!)*/; // 4 + Diagnostic(ErrorCode.WRN_NullReferenceArgument, "y").WithArguments("item7", "(object?, object, object?, object, object?, (object, object?), object, object, object?, object).ValueTuple(object? item1, object item2, object? item3, object item4, object? item5, (object, object?) item6, object item7, (object, object?, object) rest)").WithLocation(14, 20), + // (18,9): warning CS8602: Dereference of a possibly null reference. + // t.Item4.ToString(); // 5 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "t.Item4").WithLocation(18, 9), + // (19,9): warning CS8602: Dereference of a possibly null reference. + // t.Item5.ToString(); // 6 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "t.Item5").WithLocation(19, 9), + // (20,9): warning CS8602: Dereference of a possibly null reference. + // t.Item6.Item1.ToString(); // 7 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "t.Item6.Item1").WithLocation(20, 9), + // (22,9): warning CS8602: Dereference of a possibly null reference. + // t.Item7.ToString(); // 8 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "t.Item7").WithLocation(22, 9), + // (26,13): warning CS8602: Dereference of a possibly null reference. + // t.Item9.ToString(); // 9 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "t.Item9").WithLocation(26, 13), + // (27,13): warning CS8602: Dereference of a possibly null reference. + // t.Item10.ToString(); // 10 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "t.Item10").WithLocation(27, 13), + // (32,13): warning CS8602: Dereference of a possibly null reference. + // t.Rest.Item2.ToString(); // 11 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "t.Rest.Item2").WithLocation(32, 13), + // (33,13): warning CS8602: Dereference of a possibly null reference. + // t.Rest.Item3.ToString(); // 12 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "t.Rest.Item3").WithLocation(33, 13)); + } + [Fact] [WorkItem(32338, "https://github.com/dotnet/roslyn/issues/32338")] public void CopyStructUnconstrainedFieldNullability_01() @@ -127508,6 +127611,80 @@ void M() Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "location").WithLocation(18, 13)); } + [Fact] + [WorkItem(46673, "https://github.com/dotnet/roslyn/issues/46673")] + public void CompareExchange_NamedArguments() + { + 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 location1 = """"; + Interlocked.CompareExchange(ref location1, comparand: """", value: null); // 1 + + object location2 = """"; + Interlocked.CompareExchange(ref location2, comparand: null, value: """"); + + object location3 = """"; + Interlocked.CompareExchange(comparand: """", value: null, location: ref location3); // 2 + + object location4 = """"; + Interlocked.CompareExchange(comparand: null, value: """", location: ref location4); + } +} +"; + var comp = CreateCompilation(source, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (17,41): warning CS8600: Converting null literal or possible null value to non-nullable type. + // Interlocked.CompareExchange(ref location1, comparand: "", value: null); // 1 + Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "location1").WithLocation(17, 41), + // (23,79): warning CS8600: Converting null literal or possible null value to non-nullable type. + // Interlocked.CompareExchange(comparand: "", value: null, location: ref location3); // 2 + Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "location3").WithLocation(23, 79)); + } + + [Fact] + [WorkItem(46673, "https://github.com/dotnet/roslyn/issues/46673")] + public void CompareExchange_NamedArgumentsWithOptionalParameters() + { + var source = @" +#pragma warning disable 0436 +using System.Threading; +namespace System.Threading +{ + class Interlocked + { + public static T CompareExchange(ref T location1, T value, T comparand = default) where T : class => value; + } +} +class C +{ + static void F(object target, object value) + { + Interlocked.CompareExchange(value: value, location1: ref target); + } +}"; + var comp = CreateCompilation(source, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (8,84): warning CS8600: Converting null literal or possible null value to non-nullable type. + // public static T CompareExchange(ref T location1, T value, T comparand = default) where T : class => value; + Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "default").WithLocation(8, 84), + // (15,9): warning CS8634: The type 'object?' cannot be used as type parameter 'T' in the generic type or method 'Interlocked.CompareExchange(ref T, T, T)'. Nullability of type argument 'object?' doesn't match 'class' constraint. + // Interlocked.CompareExchange(value: value, location1: ref target); + Diagnostic(ErrorCode.WRN_NullabilityMismatchInTypeParameterReferenceTypeConstraint, "Interlocked.CompareExchange").WithArguments("System.Threading.Interlocked.CompareExchange(ref T, T, T)", "T", "object?").WithLocation(15, 9)); + } + [Fact] [WorkItem(37187, "https://github.com/dotnet/roslyn/issues/37187")] public void IEquatableContravariantNullability() -- GitLab