dotnet / runtime

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

Structs cannot be annotated as `RequiresUnreferencedCode` #90115

Open m-redding opened 1 year ago

m-redding commented 1 year ago

Description

Structs cannot be annotated as RequiresUnreferencedCode. I'm trying to make a library AOT compatible, and we have multiple structs that are not compatible with trimming. In most cases, we can just annotate all incompatible methods with RequiresUnreferencedCode. However, we have 2 cases where we have an overload for ToString which is incompatible with trimming. Since we cannot annotate the method or struct the warning will never be resolved.

Reproduction Steps

internal struct Sample
{
        public override string ToString()
        {
                // Reflection-based serialization code not compatible with trimming
        }
}

Expected behavior

Ability to annotate structs as RequiresUnreferencedCode

Actual behavior

Adding RequiresUnreferencedCode attribute to a struct results in CS0592

Regression?

No response

Known Workarounds

For the ToString() overload case, none. For the general case, every method must be annotated.

Configuration

No response

Other information

No response

ghost commented 1 year ago

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

Issue Details
### Description Structs cannot be annotated as `RequiresUnreferencedCode`. I'm trying to make a library AOT compatible, and we have multiple structs that are not compatible with trimming. In most cases, we can just annotate all incompatible methods with `RequiresUnreferencedCode`. However, we have 2 cases where we have an overload for `ToString` which is incompatible with trimming. Since we cannot annotate the method or struct the warning will never be resolved. ### Reproduction Steps ```csharp internal struct Sample { public override string ToString() { // Reflection-based serialization code not compatible with trimming } } ``` ### Expected behavior Ability to annotate structs as `RequiresUnreferencedCode` ### Actual behavior Adding `RequiresUnreferencedCode` attribute to a struct results in CS0592 ### Regression? _No response_ ### Known Workarounds For the ToString() overload case, none. For the general case, every method must be annotated. ### Configuration _No response_ ### Other information _No response_
Author: m-redding
Assignees: -
Labels: `untriaged`, `area-Tools-ILLink`, `needs-area-label`
Milestone: -
jkotas commented 1 year ago

@vitek-karas @sbomer Is this just an oversight?

sbomer commented 1 year ago

The annotation was intentionally only supported on classes according to the reasoning here: https://github.com/dotnet/runtime/issues/50122.

If we allow this on structs, we would have to warn on instance methods too, plus it would introduce problems when unsafe instance methods get called indirectly through interfaces (the tool would not be able to see that at all).

eerhardt commented 1 year ago

How can an "unsafe" ToString on a struct be annotated correctly? You can't put RequiresUnreferencedCode on ToString(). You can't put RequiresUnreferencedCode on the struct itself. What are your other options beyond suppressing the warning?

sbomer commented 1 year ago

As far as I know, there's currently no "safe" way to annotate that, without changes to the analysis that we do. In the API review for RUC, we discussed maybe adding support for annotating structs, but it would be a new feature. To catch all cases, we'd need to decide where to warn in this example:

using System.Diagnostics.CodeAnalysis;

class Program {
    static void Main() {
        S s;
        CallInterfaceMethod(s);
    }

    static void CallInterfaceMethod(I i) => i.M();
}

interface I {
    void M();
}

struct S : I {
    public S() => throw new Exception("ctor called");

    public void M() => Helper.RUC();
}

class Helper {
    [RequiresUnreferencedCode("Helper")]
    public static void RUC() => throw new Exception("RUC called");
}

Note that the struct ctor is never called, so the warning would probably need to be about the declaration in Main.

MichalStrehovsky commented 1 year ago

On a high level, we'd need to detect when the struct is boxed, or a target of a constrained call. The analysis in IL Linker assumes that structs are boxed implicitly (the moment a struct type is reachable, we mark all the virtuals, etc.). This is a shortcut - if we could detect boxing or constrained calls, we could treat these the same as reference types. The rest are "details", but of course the devil is in them.

Alternatively, we could say that a struct annotated as RUC warns every time someone touches it in any way (use as a generic argument, make an array of it, etc.). It will produce a lot of warnings - the warnings we try to avoid by the careful treatment of RUC on classes - even in code that is not too problematic in general.

eerhardt commented 1 year ago

Connecting the dots here, the problematic code in Azure.Core is:

https://github.com/Azure/azure-sdk-for-net/blob/c4fb4c52117622c2107163ba6d3efbb20743836d/sdk/core/Azure.Core/src/DynamicData/MutableJsonChange.cs#L52-L61

This code is eventually called from MutableJsonChange's ToString().

@m-redding - In this case, since this struct is internal, I would annotate the constructor as RequiresUnreferencedCode and then suppress the warning in the ToString. The object? Value readonly property (which is the root of being incompatible with trimming here) can only be set by the constructor. So only callers using the constructor will be able to use this struct.

Following all the callsites up, the only place I see this exposed publicly is through APIs like this:

        public static dynamic ToDynamicFromJson(this BinaryData utf8Json)
        {
            DynamicDataOptions options = new DynamicDataOptions();
            return utf8Json.ToDynamicFromJson(options);
        }

Which will never be trim or AOT compatible. So we should just mark all this code as RequiresUnreferencedCode, and any Azure library that wants to be trim/AOT compatible will need to use alternate APIs.

MichalStrehovsky commented 7 months ago

Thinking about this more, I don't think it will be possible to detect all the places where a struct could become boxed. There's just too many ways, and once the struct is boxed, any virtual methods on it are callable.

I think the only reasonable way to implement this on structs would be to turn any reference to the struct into a warning - if the type is referenced from a parameter, local, field, any IL instruction - all of them will need to generate a warning. The question is whether it would still be practical in this shape.