diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LValueFlowCaptureProvider.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LValueFlowCaptureProvider.cs index 446379586e5b2..598480f41b935 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LValueFlowCaptureProvider.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LValueFlowCaptureProvider.cs @@ -44,12 +44,6 @@ static bool IsLValueFlowCapture (IFlowCaptureReferenceOperation flowCaptureRefer if (assignment?.Target == flowCaptureReference) return true; - if (flowCaptureReference.Parent is IArrayElementReferenceOperation arrayAlementRef) { - assignment = arrayAlementRef.Parent as IAssignmentOperation; - if (assignment?.Target == arrayAlementRef) - return true; - } - assignment = null; return flowCaptureReference.IsInLeftOfDeconstructionAssignment (out _); } diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs index 41763938dfe53..daada3232b287 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs @@ -235,45 +235,11 @@ TValue ProcessSingleTargetAssignment (IOperation targetOperation, ISimpleAssignm if (arrayElementRef.Indices.Length != 1) break; - // Similarly to VisitSimpleAssignment, this needs to handle cases where the array reference - // is a captured variable, even if the target of the assignment (the array element reference) is not. - - TValue arrayRef; - TValue index; - TValue value; - if (arrayElementRef.ArrayReference is not IFlowCaptureReferenceOperation captureReference) { - arrayRef = Visit (arrayElementRef.ArrayReference, state); - index = Visit (arrayElementRef.Indices[0], state); - value = Visit (operation.Value, state); - HandleArrayElementWrite (arrayRef, index, value, operation, merge: merge); - return value; - } - - index = Visit (arrayElementRef.Indices[0], state); - value = Visit (operation.Value, state); - - var capturedReferences = state.Current.CapturedReferences.Get (captureReference.Id); - if (!capturedReferences.HasMultipleValues) { - // Single captured reference. Treat this as an overwriting assignment, - // unless the caller already told us to merge values because this is an - // assignment to one of multiple captured array element references. - var enumerator = capturedReferences.GetEnumerator (); - enumerator.MoveNext (); - var capture = enumerator.Current; - arrayRef = Visit (capture.Reference, state); - HandleArrayElementWrite (arrayRef, index, value, operation, merge: merge); - return value; - } - // The capture id may have captured multiple references, as in: - // (b ? arr1 : arr2)[0] = value; - // We treat this as possible write to each of the captured references, - // which requires merging with the previous values of each. - - foreach (var capture in state.Current.CapturedReferences.Get (captureReference.Id)) { - arrayRef = Visit (capture.Reference, state); - HandleArrayElementWrite (arrayRef, index, value, operation, merge: true); - } + TValue arrayRef = Visit (arrayElementRef.ArrayReference, state); + TValue index = Visit (arrayElementRef.Indices[0], state); + TValue value = Visit (operation.Value, state); + HandleArrayElementWrite (arrayRef, index, value, operation, merge: merge); return value; } case IInlineArrayAccessOperation inlineArrayAccess: { @@ -371,19 +337,10 @@ public override TValue VisitSimpleAssignment (ISimpleAssignmentOperation operati TValue GetFlowCaptureValue (IFlowCaptureReferenceOperation operation, LocalDataFlowState state) { - if (!operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Read)) { - // There are known cases where this assert doesn't hold, because LValueFlowCaptureProvider - // produces the wrong result in some cases for flow captures with IsInitialization = true. - // https://github.com/dotnet/linker/issues/2749 - // Debug.Assert (IsLValueFlowCapture (operation.Id)); - return TopValue; - } - - // This assert is incorrect for cases like (b ? arr1 : arr2)[0] = v; - // Here the ValueUsageInfo shows that the value usage is for reading (this is probably wrong!) - // but the value is actually an LValueFlowCapture. - // Let's just disable the assert for now. - // Debug.Assert (IsRValueFlowCapture (operation.Id)); + Debug.Assert (!IsLValueFlowCapture (operation.Id), + $"{operation.Syntax.GetLocation ().GetLineSpan ()}"); + Debug.Assert (operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Read), + $"{operation.Syntax.GetLocation ().GetLineSpan ()}"); return state.Get (new LocalKey (operation.Id)); } @@ -391,6 +348,20 @@ TValue GetFlowCaptureValue (IFlowCaptureReferenceOperation operation, LocalDataF // Similar to VisitLocalReference public override TValue VisitFlowCaptureReference (IFlowCaptureReferenceOperation operation, LocalDataFlowState state) { + if (operation.IsInitialization) { + // This capture reference is a temporary byref. This can happen for string + // interpolation handlers: https://github.com/dotnet/roslyn/issues/57484. + // Should really be treated as creating a new l-value flow capture, + // but this is likely irrelevant for dataflow analysis. + + // LValueFlowCaptureProvider doesn't take into account IsInitialization = true, + // so it doesn't properly detect this as an l-value capture. + // Context: https://github.com/dotnet/roslyn/issues/60757 + // Debug.Assert (IsLValueFlowCapture (operation.Id)); + Debug.Assert (operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Write), + $"{operation.Syntax.GetLocation ().GetLineSpan ()}"); + return TopValue; + } return GetFlowCaptureValue (operation, state); } @@ -399,26 +370,41 @@ public override TValue VisitFlowCaptureReference (IFlowCaptureReferenceOperation // is like a local reference. public override TValue VisitFlowCapture (IFlowCaptureOperation operation, LocalDataFlowState state) { - // If the captured value is a property reference, we can't easily tell inside of - // VisitPropertyReference whether it is accessed for reads or writes. - // https://github.com/dotnet/roslyn/issues/25057 - // Avoid visiting the captured value unless it is an RValue. if (IsLValueFlowCapture (operation.Id)) { + // Should never see an l-value flow capture of another flow capture. + Debug.Assert (operation.Value is not IFlowCaptureReferenceOperation); + if (operation.Value is IFlowCaptureReferenceOperation) + return TopValue; + // Note: technically we should save some information about the value for LValue flow captures // (for example, the object instance of a property reference) and avoid re-computing it when // assigning to the FlowCaptureReference. + var capturedRef = new CapturedReferenceValue (operation.Value); var currentState = state.Current; - currentState.CapturedReferences.Set (operation.Id, new CapturedReferenceValue (operation.Value)); + currentState.CapturedReferences.Set (operation.Id, capturedRef); state.Current = currentState; - } + return TopValue; + } else { + TValue capturedValue; + if (operation.Value is IFlowCaptureReferenceOperation captureRef) { + if (IsLValueFlowCapture (captureRef.Id)) { + // If an r-value captures an l-value, we must dereference the l-value + // and copy out the value to capture. + capturedValue = TopValue; + foreach (var capturedReference in state.Current.CapturedReferences.Get (captureRef.Id)) { + var value = Visit (capturedReference.Reference, state); + capturedValue = LocalStateLattice.Lattice.ValueLattice.Meet (capturedValue, value); + } + } else { + capturedValue = state.Get (new LocalKey (captureRef.Id)); + } + } else { + capturedValue = Visit (operation.Value, state); + } - if (IsRValueFlowCapture (operation.Id)) { - TValue value = Visit (operation.Value, state); - state.Set (new LocalKey (operation.Id), value); - return value; + state.Set (new LocalKey (operation.Id), capturedValue); + return capturedValue; } - - return TopValue; } public override TValue VisitExpressionStatement (IExpressionStatementOperation operation, LocalDataFlowState state) diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/DataFlowTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/DataFlowTests.cs index 71690c473c1e3..c258ccf5a0ccb 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/DataFlowTests.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/DataFlowTests.cs @@ -161,6 +161,12 @@ public Task InlineArrayDataflow () return RunTest (); } + [Fact] + public Task InterpolatedStringHandlerDataFlow () + { + return RunTest (); + } + [Fact] public Task MakeGenericDataFlow () { diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ArrayDataFlow.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ArrayDataFlow.cs index 7f2ccf14eba3d..1fc103037b388 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ArrayDataFlow.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ArrayDataFlow.cs @@ -3,6 +3,8 @@ using System; using System.Diagnostics.CodeAnalysis; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; using Mono.Linker.Tests.Cases.Expectations.Assertions; using Mono.Linker.Tests.Cases.Expectations.Helpers; @@ -30,6 +32,7 @@ public static void Main () TestArraySetElementAndInitializerMultipleElementsMix (typeof (TestType)); TestGetElementAtUnknownIndex (); + TestGetMergedArrayElement (); TestMergedArrayElementWithUnknownIndex (0); // Array reset - certain operations on array are not tracked fully (or impossible due to unknown inputs) @@ -47,6 +50,8 @@ public static void Main () WriteCapturedArrayElement.Test (); + WriteElementOfCapturedArray.Test (); + ConstantFieldValuesAsIndex.Test (); HoistedArrayMutation.Test (); @@ -205,6 +210,23 @@ static void TestGetElementAtUnknownIndex (int i = 0) arr[i].RequiresPublicFields (); } + // https://github.com/dotnet/runtime/issues/93416 tracks the discrepancy between + // the analyzer and ILLink/ILCompiler. + [ExpectedWarning ("IL2062", nameof (DataFlowTypeExtensions.RequiresAll), + ProducedBy = Tool.Trimmer | Tool.NativeAot)] + [ExpectedWarning ("IL2072", nameof (GetMethods), nameof (DataFlowTypeExtensions.RequiresAll), + ProducedBy = Tool.Analyzer)] + [ExpectedWarning ("IL2072", nameof (GetFields), nameof (DataFlowTypeExtensions.RequiresAll), + ProducedBy = Tool.Analyzer)] + static void TestGetMergedArrayElement (bool b = true) + { + Type[] arr = new Type[] { GetMethods () }; + Type[] arr2 = new Type[] { GetFields () }; + if (b) + arr = arr2; + arr[0].RequiresAll (); + } + // Trimmer code doesnt handle locals from different branches separetely, therefore merges incorrectly GetMethods with Unknown producing both warnings [ExpectedWarning ("IL2072", nameof (ArrayDataFlow.GetMethods), ProducedBy = Tool.Trimmer | Tool.NativeAot)] [ExpectedWarning ("IL2062", nameof (DataFlowTypeExtensions.RequiresAll))] @@ -614,6 +636,102 @@ public static void Test () } } + class WriteElementOfCapturedArray + { + [Kept] + [ExpectedWarning ("IL2072", nameof (GetUnknownType), nameof (DataFlowTypeExtensions.RequiresAll))] + [ExpectedWarning ("IL2072", nameof (GetTypeWithPublicConstructors), nameof (DataFlowTypeExtensions.RequiresAll))] + // Analysis hole: https://github.com/dotnet/runtime/issues/90335 + // The array element assignment assigns to a temp array created as a copy of + // arr1 or arr2, and writes to it aren't reflected back in arr1/arr2. + static void TestArrayElementAssignment (bool b = true) + { + var arr1 = new Type[] { GetUnknownType () }; + var arr2 = new Type[] { GetTypeWithPublicConstructors () }; + (b ? arr1 : arr2)[0] = GetWithPublicMethods (); + arr1[0].RequiresAll (); + arr2[0].RequiresAll (); + } + + [Kept] + [KeptAttributeAttribute (typeof (InlineArrayAttribute))] + [InlineArray (8)] + public struct InlineTypeArray + { + [Kept] + public Type t; + } + + [Kept] + [ExpectedWarning ("IL2062", nameof (DataFlowTypeExtensions.RequiresAll))] + [ExpectedWarning ("IL2062", nameof (DataFlowTypeExtensions.RequiresAll))] + static void TestInlineArrayElementReferenceAssignment (bool b = true) + { + var arr1 = new InlineTypeArray (); + arr1[0] = GetUnknownType (); + var arr2 = new InlineTypeArray (); + arr2[0] = GetTypeWithPublicConstructors (); + (b ? ref arr1[0] : ref arr2[0]) = GetTypeWithPublicConstructors (); + arr1[0].RequiresAll (); + arr2[0].RequiresAll (); + } + + // Inline array references are not allowed in conditionals, unlike array references. + // static void TestInlineArrayElementAssignment (bool b = true) + // { + // var arr1 = new InlineTypeArray (); + // arr1[0] = GetUnknownType (); + // var arr2 = new InlineTypeArray (); + // arr2[0] = GetTypeWithPublicConstructors (); + // (b ? arr1 : arr2)[0] = GetWithPublicMethods (); + // arr1[0].RequiresAll (); + // arr2[0].RequiresAll (); + // } + + [ExpectedWarning ("IL2087", nameof (T), nameof (DataFlowTypeExtensions.RequiresAll))] + [ExpectedWarning ("IL2087", nameof (U), nameof (DataFlowTypeExtensions.RequiresPublicFields))] + // Missing warnings for 'V' possibly assigned to arr or arr2 because write to temp + // array isn't reflected back in the local variables. https://github.com/dotnet/linker/issues/2158 + static void TestNullCoalesce (bool b = false) + { + Type[]? arr = new Type[1] { typeof (T) }; + Type[] arr2 = new Type[1] { typeof (U) }; + + (arr ?? arr2)[0] = typeof (V); + arr[0].RequiresAll (); + arr2[0].RequiresPublicFields (); + } + + [ExpectedWarning ("IL2087", nameof (T), nameof (DataFlowTypeExtensions.RequiresAll), ProducedBy = Tool.Analyzer)] + [ExpectedWarning ("IL2087", nameof (U), nameof (DataFlowTypeExtensions.RequiresPublicFields))] + // Missing warnings for 'V' possibly assigned to arr or arr2 because write to temp + // array isn't reflected back in the local variables. https://github.com/dotnet/linker/issues/2158 + // This also causes an extra analyzer warning for 'U' in 'arr', because the analyzer models the + // possible assignment of arr2 to arr, without overwriting index '0'. And it produces a warning + // for each possible value, unlike ILLink/ILCompiler, which produce an unknown value for a merged + // array value: https://github.com/dotnet/runtime/issues/93416 + [ExpectedWarning ("IL2087", nameof (U), nameof (DataFlowTypeExtensions.RequiresAll), ProducedBy = Tool.Analyzer)] + [ExpectedWarning ("IL2062", nameof (DataFlowTypeExtensions.RequiresAll), ProducedBy = Tool.Trimmer | Tool.NativeAot)] + static void TestNullCoalescingAssignment (bool b = true) + { + Type[]? arr = new Type[1] { typeof (T) }; + Type[] arr2 = new Type[1] { typeof (U) }; + + (arr ??= arr2)[0] = typeof (V); + arr[0].RequiresAll (); + arr2[0].RequiresPublicFields (); + } + + public static void Test () + { + TestArrayElementAssignment (); + TestInlineArrayElementReferenceAssignment (); + // TestInlineArrayElementAssignment (); + TestNullCoalesce (); + TestNullCoalescingAssignment (); + } + } + class ConstantFieldValuesAsIndex { private const sbyte ConstSByte = 1; diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ByRefDataflow.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ByRefDataflow.cs index 6435023965436..d00af37b2bebb 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ByRefDataflow.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ByRefDataflow.cs @@ -4,8 +4,6 @@ using System; using System.Diagnostics.CodeAnalysis; using System.Reflection; -using System.Runtime.CompilerServices; -using System.Runtime.InteropServices; using Mono.Linker.Tests.Cases.Expectations.Assertions; using Mono.Linker.Tests.Cases.Expectations.Metadata; using Mono.Linker.Tests.Cases.Expectations.Helpers; @@ -261,56 +259,6 @@ static void TestArrayElementReferenceAssignment (bool b = true) arr2[0].RequiresAll (); } - [Kept] - [ExpectedWarning ("IL2072", nameof (GetUnknownType), nameof (DataFlowTypeExtensions.RequiresAll))] - [ExpectedWarning ("IL2072", nameof (GetTypeWithPublicConstructors), nameof (DataFlowTypeExtensions.RequiresAll))] - // ILLink/ILCompiler analysis hole: https://github.com/dotnet/runtime/issues/90335 - [ExpectedWarning ("IL2072", nameof (GetTypeWithPublicFields), nameof (DataFlowTypeExtensions.RequiresAll), ProducedBy = Tool.Analyzer)] - [ExpectedWarning ("IL2072", nameof (GetTypeWithPublicFields), nameof (DataFlowTypeExtensions.RequiresAll), ProducedBy = Tool.Analyzer)] - static void TestArrayElementAssignment (bool b = true) - { - var arr1 = new Type[] { GetUnknownType () }; - var arr2 = new Type[] { GetTypeWithPublicConstructors () }; - (b ? arr1 : arr2)[0] = GetTypeWithPublicFields (); - arr1[0].RequiresAll (); - arr2[0].RequiresAll (); - } - - [Kept] - [KeptAttributeAttribute (typeof (InlineArrayAttribute))] - [InlineArray (8)] - public struct InlineTypeArray - { - [Kept] - public Type t; - } - - [Kept] - [ExpectedWarning ("IL2062", nameof (DataFlowTypeExtensions.RequiresAll))] - [ExpectedWarning ("IL2062", nameof (DataFlowTypeExtensions.RequiresAll))] - static void TestInlineArrayElementReferenceAssignment (bool b = true) - { - var arr1 = new InlineTypeArray (); - arr1[0] = GetUnknownType (); - var arr2 = new InlineTypeArray (); - arr2[0] = GetTypeWithPublicConstructors (); - (b ? ref arr1[0] : ref arr2[0]) = GetTypeWithPublicFields (); - arr1[0].RequiresAll (); - arr2[0].RequiresAll (); - } - - // Inline array references are not allowed in conditionals, unlike array references. - // static void TestInlineArrayElementAssignment (bool b = true) - // { - // var arr1 = new InlineTypeArray (); - // arr1[0] = GetUnknownType (); - // var arr2 = new InlineTypeArray (); - // arr2[0] = GetTypeWithPublicConstructors (); - // (b ? arr1 : arr2)[0] = GetTypeWithPublicFields (); - // arr1[0].RequiresAll (); - // arr2[0].RequiresAll (); - // } - [Kept] [ExpectedWarning ("IL2074", nameof (_publicMethodsField), nameof (GetUnknownType))] [ExpectedWarning ("IL2074", nameof (_publicPropertiesField), nameof (GetUnknownType))] @@ -358,9 +306,6 @@ public static void Test () TestParameterAssignment (); TestLocalAssignment (); TestArrayElementReferenceAssignment (); - TestArrayElementAssignment (); - TestInlineArrayElementReferenceAssignment (); - // TestInlineArrayElementAssignment (); TestNullCoalescingAssignment (); TestNullCoalescingAssignmentComplex (); TestDataFlowOnRightHandOfAssignment (); diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/FieldDataFlow.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/FieldDataFlow.cs index 5abbd0d520200..9fcc94ed7a02f 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/FieldDataFlow.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/FieldDataFlow.cs @@ -33,6 +33,7 @@ public static void Main () instance.WriteUnknownValue (); WriteCapturedField.Test (); + WriteFieldOfCapturedInstance.Test (); _ = _annotationOnWrongType; @@ -192,6 +193,35 @@ public static void Test () } } + class WriteFieldOfCapturedInstance + { + class ClassWithAnnotatedField + { + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] + public Type field; + } + + [ExpectedWarning ("IL2074", nameof (GetUnknownType), nameof (ClassWithAnnotatedField.field))] + static void TestNullCoalesce () + { + ClassWithAnnotatedField? instance = null; + (instance ?? new ClassWithAnnotatedField ()).field = GetUnknownType (); + } + + [ExpectedWarning ("IL2074", nameof (GetUnknownType), nameof (ClassWithAnnotatedField.field))] + static void TestNullCoalescingAssignment () + { + ClassWithAnnotatedField? instance = null; + (instance ??= new ClassWithAnnotatedField ()).field = GetUnknownType (); + } + + public static void Test () + { + TestNullCoalesce (); + TestNullCoalescingAssignment (); + } + } + class AccessReturnedInstanceField { [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterpolatedStringHandlerDataFlow.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterpolatedStringHandlerDataFlow.cs new file mode 100644 index 0000000000000..6e30b600d736a --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterpolatedStringHandlerDataFlow.cs @@ -0,0 +1,35 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System; +using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using Mono.Linker.Tests.Cases.Expectations.Assertions; +using Mono.Linker.Tests.Cases.Expectations.Helpers; +using Mono.Linker.Tests.Cases.Expectations.Metadata; + +namespace Mono.Linker.Tests.Cases.DataFlow +{ + [ExpectedNoWarnings] + [SkipKeptItemsValidation] + [Define ("DEBUG")] + public class InterpolatedStringHandlerDataFlow + { + public static void Main () + { + Test (); + } + + static void Test(bool b = true) { + // Creates a control-flow graph for the analyzer that has an + // IFlowCaptureReferenceOperation that represents a capture + // because it is used as an out param (so has IsInitialization = true). + // See https://github.com/dotnet/roslyn/issues/57484 for context. + // This test ensures the analyzer has coverage for cases + // where IsInitialization = true. + Debug.Assert (b, $"Debug interpolated string handler {b}"); + } + } +}