From f3c72388851df0f86f74371874f8e0f71e8c7740 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Wed, 3 Jul 2024 23:12:55 +0000 Subject: [PATCH 1/9] Add test for existing behavior --- ...terfaceImplementedThroughBaseValidation.cs | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs new file mode 100644 index 0000000000000..ae2a3375f4225 --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs @@ -0,0 +1,103 @@ +// 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.CodeAnalysis; +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 +{ + [SkipKeptItemsValidation] + [ExpectedNoWarnings] + class InterfaceImplementedThroughBaseValidation + { + public static void Main () + { + RUCOnInterfaceMethod.Test (); + RUCOnBaseMethod.Test (); + DAMOnInterfaceMethod.Test (); + DAMOnBaseMethod.Test (); + } + + class RUCOnInterfaceMethod () + { + interface Interface { + [RequiresUnreferencedCode (nameof (Method))] + void Method (); + } + + class Base { + [UnexpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] + public void Method () {} + } + + class Derived : Base, Interface {} + + [ExpectedWarning ("IL2026")] + public static void Test () { + Interface i = new Derived (); + i.Method (); + } + } + + class RUCOnBaseMethod () + { + interface Interface { + void Method (); + } + + class Base { + [UnexpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] + [RequiresUnreferencedCode (nameof (Method))] + public void Method () {} + } + + class Derived : Base, Interface {} + + public static void Test () { + Interface i = new Derived (); + i.Method (); + } + } + + class DAMOnInterfaceMethod () + { + interface Interface { + void Method ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t); + } + + class Base { + [UnexpectedWarning ("IL2092", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] + public void Method (Type t) {} + } + + class Derived : Base, Interface {} + + public static void Test () { + Interface i = new Derived (); + i.Method (typeof (int)); + } + } + + class DAMOnBaseMethod () + { + interface Interface { + void Method (Type t); + } + + class Base { + [UnexpectedWarning ("IL2092", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] + public void Method ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t) {} + } + + class Derived : Base, Interface {} + + public static void Test () { + Interface i = new Derived (); + i.Method (typeof (int)); + } + } + } +} From 8a00fdef1da287cc481340f2e90a2a78f1ca5c56 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Thu, 11 Jul 2024 17:17:47 +0000 Subject: [PATCH 2/9] Fix ILLink warning behavior --- .../linker/Linker.Dataflow/FlowAnnotations.cs | 39 +++--- .../ValidateVirtualMethodAnnotationsStep.cs | 17 ++- ...terfaceImplementedThroughBaseValidation.cs | 112 ++++++++++++++++-- ...odHierarchyDataflowAnnotationValidation.cs | 10 +- 4 files changed, 142 insertions(+), 36 deletions(-) diff --git a/src/tools/illink/src/linker/Linker.Dataflow/FlowAnnotations.cs b/src/tools/illink/src/linker/Linker.Dataflow/FlowAnnotations.cs index 3b148cf3a1ac5..14617b0de9ca1 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/FlowAnnotations.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/FlowAnnotations.cs @@ -461,19 +461,21 @@ bool ScanMethodBodyForFieldAccess (MethodBody body, bool write, out FieldDefinit return true; } - internal void ValidateMethodAnnotationsAreSame (MethodDefinition method, MethodDefinition baseMethod) + internal void ValidateMethodAnnotationsAreSame (OverrideInformation ov) { + var method = ov.Override; + var baseMethod = ov.Base; GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var methodAnnotations); GetAnnotations (baseMethod.DeclaringType).TryGetAnnotation (baseMethod, out var baseMethodAnnotations); if (methodAnnotations.ReturnParameterAnnotation != baseMethodAnnotations.ReturnParameterAnnotation) - LogValidationWarning (method.MethodReturnType, baseMethod.MethodReturnType, method); + LogValidationWarning (method.MethodReturnType, baseMethod.MethodReturnType, ov); if (methodAnnotations.ParameterAnnotations != null || baseMethodAnnotations.ParameterAnnotations != null) { if (methodAnnotations.ParameterAnnotations == null) - ValidateMethodParametersHaveNoAnnotations (baseMethodAnnotations.ParameterAnnotations!, method, baseMethod, method); + ValidateMethodParametersHaveNoAnnotations (baseMethodAnnotations.ParameterAnnotations!, ov); else if (baseMethodAnnotations.ParameterAnnotations == null) - ValidateMethodParametersHaveNoAnnotations (methodAnnotations.ParameterAnnotations, method, baseMethod, method); + ValidateMethodParametersHaveNoAnnotations (methodAnnotations.ParameterAnnotations, ov); else { if (methodAnnotations.ParameterAnnotations.Length != baseMethodAnnotations.ParameterAnnotations.Length) return; @@ -483,16 +485,16 @@ internal void ValidateMethodAnnotationsAreSame (MethodDefinition method, MethodD LogValidationWarning ( method.TryGetParameter ((ParameterIndex) parameterIndex)?.GetCustomAttributeProvider ()!, baseMethod.TryGetParameter ((ParameterIndex) parameterIndex)?.GetCustomAttributeProvider ()!, - method); + ov); } } } if (methodAnnotations.GenericParameterAnnotations != null || baseMethodAnnotations.GenericParameterAnnotations != null) { if (methodAnnotations.GenericParameterAnnotations == null) - ValidateMethodGenericParametersHaveNoAnnotations (baseMethodAnnotations.GenericParameterAnnotations!, method, baseMethod, method); + ValidateMethodGenericParametersHaveNoAnnotations (baseMethodAnnotations.GenericParameterAnnotations!, ov); else if (baseMethodAnnotations.GenericParameterAnnotations == null) - ValidateMethodGenericParametersHaveNoAnnotations (methodAnnotations.GenericParameterAnnotations, method, baseMethod, method); + ValidateMethodGenericParametersHaveNoAnnotations (methodAnnotations.GenericParameterAnnotations, ov); else { if (methodAnnotations.GenericParameterAnnotations.Length != baseMethodAnnotations.GenericParameterAnnotations.Length) return; @@ -502,39 +504,42 @@ internal void ValidateMethodAnnotationsAreSame (MethodDefinition method, MethodD LogValidationWarning ( method.GenericParameters[genericParameterIndex], baseMethod.GenericParameters[genericParameterIndex], - method); + ov); } } } } } - void ValidateMethodParametersHaveNoAnnotations (DynamicallyAccessedMemberTypes[] parameterAnnotations, MethodDefinition method, MethodDefinition baseMethod, IMemberDefinition origin) + void ValidateMethodParametersHaveNoAnnotations (DynamicallyAccessedMemberTypes[] parameterAnnotations, OverrideInformation ov) { for (int parameterIndex = 0; parameterIndex < parameterAnnotations.Length; parameterIndex++) { var annotation = parameterAnnotations[parameterIndex]; if (annotation != DynamicallyAccessedMemberTypes.None) LogValidationWarning ( - method.GetParameter ((ParameterIndex) parameterIndex).GetCustomAttributeProvider ()!, - baseMethod.GetParameter ((ParameterIndex) parameterIndex).GetCustomAttributeProvider ()!, - origin); + ov.Override.GetParameter ((ParameterIndex) parameterIndex).GetCustomAttributeProvider ()!, + ov.Base.GetParameter ((ParameterIndex) parameterIndex).GetCustomAttributeProvider ()!, + ov); } } - void ValidateMethodGenericParametersHaveNoAnnotations (DynamicallyAccessedMemberTypes[] genericParameterAnnotations, MethodDefinition method, MethodDefinition baseMethod, IMemberDefinition origin) + void ValidateMethodGenericParametersHaveNoAnnotations (DynamicallyAccessedMemberTypes[] genericParameterAnnotations, OverrideInformation ov) { for (int genericParameterIndex = 0; genericParameterIndex < genericParameterAnnotations.Length; genericParameterIndex++) { if (genericParameterAnnotations[genericParameterIndex] != DynamicallyAccessedMemberTypes.None) { LogValidationWarning ( - method.GenericParameters[genericParameterIndex], - baseMethod.GenericParameters[genericParameterIndex], - origin); + ov.Override.GenericParameters[genericParameterIndex], + ov.Base.GenericParameters[genericParameterIndex], + ov); } } } - void LogValidationWarning (IMetadataTokenProvider provider, IMetadataTokenProvider baseProvider, IMemberDefinition origin) + void LogValidationWarning (IMetadataTokenProvider provider, IMetadataTokenProvider baseProvider, OverrideInformation ov) { + IMemberDefinition origin = (ov.IsOverrideOfInterfaceMember && ov.InterfaceImplementor.Implementor != ov.Override.DeclaringType) + ? ov.InterfaceImplementor.Implementor + : ov.Override; Debug.Assert (provider.GetType () == baseProvider.GetType ()); Debug.Assert (!(provider is GenericParameter genericParameter) || genericParameter.DeclaringMethod != null); switch (provider) { diff --git a/src/tools/illink/src/linker/Linker.Steps/ValidateVirtualMethodAnnotationsStep.cs b/src/tools/illink/src/linker/Linker.Steps/ValidateVirtualMethodAnnotationsStep.cs index ac4b07e6681b4..aa9c7ee0d9f09 100644 --- a/src/tools/illink/src/linker/Linker.Steps/ValidateVirtualMethodAnnotationsStep.cs +++ b/src/tools/illink/src/linker/Linker.Steps/ValidateVirtualMethodAnnotationsStep.cs @@ -16,8 +16,8 @@ protected override void Process () var baseOverrideInformations = annotations.GetBaseMethods (method); if (baseOverrideInformations != null) { foreach (var baseOv in baseOverrideInformations) { - annotations.FlowAnnotations.ValidateMethodAnnotationsAreSame (method, baseOv.Base); - ValidateMethodRequiresUnreferencedCodeAreSame (method, baseOv.Base); + annotations.FlowAnnotations.ValidateMethodAnnotationsAreSame (baseOv); + ValidateMethodRequiresUnreferencedCodeAreSame (baseOv); } } @@ -30,15 +30,17 @@ protected override void Process () if (annotations.VirtualMethodsWithAnnotationsToValidate.Contains (overrideInformation.Override)) continue; - annotations.FlowAnnotations.ValidateMethodAnnotationsAreSame (overrideInformation.Override, method); - ValidateMethodRequiresUnreferencedCodeAreSame (overrideInformation.Override, method); + annotations.FlowAnnotations.ValidateMethodAnnotationsAreSame (overrideInformation); + ValidateMethodRequiresUnreferencedCodeAreSame (overrideInformation); } } } } - void ValidateMethodRequiresUnreferencedCodeAreSame (MethodDefinition method, MethodDefinition baseMethod) + void ValidateMethodRequiresUnreferencedCodeAreSame (OverrideInformation ov) { + var method = ov.Override; + var baseMethod = ov.Base; var annotations = Context.Annotations; bool methodSatisfies = annotations.IsInRequiresUnreferencedCodeScope (method, out _); bool baseRequires = annotations.DoesMethodRequireUnreferencedCode (baseMethod, out _); @@ -49,7 +51,10 @@ void ValidateMethodRequiresUnreferencedCodeAreSame (MethodDefinition method, Met nameof (RequiresUnreferencedCodeAttribute), method.GetDisplayName (), baseMethod.GetDisplayName ()); - Context.LogWarning (method, DiagnosticId.RequiresUnreferencedCodeAttributeMismatch, message); + IMemberDefinition origin = (ov.IsOverrideOfInterfaceMember && ov.InterfaceImplementor.Implementor != method.DeclaringType) + ? ov.InterfaceImplementor.Implementor + : method; + Context.LogWarning (origin, DiagnosticId.RequiresUnreferencedCodeAttributeMismatch, message); } } } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs index ae2a3375f4225..2ab104cf60e5c 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs @@ -19,9 +19,14 @@ public static void Main () RUCOnBaseMethod.Test (); DAMOnInterfaceMethod.Test (); DAMOnBaseMethod.Test (); + + RUCOnInterfaceWithDIM.Test (); + RUCOnDIM.Test (); + DAMOnInterfaceWithDIM.Test (); + DAMOnDIM.Test (); } - class RUCOnInterfaceMethod () + class RUCOnInterfaceMethod { interface Interface { [RequiresUnreferencedCode (nameof (Method))] @@ -29,10 +34,10 @@ interface Interface { } class Base { - [UnexpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] public void Method () {} } + [ExpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] class Derived : Base, Interface {} [ExpectedWarning ("IL2026")] @@ -42,18 +47,18 @@ public static void Test () { } } - class RUCOnBaseMethod () + class RUCOnBaseMethod { interface Interface { void Method (); } class Base { - [UnexpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] [RequiresUnreferencedCode (nameof (Method))] public void Method () {} } + [ExpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] class Derived : Base, Interface {} public static void Test () { @@ -62,17 +67,17 @@ public static void Test () { } } - class DAMOnInterfaceMethod () + class DAMOnInterfaceMethod { interface Interface { void Method ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t); } class Base { - [UnexpectedWarning ("IL2092", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] public void Method (Type t) {} } + [ExpectedWarning ("IL2092", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] class Derived : Base, Interface {} public static void Test () { @@ -81,17 +86,17 @@ public static void Test () { } } - class DAMOnBaseMethod () + class DAMOnBaseMethod { interface Interface { void Method (Type t); } class Base { - [UnexpectedWarning ("IL2092", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] public void Method ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t) {} } + [ExpectedWarning ("IL2092", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] class Derived : Base, Interface {} public static void Test () { @@ -99,5 +104,96 @@ public static void Test () { i.Method (typeof (int)); } } + + class RUCOnInterfaceWithDIM + { + interface Interface { + [RequiresUnreferencedCode (nameof (Method))] + void Method (); + } + + interface InterfaceImpl : Interface { + [ExpectedWarning ("IL2046")] + void Interface.Method() {} + } + + class C : InterfaceImpl {} + + class D : InterfaceImpl {} + + [ExpectedWarning ("IL2026")] + public static void Test () { + Interface i = new C (); + i = new D (); + i.Method (); + } + } + + class RUCOnDIM + { + interface Interface { + void Method (); + } + + interface InterfaceImpl : Interface { + [ExpectedWarning ("IL2046")] + [RequiresUnreferencedCode (nameof (Method))] + void Interface.Method() {} + } + + class C : InterfaceImpl {} + + class D : InterfaceImpl {} + + public static void Test () { + Interface i = new C (); + i = new D (); + i.Method (); + } + } + + class DAMOnInterfaceWithDIM + { + interface Interface { + void Method ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t); + } + + interface InterfaceImpl : Interface { + [ExpectedWarning ("IL2092")] + void Interface.Method (Type t) {} + } + + class C : InterfaceImpl {} + + class D : InterfaceImpl {} + + public static void Test () { + Interface i = new C (); + i = new D (); + i.Method (typeof (int)); + } + } + + class DAMOnDIM + { + interface Interface { + void Method (Type t); + } + + interface InterfaceImpl : Interface { + [ExpectedWarning ("IL2092")] + void Interface.Method ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t) {} + } + + class C : InterfaceImpl {} + + class D : InterfaceImpl {} + + public static void Test () { + Interface i = new C (); + i = new D (); + i.Method (typeof (int)); + } + } } } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/VirtualMethodHierarchyDataflowAnnotationValidation.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/VirtualMethodHierarchyDataflowAnnotationValidation.cs index d19bcfe349aac..ea71c1f250abf 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/VirtualMethodHierarchyDataflowAnnotationValidation.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/VirtualMethodHierarchyDataflowAnnotationValidation.cs @@ -574,17 +574,17 @@ interface IBaseImplementedInterface class BaseImplementsInterfaceViaDerived { - [LogContains ( - "'DynamicallyAccessedMemberTypes' in 'DynamicallyAccessedMembersAttribute' on the return value of method 'Mono.Linker.Tests.Cases.DataFlow.VirtualMethodHierarchyDataflowAnnotationValidation.BaseImplementsInterfaceViaDerived.ReturnValueBaseWithInterfaceWithout()' " + - "don't match overridden return value of method 'Mono.Linker.Tests.Cases.DataFlow.VirtualMethodHierarchyDataflowAnnotationValidation.IBaseImplementedInterface.ReturnValueBaseWithInterfaceWithout()'. " + - "All overridden members must have the same 'DynamicallyAccessedMembersAttribute' usage.")] [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] public virtual Type ReturnValueBaseWithInterfaceWithout () => null; - [ExpectedWarning ("IL2046", "BaseImplementsInterfaceViaDerived.RequiresUnreferencedCodeBaseWithoutInterfaceWith")] public virtual void RequiresUnreferencedCodeBaseWithoutInterfaceWith () { } } + [ExpectedWarning ("IL2046", "BaseImplementsInterfaceViaDerived.RequiresUnreferencedCodeBaseWithoutInterfaceWith")] + [LogContains ( + "'DynamicallyAccessedMemberTypes' in 'DynamicallyAccessedMembersAttribute' on the return value of method 'Mono.Linker.Tests.Cases.DataFlow.VirtualMethodHierarchyDataflowAnnotationValidation.BaseImplementsInterfaceViaDerived.ReturnValueBaseWithInterfaceWithout()' " + + "don't match overridden return value of method 'Mono.Linker.Tests.Cases.DataFlow.VirtualMethodHierarchyDataflowAnnotationValidation.IBaseImplementedInterface.ReturnValueBaseWithInterfaceWithout()'. " + + "All overridden members must have the same 'DynamicallyAccessedMembersAttribute' usage.")] class DerivedWithInterfaceImplementedByBase : BaseImplementsInterfaceViaDerived, IBaseImplementedInterface { } From 67a0eac300698be47935e000084978c4c359b911 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Thu, 11 Jul 2024 17:27:58 +0000 Subject: [PATCH 3/9] Clean up test --- .../InterfaceImplementedThroughBaseValidation.cs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs index 2ab104cf60e5c..fd75ab75668a1 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs @@ -37,7 +37,7 @@ class Base { public void Method () {} } - [ExpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] + [ExpectedWarning ("IL2046")] class Derived : Base, Interface {} [ExpectedWarning ("IL2026")] @@ -58,7 +58,7 @@ class Base { public void Method () {} } - [ExpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] + [ExpectedWarning ("IL2046")] class Derived : Base, Interface {} public static void Test () { @@ -77,7 +77,7 @@ class Base { public void Method (Type t) {} } - [ExpectedWarning ("IL2092", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] + [ExpectedWarning ("IL2092")] class Derived : Base, Interface {} public static void Test () { @@ -96,7 +96,7 @@ class Base { public void Method ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t) {} } - [ExpectedWarning ("IL2092", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] + [ExpectedWarning ("IL2092")] class Derived : Base, Interface {} public static void Test () { @@ -114,6 +114,8 @@ interface Interface { interface InterfaceImpl : Interface { [ExpectedWarning ("IL2046")] + [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] + [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] void Interface.Method() {} } @@ -137,6 +139,8 @@ interface Interface { interface InterfaceImpl : Interface { [ExpectedWarning ("IL2046")] + [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] + [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] [RequiresUnreferencedCode (nameof (Method))] void Interface.Method() {} } @@ -160,6 +164,8 @@ interface Interface { interface InterfaceImpl : Interface { [ExpectedWarning ("IL2092")] + [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] + [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] void Interface.Method (Type t) {} } @@ -182,6 +188,8 @@ interface Interface { interface InterfaceImpl : Interface { [ExpectedWarning ("IL2092")] + [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] + [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] void Interface.Method ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t) {} } From 481b87d9665c792ab8c2c2cf461501986ac4676d Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Thu, 11 Jul 2024 17:29:35 +0000 Subject: [PATCH 4/9] Fix analyzer warning location --- .../DynamicallyAccessedMembersAnalyzer.cs | 38 ++++++++++++++----- .../RequiresAnalyzerBase.cs | 20 ++++++++-- 2 files changed, 45 insertions(+), 13 deletions(-) diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs index c7f7eb2eb6aa0..072433e7925d9 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs @@ -75,7 +75,12 @@ void AddRange (DiagnosticId first, DiagnosticId last) public override ImmutableArray SupportedDiagnostics => GetSupportedDiagnostics (); - static Location GetPrimaryLocation (ImmutableArray locations) => locations.Length > 0 ? locations[0] : Location.None; + static Location GetPrimaryLocation (ImmutableArray? locations) { + if (locations is null) + return Location.None; + + return locations.Value.Length > 0 ? locations.Value[0] : Location.None; + } public override void Initialize (AnalysisContext context) { @@ -167,7 +172,7 @@ static void VerifyDamOnDerivedAndBaseMethodsMatch (SymbolAnalysisContext context } } - static void VerifyDamOnMethodsMatch (SymbolAnalysisContext context, IMethodSymbol overrideMethod, IMethodSymbol baseMethod) + static void VerifyDamOnMethodsMatch (SymbolAnalysisContext context, IMethodSymbol overrideMethod, IMethodSymbol baseMethod, ISymbol? origin = null) { var overrideMethodReturnAnnotation = FlowAnnotations.GetMethodReturnValueAnnotation (overrideMethod); var baseMethodReturnAnnotation = FlowAnnotations.GetMethodReturnValueAnnotation (baseMethod); @@ -184,9 +189,10 @@ static void VerifyDamOnMethodsMatch (SymbolAnalysisContext context, IMethodSymbo && baseMethod.TryGetReturnAttribute (DynamicallyAccessedMembersAnalyzer.DynamicallyAccessedMembersAttribute, out var _)) ) ? (null, null) : CreateArguments (attributableSymbolLocation, missingAttribute); + var returnOrigin = origin ??= overrideMethod; context.ReportDiagnostic (Diagnostic.Create ( DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnMethodReturnValueBetweenOverrides), - GetPrimaryLocation (overrideMethod.Locations), sourceLocation, DAMArgs?.ToImmutableDictionary (), overrideMethod.GetDisplayName (), baseMethod.GetDisplayName ())); + GetPrimaryLocation (returnOrigin.Locations), sourceLocation, DAMArgs?.ToImmutableDictionary (), overrideMethod.GetDisplayName (), baseMethod.GetDisplayName ())); } foreach (var overrideParam in overrideMethod.GetMetadataParameters ()) { @@ -205,9 +211,10 @@ static void VerifyDamOnMethodsMatch (SymbolAnalysisContext context, IMethodSymbo && baseParam.ParameterSymbol!.TryGetAttribute (DynamicallyAccessedMembersAnalyzer.DynamicallyAccessedMembersAttribute, out var _)) ) ? (null, null) : CreateArguments (attributableSymbolLocation, missingAttribute); + var parameterOrigin = origin ?? overrideParam.ParameterSymbol; context.ReportDiagnostic (Diagnostic.Create ( DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnMethodParameterBetweenOverrides), - overrideParam.Location, sourceLocation, DAMArgs?.ToImmutableDictionary (), + GetPrimaryLocation (parameterOrigin?.Locations), sourceLocation, DAMArgs?.ToImmutableDictionary (), overrideParam.GetDisplayName (), overrideMethod.GetDisplayName (), baseParam.GetDisplayName (), baseMethod.GetDisplayName ())); } } @@ -228,27 +235,38 @@ static void VerifyDamOnMethodsMatch (SymbolAnalysisContext context, IMethodSymbo && baseMethod.TypeParameters[i].TryGetAttribute (DynamicallyAccessedMembersAnalyzer.DynamicallyAccessedMembersAttribute, out var _)) ) ? (null, null) : CreateArguments (attributableSymbolLocation, missingAttribute); + var typeParameterOrigin = origin ?? overrideMethod.TypeParameters[i]; context.ReportDiagnostic (Diagnostic.Create ( DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnGenericParameterBetweenOverrides), - GetPrimaryLocation (overrideMethod.TypeParameters[i].Locations), sourceLocation, DAMArgs?.ToImmutableDictionary (), + GetPrimaryLocation (typeParameterOrigin.Locations), sourceLocation, DAMArgs?.ToImmutableDictionary (), overrideMethod.TypeParameters[i].GetDisplayName (), overrideMethod.GetDisplayName (), baseMethod.TypeParameters[i].GetDisplayName (), baseMethod.GetDisplayName ())); } } - if (!overrideMethod.IsStatic && overrideMethod.GetDynamicallyAccessedMemberTypes () != baseMethod.GetDynamicallyAccessedMemberTypes ()) + if (!overrideMethod.IsStatic && overrideMethod.GetDynamicallyAccessedMemberTypes () != baseMethod.GetDynamicallyAccessedMemberTypes ()) { + var methodOrigin = origin ?? overrideMethod; context.ReportDiagnostic (Diagnostic.Create ( DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnImplicitThisBetweenOverrides), - GetPrimaryLocation (overrideMethod.Locations), + GetPrimaryLocation (methodOrigin.Locations), overrideMethod.GetDisplayName (), baseMethod.GetDisplayName ())); + } } static void VerifyDamOnInterfaceAndImplementationMethodsMatch (SymbolAnalysisContext context, INamedTypeSymbol type) { foreach (var (interfaceMember, implementationMember) in type.GetMemberInterfaceImplementationPairs ()) { - if (implementationMember is IMethodSymbol implementationMethod - && interfaceMember is IMethodSymbol interfaceMethod) - VerifyDamOnMethodsMatch (context, implementationMethod, interfaceMethod); + if (implementationMember is IMethodSymbol implementationMethod && interfaceMember is IMethodSymbol interfaceMethod) { + ISymbol origin = implementationMethod; + INamedTypeSymbol implementationType = implementationMethod.ContainingType; + + // If this type implements an interface method through a base class, the origin of the warning is this type, + // not the member on the base class. + if (!implementationType.IsInterface () && !SymbolEqualityComparer.Default.Equals (implementationType, type)) + origin = type; + + VerifyDamOnMethodsMatch (context, implementationMethod, interfaceMethod, origin); + } } } diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs index 28118233b656a..b104fc7dd8ead 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs @@ -123,8 +123,21 @@ void CheckMatchingAttributesInInterfaces ( INamedTypeSymbol type) { foreach (var memberpair in type.GetMemberInterfaceImplementationPairs ()) { + var implementationType = memberpair.ImplementationMember switch { + IMethodSymbol method => method.ContainingType, + IPropertySymbol property => property.ContainingType, + IEventSymbol @event => @event.ContainingType, + _ => throw new NotSupportedException () + }; + ISymbol origin = memberpair.ImplementationMember; + + // If this type implements an interface method through a base class, the origin of the warning is this type, + // not the member on the base class. + if (!implementationType.IsInterface () && !SymbolEqualityComparer.Default.Equals (implementationType, type)) + origin = type; + if (HasMismatchingAttributes (memberpair.InterfaceMember, memberpair.ImplementationMember)) { - ReportMismatchInAttributesDiagnostic (symbolAnalysisContext, memberpair.ImplementationMember, memberpair.InterfaceMember, isInterface: true); + ReportMismatchInAttributesDiagnostic (symbolAnalysisContext, memberpair.ImplementationMember, memberpair.InterfaceMember, isInterface: true, origin); } } } @@ -230,12 +243,13 @@ private void ReportRequiresOnStaticCtorDiagnostic (SymbolAnalysisContext symbolA ctor.GetDisplayName ())); } - private void ReportMismatchInAttributesDiagnostic (SymbolAnalysisContext symbolAnalysisContext, ISymbol member, ISymbol baseMember, bool isInterface = false) + private void ReportMismatchInAttributesDiagnostic (SymbolAnalysisContext symbolAnalysisContext, ISymbol member, ISymbol baseMember, bool isInterface = false, ISymbol? origin = null) { + origin ??= member; string message = MessageFormat.FormatRequiresAttributeMismatch (member.HasAttribute (RequiresAttributeName), isInterface, RequiresAttributeName, member.GetDisplayName (), baseMember.GetDisplayName ()); symbolAnalysisContext.ReportDiagnostic (Diagnostic.Create ( RequiresAttributeMismatch, - member.Locations[0], + origin.Locations[0], message)); } From eec135075d564f08b0ca8da30a7c7f2c9be52a83 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Thu, 11 Jul 2024 17:34:57 +0000 Subject: [PATCH 5/9] Update issue links --- .../InterfaceImplementedThroughBaseValidation.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs index fd75ab75668a1..0783b0d6c7c7d 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs @@ -114,8 +114,8 @@ interface Interface { interface InterfaceImpl : Interface { [ExpectedWarning ("IL2046")] - [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] - [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] + [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] + [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] void Interface.Method() {} } @@ -139,8 +139,8 @@ interface Interface { interface InterfaceImpl : Interface { [ExpectedWarning ("IL2046")] - [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] - [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] + [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] + [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] [RequiresUnreferencedCode (nameof (Method))] void Interface.Method() {} } @@ -164,8 +164,8 @@ interface Interface { interface InterfaceImpl : Interface { [ExpectedWarning ("IL2092")] - [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] - [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] + [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] + [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] void Interface.Method (Type t) {} } @@ -188,8 +188,8 @@ interface Interface { interface InterfaceImpl : Interface { [ExpectedWarning ("IL2092")] - [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] - [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104746")] + [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] + [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] void Interface.Method ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t) {} } From c85962acca208292852e7477e6db28c4dd90982f Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Thu, 11 Jul 2024 17:38:45 +0000 Subject: [PATCH 6/9] Update expected warnings for ILC --- .../DataFlow/InterfaceImplementedThroughBaseValidation.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs index 0783b0d6c7c7d..552db6b8c48a4 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs @@ -114,7 +114,7 @@ interface Interface { interface InterfaceImpl : Interface { [ExpectedWarning ("IL2046")] - [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] + [ExpectedWarning ("IL2046", Tool.Analyzer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/104627")] [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] void Interface.Method() {} } @@ -139,7 +139,7 @@ interface Interface { interface InterfaceImpl : Interface { [ExpectedWarning ("IL2046")] - [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] + [ExpectedWarning ("IL2046", Tool.Analyzer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/104627")] [ExpectedWarning ("IL2046", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] [RequiresUnreferencedCode (nameof (Method))] void Interface.Method() {} @@ -164,7 +164,7 @@ interface Interface { interface InterfaceImpl : Interface { [ExpectedWarning ("IL2092")] - [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] + [ExpectedWarning ("IL2092", Tool.Analyzer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/104627")] [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] void Interface.Method (Type t) {} } @@ -188,7 +188,7 @@ interface Interface { interface InterfaceImpl : Interface { [ExpectedWarning ("IL2092")] - [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] + [ExpectedWarning ("IL2092", Tool.Analyzer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/104627")] [ExpectedWarning ("IL2092", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/104627")] void Interface.Method ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t) {} } From 0b66cb115142612e00fef94049899db64e62052e Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Thu, 11 Jul 2024 18:12:08 +0000 Subject: [PATCH 7/9] Add some tests for ILC behavior --- ...terfaceImplementedThroughBaseValidation.cs | 115 ++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs index 552db6b8c48a4..472cc88b0b3e4 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs @@ -24,6 +24,12 @@ public static void Main () RUCOnDIM.Test (); DAMOnInterfaceWithDIM.Test (); DAMOnDIM.Test (); + + GenericVirtualMethod.Test (); + GenericVirtualMethodWithDIM.Test (); + + CalledThroughConstraint.Test (); + CalledThroughConstraintWithDIM.Test (); } class RUCOnInterfaceMethod @@ -203,5 +209,114 @@ public static void Test () { i.Method (typeof (int)); } } + + class CalledThroughConstraint + { + interface Interface { + [RequiresUnreferencedCode (nameof (Method))] + void Method (); + } + + struct Struct : Interface { + [ExpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] + // NativeAot produces one warning for each call to NoteOverridingMethod. + // This is done on each callsite while compiling generic code with constraints. + [ExpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] + [ExpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] + public void Method () {} + } + + [ExpectedWarning ("IL2026")] + [ExpectedWarning ("IL2026")] + [ExpectedWarning ("IL2026")] + static void Call (T t) where T : Interface { + t.Method (); + t.Method (); + t.Method (); + } + + public static void Test () { + Call(new Struct ()); + } + } + + class CalledThroughConstraintWithDIM + { + // Instance DIMs require boxing the struct. + // let's try a static DIM. + interface Interface { + [RequiresUnreferencedCode (nameof (Method))] + static abstract void Method (); + } + + interface InterfaceImpl : Interface { + [ExpectedWarning ("IL2046")] + [ExpectedWarning ("IL2046", Tool.NativeAot, "https://github.com/dotnet/runtime/issues/104702")] + static void Interface.Method() {} + } + + struct Struct : InterfaceImpl {} + + [ExpectedWarning ("IL2026")] + [ExpectedWarning ("IL2026")] + static void Call () where T : Interface { + T.Method (); + T.Method (); + } + + public static void Test () { + Call (); + } + } + + class GenericVirtualMethod + { + interface Interface { + [RequiresUnreferencedCode (nameof (Method))] + void Method (); + } + + class Base { + public void Method () {} + } + + [ExpectedWarning ("IL2046")] + class Derived : Base, Interface {} + + [ExpectedWarning ("IL2026")] + [ExpectedWarning ("IL2026")] + public static void Test () { + Interface i = new Derived (); + i.Method (); + i.Method (); + } + } + + class GenericVirtualMethodWithDIM + { + interface Interface { + [RequiresUnreferencedCode (nameof (Method))] + static abstract void Method (); + } + + interface Impl : Interface { + [ExpectedWarning ("IL2046")] + [ExpectedWarning ("IL2046", Tool.NativeAot, "https://github.com/dotnet/runtime/issues/104702")] + static void Interface.Method () {} + } + + class Class : Impl {} + + [ExpectedWarning ("IL2026")] + [ExpectedWarning ("IL2026")] + static void Call () where T : Interface { + T.Method (); + T.Method (); + } + + public static void Test () { + Call (); + } + } } } From 7c1bac5b13da1057a760eaaeab31efc28ae9ad78 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Thu, 11 Jul 2024 20:51:29 +0000 Subject: [PATCH 8/9] Fix origin for ILC --- .../Compiler/Dataflow/FlowAnnotations.cs | 29 +++++++------- .../Compiler/DependencyAnalysis/EETypeNode.cs | 3 +- .../DependencyAnalysis/GVMDependenciesNode.cs | 3 +- .../Compiler/MetadataManager.cs | 2 +- .../Compiler/UsageBasedMetadataManager.cs | 8 ++-- ...terfaceImplementedThroughBaseValidation.cs | 39 ++++++++++++++----- 6 files changed, 55 insertions(+), 29 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs index fd8094bc70283..19f13f9863b1b 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs @@ -706,7 +706,7 @@ private static bool ScanMethodBodyForFieldAccess(MethodIL body, bool write, out } } - internal void ValidateMethodAnnotationsAreSame(MethodDesc method, MethodDesc baseMethod) + internal void ValidateMethodAnnotationsAreSame(MethodDesc method, MethodDesc baseMethod, TypeSystemEntity origin) { method = method.GetTypicalMethodDefinition(); baseMethod = baseMethod.GetTypicalMethodDefinition(); @@ -715,14 +715,14 @@ internal void ValidateMethodAnnotationsAreSame(MethodDesc method, MethodDesc bas GetAnnotations(baseMethod.OwningType).TryGetAnnotation(baseMethod, out var baseMethodAnnotations); if (methodAnnotations.ReturnParameterAnnotation != baseMethodAnnotations.ReturnParameterAnnotation) - LogValidationWarning(method.Signature.ReturnType, baseMethod, method); + LogValidationWarning((method.Signature.ReturnType, method), baseMethod, origin); if (methodAnnotations.ParameterAnnotations != null || baseMethodAnnotations.ParameterAnnotations != null) { if (methodAnnotations.ParameterAnnotations == null) - ValidateMethodParametersHaveNoAnnotations(baseMethodAnnotations.ParameterAnnotations!, method, baseMethod, method); + ValidateMethodParametersHaveNoAnnotations(baseMethodAnnotations.ParameterAnnotations!, method, baseMethod, origin); else if (baseMethodAnnotations.ParameterAnnotations == null) - ValidateMethodParametersHaveNoAnnotations(methodAnnotations.ParameterAnnotations, method, baseMethod, method); + ValidateMethodParametersHaveNoAnnotations(methodAnnotations.ParameterAnnotations, method, baseMethod, origin); else { if (methodAnnotations.ParameterAnnotations.Length != baseMethodAnnotations.ParameterAnnotations.Length) @@ -734,7 +734,7 @@ internal void ValidateMethodAnnotationsAreSame(MethodDesc method, MethodDesc bas LogValidationWarning( (new MethodProxy(method)).GetParameter((ParameterIndex)parameterIndex), (new MethodProxy(baseMethod)).GetParameter((ParameterIndex)parameterIndex), - method); + origin); } } } @@ -742,9 +742,9 @@ internal void ValidateMethodAnnotationsAreSame(MethodDesc method, MethodDesc bas if (methodAnnotations.GenericParameterAnnotations != null || baseMethodAnnotations.GenericParameterAnnotations != null) { if (methodAnnotations.GenericParameterAnnotations == null) - ValidateMethodGenericParametersHaveNoAnnotations(baseMethodAnnotations.GenericParameterAnnotations!, method, baseMethod, method); + ValidateMethodGenericParametersHaveNoAnnotations(baseMethodAnnotations.GenericParameterAnnotations!, method, baseMethod, origin); else if (baseMethodAnnotations.GenericParameterAnnotations == null) - ValidateMethodGenericParametersHaveNoAnnotations(methodAnnotations.GenericParameterAnnotations, method, baseMethod, method); + ValidateMethodGenericParametersHaveNoAnnotations(methodAnnotations.GenericParameterAnnotations, method, baseMethod, origin); else { if (methodAnnotations.GenericParameterAnnotations.Length != baseMethodAnnotations.GenericParameterAnnotations.Length) @@ -757,14 +757,14 @@ internal void ValidateMethodAnnotationsAreSame(MethodDesc method, MethodDesc bas LogValidationWarning( method.Instantiation[genericParameterIndex], baseMethod.Instantiation[genericParameterIndex], - method); + origin); } } } } } - private void ValidateMethodParametersHaveNoAnnotations(DynamicallyAccessedMemberTypes[] parameterAnnotations, MethodDesc method, MethodDesc baseMethod, MethodDesc origin) + private void ValidateMethodParametersHaveNoAnnotations(DynamicallyAccessedMemberTypes[] parameterAnnotations, MethodDesc method, MethodDesc baseMethod, TypeSystemEntity origin) { for (int parameterIndex = 0; parameterIndex < parameterAnnotations.Length; parameterIndex++) { @@ -777,7 +777,7 @@ private void ValidateMethodParametersHaveNoAnnotations(DynamicallyAccessedMember } } - private void ValidateMethodGenericParametersHaveNoAnnotations(DynamicallyAccessedMemberTypes[] genericParameterAnnotations, MethodDesc method, MethodDesc baseMethod, MethodDesc origin) + private void ValidateMethodGenericParametersHaveNoAnnotations(DynamicallyAccessedMemberTypes[] genericParameterAnnotations, MethodDesc method, MethodDesc baseMethod, TypeSystemEntity origin) { for (int genericParameterIndex = 0; genericParameterIndex < genericParameterAnnotations.Length; genericParameterIndex++) { @@ -791,8 +791,11 @@ private void ValidateMethodGenericParametersHaveNoAnnotations(DynamicallyAccesse } } - private void LogValidationWarning(object provider, object baseProvider, MethodDesc origin) + private void LogValidationWarning(object provider, object baseProvider, TypeSystemEntity origin) { + if (origin is TypeDesc) { + Debug.WriteLine("Here"); + } switch (provider) { case ParameterProxy parameter: @@ -810,9 +813,9 @@ private void LogValidationWarning(object provider, object baseProvider, MethodDe genericParameterOverride.Name, DiagnosticUtilities.GetGenericParameterDeclaringMemberDisplayName(genericParameterOverride), ((GenericParameterDesc)baseProvider).Name, DiagnosticUtilities.GetGenericParameterDeclaringMemberDisplayName((GenericParameterDesc)baseProvider)); break; - case TypeDesc: + case (TypeDesc, MethodDesc method): _logger.LogWarning(origin, DiagnosticId.DynamicallyAccessedMembersMismatchOnMethodReturnValueBetweenOverrides, - DiagnosticUtilities.GetMethodSignatureDisplayName(origin), DiagnosticUtilities.GetMethodSignatureDisplayName((MethodDesc)baseProvider)); + DiagnosticUtilities.GetMethodSignatureDisplayName(method), DiagnosticUtilities.GetMethodSignatureDisplayName((MethodDesc)baseProvider)); break; // No fields - it's not possible to have a virtual field and override it default: diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs index 10bac5be472fe..8b8f34ada6c83 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs @@ -534,7 +534,8 @@ public sealed override IEnumerable GetConditionalSt result.Add(new CombinedDependencyListEntry(factory.VirtualMethodUse(interfaceMethod), factory.VariantInterfaceMethodUse(typicalInterfaceMethod), "Interface method")); } - factory.MetadataManager.NoteOverridingMethod(interfaceMethod, implMethod); + TypeSystemEntity origin = (implMethod.OwningType != defType) ? defType : null; + factory.MetadataManager.NoteOverridingMethod(interfaceMethod, implMethod, origin); factory.MetadataManager.GetDependenciesForOverridingMethod(ref result, factory, interfaceMethod, implMethod); } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs index 26526cc5628d7..618d7e68c9b7b 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs @@ -149,7 +149,8 @@ public override IEnumerable SearchDynamicDependenci else dynamicDependencies.Add(new CombinedDependencyListEntry(factory.GVMDependencies(implementingMethodInstantiation.GetCanonMethodTarget(CanonicalFormKind.Specific)), null, "ImplementingMethodInstantiation")); - factory.MetadataManager.NoteOverridingMethod(_method, implementingMethodInstantiation); + TypeSystemEntity origin = (implementingMethodInstantiation.OwningType != potentialOverrideType) ? potentialOverrideType : null; + factory.MetadataManager.NoteOverridingMethod(_method, implementingMethodInstantiation, origin); } } } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs index 2aaa93c9efdcc..835b601cf6496 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs @@ -1186,7 +1186,7 @@ public virtual DependencyList GetDependenciesForCustomAttribute(NodeFactory fact return null; } - public virtual void NoteOverridingMethod(MethodDesc baseMethod, MethodDesc overridingMethod) + public virtual void NoteOverridingMethod(MethodDesc baseMethod, MethodDesc overridingMethod, TypeSystemEntity origin = null) { } } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs index 991fe7aa62401..7748b5969ce6d 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs @@ -784,7 +784,7 @@ public bool GeneratesAttributeMetadata(TypeDesc attributeType) return true; } - public override void NoteOverridingMethod(MethodDesc baseMethod, MethodDesc overridingMethod) + public override void NoteOverridingMethod(MethodDesc baseMethod, MethodDesc overridingMethod, TypeSystemEntity origin) { baseMethod = baseMethod.GetTypicalMethodDefinition(); overridingMethod = overridingMethod.GetTypicalMethodDefinition(); @@ -792,6 +792,8 @@ public override void NoteOverridingMethod(MethodDesc baseMethod, MethodDesc over if (baseMethod == overridingMethod) return; + origin ??= overridingMethod; + bool baseMethodTypeIsInterface = baseMethod.OwningType.IsInterface; foreach (var requiresAttribute in _requiresAttributeMismatchNameAndId) { @@ -803,7 +805,7 @@ public override void NoteOverridingMethod(MethodDesc baseMethod, MethodDesc over string message = MessageFormat.FormatRequiresAttributeMismatch(overridingMethod.DoesMethodRequire(requiresAttribute.AttributeName, out _), baseMethodTypeIsInterface, requiresAttribute.AttributeName, overridingMethodName, baseMethodName); - Logger.LogWarning(overridingMethod, requiresAttribute.Id, message); + Logger.LogWarning(origin, requiresAttribute.Id, message); } } @@ -811,7 +813,7 @@ public override void NoteOverridingMethod(MethodDesc baseMethod, MethodDesc over bool overridingMethodRequiresDataflow = FlowAnnotations.RequiresVirtualMethodDataflowAnalysis(overridingMethod); if (baseMethodRequiresDataflow || overridingMethodRequiresDataflow) { - FlowAnnotations.ValidateMethodAnnotationsAreSame(overridingMethod, baseMethod); + FlowAnnotations.ValidateMethodAnnotationsAreSame(overridingMethod, baseMethod, origin); } } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs index 472cc88b0b3e4..ba39e6f5071eb 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs @@ -27,6 +27,7 @@ public static void Main () GenericVirtualMethod.Test (); GenericVirtualMethodWithDIM.Test (); + GenericVirtualMethodWithStaticDIM.Test (); CalledThroughConstraint.Test (); CalledThroughConstraintWithDIM.Test (); @@ -218,21 +219,16 @@ interface Interface { } struct Struct : Interface { - [ExpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] - // NativeAot produces one warning for each call to NoteOverridingMethod. - // This is done on each callsite while compiling generic code with constraints. - [ExpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] - [ExpectedWarning ("IL2046", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/97676")] + [ExpectedWarning ("IL2046")] + [ExpectedWarning ("IL2046", Tool.Analyzer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/104627")] public void Method () {} } - [ExpectedWarning ("IL2026")] [ExpectedWarning ("IL2026")] [ExpectedWarning ("IL2026")] static void Call (T t) where T : Interface { t.Method (); t.Method (); - t.Method (); } public static void Test () { @@ -251,7 +247,7 @@ interface Interface { interface InterfaceImpl : Interface { [ExpectedWarning ("IL2046")] - [ExpectedWarning ("IL2046", Tool.NativeAot, "https://github.com/dotnet/runtime/issues/104702")] + [ExpectedWarning ("IL2046", Tool.Analyzer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/104702")] static void Interface.Method() {} } @@ -290,9 +286,32 @@ public static void Test () { i.Method (); i.Method (); } - } + } class GenericVirtualMethodWithDIM + { + interface Interface { + [RequiresUnreferencedCode (nameof (Method))] + void Method (); + } + + class Impl : Interface { + [ExpectedWarning ("IL2046")] + void Interface.Method () {} + } + + class Class : Impl {} + + [ExpectedWarning ("IL2026")] + [ExpectedWarning ("IL2026")] + public static void Test () { + Interface i = new Class (); + i.Method (); + i.Method (); + } + } + + class GenericVirtualMethodWithStaticDIM { interface Interface { [RequiresUnreferencedCode (nameof (Method))] @@ -301,7 +320,7 @@ interface Interface { interface Impl : Interface { [ExpectedWarning ("IL2046")] - [ExpectedWarning ("IL2046", Tool.NativeAot, "https://github.com/dotnet/runtime/issues/104702")] + [ExpectedWarning ("IL2046", Tool.Analyzer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/104702")] static void Interface.Method () {} } From e2c722ddd950dacac7a0ac20f13a51073680fe4a Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Mon, 15 Jul 2024 16:17:26 +0000 Subject: [PATCH 9/9] Remove left-over Debug.WriteLine And left-over comment --- .../ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs | 3 --- .../DataFlow/InterfaceImplementedThroughBaseValidation.cs | 2 -- 2 files changed, 5 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs index 19f13f9863b1b..e7682e267b344 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs @@ -793,9 +793,6 @@ private void ValidateMethodGenericParametersHaveNoAnnotations(DynamicallyAccesse private void LogValidationWarning(object provider, object baseProvider, TypeSystemEntity origin) { - if (origin is TypeDesc) { - Debug.WriteLine("Here"); - } switch (provider) { case ParameterProxy parameter: diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs index ba39e6f5071eb..b92d3fa3943f2 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/InterfaceImplementedThroughBaseValidation.cs @@ -238,8 +238,6 @@ public static void Test () { class CalledThroughConstraintWithDIM { - // Instance DIMs require boxing the struct. - // let's try a static DIM. interface Interface { [RequiresUnreferencedCode (nameof (Method))] static abstract void Method ();