From 1fb6148f03f206be40d08c02166ef7d850fea1ff Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Mon, 6 Jan 2025 13:07:21 -0500 Subject: [PATCH] Unwrap NullableValue for diagnostics creation and add try/catch for formatting diagnostics (#110229) Fixes #109536 See #93800 for more context. When a NullableValue is the source of an assignment, we consider use the TypeArgumentTargetsX diagnostic since the underlying value is a generic argument, but use the source of the generic argument for diagnostics. This leads to some confusing warning messages, and sometimes crashes in formatting the diagnostics. Ideally, we would want to unwrap the NullableValues and use the UnderlyingTypeValue as the source, but this would be a breaking change for the situations that don't crash. FieldValue and MethodReturnValue only provide 1 diagnostic arguments, but the diagnostics for TypeArgumentTargetsX expects 2 arguments from the source. This caused crashes when the situation was encountered. Since they didn't work before at all, this won't be a breaking change to provide the correct warning here. --- .../illink/src/ILLink.Shared/Annotations.cs | 9 +++ .../DataFlow/NullableAnnotations.cs | 63 +++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/src/tools/illink/src/ILLink.Shared/Annotations.cs b/src/tools/illink/src/ILLink.Shared/Annotations.cs index 3eaf8378cc00b..97d244a712f24 100644 --- a/src/tools/illink/src/ILLink.Shared/Annotations.cs +++ b/src/tools/illink/src/ILLink.Shared/Annotations.cs @@ -85,6 +85,15 @@ private static DynamicallyAccessedMemberTypes[] GetAllDynamicallyAccessedMemberT public static (DiagnosticId Id, string[] Arguments) GetDiagnosticForAnnotationMismatch (ValueWithDynamicallyAccessedMembers source, ValueWithDynamicallyAccessedMembers target, string missingAnnotations) { + source = source switch { + // FieldValue and MethodReturnValue have only one diagnostic argument, so formatting throws when the source + // is a NullableValueWithDynamicallyAccessedMembers. + // The correct behavior here is to unwrap always, as the underlying type is the one that has the annotations, + // but it is a breaking change for other UnderlyingTypeValues. + // https://github.com/dotnet/runtime/issues/93800 + NullableValueWithDynamicallyAccessedMembers { UnderlyingTypeValue: FieldValue or MethodReturnValue } nullable => nullable.UnderlyingTypeValue, + _ => source + }; DiagnosticId diagnosticId = (source, target) switch { (MethodParameterValue maybeThisSource, MethodParameterValue maybeThisTarget) when maybeThisSource.IsThisParameter () && maybeThisTarget.IsThisParameter () => DiagnosticId.DynamicallyAccessedMembersMismatchThisParameterTargetsThisParameter, (MethodParameterValue maybeThis, MethodParameterValue) when maybeThis.IsThisParameter () => DiagnosticId.DynamicallyAccessedMembersMismatchThisParameterTargetsParameter, diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/NullableAnnotations.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/NullableAnnotations.cs index 29b8bf650cbff..b9f4d8fb9e2de 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/NullableAnnotations.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/NullableAnnotations.cs @@ -65,6 +65,7 @@ public static void Main () GetUnderlyingTypeOnNonNullableKnownType.Test (); MakeGenericTypeWithUnknownValue (new object[2] { 1, 2 }); MakeGenericTypeWithKnowAndUnknownArray (); + RequiresOnNullableMakeGenericType.Test(); // Prevents optimizing away 'as Type' conversion. PreserveSystemType (); @@ -100,6 +101,68 @@ static void RequireAllFromMadeGenericNullableOfTypeWithMethodWithRuc () typeof (Nullable<>).MakeGenericType (typeof (TestStructWithRucMethod)).RequiresAll (); } + public class RequiresOnNullableMakeGenericType + { + [Kept] + static Type UnannotatedField; + [Kept] + [KeptAttributeAttribute(typeof(DynamicallyAccessedMembersAttribute))] + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] + static Type FieldWithMethods; + [Kept] + [UnexpectedWarning("IL2080", nameof(UnannotatedField), Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/93800")] + static void Field() + { + typeof (Nullable<>).MakeGenericType (UnannotatedField).GetMethods (); + typeof (Nullable<>).MakeGenericType (FieldWithMethods).GetMethods (); + } + + [Kept] + [UnexpectedWarning("IL2090", nameof(unannotated), Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/93800")] + static void Parameter( + Type unannotated, + [KeptAttributeAttribute(typeof(DynamicallyAccessedMembersAttribute))] + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type annotated) + { + typeof (Nullable<>).MakeGenericType (unannotated).GetMethods (); + typeof (Nullable<>).MakeGenericType (annotated).GetMethods (); + } + + [Kept] + [ExpectedWarning("IL2090", "TUnannotated")] + static void TypeParameter< + TUnannotated, + [KeptAttributeAttribute(typeof(DynamicallyAccessedMembersAttribute))] + [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] TAnnotated>() + { + typeof (Nullable<>).MakeGenericType (typeof(TUnannotated)).GetMethods (); + typeof (Nullable<>).MakeGenericType (typeof(TAnnotated)).GetMethods (); + } + + [Kept] + [UnexpectedWarning("IL2075", nameof(GetUnannotated), Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/93800")] + static void ReturnValue() + { + typeof (Nullable<>).MakeGenericType (GetUnannotated()).GetMethods (); + typeof (Nullable<>).MakeGenericType (GetAnnotated()).GetMethods (); + } + [Kept] + static Type GetUnannotated() => null; + [Kept] + [return: KeptAttributeAttribute(typeof(DynamicallyAccessedMembersAttribute))] + [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] + static Type GetAnnotated() => null; + + [Kept] + public static void Test() + { + Field(); + Parameter(null, null); + TypeParameter(); + ReturnValue(); + } + } + [Kept] static void TestRequireRucMethodThroughNullable () {