Does Capability attributes need full reachability analysis?

[tlakollo]

We introduced capability attribute support in order to flag code members as requiring certain capability in code. Once the annotations are in code, we can enable the analysis of the capability attributes and provide feedback about the usage of members considering if the capability is met or not. That is, if the member is being accessed, and is marked as requiring a certain capability and that capability is not met we would rise a warning to the user.
For example, the following:

public class Program
{
    public static void Main ()
    {
        CallMethod ();
    }

    // AOT analysis warning IL3050: Mono.Linker.Tests.Cases.Repro.Program.Main(): Using member 'Program.CallMethod()' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. This method is dangerous for dynamic code.
    [RequiresDynamicCode("This method is dangerous for dynamic code")]
    public static void CallMethod ()
    { }
}

The code if executed would generate a warning representing that the analysis does not meet the requirement for certain tools like NativeAOT which doesn't support dynamic code, and therefore cannot fulfill the requirement expressed in the attribute.

As mentioned before, in order to generate the warning we verify if the capability is met on the member being accessed, therefore the analysis centers on trying to statically understand if a member will be accessed at runtime. There are basically two ways to make the reachability analysis, only understanding simple direct calls to methods and doing full reachability analysis which also tries to understand access via methods like reflection.
See the following example:

public class Program
{
    public static void Main ()
    {
        var method = typeof (Program).GetMethod ("CallMethod");
        method.Invoke (null, new object[] { });
    }

    [RequiresDynamicCode("This method is dangerous for dynamic code")]
    public static void CallMethod () { }
}

In this case, the way of accessing the method is not visible as a simple call, it's using the reflection APIs which hide the fact that we are about to call the CallMethod() method. With a more complex static analysis, we can infer that CallMethod will be used since we have all the information available in code. After executing this analysis we can generate the warning telling the user that they are about to use a member that dont fulfill the capability requirement.
Full reachability analysis is a requirement for trimming applications, so it's something that has been developed and even implemented for some of the RequiresCapability attributes, for example, the RequiresUnreferencedCode attribute. Running the previous example code in the dotnet/linker codebase using the RequiresUnreferencedCode attribute instead of RequiresDynamicCode will correctly generate a warning to the user.

public class Program
{
    public static void Main ()
    {
        var method = typeof (RequiresOnMethodAccessViaReflection).GetMethod ("CallMethod");
        method.Invoke (null, new object[] { });
    }

    // Trim analysis warning IL2026: Program.Main(): Using member 'Program.CallMethod()' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. This method is dangerous for trimming.
    [RequiresUnreferencedCode ("This method is dangerous for trimming")]
    public static void CallMethod () { }
}

RequiresUnreferencedCode attribute not only does care about reflection calls but also interacts with other attributes like DynamicallyAccessedMembers to help to be able to achieve full reachability analysis by helping on cross-method analysis. More info on reflection-flow document

The point of this discussion is to talk about if the full reachability analysis should apply to all Capability attributes, from my standpoint without full reachability analysis, we are leaving holes that later on could result in a broken app at runtime without any warning to the user. As per previous discussions, most of the concerns about doing full reachability analysis on these attributes is what is going to be the warning when the analysis cannot predict what the behavior of the application will be at runtime and if the user should even be bothered of obtaining those warnings if they are not doing something like trimming.
For example:

public class Program
{
    public static void Main ()
    {
        var memberString = File.ReadLines("MyFile.txt").First();
        var method = typeof (Program).GetMethod (memberString);
        method.Invoke (null, new object[] { });
    }

    public static void MyFileKeptMethod() { }
}

In this case, the full reachability analysis would have no option but to warn since the analysis cannot know which member is going to be kept by the GetMethod() method. Is it a member that contains RequiresDynamicCode or a member that contains RequiresAssemblyFiles? is impossible to know, but now the user will be bothered about it. Something that wouldn't happen if not in full reachability analysis.

[tlakollo]

cc @dotnet/ilc-contrib

[sbomer]

Related: dotnet/linker#2739

For the linker and trim analysis we had a goal that the published application behave the same as it does during dotnet run, and we defined feature switches that get set with <PublishTrimmed>true</PublishTrimmed> to make unsupported APIs behave consistently.

Do we want to have the same goal for PublishAot and PublishSingleFile - that is, define feature switches that will cause the unsupported APIs to throw (or return null, etc.), whether or not the app was published? If we rely on feature switches to make the build/publish behavior consistent, then maybe we don't need to warn for these.

On the other hand if we let the RequiresFeature APIs work on dotnet run but fail when running a published app, it would be useful to provide warnings for these. We probably wouldn't want to do whole-program analysis for PublishSingleFile though, so the single-file experience would be worse than trimming or nativeaot.

[tlakollo]

Do we want to have the same goal for PublishAot and PublishSingleFile - that is, define feature switches that will cause the unsupported APIs to throw (or return null, etc.), whether or not called from a published aot'd or single-file app? If we rely on feature switches to make the build/publish behavior consistent, then maybe we don't need to warn for these.

NativeAOT already supports feature switches (at least is able to read a substitution file and apply the feature switches values) and even some of the RequiresCapability attributes have their own mechanism to restrict code execution using feature switches like properties, for example, the RuntimeFeature.IsDynamicCodeSupported Property.

We probably wouldn't want to do whole-program analysis for PublishSingleFile though, so the single-file experience would be worse than trimming or nativeaot.

I think that scenario is kind of what is being discussed in #72496 (comment), we currently are thinking of warning for RDC because currently the only tool supporting it is NativeAOT which also implies trimming. The reality is that currently none of the other RequiresCapability attributes (apart from RUC) imply trimming, but they all can be reflected on and break the app.

[vitek-karas]

Ignoring if we want to do this or not, there's a design problem here. We can't reason about reflection without trim analysis. And trim analysis only really works if there are no trim warnings, otherwise the tool is effectively guessing. So to provide the same level of guarantee we do for trimming we would have to process and report all trim warnings as well. So the experience would be: "I'm trying to make a single-file app, I don't want to trim, but the tooling is generating 100s of warnings about feature I'm not interested in.".

So I think that without trim analysis/warnings everything we do is best effort since we can't guarantee full understanding of the app. So this basically means there are two questions to answer:

• For the case without trim analysis/warnings: What kind of best effort we want to do (and where). Currently we only analyze directly reachable code, without reflection. Maybe it's good enough.
• For the case where we already do trim analysis/warnings: Do we want to provide better analysis/guarantees even about features not directly related to trimming, like single-file or AOT?

Personally I don't know what we should do... maybe we should keep it simple for now (so keep existing analyzers as-is and don't complicate linker/AOT with things like reflection access to RAF methods).

[sbomer]

For the case where we already do trim analysis/warnings: Do we want to provide better analysis/guarantees even about features not directly related to trimming, like single-file or AOT?

I'd prefer not to. I would rather design a solution that works consistently (independently of trimming) for the single-file or AOT attributes. If somebody reflects over single-file APIs and gets unexpected behavior after publishing, I don't want our answer to be "turn on trimming" - it would be better if we could give a predictable experience for single-file in the first place.

[MichalStrehovsky]

I'd keep things simple and keep reflection analysis together with trimming analysis. People generally don't expect static analysis to cover reflection. We do this for trimming because it was relatively cheap to do and we need it to protect invariants that the trimming optimization is based on (the invariant is "we know all callsites"). C# nullable analysis doesn't cover reflection; platform analyzer doesn't cover reflection. I haven't heard people complaining/not understanding it.

[vitek-karas]

The only counterexamples where it might come handy are around MakeGeneric... but it's probably OK to not do this as it's rare.