From 7734b52d9655a708f2a59aa58403d5d4424f4b68 Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Wed, 27 Nov 2024 11:45:39 -0500 Subject: [PATCH 1/3] Unwrap NullableValue for diagnostics creation and add try/catch for formatting --- .../illink/src/ILLink.Shared/Annotations.cs | 9 +++ .../src/ILLink.Shared/DiagnosticString.cs | 4 +- .../illink/src/ILLink.Shared/MessageFormat.cs | 21 ++++++- .../DataFlow/NullableAnnotations.cs | 63 +++++++++++++++++++ 4 files changed, 94 insertions(+), 3 deletions(-) diff --git a/src/tools/illink/src/ILLink.Shared/Annotations.cs b/src/tools/illink/src/ILLink.Shared/Annotations.cs index 3eaf8378cc00b5..97d244a712f24d 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/src/ILLink.Shared/DiagnosticString.cs b/src/tools/illink/src/ILLink.Shared/DiagnosticString.cs index ce505e1c7727b1..b5a8cd4695c1ec 100644 --- a/src/tools/illink/src/ILLink.Shared/DiagnosticString.cs +++ b/src/tools/illink/src/ILLink.Shared/DiagnosticString.cs @@ -28,12 +28,12 @@ public DiagnosticString (string diagnosticResourceStringName) } public string GetMessage (params string[] args) => - string.Format (_messageFormat, args); + MessageFormat.TryFormat (_messageFormat, args); public string GetMessageFormat () => _messageFormat; public string GetTitle (params string[] args) => - string.Format (_titleFormat, args); + MessageFormat.TryFormat (_titleFormat, args); public string GetTitleFormat () => _titleFormat; } diff --git a/src/tools/illink/src/ILLink.Shared/MessageFormat.cs b/src/tools/illink/src/ILLink.Shared/MessageFormat.cs index 0e67c71b676efc..a2bae2d31afbc3 100644 --- a/src/tools/illink/src/ILLink.Shared/MessageFormat.cs +++ b/src/tools/illink/src/ILLink.Shared/MessageFormat.cs @@ -4,6 +4,8 @@ // This is needed due to NativeAOT which doesn't enable nullable globally yet #nullable enable +using System; + namespace ILLink.Shared { internal static class MessageFormat @@ -33,7 +35,24 @@ public static string FormatRequiresAttributeMismatch (bool memberHasAttribute, b (true, false) => SharedStrings.DerivedRequiresMismatchMessage }; - return string.Format (format, args); + return TryFormat (format, args); + } + + public static string TryFormat (string format, params object[] args) + { + string formattedString; + try + { + formattedString = string.Format(format, args); + } + catch (FormatException) + { +#pragma warning disable RS1035 // Do not use APIs banned for analyzers - We just need a newline + formattedString = "Internal error formatting diagnostic. Please report the issue at https://aka.ms/report-illink" + Environment.NewLine + + $"'{format}', " + $"[{string.Join(", ", args)}]"; +#pragma warning restore RS1035 // Do not use APIs banned for analyzers + } + return formattedString; } } } 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 8f11a98281eee7..373670ab2cdfdb 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(); } [Kept] @@ -97,6 +98,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", "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", "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", "unannotated", 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 () { From e8041a12e4a911d2e92551cafbaea1444f68dea8 Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Mon, 2 Dec 2024 10:43:04 -0500 Subject: [PATCH 2/3] Use nameof and proper names in diagnostics assertions --- .../Mono.Linker.Tests.Cases/DataFlow/NullableAnnotations.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 373670ab2cdfdb..5b43c1c4af4da2 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 @@ -107,7 +107,7 @@ public class RequiresOnNullableMakeGenericType [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] static Type FieldWithMethods; [Kept] - [UnexpectedWarning("IL2080", "UnannotatedField", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/93800")] + [UnexpectedWarning("IL2080", nameof(UnannotatedField), Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/93800")] static void Field() { typeof (Nullable<>).MakeGenericType (UnannotatedField).GetMethods (); @@ -115,7 +115,7 @@ static void Field() } [Kept] - [UnexpectedWarning("IL2090", "unannotated", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/93800")] + [UnexpectedWarning("IL2090", nameof(unannotated), Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/93800")] static void Parameter( Type unannotated, [KeptAttributeAttribute(typeof(DynamicallyAccessedMembersAttribute))] @@ -137,7 +137,7 @@ static void TypeParameter< } [Kept] - [UnexpectedWarning("IL2075", "unannotated", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/93800")] + [UnexpectedWarning("IL2075", nameof(GetUnannotated), Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/93800")] static void ReturnValue() { typeof (Nullable<>).MakeGenericType (GetUnannotated()).GetMethods (); From e323802d7405dbccf9f4bac7d36b4da265e06008 Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Fri, 3 Jan 2025 12:02:35 -0600 Subject: [PATCH 3/3] Revert formatting try/catch --- .../src/ILLink.Shared/DiagnosticString.cs | 4 ++-- .../illink/src/ILLink.Shared/MessageFormat.cs | 21 +------------------ 2 files changed, 3 insertions(+), 22 deletions(-) diff --git a/src/tools/illink/src/ILLink.Shared/DiagnosticString.cs b/src/tools/illink/src/ILLink.Shared/DiagnosticString.cs index b5a8cd4695c1ec..ce505e1c7727b1 100644 --- a/src/tools/illink/src/ILLink.Shared/DiagnosticString.cs +++ b/src/tools/illink/src/ILLink.Shared/DiagnosticString.cs @@ -28,12 +28,12 @@ public DiagnosticString (string diagnosticResourceStringName) } public string GetMessage (params string[] args) => - MessageFormat.TryFormat (_messageFormat, args); + string.Format (_messageFormat, args); public string GetMessageFormat () => _messageFormat; public string GetTitle (params string[] args) => - MessageFormat.TryFormat (_titleFormat, args); + string.Format (_titleFormat, args); public string GetTitleFormat () => _titleFormat; } diff --git a/src/tools/illink/src/ILLink.Shared/MessageFormat.cs b/src/tools/illink/src/ILLink.Shared/MessageFormat.cs index a2bae2d31afbc3..0e67c71b676efc 100644 --- a/src/tools/illink/src/ILLink.Shared/MessageFormat.cs +++ b/src/tools/illink/src/ILLink.Shared/MessageFormat.cs @@ -4,8 +4,6 @@ // This is needed due to NativeAOT which doesn't enable nullable globally yet #nullable enable -using System; - namespace ILLink.Shared { internal static class MessageFormat @@ -35,24 +33,7 @@ public static string FormatRequiresAttributeMismatch (bool memberHasAttribute, b (true, false) => SharedStrings.DerivedRequiresMismatchMessage }; - return TryFormat (format, args); - } - - public static string TryFormat (string format, params object[] args) - { - string formattedString; - try - { - formattedString = string.Format(format, args); - } - catch (FormatException) - { -#pragma warning disable RS1035 // Do not use APIs banned for analyzers - We just need a newline - formattedString = "Internal error formatting diagnostic. Please report the issue at https://aka.ms/report-illink" + Environment.NewLine - + $"'{format}', " + $"[{string.Join(", ", args)}]"; -#pragma warning restore RS1035 // Do not use APIs banned for analyzers - } - return formattedString; + return string.Format (format, args); } } }