RequiresDynamicCode analyzer should understand RuntimeFeature.IsDynamicCodeSupported

[MichalStrehovsky]

Even though M1 below calls a method annotated RequiresDynamicCode, it does so in a code path that is not reachable at runtime when dynamic code is not supported. We should not generate a warning.

static void M1()
{
    if (RuntimeFeature.IsDynamicCodeSupported)
    {
        M2();
    }
}

[RequiresDynamicCode("...")]
static void M2()
{
}

Cc @tlakollo

[sbomer]

Related is a similar issue for feature switches in the linker - there it's a problem both for the trim analyzer, and when trimming libraries in "library mode" where the feature switches aren't baked in. This is the issue I mentioned in #2004 (comment).

On the linker side I think this is +1 to the refcount for moving feature switch definitions out of XML. Otherwise we would need to add support to the analyzer for reading XML, which we have been trying so hard to avoid.

[MichalStrehovsky]

Yep, this could be a special case of feature switch support.

I think a problem that we'll run into as we're trying to generalize this is the constant propagation across methods that illink is doing - the constant propagation affects the analysis that happens within illink, but it's not something we can fully emulate in the analyzer - we can do at most one level.

I've had mixed feelings about the multi-layer constant propagation - one one hand I understand the problem constant propagation is trying to solve, but on the other hand I wish we instead worked on refactorings within framework so that we don't need to constant propagate across more than one level (e.g. if someone wraps RuntimeFeature.IsDynamicCodeSupported in a wrapper method and then calls that wrapper and expects the bool to get propagated across 2 methods).

[marek-safar]

I wish we instead worked on refactorings within framework so that we don't need to constant propagate across more than one level

I don't think that would solve the problem either. I'm not sure if with the existing analyzers design we can solve this effectively because the analyzer works with assembly references hence have a minimal understanding of what external dependencies do.

[MichalStrehovsky]

I wish we instead worked on refactorings within framework so that we don't need to constant propagate across more than one level

I don't think that would solve the problem either. I'm not sure if with the existing analyzers design we can solve this effectively because the analyzer works with assembly references hence have a minimal understanding of what external dependencies do.

It would require a combination of what Sven wrote (feature switches encoded as custom attributes) + only doing one level of constant propagation. We would then see that RuntimeFeature.IsDynamicCodeSupported is a constant even from a ref assembly and be able to handle it as such in the analyzer.

[eerhardt]

Should the same rule apply to RuntimeFeature.IsDynamicCodeCompiled?

I assume I shouldn't get an error there because using the "AOT-unfriendly" API is behind a RuntimeFeature.IsDynamicCodeCompiled switch.

[MichalStrehovsky]

Yeah, IsDynamicCodeCompiled would never be true if IsDynamicCodeSupported is false, so this should also be treated as unreachable code in full-AOT-with-no-fallbacks modes.

[agocke]

Sounds like this is something worth solving in .NET 8. I'd like to do the "extended" feature where we basically allow this to be part of a Requires attribute definition and it's all well understood and supported in the code, but I think special-casing would also be fine for now.

[vitek-karas]

Depending on how much we want this to work this will require:

• Constant expression eval (we already have some bits of this, but we'll need more) - note that we can't rely on Roslyn to do this for us since we will turn a property getter into a constant expression (so it's not a constant from Roslyn's point of view, and AFAIK we can't create a second copy of the expression tree from Roslyn and make it work with existing constant prop)
• Ability to avoid analyzing branches of code

Eventually it would be really nice to support

• Feature switches - since they're basically the same thing, just more generalized
• Constant propagation - since feature switches rely on the ability to propagate the const over multiple returns (for example EventSource feature switch is like that)
• Probably some more constant expression eval/data flow

This also probably means that we should move the "Requires" analysis into the data flow analyzer - since we'll need CFGs for this and we already have them there. We also want to avoid analyzing the dead branches with data flow anyway, so we need to unite the two.

[MichalStrehovsky]

Can we just do something dumb as we walk the AST and check if the condition on the if and ? : is RuntimeFeature.IsDynamicCodeXXX and stop walking the part of the AST that is unreachable? It can be documented as the only supported thing and we could move on with life, looking at feedback.

The constant propagation rules are different between what IL Linker constprop is doing and the engine we have in dataflow. Unifying those would be another necessary undertaking (not to mention NativeAOT also has a constprop, and that does even fewer things because I went with 20% of effort for 80% of result).

I think a minimal implementation would still go a long way and cover majority of use cases. Yep, one would have to add an else branch for code like this: https://github.com/dotnet/runtime/blob/b34029154a1da85e1ce37471bd93eb25b015b0db/src/libraries/System.Data.Common/src/System/Data/DataRowExtensions.cs#L151-L154. But it's not the end of the world.

[vitek-karas]

I'd be perfectly happy to start small on this. There are some things though which we can't really avoid:

• Moving some of the Requires analysis into the data flow engine - mainly because currently some of this is based on syntax analysis, which is not the right level to decide about conditions. And data flow already deals with CFG and intrinsics, so it would be much easier to add there. We already do this mostly for RUC, so it's not super complex.
• Improve the CFG analysis to be able to eliminate branches - this is probably not too difficult either, we just don't process the basic blocks in the eliminated branch.
• Very basic constant propagation (not sure how much we need, but for the typical case it's probably like 10 lines of code)

[agocke]

I like the idea of only supporting feature switches in an if check, if we won't have big back-compat problems.

I think the predictability would be nice. However, I think it would be better if we could use the analyzer to warn if the check is not in an if switch.

[marek-safar]

Btw, the same if condition with && and || conditions is supported by Platform compatibility analyzer and I expect the analyzer logic to be very much same.

[MichalStrehovsky]

Very basic constant propagation (not sure how much we need, but for the typical case it's probably like 10 lines of code)

This is the place that I want to be very careful about. There's a difference between what dataflow analysis can model and what consprop (in IL linker or in NativeAOT) can do.

For example for this:

if (IntPtr.Size == 3)
    Console.WriteLine("Hi");

int size = IntPtr.Size;
Type t = typeof(List<>);

try
{
  new object();
}
catch (Exception)
{
}

if (size == 3)
    Console.WriteLine("There");

t.GetMethod("Add");

Illink's consprop eliminates the first if as unreachable, but not the second. Dataflow analysis on the other hand doesn't complain about the GetMethod call because it can easily track through the local.

Add to that that NativeAOT does even less consprop work, because the north star there is for this to be handled by RyuJIT and there's no point trying to write a better C# logic when RyuJIT can already do it, and a lot better.

Analyzer and the tool used by publish differing in their behaviors is a source of pain, especially for library developers (who may only run the analyzer). If the analyzer is too smart, we end up with tools producing warnings.