dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.95k stars 4.65k forks source link

ILCompiler doesn't resolve type names for attributes on fields/properties #92259

Closed sbomer closed 6 months ago

sbomer commented 12 months ago

Using a type name string as an attribute property that has DynamicallyAccessedMembers requirements works when the attribute is on a method, but not on a field/property:

using System;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;

class Program {
    [KeepsPublicMethods(TypeName = "ClassWithKeptPublicMethods")] // IL2105 ('ClassWithKeptPublicMethods' was not found)
    public static int field;

    [KeepsPublicMethods(TypeName = "ClassWithKeptPublicMethods")] // IL2026 (KeptMethod has RUC)
    public static void Method() {
        typeof (Program).GetMethod("Method")?.GetCustomAttribute(typeof (KeepsPublicMethodsAttribute));
    }

    [KeepsPublicMethods(TypeName = "ClassWithKeptPublicMethods")] // IL2105 ('ClassWithKeptPublicMethods' was not found)
    public static int Property { get; set; }

    public static void Main() {
        field = 0;
        var a = typeof (Program).GetField("field")?.GetCustomAttribute(typeof (KeepsPublicMethodsAttribute));
        a = typeof (Program).GetProperty("Property")?.GetCustomAttribute(typeof (KeepsPublicMethodsAttribute));
        a = typeof (Program).GetMethod("Method")?.GetCustomAttribute(typeof (KeepsPublicMethodsAttribute));
    }
}

class KeepsPublicMethodsAttribute : Attribute {
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
    public string? TypeName;

    public KeepsPublicMethodsAttribute() {
    }
}

class ClassWithKeptPublicMethods {
    [RequiresUnreferencedCode("Message")]
    public static void KeptMethod() {}
    static void Method() {}
}

Not sure if I'm missing something, but this doesn't seem right.

ghost commented 12 months ago

Tagging subscribers to this area: @dotnet/area-system-reflection See info in area-owners.md if you want to be subscribed.

Issue Details
Using a type name string as an attribute property that has DynamicallyAccessedMembers requirements works when the attribute is on a method, but not on a field/property: ```csharp using System; using System.Diagnostics.CodeAnalysis; using System.Reflection; class Program { [KeepsPublicMethods(TypeName = "ClassWithKeptPublicMethods")] // IL2105 ('ClassWithKeptPublicMethods' was not found) public static int field; [KeepsPublicMethods(TypeName = "ClassWithKeptPublicMethods")] // IL2026 (KeptMethod has RUC) public static void Method() { typeof (Program).GetMethod("Method")?.GetCustomAttribute(typeof (KeepsPublicMethodsAttribute)); } [KeepsPublicMethods(TypeName = "ClassWithKeptPublicMethods")] // IL2105 ('ClassWithKeptPublicMethods' was not found) public static int Property { get; set; } public static void Main() { field = 0; var a = typeof (Program).GetField("field")?.GetCustomAttribute(typeof (KeepsPublicMethodsAttribute)); a = typeof (Program).GetProperty("Property")?.GetCustomAttribute(typeof (KeepsPublicMethodsAttribute)); a = typeof (Program).GetMethod("Method")?.GetCustomAttribute(typeof (KeepsPublicMethodsAttribute)); } } class KeepsPublicMethodsAttribute : Attribute { [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] public string? TypeName; public KeepsPublicMethodsAttribute() { } } class ClassWithKeptPublicMethods { [RequiresUnreferencedCode("Message")] public static void KeptMethod() {} static void Method() {} } ``` Not sure if I'm missing something, but this doesn't seem right.
Author: sbomer
Assignees: -
Labels: `area-System.Reflection`
Milestone: -
vitek-karas commented 12 months ago

I'll take a look

ghost commented 12 months ago

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas See info in area-owners.md if you want to be subscribed.

Issue Details
Using a type name string as an attribute property that has DynamicallyAccessedMembers requirements works when the attribute is on a method, but not on a field/property: ```csharp using System; using System.Diagnostics.CodeAnalysis; using System.Reflection; class Program { [KeepsPublicMethods(TypeName = "ClassWithKeptPublicMethods")] // IL2105 ('ClassWithKeptPublicMethods' was not found) public static int field; [KeepsPublicMethods(TypeName = "ClassWithKeptPublicMethods")] // IL2026 (KeptMethod has RUC) public static void Method() { typeof (Program).GetMethod("Method")?.GetCustomAttribute(typeof (KeepsPublicMethodsAttribute)); } [KeepsPublicMethods(TypeName = "ClassWithKeptPublicMethods")] // IL2105 ('ClassWithKeptPublicMethods' was not found) public static int Property { get; set; } public static void Main() { field = 0; var a = typeof (Program).GetField("field")?.GetCustomAttribute(typeof (KeepsPublicMethodsAttribute)); a = typeof (Program).GetProperty("Property")?.GetCustomAttribute(typeof (KeepsPublicMethodsAttribute)); a = typeof (Program).GetMethod("Method")?.GetCustomAttribute(typeof (KeepsPublicMethodsAttribute)); } } class KeepsPublicMethodsAttribute : Attribute { [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] public string? TypeName; public KeepsPublicMethodsAttribute() { } } class ClassWithKeptPublicMethods { [RequiresUnreferencedCode("Message")] public static void KeptMethod() {} static void Method() {} } ``` Not sure if I'm missing something, but this doesn't seem right.
Author: sbomer
Assignees: -
Labels: `area-System.Reflection`, `untriaged`, `area-NativeAOT-coreclr`
Milestone: -
vitek-karas commented 12 months ago

The fix is to change this line: https://github.com/dotnet/runtime/blob/e4df4329df8e78c21530ae1f4af32dd87e950825/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs#L87C29-L87C29

- ModuleDesc? callingModule = ((diagnosticContext.Origin.MemberDefinition as MethodDesc)?.OwningType as MetadataType)?.Module;
+ ModuleDesc? callingModule = (diagnosticContext.Origin.MemberDefinition.GetOwningType() as MetadataType)?.Module;

I won't try to create a PR as the tests will collide with https://github.com/dotnet/runtime/pull/92166. So it's better to wait for that to merge.