diff --git a/src/coreclr/tools/ILTrim.Tests/ILTrimExpectedFailures.txt b/src/coreclr/tools/ILTrim.Tests/ILTrimExpectedFailures.txt index bbba23725c4938..be1e6bbf616f2b 100644 --- a/src/coreclr/tools/ILTrim.Tests/ILTrimExpectedFailures.txt +++ b/src/coreclr/tools/ILTrim.Tests/ILTrimExpectedFailures.txt @@ -105,6 +105,7 @@ DataFlow.RuntimeAsyncMethods DataFlow.StaticInterfaceMethodDataflow DataFlow.SuppressWarningWithLinkAttributes DataFlow.UnresolvedMembers +DataFlow.UnusedVirtualMethodAnnotations DataFlow.VirtualMethodHierarchyDataflowAnnotationValidation DataFlow.XmlAnnotations DynamicDependencies.DynamicDependencyField diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs index 1c5d2c6d252e7c..7a7579c792284f 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs @@ -67,5 +67,11 @@ public Task UnsafeDataFlow() return RunTest(allowMissingWarnings: true); } + [Fact] + public Task UnusedVirtualMethodAnnotations() + { + return RunTest(allowMissingWarnings: true); + } + } } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/Dependencies/UnusedVirtualMethodAnnotationsBase.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/Dependencies/UnusedVirtualMethodAnnotationsBase.cs new file mode 100644 index 00000000000000..9a12868bd5caa0 --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/Dependencies/UnusedVirtualMethodAnnotationsBase.cs @@ -0,0 +1,41 @@ +// 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; + +namespace Mono.Linker.Tests.Cases.DataFlow.Dependencies +{ + public interface IUnusedInPreservedAssembly + { + [RequiresUnreferencedCode(nameof(Method))] + void Method([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type type); + } + + public interface ITypeOnlyInPreservedAssembly + { + [RequiresUnreferencedCode(nameof(Method))] + void Method([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type type); + } + + public interface IPartiallyUsedInPreservedAssembly + { + [RequiresUnreferencedCode(nameof(Method))] + void Method([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type type); + } + + public abstract class BaseInPreservedAssembly + { + [RequiresUnreferencedCode(nameof(Method))] + public abstract void Method([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type type); + } + + // Annotations on methods of a System.Type-derived type apply to the implicit 'this' parameter, + // so a mismatch on an override is reported as IL2094 rather than IL2092. The issue used + // System.Reflection.IReflect, which the trimmer treats the same way. + public abstract class TypeWithAnnotatedThisInPreservedAssembly : Type + { + [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] + public virtual void Method() { } + } +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/UnusedVirtualMethodAnnotations.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/UnusedVirtualMethodAnnotations.cs new file mode 100644 index 00000000000000..64ed36b5e77d8a --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/UnusedVirtualMethodAnnotations.cs @@ -0,0 +1,160 @@ +// 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.DataFlow.Dependencies; +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] + [SetupCompileBefore("base.dll", new[] { "Dependencies/UnusedVirtualMethodAnnotationsBase.cs" })] + [SetupLinkerAction("copy", "base")] + class UnusedVirtualMethodAnnotations + { + [UnconditionalSuppressMessage("Test", "IL2026")] + public static void Main() + { + _ = typeof(TypeOnlyImplementation); + + IUsed used = new UsedImplementation(); + used.Method(typeof(object)); + + IPartiallyUsed partiallyUsed = new PartiallyUsedImplementation(); + partiallyUsed.Method(typeof(object)); + + _ = typeof(TypeOnlyImplementationOfPreservedInterface); + + IPartiallyUsedInPreservedAssembly partiallyUsedFromPreservedAssembly = new UsedImplementationOfPreservedInterface(); + partiallyUsedFromPreservedAssembly.Method(typeof(object)); + } + + interface IUnused + { + [RequiresUnreferencedCode(nameof(Method))] + void Method([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type type); + } + + class UnusedImplementation : IUnused + { + [ExpectedWarning("IL2046", Tool.Analyzer, "Analyzer does not track reachability")] + [ExpectedWarning("IL2092", Tool.Analyzer, "Analyzer does not track reachability")] + public void Method(Type type) { } + } + + interface ITypeOnly + { + [RequiresUnreferencedCode(nameof(Method))] + void Method([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type type); + } + + class TypeOnlyImplementation : ITypeOnly + { + [ExpectedWarning("IL2046", Tool.Analyzer, "Analyzer does not track reachability")] + [ExpectedWarning("IL2092", Tool.Analyzer, "Analyzer does not track reachability")] + public void Method(Type type) { } + } + + interface IUsed + { + [RequiresUnreferencedCode(nameof(Method))] + void Method([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type type); + } + + class UsedImplementation : IUsed + { + [ExpectedWarning("IL2046")] + [ExpectedWarning("IL2092")] + public void Method(Type type) { } + } + + interface IPartiallyUsed + { + [RequiresUnreferencedCode(nameof(Method))] + void Method([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type type); + } + + class PartiallyUsedImplementation : IPartiallyUsed + { + [ExpectedWarning("IL2046")] + [ExpectedWarning("IL2092")] + public void Method(Type type) { } + } + + // The trimmer warns about this unused implementation because the interface method was + // marked (by PartiallyUsedImplementation) and validation walks every override of a marked + // virtual method, whether or not the override was kept. + class UnusedImplementationOfPartiallyUsedInterface : IPartiallyUsed + { + [ExpectedWarning("IL2046", Tool.Analyzer, "Analyzer does not track reachability")] + [UnexpectedWarning("IL2046", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/98870")] + [ExpectedWarning("IL2092", Tool.Analyzer, "Analyzer does not track reachability")] + [UnexpectedWarning("IL2092", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/98870")] + public void Method(Type type) { } + } + + // The cases below declare the base member in an assembly the trimmer doesn't link. + // Every virtual method of such an assembly is queued for annotation validation, and each + // of its overrides is then validated, so an implementation warns even when nothing in the + // app can reach it. This is the shape reported in the issue, where the base member is + // System.Reflection.IReflect in the (unlinked) framework. + + class UnusedImplementationOfPreservedInterface : IUnusedInPreservedAssembly + { + [ExpectedWarning("IL2046", Tool.Analyzer, "Analyzer does not track reachability")] + [UnexpectedWarning("IL2046", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/98870")] + [ExpectedWarning("IL2092", Tool.Analyzer, "Analyzer does not track reachability")] + [UnexpectedWarning("IL2092", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/98870")] + public void Method(Type type) { } + } + + class TypeOnlyImplementationOfPreservedInterface : ITypeOnlyInPreservedAssembly + { + [ExpectedWarning("IL2046", Tool.Analyzer, "Analyzer does not track reachability")] + [UnexpectedWarning("IL2046", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/98870")] + [ExpectedWarning("IL2092", Tool.Analyzer, "Analyzer does not track reachability")] + [UnexpectedWarning("IL2092", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/98870")] + public void Method(Type type) { } + } + + class UsedImplementationOfPreservedInterface : IPartiallyUsedInPreservedAssembly + { + [ExpectedWarning("IL2046")] + [ExpectedWarning("IL2092")] + public void Method(Type type) { } + } + + class UnusedImplementationOfUsedPreservedInterface : IPartiallyUsedInPreservedAssembly + { + [ExpectedWarning("IL2046", Tool.Analyzer, "Analyzer does not track reachability")] + [UnexpectedWarning("IL2046", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/98870")] + [ExpectedWarning("IL2092", Tool.Analyzer, "Analyzer does not track reachability")] + [UnexpectedWarning("IL2092", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/98870")] + public void Method(Type type) { } + } + + class UnusedOverrideOfPreservedBaseClass : BaseInPreservedAssembly + { + [ExpectedWarning("IL2046", Tool.Analyzer, "Analyzer does not track reachability")] + [UnexpectedWarning("IL2046", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/98870")] + [ExpectedWarning("IL2092", Tool.Analyzer, "Analyzer does not track reachability")] + [UnexpectedWarning("IL2092", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/98870")] + public override void Method(Type type) { } + } + + // The shape from the issue, where the base member's annotation applies to the implicit + // 'this' parameter so the mismatch is reported as IL2094 rather than IL2092. Only + // System.Type and System.Reflection.IReflect hierarchies annotate 'this'; the issue used + // IReflect, and deriving from System.Type covers the same validation path. + abstract class UnusedOverrideOfPreservedTypeWithAnnotatedThis : TypeWithAnnotatedThisInPreservedAssembly + { + [ExpectedWarning("IL2094", Tool.Analyzer, "Analyzer does not track reachability")] + [UnexpectedWarning("IL2094", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/98870")] + public override void Method() { } + } + } +} diff --git a/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs b/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs index 4663dcae82c15c..e7dbad5bc01eed 100644 --- a/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs +++ b/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs @@ -915,6 +915,12 @@ void VerifyLoggedMessages(AssemblyDefinition original, TrimmingTestLogger logger List<(ICustomAttributeProvider, CustomAttribute)> expectedNoWarningsAttributes = new(); List missingMessageWarnings = []; List unexpectedMessageWarnings = []; + + // A warning may be reported on a member which the trimmer then removed. Cecil detaches a + // removed member from its declaring type, so reading the member's names now would give a + // FullName without the namespace or declaring type, which no member of the original + // assembly matches. Use the names the logger recorded when the message was logged. + TrimmingTestLogger.OriginNames? OriginNames(MessageContainer message) => logger.GetOriginNames(message); foreach (var attrProvider in GetAttributeProviders(original)) { if (attrProvider is MethodDefinition attrMethod && @@ -1070,9 +1076,14 @@ attrMethod.DeclaringType is TypeDefinition declaringType && if (loggedMessage.Origin?.Provider is not IMemberDefinition memberDefinition) continue; + var originNames = OriginNames(loggedMessage); + string actualMemberName = originNames?.Name ?? memberDefinition.Name; + string actualDeclaringTypeFullName = originNames?.DeclaringTypeFullName ?? memberDefinition.DeclaringType?.FullName; + string actualDeclaringTypeName = originNames?.DeclaringTypeName ?? memberDefinition.DeclaringType?.Name; + if (attrProvider is IMemberDefinition expectedMember) { - string actualName = memberDefinition.DeclaringType.FullName + "." + memberDefinition.Name; + string actualName = actualDeclaringTypeFullName + "." + actualMemberName; if (actualName.StartsWith(expectedMember.DeclaringType.FullName) && (actualName.Contains("<" + expectedMember.Name + ">") || @@ -1087,21 +1098,21 @@ attrMethod.DeclaringType is TypeDefinition declaringType && continue; if (actualName.StartsWith(expectedMember.DeclaringType.FullName)) { - if (memberDefinition.Name == ".cctor" && + if (actualMemberName == ".cctor" && (expectedMember is FieldDefinition || expectedMember is PropertyDefinition)) { expectedWarningFound = true; unmatchedMessages.Remove(loggedMessage); break; } - if (memberDefinition.Name == ".ctor" && - (expectedMember is FieldDefinition || expectedMember is PropertyDefinition || memberDefinition.DeclaringType.FullName == expectedMember.FullName)) + if (actualMemberName == ".ctor" && + (expectedMember is FieldDefinition || expectedMember is PropertyDefinition || actualDeclaringTypeFullName == expectedMember.FullName)) { expectedWarningFound = true; unmatchedMessages.Remove(loggedMessage); break; } - if (memberDefinition.DeclaringType.Name.StartsWith("") && memberDefinition.Name == expectedMember.Name) + if (actualDeclaringTypeName?.StartsWith("") == true && actualMemberName == expectedMember.Name) { expectedWarningFound = true; unmatchedMessages.Remove(loggedMessage); @@ -1112,8 +1123,8 @@ attrMethod.DeclaringType is TypeDefinition declaringType && else if (attrProvider is AssemblyDefinition expectedAssembly) { // Allow assembly-level attributes to match warnings from compiler-generated Main - if (memberDefinition.Name == "
$" && - memberDefinition.DeclaringType.FullName == "Program" && + if (actualMemberName == "
$" && + actualDeclaringTypeFullName == "Program" && memberDefinition.DeclaringType.Module.Assembly.Name.Name == expectedAssembly.Name.Name) { expectedWarningFound = true; @@ -1187,7 +1198,8 @@ attrMethod.DeclaringType is TypeDefinition declaringType && continue; // This is a hacky way to say anything in the "subtree" of the attrProvider - if (attrProvider is IMemberDefinition attrMember && (mc.Origin?.Provider is IMemberDefinition member) && member.FullName.Contains(attrMember.FullName) != true) + if (attrProvider is IMemberDefinition attrMember && (mc.Origin?.Provider is IMemberDefinition member) + && (OriginNames(mc)?.FullName ?? member.FullName).Contains(attrMember.FullName) != true) continue; unexpectedMessageWarnings.Add($"Unexpected warning found: {mc}"); @@ -1197,7 +1209,16 @@ attrMethod.DeclaringType is TypeDefinition declaringType && if (missingMessageWarnings.Any()) { missingMessageWarnings.Add("Unmatched Messages:" + Environment.NewLine); - missingMessageWarnings.AddRange(unmatchedMessages.Select(m => m.ToString())); + missingMessageWarnings.AddRange(unmatchedMessages.Select(m => + { + // A message whose origin member was removed by the trimmer renders without its + // namespace or declaring type, so also show the name the member had when the + // message was logged. + var names = OriginNames(m); + return names is null || m.ToString().Contains(names.Value.DeclaringTypeFullName ?? string.Empty) + ? m.ToString() + : $"{m} (origin was removed by the trimmer: {names.Value.FullName})"; + })); // Uncomment to show all messages when diagnosing test infrastructure issues // missingMessageWarnings.Add(Environment.NewLine + "All Messages:" + Environment.NewLine); // missingMessageWarnings.AddRange(allMessages.Select(m => m.ToString())); @@ -1228,7 +1249,7 @@ bool LogMessageHasSameOriginMember(MessageContainer mc, ICustomAttributeProvider if (expectedOriginProvider is not IMemberDefinition expectedOriginMember) return false; - return actualMember.FullName == expectedOriginMember.FullName; + return (OriginNames(mc)?.FullName ?? actualMember.FullName) == expectedOriginMember.FullName; } } diff --git a/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/TrimmingTestLogger.cs b/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/TrimmingTestLogger.cs index c129822af85447..f7ff22ecae92e3 100644 --- a/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/TrimmingTestLogger.cs +++ b/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/TrimmingTestLogger.cs @@ -4,16 +4,26 @@ using System; using System.Collections.Generic; using System.Collections.Immutable; +using Mono.Cecil; namespace Mono.Linker.Tests.TestCasesRunner { public class TrimmingTestLogger : ILogger { + /// + /// The names of a warning's origin member, captured while the member is still attached to + /// its declaring type. + /// + public readonly record struct OriginNames(string FullName, string DeclaringTypeFullName, string DeclaringTypeName, string Name); + readonly List MessageContainers; + readonly Dictionary OriginNamesByProvider; + public TrimmingTestLogger() { MessageContainers = new List(); + OriginNamesByProvider = new Dictionary(ReferenceEqualityComparer.Instance); } public ImmutableArray GetLoggedMessages() @@ -21,6 +31,18 @@ public ImmutableArray GetLoggedMessages() return MessageContainers.ToImmutableArray(); } + /// + /// Returns the names the origin member of had when the message was + /// logged, or null if the message has no member origin. + /// + public OriginNames? GetOriginNames(MessageContainer message) + { + if (message.Origin?.Provider is not object provider) + return null; + + return OriginNamesByProvider.TryGetValue(provider, out var names) ? names : null; + } + public void LogMessage(MessageContainer message) { // This is to force Cecil to load all the information from the assembly @@ -28,6 +50,19 @@ public void LogMessage(MessageContainer message) // later on during validation, it may already be closed and Cecil's lazy loading might fail. message.ToString(); + // Warnings can be reported on members which the trimmer goes on to remove. Removing a + // member detaches it from its declaring type, so afterwards its FullName no longer + // includes the namespace or the declaring type and can't be matched against the names + // the test's expectations refer to. Record the names while they are still accurate. + if (message.Origin?.Provider is IMemberDefinition member && !OriginNamesByProvider.ContainsKey(member)) + { + OriginNamesByProvider.Add(member, new OriginNames( + member.FullName, + member.DeclaringType?.FullName, + member.DeclaringType?.Name, + member.Name)); + } + MessageContainers.Add(message); } }