From dee0419b72c93f9145c58001ff3220d33566d4a0 Mon Sep 17 00:00:00 2001 From: vitek-karas Date: Wed, 23 Jun 2021 07:56:12 -0700 Subject: [PATCH 1/6] Warn when accessing DAM annotated method via reflection Any access to DAM annotated method must be validated by the trimmer. Normal calls and such are handled already, but direct reflection access or other indirect accesses can lead to trimmer allowing potential invocation of DAM annotated method without validating the annotations. This change will warn whenever such potential "leak" happens. This applies to method parameter, method return value and field annotations. Uses the same infra as RUC already just adds additional checks. Tests for the new cases. --- docs/error-codes.md | 33 +- src/linker/Linker.Dataflow/FlowAnnotations.cs | 67 ++- src/linker/Linker.Steps/MarkStep.cs | 65 ++- .../AnnotatedMembersAccessedViaReflection.cs | 409 ++++++++++++++++++ ...odHierarchyDataflowAnnotationValidation.cs | 3 + .../Reflection/ExpressionCallString.cs | 26 +- 6 files changed, 528 insertions(+), 75 deletions(-) create mode 100644 test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs diff --git a/docs/error-codes.md b/docs/error-codes.md index ba6d39f93c5b..cafb2a506cd2 100644 --- a/docs/error-codes.md +++ b/docs/error-codes.md @@ -1689,7 +1689,7 @@ The only scopes supported on global unconditional suppressions are 'module', 'ty it is assumed that the suppression is put on the module. Global unconditional suppressions using invalid scopes are ignored. ```C# -// Invalid scope 'method' used in 'UnconditionalSuppressMessageAttribute' on module 'Warning' with target 'MyTarget'. +// IL2108: Invalid scope 'method' used in 'UnconditionalSuppressMessageAttribute' on module 'Warning' with target 'MyTarget'. [module: UnconditionalSuppressMessage ("Test suppression with invalid scope", "IL2026", Scope = "method", Target = "MyTarget")] class Warning @@ -1722,6 +1722,37 @@ class Warning class Derived : UnsafeClass {} ``` +#### `IL2110`: Trim analysis: Field 'field' with 'DynamicallyAccessedMembersAttribute' is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field. + +- Trimmer currently can't guarantee that all requirements of the `DynamicallyAccessedMembersAttribute` are fulfilled if the field is accessed via reflection. + +```C# +[DynamicallyAccessedMembers(DynamicallyAccessedMemeberTypes.PublicMethods)] +Type _field; + +void TestMethod() +{ + // IL2110: Field '_field' with 'DynamicallyAccessedMembersAttribute' is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field. + typeof(Test).GetField("_field"); +} +``` + +#### `IL2111`: Trim analysis: Method 'method' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field. + +- Trimmer currently can't guarantee that all requirements of the `DynamicallyAccessedMembersAttribute` are fulfilled if the method is accessed via reflection. + +```C# +void MethodWithRequirements([DynamicallyAccessedMembers(DynamicallyAccessedMemeberTypes.PublicMethods)] Type type) +{ +} + +void TestMethod() +{ + // IL2111: Method 'MethodWithRequirements' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field. + typeof(Test).GetMethod("MethodWithRequirements"); +} +``` + ## Single-File Warning Codes #### `IL3000`: 'member' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory' diff --git a/src/linker/Linker.Dataflow/FlowAnnotations.cs b/src/linker/Linker.Dataflow/FlowAnnotations.cs index 898c7074b92a..0163425eb91b 100644 --- a/src/linker/Linker.Dataflow/FlowAnnotations.cs +++ b/src/linker/Linker.Dataflow/FlowAnnotations.cs @@ -23,57 +23,37 @@ public FlowAnnotations (LinkContext context) _hierarchyInfo = new TypeHierarchyCache (context); } - public bool RequiresDataFlowAnalysis (MethodDefinition method) - { - return GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out _); - } + public bool RequiresDataFlowAnalysis (MethodDefinition method) => + GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out _); - public bool RequiresDataFlowAnalysis (FieldDefinition field) - { - return GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out _); - } + public bool RequiresDataFlowAnalysis (FieldDefinition field) => + GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out _); - public bool RequiresDataFlowAnalysis (GenericParameter genericParameter) - { - return GetGenericParameterAnnotation (genericParameter) != DynamicallyAccessedMemberTypes.None; - } + public bool RequiresDataFlowAnalysis (GenericParameter genericParameter) => + GetGenericParameterAnnotation (genericParameter) != DynamicallyAccessedMemberTypes.None; /// /// Retrieves the annotations for the given parameter. /// /// Parameter index in the IL sense. Parameter 0 on instance methods is `this`. /// - public DynamicallyAccessedMemberTypes GetParameterAnnotation (MethodDefinition method, int parameterIndex) - { - if (GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) && annotation.ParameterAnnotations != null) { - return annotation.ParameterAnnotations[parameterIndex]; - } + public DynamicallyAccessedMemberTypes GetParameterAnnotation (MethodDefinition method, int parameterIndex) => + (GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) && annotation.ParameterAnnotations != null) + ? annotation.ParameterAnnotations[parameterIndex] + : DynamicallyAccessedMemberTypes.None; - return DynamicallyAccessedMemberTypes.None; - } + public DynamicallyAccessedMemberTypes GetReturnParameterAnnotation (MethodDefinition method) => + GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) + ? annotation.ReturnParameterAnnotation + : DynamicallyAccessedMemberTypes.None; - public DynamicallyAccessedMemberTypes GetReturnParameterAnnotation (MethodDefinition method) - { - if (GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation)) { - return annotation.ReturnParameterAnnotation; - } + public DynamicallyAccessedMemberTypes GetFieldAnnotation (FieldDefinition field) => + GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out var annotation) + ? annotation.Annotation + : DynamicallyAccessedMemberTypes.None; - return DynamicallyAccessedMemberTypes.None; - } - - public DynamicallyAccessedMemberTypes GetFieldAnnotation (FieldDefinition field) - { - if (GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out var annotation)) { - return annotation.Annotation; - } - - return DynamicallyAccessedMemberTypes.None; - } - - public DynamicallyAccessedMemberTypes GetTypeAnnotation (TypeDefinition type) - { - return GetAnnotations (type).TypeAnnotation; - } + public DynamicallyAccessedMemberTypes GetTypeAnnotation (TypeDefinition type) => + GetAnnotations (type).TypeAnnotation; public DynamicallyAccessedMemberTypes GetGenericParameterAnnotation (GenericParameter genericParameter) { @@ -93,6 +73,13 @@ public DynamicallyAccessedMemberTypes GetGenericParameterAnnotation (GenericPara return DynamicallyAccessedMemberTypes.None; } + public bool ShouldWarnWhenAccessedForReflection (MethodDefinition method) => + GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) && + (annotation.ParameterAnnotations != null || annotation.ReturnParameterAnnotation != DynamicallyAccessedMemberTypes.None); + + public bool ShouldWarnWhenAccessedForReflection (FieldDefinition field) => + GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out _); + TypeAnnotations GetAnnotations (TypeDefinition type) { if (!_annotations.TryGetValue (type, out TypeAnnotations value)) { diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 39f70f6225c5..d1026003173d 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -1557,6 +1557,22 @@ void MarkField (FieldDefinition field, in DependencyInfo reason) Annotations.Mark (field, reason); } + switch (reason.Kind) { + case DependencyKind.AccessedViaReflection: + case DependencyKind.DynamicDependency: + case DependencyKind.DynamicallyAccessedMember: + case DependencyKind.InteropMethodDependency: + if (_context.Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (field)) + _context.LogWarning ( + $"Field '{field.GetDisplayName ()}' with 'DynamicallyAccessedMembersAttribute' is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field.", + 2110, + _scopeStack.CurrentScope.Origin, + MessageSubCategory.TrimAnalysis); + + break; + } + + if (CheckProcessed (field)) return; @@ -2711,12 +2727,12 @@ protected virtual MethodDefinition MarkMethod (MethodReference reference, Depend // Use the original reason as it's important to correctly generate warnings // the updated reason is only useful for better tracking of dependencies. - ProcessRequiresUnreferencedCode (method, originalReasonKind); + ProcessAnalysisAnnotationsForMethod (method, originalReasonKind); return method; } - void ProcessRequiresUnreferencedCode (MethodDefinition method, DependencyKind dependencyKind) + void ProcessAnalysisAnnotationsForMethod (MethodDefinition method, DependencyKind dependencyKind) { switch (dependencyKind) { // DirectCall, VirtualCall and NewObj are handled by ReflectionMethodBodyScanner @@ -2773,24 +2789,43 @@ void ProcessRequiresUnreferencedCode (MethodDefinition method, DependencyKind de return; default: - // DirectCall, VirtualCall and NewObj are handled by ReflectionMethodBodyScanner - // This is necessary since the ReflectionMethodBodyScanner has intrinsic handling for some - // of the annotated methods annotated (for example Type.GetType) - // and it knows when it's OK and when it needs a warning. In this place we don't know - // and would have to warn every time - // All other cases have the potential of us missing a warning if we don't report it // It is possible that in some cases we may report the same warning twice, but that's better than not reporting it. break; } - // All override methods should have the same annotations as their base methods (else we will produce warning IL2046.) - // When marking override methods with RequiresUnreferencedCode on a type annotated with DynamicallyAccessedMembers, - // we should only issue a warning for the base method. - if (dependencyKind != DependencyKind.DynamicallyAccessedMember || - !method.IsVirtual || - Annotations.GetBaseMethods (method) == null) - CheckAndReportRequiresUnreferencedCode (method); + // All override methods should have the same annotations as their base methods + // (else we will produce warning IL2046 or IL2092 or some other warning). + // When marking override methods via DynamicallyAccessedMembers, we should only issue a warning for the base method. + if (dependencyKind == DependencyKind.DynamicallyAccessedMember && + method.IsVirtual && + Annotations.GetBaseMethods (method) != null) + return; + + CheckAndReportRequiresUnreferencedCode (method); + + if (_context.Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (method)) { + // If the current scope has analysis warnings suppressed, don't generate any + if (ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode ()) + return; + + // ReflectionMethodBodyScanner handles more cases for data flow annotations + // so don't warn for those. + switch (dependencyKind) { + case DependencyKind.AttributeConstructor: + case DependencyKind.AttributeProperty: + return; + + default: + break; + } + + _context.LogWarning ( + $"Method '{method.GetDisplayName ()}' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field.", + 2111, + _scopeStack.CurrentScope.Origin, + MessageSubCategory.TrimAnalysis); + } } internal bool ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode () diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs b/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs new file mode 100644 index 000000000000..f54e2cd88fb3 --- /dev/null +++ b/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs @@ -0,0 +1,409 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; +using System.Linq; +using System.Linq.Expressions; +using System.Runtime.InteropServices; +using System.Text; +using System.Threading.Tasks; +using Mono.Linker.Tests.Cases.Expectations.Assertions; +using Mono.Linker.Tests.Cases.Expectations.Helpers; + +namespace Mono.Linker.Tests.Cases.DataFlow +{ + [SkipKeptItemsValidation] + [ExpectedNoWarnings] + class AnnotatedMembersAccessedViaReflection + { + public static void Main () + { + AnnotatedField.Test (); + AnnotatedMethodParameters.Test (); + AnnotatedMethodReturnValue.Test (); + AnnotatedProperty.Test (); + AnnotatedGenerics.Test (); + AnnotationOnGenerics.Test (); + AnnotationOnInteropMethod.Test (); + AccessThroughLdToken.Test (); + } + + class AnnotatedField + { + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] + public static Type _annotatedField; + + [ExpectedWarning ("IL2110", nameof (_annotatedField))] + static void Reflection () + { + typeof (AnnotatedField).GetField ("_annotatedField").SetValue (null, typeof (TestType)); + } + + [ExpectedWarning ("IL2110", nameof (_annotatedField))] + static void ReflectionReadOnly () + { + typeof (AnnotatedField).GetField ("_annotatedField").GetValue (null); + } + + [ExpectedWarning ("IL2110", nameof (_annotatedField))] + [DynamicDependency (DynamicallyAccessedMemberTypes.PublicFields, typeof (AnnotatedField))] + static void DynamicDependency () + { + } + + [ExpectedWarning ("IL2110", nameof (_annotatedField))] + [DynamicDependency (nameof (_annotatedField), typeof (AnnotatedField))] + static void DynamicDependencyByName () + { + } + + [ExpectedWarning ("IL2110", nameof (_annotatedField))] + static void DynamicallyAccessedMembers () + { + typeof (AnnotatedField).RequiresPublicFields (); + } + + public static void Test () + { + Reflection (); + ReflectionReadOnly (); + DynamicDependency (); + DynamicDependencyByName (); + DynamicallyAccessedMembers (); + } + } + + class AnnotatedMethodParameters + { + public static void MethodWithSingleAnnotatedParameter ( + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type type) + { } + + class AttributeWithConstructorWithAnnotation : Attribute + { + public AttributeWithConstructorWithAnnotation ( + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type type) { } + } + + [ExpectedWarning ("IL2111", nameof (MethodWithSingleAnnotatedParameter))] + static void Reflection () + { + typeof (AnnotatedMethodParameters).GetMethod (nameof (MethodWithSingleAnnotatedParameter)).Invoke (null, null); + } + + // Should not warn, there's nothing wrong about this + [AttributeWithConstructorWithAnnotation(typeof (TestType))] + static void AnnotatedAttributeConstructor () + { + } + + [ExpectedWarning ("IL2111", nameof (MethodWithSingleAnnotatedParameter))] + [DynamicDependency (DynamicallyAccessedMemberTypes.PublicMethods, typeof (AnnotatedMethodParameters))] + static void DynamicDependency () + { + } + + [ExpectedWarning ("IL2111", nameof (MethodWithSingleAnnotatedParameter))] + [DynamicDependency (nameof (MethodWithSingleAnnotatedParameter), typeof (AnnotatedMethodParameters))] + static void DynamicDependencyByName () + { + } + + [ExpectedWarning ("IL2111", nameof (MethodWithSingleAnnotatedParameter))] + static void DynamicallyAccessedMembers () + { + typeof (AnnotatedMethodParameters).RequiresPublicMethods (); + } + + [ExpectedWarning ("IL2111", nameof (MethodWithSingleAnnotatedParameter))] + static void Ldftn () + { + var _ = new Action (AnnotatedMethodParameters.MethodWithSingleAnnotatedParameter); + } + + interface IWithAnnotatedMethod + { + public void AnnotatedMethod ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors)] Type type); + } + + [ExpectedWarning ("IL2111", nameof (IWithAnnotatedMethod.AnnotatedMethod))] + static void Ldvirtftn () + { + IWithAnnotatedMethod instance = null; + var _ = new Action (instance.AnnotatedMethod); + } + + public static void Test () + { + Reflection (); + DynamicDependency (); + DynamicDependencyByName (); + DynamicallyAccessedMembers (); + Ldftn (); + Ldvirtftn (); + } + } + + class AnnotatedMethodReturnValue + { + [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] + public static Type MethodWithAnnotatedReturnValue () => null; + + [ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))] + static void Reflection () + { + typeof (AnnotatedMethodReturnValue).GetMethod (nameof (MethodWithAnnotatedReturnValue)).Invoke (null, null); + } + + [ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))] + [DynamicDependency (DynamicallyAccessedMemberTypes.PublicMethods, typeof (AnnotatedMethodReturnValue))] + static void DynamicDependency () + { + } + + [ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))] + [DynamicDependency (nameof (MethodWithAnnotatedReturnValue), typeof (AnnotatedMethodReturnValue))] + static void DynamicDependencyByName () + { + } + + [ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))] + static void DynamicallyAccessedMembers () + { + typeof (AnnotatedMethodReturnValue).RequiresPublicMethods (); + } + + [ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))] + static void Ldftn () + { + var _ = new Func (AnnotatedMethodReturnValue.MethodWithAnnotatedReturnValue); + } + + public static void Test () + { + Reflection (); + DynamicDependency (); + DynamicDependencyByName (); + DynamicallyAccessedMembers (); + Ldftn (); + } + } + + class AnnotatedProperty + { + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicNestedTypes)] + public static Type PropertyWithAnnotation { get; set; } + + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicEvents)] + public static Type PropertyWithAnnotationGetterOnly { get => null; } + + class AttributeWithPropertyWithAnnotation : Attribute + { + public AttributeWithPropertyWithAnnotation () { } + + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)] + public Type PropertyWithAnnotation { get; set; } + } + + [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation))] + static void ReflectionOnPropertyItself () + { + // TODO: Technically this should warn if one of the setter is annotated since + // linker can't guarantee that it will not be used. + typeof (AnnotatedProperty).GetProperty (nameof (PropertyWithAnnotation)); + } + + [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotationGetterOnly))] + static void ReflectionOnPropertyWithGetterOnly () + { + // Following the rules we maintain on normal methods, just returning annotated value is considered dangerous + // (in theory one could use type builder to create an override for the method, and its body would not be validated + // and would need to fulfill the annotation on the return value anyway) + typeof (AnnotatedProperty).GetProperty (nameof (PropertyWithAnnotationGetterOnly)); + } + + [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".get")] + static void ReflectionOnGetter () + { + typeof (AnnotatedProperty).GetMethod ("get_" + nameof (PropertyWithAnnotation)); + } + + [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".set")] + static void ReflectionOnSetter () + { + typeof (AnnotatedProperty).GetMethod ("set_" + nameof (PropertyWithAnnotation)); + } + + // Should not warn - there's nothing wrong with this + [AttributeWithPropertyWithAnnotation (PropertyWithAnnotation = typeof (TestType))] + static void AnnotatedAttributeProperty () + { + } + + [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".get")] + [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".set")] + [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotationGetterOnly) + ".get")] + [DynamicDependency (DynamicallyAccessedMemberTypes.PublicProperties, typeof (AnnotatedProperty))] + static void DynamicDependency () + { + } + + [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".get")] + [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".set")] + [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotationGetterOnly) + ".get")] + static void DynamicallyAccessedMembers () + { + typeof (AnnotatedProperty).RequiresPublicProperties (); + } + + public static void Test () + { + ReflectionOnPropertyItself (); + ReflectionOnPropertyWithGetterOnly (); + ReflectionOnGetter (); + ReflectionOnSetter (); + AnnotatedAttributeProperty (); + DynamicDependency (); + DynamicallyAccessedMembers (); + } + } + + // Annotation on generic parameter + class AnnotatedGenerics + { + public static void GenericWithAnnotation< + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.Interfaces)] T> () + { } + + static void ReflectionOnly() + { + // Should not warn - there's nothing wrong with asking for MethodInfo alone + typeof (AnnotatedGenerics).GetMethod (nameof (GenericWithAnnotation)); + } + + // Similarly to direct reflection - no warning expected + [DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(AnnotatedGenerics))] + static void DynamicDependency () + { + } + + // Similarly to direct reflection - no warning expected + static void DynamicallyAccessedMembers() + { + typeof (AnnotatedGenerics).RequiresPublicMethods (); + } + + // This should produce IL2071 https://github.com/mono/linker/issues/2144 + [ExpectedWarning ("IL2070", "MakeGenericMethod")] + static void InstantiateGeneric(Type type = null) + { + // This should warn due to MakeGenericMethod - in this case the generic parameter is unannotated type + typeof (AnnotatedGenerics).GetMethod (nameof (GenericWithAnnotation)).MakeGenericMethod (type); + } + + public static void Test () + { + ReflectionOnly (); + DynamicDependency (); + DynamicallyAccessedMembers (); + InstantiateGeneric (); + } + } + + // Annotation on non-generic parameter but on generic methods + class AnnotationOnGenerics + { + class GenericWithAnnotatedMethod + { + public static void AnnotatedMethod ( + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type type) { } + } + + public static void GenericMethodWithAnnotation( + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type type) {} + + [ExpectedWarning ("IL2111", nameof (GenericWithAnnotatedMethod.AnnotatedMethod))] + public static void GenericTypeWithStaticMethodViaLdftn () + { + var _ = new Action (GenericWithAnnotatedMethod.AnnotatedMethod); + } + + [ExpectedWarning ("IL2111", nameof (GenericMethodWithAnnotation))] + public static void GenericMethodWithAnnotationReflection () + { + typeof (AnnotationOnGenerics).GetMethod (nameof (GenericMethodWithAnnotation)); + } + + public static void GenericMethodWithAnnotationDirectCall () + { + // Should not warn, nothing wrong about this + GenericMethodWithAnnotation (typeof (TestType)); + } + + [ExpectedWarning ("IL2111", nameof(GenericMethodWithAnnotation))] + public static void GenericMethodWithAnnotationViaLdftn () + { + var _ = new Action (GenericMethodWithAnnotation); + } + + [ExpectedWarning ("IL2111", nameof (GenericMethodWithAnnotation))] + public static void GenericMethodDynamicallyAccessedMembers () + { + typeof (AnnotationOnGenerics).RequiresPublicMethods (); + } + + public static void Test () + { + GenericTypeWithStaticMethodViaLdftn (); + GenericMethodWithAnnotationReflection (); + GenericMethodWithAnnotationDirectCall (); + GenericMethodWithAnnotationViaLdftn (); + GenericMethodDynamicallyAccessedMembers (); + } + } + + class AnnotationOnInteropMethod + { + struct ValueWithAnnotatedField + { + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] + public Type _typeField; + } + + [ExpectedWarning ("IL2110", nameof (ValueWithAnnotatedField._typeField))] + [DllImport ("nonexistent")] + static extern ValueWithAnnotatedField GetValueWithAnnotatedField (); + + [ExpectedWarning ("IL2110", nameof (ValueWithAnnotatedField._typeField))] + [DllImport ("nonexistent")] + static extern void AcceptValueWithAnnotatedField (ValueWithAnnotatedField value); + + public static void Test () + { + GetValueWithAnnotatedField (); + AcceptValueWithAnnotatedField (default (ValueWithAnnotatedField)); + } + } + + class AccessThroughLdToken + { + static Type PropertyWithLdToken { + [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] + get { + return null; + } + } + + [ExpectedWarning ("IL2111", nameof (PropertyWithLdToken))] + public static void Test () + { + Expression> getter = () => PropertyWithLdToken; + } + } + + class TestType { } + } +} diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/VirtualMethodHierarchyDataflowAnnotationValidation.cs b/test/Mono.Linker.Tests.Cases/DataFlow/VirtualMethodHierarchyDataflowAnnotationValidation.cs index c21c32b1d29b..fbaa7453519a 100644 --- a/test/Mono.Linker.Tests.Cases/DataFlow/VirtualMethodHierarchyDataflowAnnotationValidation.cs +++ b/test/Mono.Linker.Tests.Cases/DataFlow/VirtualMethodHierarchyDataflowAnnotationValidation.cs @@ -13,6 +13,9 @@ namespace Mono.Linker.Tests.Cases.DataFlow { [SkipKeptItemsValidation] [SandboxDependency ("Dependencies/TestSystemTypeBase.cs")] + + // Suppress warnings about accessing methods with annotations via reflection - the test below does that a LOT + [UnconditionalSuppressMessage("test", "IL2111")] class VirtualMethodHierarchyDataflowAnnotationValidation { public static void Main () diff --git a/test/Mono.Linker.Tests.Cases/Reflection/ExpressionCallString.cs b/test/Mono.Linker.Tests.Cases/Reflection/ExpressionCallString.cs index 142e82dd5720..2c20f3c9caed 100644 --- a/test/Mono.Linker.Tests.Cases/Reflection/ExpressionCallString.cs +++ b/test/Mono.Linker.Tests.Cases/Reflection/ExpressionCallString.cs @@ -117,21 +117,6 @@ static string GetUnknownString () [Kept] class TestUnknownType { - [Kept] - public static void PublicMethod () - { - } - - [Kept] - protected static void ProtectedMethod () - { - } - - [Kept] - private static void PrivateMethod () - { - } - [Kept] [UnrecognizedReflectionAccessPattern (typeof (Expression), nameof (Expression.Call), new Type[] { typeof (Type), typeof (string), typeof (Type[]), typeof (Expression[]) }, messageCode: "IL2072")] @@ -148,13 +133,13 @@ public static void Test () [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods)] static Type GetUnknownType () { - return typeof (TestUnknownType); + return typeof (TestType); } [Kept] static Type TriggerUnrecognizedPattern () { - return typeof (TestUnknownType); + return typeof (TestType); } } @@ -228,13 +213,13 @@ public static void GenericMethodWithNoRequirements () { } [Kept] public static void GenericMethodWithRequirements< [KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))] - [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)] T> () + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)] T> () { } [Kept] public static void GenericMethodWithRequirementsNoArguments< [KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))] - [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)] T> () + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)] T> () { } [Kept] @@ -355,5 +340,8 @@ public static void Test () [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)] static Type GetUnknownTypeWithRequrements () { return null; } } + + [Kept] + class TestType { } } } From 4ddd3edf0cc6ed3dd27417174cf84ebbe6a720d7 Mon Sep 17 00:00:00 2001 From: vitek-karas Date: Wed, 14 Jul 2021 07:16:13 -0700 Subject: [PATCH 2/6] Formatting --- src/linker/Linker.Steps/MarkStep.cs | 2 +- .../AnnotatedMembersAccessedViaReflection.cs | 23 +++++++++++-------- ...odHierarchyDataflowAnnotationValidation.cs | 3 ++- .../Reflection/ExpressionCallString.cs | 4 ++-- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index d1026003173d..59a1cb4171b2 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -1571,7 +1571,7 @@ void MarkField (FieldDefinition field, in DependencyInfo reason) break; } - + if (CheckProcessed (field)) return; diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs b/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs index f54e2cd88fb3..3abe3d56c12d 100644 --- a/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs +++ b/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs @@ -85,7 +85,8 @@ public static void MethodWithSingleAnnotatedParameter ( class AttributeWithConstructorWithAnnotation : Attribute { public AttributeWithConstructorWithAnnotation ( - [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type type) { } + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type type) + { } } [ExpectedWarning ("IL2111", nameof (MethodWithSingleAnnotatedParameter))] @@ -95,7 +96,7 @@ static void Reflection () } // Should not warn, there's nothing wrong about this - [AttributeWithConstructorWithAnnotation(typeof (TestType))] + [AttributeWithConstructorWithAnnotation (typeof (TestType))] static void AnnotatedAttributeConstructor () { } @@ -278,27 +279,27 @@ public static void GenericWithAnnotation< [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.Interfaces)] T> () { } - static void ReflectionOnly() + static void ReflectionOnly () { // Should not warn - there's nothing wrong with asking for MethodInfo alone typeof (AnnotatedGenerics).GetMethod (nameof (GenericWithAnnotation)); } // Similarly to direct reflection - no warning expected - [DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(AnnotatedGenerics))] + [DynamicDependency (DynamicallyAccessedMemberTypes.PublicMethods, typeof (AnnotatedGenerics))] static void DynamicDependency () { } // Similarly to direct reflection - no warning expected - static void DynamicallyAccessedMembers() + static void DynamicallyAccessedMembers () { typeof (AnnotatedGenerics).RequiresPublicMethods (); } // This should produce IL2071 https://github.com/mono/linker/issues/2144 [ExpectedWarning ("IL2070", "MakeGenericMethod")] - static void InstantiateGeneric(Type type = null) + static void InstantiateGeneric (Type type = null) { // This should warn due to MakeGenericMethod - in this case the generic parameter is unannotated type typeof (AnnotatedGenerics).GetMethod (nameof (GenericWithAnnotation)).MakeGenericMethod (type); @@ -319,11 +320,13 @@ class AnnotationOnGenerics class GenericWithAnnotatedMethod { public static void AnnotatedMethod ( - [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type type) { } + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type type) + { } } - public static void GenericMethodWithAnnotation( - [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type type) {} + public static void GenericMethodWithAnnotation ( + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type type) + { } [ExpectedWarning ("IL2111", nameof (GenericWithAnnotatedMethod.AnnotatedMethod))] public static void GenericTypeWithStaticMethodViaLdftn () @@ -343,7 +346,7 @@ public static void GenericMethodWithAnnotationDirectCall () GenericMethodWithAnnotation (typeof (TestType)); } - [ExpectedWarning ("IL2111", nameof(GenericMethodWithAnnotation))] + [ExpectedWarning ("IL2111", nameof (GenericMethodWithAnnotation))] public static void GenericMethodWithAnnotationViaLdftn () { var _ = new Action (GenericMethodWithAnnotation); diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/VirtualMethodHierarchyDataflowAnnotationValidation.cs b/test/Mono.Linker.Tests.Cases/DataFlow/VirtualMethodHierarchyDataflowAnnotationValidation.cs index fbaa7453519a..25107c005ee2 100644 --- a/test/Mono.Linker.Tests.Cases/DataFlow/VirtualMethodHierarchyDataflowAnnotationValidation.cs +++ b/test/Mono.Linker.Tests.Cases/DataFlow/VirtualMethodHierarchyDataflowAnnotationValidation.cs @@ -15,7 +15,8 @@ namespace Mono.Linker.Tests.Cases.DataFlow [SandboxDependency ("Dependencies/TestSystemTypeBase.cs")] // Suppress warnings about accessing methods with annotations via reflection - the test below does that a LOT - [UnconditionalSuppressMessage("test", "IL2111")] + // (The test accessed these methods through DynamicallyAccessedMembers annotations which is effectively the same reflection access) + [UnconditionalSuppressMessage ("test", "IL2111")] class VirtualMethodHierarchyDataflowAnnotationValidation { public static void Main () diff --git a/test/Mono.Linker.Tests.Cases/Reflection/ExpressionCallString.cs b/test/Mono.Linker.Tests.Cases/Reflection/ExpressionCallString.cs index 2c20f3c9caed..b4b71d5ebfbf 100644 --- a/test/Mono.Linker.Tests.Cases/Reflection/ExpressionCallString.cs +++ b/test/Mono.Linker.Tests.Cases/Reflection/ExpressionCallString.cs @@ -213,13 +213,13 @@ public static void GenericMethodWithNoRequirements () { } [Kept] public static void GenericMethodWithRequirements< [KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))] - [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)] T> () + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)] T> () { } [Kept] public static void GenericMethodWithRequirementsNoArguments< [KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))] - [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)] T> () + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)] T> () { } [Kept] From e640a9ef40eb32db77bc8e93b99aad1e12cd1558 Mon Sep 17 00:00:00 2001 From: vitek-karas Date: Thu, 15 Jul 2021 10:55:36 -0700 Subject: [PATCH 3/6] Only warn on virtual methods with annotate return value Annotated return value is mostly not problematic (the caller only "Reads" from it, which is always safe). The only case where it's problematic is if something would override the method at runtime (ref emit) in which case the trimmer can't validate the new code that it fulfills the return value requirements. But that can only happen if the method is virtual. --- src/linker/Linker.Dataflow/FlowAnnotations.cs | 6 +- src/linker/Linker.Steps/MarkStep.cs | 8 ++ .../AnnotatedMembersAccessedViaReflection.cs | 109 +++++++++++++----- 3 files changed, 94 insertions(+), 29 deletions(-) diff --git a/src/linker/Linker.Dataflow/FlowAnnotations.cs b/src/linker/Linker.Dataflow/FlowAnnotations.cs index 0163425eb91b..64c1547259ae 100644 --- a/src/linker/Linker.Dataflow/FlowAnnotations.cs +++ b/src/linker/Linker.Dataflow/FlowAnnotations.cs @@ -75,7 +75,11 @@ public DynamicallyAccessedMemberTypes GetGenericParameterAnnotation (GenericPara public bool ShouldWarnWhenAccessedForReflection (MethodDefinition method) => GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) && - (annotation.ParameterAnnotations != null || annotation.ReturnParameterAnnotation != DynamicallyAccessedMemberTypes.None); + (annotation.ParameterAnnotations != null || annotation.ReturnParameterAnnotation != DynamicallyAccessedMemberTypes.None); + + public bool MethodHasNoAnnotatedParameters (MethodDefinition method) => + GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) && + annotation.ParameterAnnotations == null; public bool ShouldWarnWhenAccessedForReflection (FieldDefinition field) => GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out _); diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 59a1cb4171b2..b99944d658f9 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -2820,6 +2820,14 @@ void ProcessAnalysisAnnotationsForMethod (MethodDefinition method, DependencyKin break; } + // If the method only has annotation on the return value and it's not virtual avoid warning. + // Return value annotations are "consumed" by the caller of a method, and as such there is nothing + // wrong calling these dynamically. The only problem can happen if something overrides a virtual + // method with annotated return value at runtime - in this case the trimmer can't validate + // that the method will return only types which fulfill the annotation's requirements. + if (!method.IsVirtual && _context.Annotations.FlowAnnotations.MethodHasNoAnnotatedParameters (method)) + return; + _context.LogWarning ( $"Method '{method.GetDisplayName ()}' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field.", 2111, diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs b/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs index 3abe3d56c12d..d8fbce480cd6 100644 --- a/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs +++ b/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs @@ -151,45 +151,90 @@ public static void Test () class AnnotatedMethodReturnValue { [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] - public static Type MethodWithAnnotatedReturnValue () => null; + public static Type StaticMethodWithAnnotatedReturnValue () => null; - [ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))] - static void Reflection () + [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] + public Type InstanceMethodWithAnnotatedReturnValue () => null; + + [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] + public virtual Type VirtualMethodWithAnnotatedReturnValue () => null; + + // Only virtual methods should warn - the problem is only possible if something overrides a virtual method. + // Getting an annotated value in itself is not dangerous in any way. + + static void ReflectionOnStatic () { - typeof (AnnotatedMethodReturnValue).GetMethod (nameof (MethodWithAnnotatedReturnValue)).Invoke (null, null); + typeof (AnnotatedMethodReturnValue).GetMethod (nameof (StaticMethodWithAnnotatedReturnValue)).Invoke (null, null); } - [ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))] + static void ReflectionOnInstance () + { + typeof (AnnotatedMethodReturnValue).GetMethod (nameof (InstanceMethodWithAnnotatedReturnValue)).Invoke (null, null); + } + + [ExpectedWarning ("IL2111", nameof (VirtualMethodWithAnnotatedReturnValue))] + static void ReflectionOnVirtual () + { + typeof (AnnotatedMethodReturnValue).GetMethod (nameof (VirtualMethodWithAnnotatedReturnValue)).Invoke (null, null); + } + + [ExpectedWarning ("IL2111", nameof (VirtualMethodWithAnnotatedReturnValue))] [DynamicDependency (DynamicallyAccessedMemberTypes.PublicMethods, typeof (AnnotatedMethodReturnValue))] static void DynamicDependency () { } - [ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))] - [DynamicDependency (nameof (MethodWithAnnotatedReturnValue), typeof (AnnotatedMethodReturnValue))] - static void DynamicDependencyByName () + [DynamicDependency (nameof (StaticMethodWithAnnotatedReturnValue), typeof (AnnotatedMethodReturnValue))] + static void DynamicDependencyByNameOnStatic () + { + } + + [DynamicDependency (nameof (InstanceMethodWithAnnotatedReturnValue), typeof (AnnotatedMethodReturnValue))] + static void DynamicDependencyByNameOnInstance () + { + } + + [ExpectedWarning ("IL2111", nameof (VirtualMethodWithAnnotatedReturnValue))] + [DynamicDependency (nameof (VirtualMethodWithAnnotatedReturnValue), typeof (AnnotatedMethodReturnValue))] + static void DynamicDependencyByNameOnVirtual () { } - [ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))] + [ExpectedWarning ("IL2111", nameof (VirtualMethodWithAnnotatedReturnValue))] static void DynamicallyAccessedMembers () { typeof (AnnotatedMethodReturnValue).RequiresPublicMethods (); } - [ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))] - static void Ldftn () + static void LdftnOnStatic () { - var _ = new Func (AnnotatedMethodReturnValue.MethodWithAnnotatedReturnValue); + var _ = new Func (AnnotatedMethodReturnValue.StaticMethodWithAnnotatedReturnValue); + } + + static void LdftnOnInstance () + { + var _ = new Func ((new AnnotatedMethodReturnValue ()).InstanceMethodWithAnnotatedReturnValue); + } + + [ExpectedWarning ("IL2111", nameof (VirtualMethodWithAnnotatedReturnValue))] + static void LdftnOnVirtual () + { + var _ = new Func ((new AnnotatedMethodReturnValue ()).VirtualMethodWithAnnotatedReturnValue); } public static void Test () { - Reflection (); + ReflectionOnStatic (); + ReflectionOnInstance (); + ReflectionOnVirtual (); DynamicDependency (); - DynamicDependencyByName (); + DynamicDependencyByNameOnStatic (); + DynamicDependencyByNameOnInstance (); + DynamicDependencyByNameOnVirtual (); DynamicallyAccessedMembers (); - Ldftn (); + LdftnOnStatic (); + LdftnOnInstance (); + LdftnOnVirtual (); } } @@ -201,6 +246,9 @@ class AnnotatedProperty [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicEvents)] public static Type PropertyWithAnnotationGetterOnly { get => null; } + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicEvents)] + public virtual Type VirtualPropertyWithAnnotationGetterOnly { get => null; } + class AttributeWithPropertyWithAnnotation : Attribute { public AttributeWithPropertyWithAnnotation () { } @@ -212,21 +260,20 @@ public AttributeWithPropertyWithAnnotation () { } [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation))] static void ReflectionOnPropertyItself () { - // TODO: Technically this should warn if one of the setter is annotated since - // linker can't guarantee that it will not be used. typeof (AnnotatedProperty).GetProperty (nameof (PropertyWithAnnotation)); } - [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotationGetterOnly))] static void ReflectionOnPropertyWithGetterOnly () { - // Following the rules we maintain on normal methods, just returning annotated value is considered dangerous - // (in theory one could use type builder to create an override for the method, and its body would not be validated - // and would need to fulfill the annotation on the return value anyway) typeof (AnnotatedProperty).GetProperty (nameof (PropertyWithAnnotationGetterOnly)); } - [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".get")] + [ExpectedWarning ("IL2111", nameof (VirtualPropertyWithAnnotationGetterOnly))] + static void ReflectionOnPropertyWithGetterOnlyOnVirtual () + { + typeof (AnnotatedProperty).GetProperty (nameof (VirtualPropertyWithAnnotationGetterOnly)); + } + static void ReflectionOnGetter () { typeof (AnnotatedProperty).GetMethod ("get_" + nameof (PropertyWithAnnotation)); @@ -238,23 +285,27 @@ static void ReflectionOnSetter () typeof (AnnotatedProperty).GetMethod ("set_" + nameof (PropertyWithAnnotation)); } + [ExpectedWarning ("IL2111", nameof (VirtualPropertyWithAnnotationGetterOnly) + ".get")] + static void ReflectionOnVirtualGetter () + { + typeof (AnnotatedProperty).GetMethod ("get_" + nameof (VirtualPropertyWithAnnotationGetterOnly)); + } + // Should not warn - there's nothing wrong with this [AttributeWithPropertyWithAnnotation (PropertyWithAnnotation = typeof (TestType))] static void AnnotatedAttributeProperty () { } - [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".get")] [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".set")] - [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotationGetterOnly) + ".get")] + [ExpectedWarning ("IL2111", nameof (VirtualPropertyWithAnnotationGetterOnly) + ".get")] [DynamicDependency (DynamicallyAccessedMemberTypes.PublicProperties, typeof (AnnotatedProperty))] static void DynamicDependency () { } - [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".get")] [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".set")] - [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotationGetterOnly) + ".get")] + [ExpectedWarning ("IL2111", nameof (VirtualPropertyWithAnnotationGetterOnly) + ".get")] static void DynamicallyAccessedMembers () { typeof (AnnotatedProperty).RequiresPublicProperties (); @@ -264,8 +315,10 @@ public static void Test () { ReflectionOnPropertyItself (); ReflectionOnPropertyWithGetterOnly (); + ReflectionOnPropertyWithGetterOnlyOnVirtual (); ReflectionOnGetter (); ReflectionOnSetter (); + ReflectionOnVirtualGetter (); AnnotatedAttributeProperty (); DynamicDependency (); DynamicallyAccessedMembers (); @@ -393,7 +446,7 @@ public static void Test () class AccessThroughLdToken { - static Type PropertyWithLdToken { + public virtual Type PropertyWithLdToken { [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] get { return null; @@ -403,7 +456,7 @@ static Type PropertyWithLdToken { [ExpectedWarning ("IL2111", nameof (PropertyWithLdToken))] public static void Test () { - Expression> getter = () => PropertyWithLdToken; + Expression> getter = () => (new AccessThroughLdToken ()).PropertyWithLdToken; } } From ac215d3884b9db6b4058b4e78dd581cb64e97d3a Mon Sep 17 00:00:00 2001 From: vitek-karas Date: Thu, 22 Jul 2021 05:37:05 -0700 Subject: [PATCH 4/6] PR feedback --- src/linker/Linker.Dataflow/FlowAnnotations.cs | 38 +++++--- src/linker/Linker.Steps/MarkStep.cs | 7 +- .../AnnotatedMembersAccessedViaReflection.cs | 88 +++++++++++++++++++ 3 files changed, 118 insertions(+), 15 deletions(-) diff --git a/src/linker/Linker.Dataflow/FlowAnnotations.cs b/src/linker/Linker.Dataflow/FlowAnnotations.cs index 64c1547259ae..dc8b5129100d 100644 --- a/src/linker/Linker.Dataflow/FlowAnnotations.cs +++ b/src/linker/Linker.Dataflow/FlowAnnotations.cs @@ -37,20 +37,30 @@ public bool RequiresDataFlowAnalysis (GenericParameter genericParameter) => /// /// Parameter index in the IL sense. Parameter 0 on instance methods is `this`. /// - public DynamicallyAccessedMemberTypes GetParameterAnnotation (MethodDefinition method, int parameterIndex) => - (GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) && annotation.ParameterAnnotations != null) - ? annotation.ParameterAnnotations[parameterIndex] - : DynamicallyAccessedMemberTypes.None; - - public DynamicallyAccessedMemberTypes GetReturnParameterAnnotation (MethodDefinition method) => - GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) - ? annotation.ReturnParameterAnnotation - : DynamicallyAccessedMemberTypes.None; - - public DynamicallyAccessedMemberTypes GetFieldAnnotation (FieldDefinition field) => - GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out var annotation) - ? annotation.Annotation - : DynamicallyAccessedMemberTypes.None; + public DynamicallyAccessedMemberTypes GetParameterAnnotation (MethodDefinition method, int parameterIndex) + { + if (GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) && + annotation.ParameterAnnotations != null) + return annotation.ParameterAnnotations[parameterIndex]; + + return DynamicallyAccessedMemberTypes.None; + } + + public DynamicallyAccessedMemberTypes GetReturnParameterAnnotation (MethodDefinition method) + { + if (GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation)) + return annotation.ReturnParameterAnnotation; + + return DynamicallyAccessedMemberTypes.None; + } + + public DynamicallyAccessedMemberTypes GetFieldAnnotation (FieldDefinition field) + { + if (GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out var annotation)) + return annotation.Annotation; + + return DynamicallyAccessedMemberTypes.None; + } public DynamicallyAccessedMemberTypes GetTypeAnnotation (TypeDefinition type) => GetAnnotations (type).TypeAnnotation; diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index b99944d658f9..45194635389e 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -1562,7 +1562,8 @@ void MarkField (FieldDefinition field, in DependencyInfo reason) case DependencyKind.DynamicDependency: case DependencyKind.DynamicallyAccessedMember: case DependencyKind.InteropMethodDependency: - if (_context.Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (field)) + if (_context.Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (field) && + !ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode ()) _context.LogWarning ( $"Field '{field.GetDisplayName ()}' with 'DynamicallyAccessedMembersAttribute' is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field.", 2110, @@ -2836,6 +2837,10 @@ void ProcessAnalysisAnnotationsForMethod (MethodDefinition method, DependencyKin } } + Type _typeField; + + private ref Type GetRefType () { Type type = null; return ref _typeField; } + internal bool ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode () { // Check if the current scope method has RequiresUnreferencedCode on it diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs b/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs index d8fbce480cd6..f94b9a45867e 100644 --- a/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs +++ b/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs @@ -42,6 +42,12 @@ static void Reflection () typeof (AnnotatedField).GetField ("_annotatedField").SetValue (null, typeof (TestType)); } + [RequiresUnreferencedCode ("test")] + static void ReflectionSuppressedByRUC () + { + typeof (AnnotatedField).GetField ("_annotatedField").SetValue (null, typeof (TestType)); + } + [ExpectedWarning ("IL2110", nameof (_annotatedField))] static void ReflectionReadOnly () { @@ -54,6 +60,12 @@ static void DynamicDependency () { } + [RequiresUnreferencedCode ("test")] + [DynamicDependency (DynamicallyAccessedMemberTypes.PublicFields, typeof (AnnotatedField))] + static void DynamicDependencySuppressedByRUC () + { + } + [ExpectedWarning ("IL2110", nameof (_annotatedField))] [DynamicDependency (nameof (_annotatedField), typeof (AnnotatedField))] static void DynamicDependencyByName () @@ -66,13 +78,23 @@ static void DynamicallyAccessedMembers () typeof (AnnotatedField).RequiresPublicFields (); } + [RequiresUnreferencedCode ("test")] + static void DynamicallyAccessedMembersSuppressedByRUC () + { + typeof (AnnotatedField).RequiresPublicFields (); + } + + [UnconditionalSuppressMessage ("test", "IL2026")] public static void Test () { Reflection (); + ReflectionSuppressedByRUC (); ReflectionReadOnly (); DynamicDependency (); + DynamicDependencySuppressedByRUC (); DynamicDependencyByName (); DynamicallyAccessedMembers (); + DynamicallyAccessedMembersSuppressedByRUC (); } } @@ -95,6 +117,12 @@ static void Reflection () typeof (AnnotatedMethodParameters).GetMethod (nameof (MethodWithSingleAnnotatedParameter)).Invoke (null, null); } + [RequiresUnreferencedCode ("test")] + static void ReflectionSuppressedByRUC () + { + typeof (AnnotatedMethodParameters).GetMethod (nameof (MethodWithSingleAnnotatedParameter)).Invoke (null, null); + } + // Should not warn, there's nothing wrong about this [AttributeWithConstructorWithAnnotation (typeof (TestType))] static void AnnotatedAttributeConstructor () @@ -107,6 +135,12 @@ static void DynamicDependency () { } + [RequiresUnreferencedCode ("test")] + [DynamicDependency (DynamicallyAccessedMemberTypes.PublicMethods, typeof (AnnotatedMethodParameters))] + static void DynamicDependencySuppressedByRUC () + { + } + [ExpectedWarning ("IL2111", nameof (MethodWithSingleAnnotatedParameter))] [DynamicDependency (nameof (MethodWithSingleAnnotatedParameter), typeof (AnnotatedMethodParameters))] static void DynamicDependencyByName () @@ -119,6 +153,12 @@ static void DynamicallyAccessedMembers () typeof (AnnotatedMethodParameters).RequiresPublicMethods (); } + [RequiresUnreferencedCode ("test")] + static void DynamicallyAccessedMembersSuppressedByRUC () + { + typeof (AnnotatedMethodParameters).RequiresPublicMethods (); + } + [ExpectedWarning ("IL2111", nameof (MethodWithSingleAnnotatedParameter))] static void Ldftn () { @@ -137,12 +177,16 @@ static void Ldvirtftn () var _ = new Action (instance.AnnotatedMethod); } + [UnconditionalSuppressMessage ("test", "IL2026")] public static void Test () { Reflection (); + ReflectionSuppressedByRUC (); DynamicDependency (); + DynamicDependencySuppressedByRUC (); DynamicDependencyByName (); DynamicallyAccessedMembers (); + DynamicallyAccessedMembersSuppressedByRUC (); Ldftn (); Ldvirtftn (); } @@ -178,12 +222,24 @@ static void ReflectionOnVirtual () typeof (AnnotatedMethodReturnValue).GetMethod (nameof (VirtualMethodWithAnnotatedReturnValue)).Invoke (null, null); } + [RequiresUnreferencedCode ("test")] + static void ReflectionOnVirtualSuppressedByRUC () + { + typeof (AnnotatedMethodReturnValue).GetMethod (nameof (VirtualMethodWithAnnotatedReturnValue)).Invoke (null, null); + } + [ExpectedWarning ("IL2111", nameof (VirtualMethodWithAnnotatedReturnValue))] [DynamicDependency (DynamicallyAccessedMemberTypes.PublicMethods, typeof (AnnotatedMethodReturnValue))] static void DynamicDependency () { } + [RequiresUnreferencedCode ("test")] + [DynamicDependency (DynamicallyAccessedMemberTypes.PublicMethods, typeof (AnnotatedMethodReturnValue))] + static void DynamicDependencySuppressedByRUC () + { + } + [DynamicDependency (nameof (StaticMethodWithAnnotatedReturnValue), typeof (AnnotatedMethodReturnValue))] static void DynamicDependencyByNameOnStatic () { @@ -206,6 +262,12 @@ static void DynamicallyAccessedMembers () typeof (AnnotatedMethodReturnValue).RequiresPublicMethods (); } + [RequiresUnreferencedCode ("test")] + static void DynamicallyAccessedMembersSuppressedByRUC () + { + typeof (AnnotatedMethodReturnValue).RequiresPublicMethods (); + } + static void LdftnOnStatic () { var _ = new Func (AnnotatedMethodReturnValue.StaticMethodWithAnnotatedReturnValue); @@ -222,16 +284,20 @@ static void LdftnOnVirtual () var _ = new Func ((new AnnotatedMethodReturnValue ()).VirtualMethodWithAnnotatedReturnValue); } + [UnconditionalSuppressMessage ("test", "IL2026")] public static void Test () { ReflectionOnStatic (); ReflectionOnInstance (); ReflectionOnVirtual (); + ReflectionOnVirtualSuppressedByRUC (); DynamicDependency (); + DynamicDependencySuppressedByRUC (); DynamicDependencyByNameOnStatic (); DynamicDependencyByNameOnInstance (); DynamicDependencyByNameOnVirtual (); DynamicallyAccessedMembers (); + DynamicallyAccessedMembersSuppressedByRUC (); LdftnOnStatic (); LdftnOnInstance (); LdftnOnVirtual (); @@ -263,6 +329,12 @@ static void ReflectionOnPropertyItself () typeof (AnnotatedProperty).GetProperty (nameof (PropertyWithAnnotation)); } + [RequiresUnreferencedCode ("test")] + static void ReflectionOnPropertyItselfSuppressedByRUC () + { + typeof (AnnotatedProperty).GetProperty (nameof (PropertyWithAnnotation)); + } + static void ReflectionOnPropertyWithGetterOnly () { typeof (AnnotatedProperty).GetProperty (nameof (PropertyWithAnnotationGetterOnly)); @@ -304,6 +376,12 @@ static void DynamicDependency () { } + [RequiresUnreferencedCode ("test")] + [DynamicDependency (DynamicallyAccessedMemberTypes.PublicProperties, typeof (AnnotatedProperty))] + static void DynamicDependencySuppressedByRUC () + { + } + [ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".set")] [ExpectedWarning ("IL2111", nameof (VirtualPropertyWithAnnotationGetterOnly) + ".get")] static void DynamicallyAccessedMembers () @@ -311,9 +389,17 @@ static void DynamicallyAccessedMembers () typeof (AnnotatedProperty).RequiresPublicProperties (); } + [RequiresUnreferencedCode ("test")] + static void DynamicallyAccessedMembersSuppressedByRUC () + { + typeof (AnnotatedProperty).RequiresPublicProperties (); + } + + [UnconditionalSuppressMessage ("test", "IL2026")] public static void Test () { ReflectionOnPropertyItself (); + ReflectionOnPropertyItselfSuppressedByRUC (); ReflectionOnPropertyWithGetterOnly (); ReflectionOnPropertyWithGetterOnlyOnVirtual (); ReflectionOnGetter (); @@ -321,7 +407,9 @@ public static void Test () ReflectionOnVirtualGetter (); AnnotatedAttributeProperty (); DynamicDependency (); + DynamicDependencySuppressedByRUC (); DynamicallyAccessedMembers (); + DynamicallyAccessedMembersSuppressedByRUC (); } } From 5d180d56d08e699935c7ee3f411d705bb82a0e68 Mon Sep 17 00:00:00 2001 From: vitek-karas Date: Mon, 26 Jul 2021 05:34:56 -0700 Subject: [PATCH 5/6] PR feedback --- src/linker/Linker.Steps/MarkStep.cs | 34 +++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 45194635389e..8dc31e3d420d 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -2826,6 +2826,40 @@ void ProcessAnalysisAnnotationsForMethod (MethodDefinition method, DependencyKin // wrong calling these dynamically. The only problem can happen if something overrides a virtual // method with annotated return value at runtime - in this case the trimmer can't validate // that the method will return only types which fulfill the annotation's requirements. + // For example: + // class BaseWithAnnotation + // { + // [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields)] + // public abstract Type GetTypeWithFields(); + // } + // + // class UsingTheBase + // { + // public void PrintFields(Base base) + // { + // // No warning here - GetTypeWithFields is correctly annotated to allow GetFields on the return value. + // Console.WriteLine(string.Join(" ", base.GetTypeWithFields().GetFields().Select(f => f.Name))); + // } + // } + // + // If at runtime (through ref emit) something generates code like this: + // class DerivedAtRuntimeFromBase + // { + // // No point in adding annotation on the return value - nothing will look at it anyway + // // Linker will not see this code, so there are no checks + // public override Type GetTypeWithFields() { return typeof(TestType); } + // } + // + // If TestType from above is trimmed, it may note have all its fields, and there would be no warnings generated. + // But there has to be code like this somewhere in the app, in order to generate the override: + // class RuntimeTypeGenerator + // { + // public MethodInfo GetBaseMethod() + // { + // // This must warn - that the GetTypeWithFields has annotation on the return value + // return typeof(BaseWithAnnotation).GetMethod("GetTypeWithFields"); + // } + // } if (!method.IsVirtual && _context.Annotations.FlowAnnotations.MethodHasNoAnnotatedParameters (method)) return; From 23a62ac924475132a3b185c4d0f93c016f0e437e Mon Sep 17 00:00:00 2001 From: vitek-karas Date: Mon, 26 Jul 2021 05:50:12 -0700 Subject: [PATCH 6/6] Remove test code. --- src/linker/Linker.Steps/MarkStep.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 8dc31e3d420d..3b779d6bf2ba 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -2871,10 +2871,6 @@ void ProcessAnalysisAnnotationsForMethod (MethodDefinition method, DependencyKin } } - Type _typeField; - - private ref Type GetRefType () { Type type = null; return ref _typeField; } - internal bool ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode () { // Check if the current scope method has RequiresUnreferencedCode on it