Trim analyzer: Doesn't produce warnings due to annotated generic parameter in various situations

[vitek-karas]

Method<string>();

Type Method<TUnknown>()
{
    // Should WARN - TUnknown does have All annotation
    return typeof(TRequires<TUnknown>);
}

class TRequires<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>
{
}

The able produces IL2091 in the trimmer, but doesn't produce it in the analyzer. The bug seems to be that we don't process generic parameter annotations in a typeof.

Also happens for a declaration of a local variable:

void Method<TUnknown>()
{
    // Trimmer produces IL2091 here, analyzer doesn't
    TRequires<TUnknown> a;
}

Note that for local variables NativeAOT doesn't produce the warning either - see the comment here for why:

runtime/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/GenericParameterWarningLocation.cs

Lines 12 to 28 in 7f81848

	// NativeAOT differences in behavior:
	//
	// Validation of generic parameters only matters if the instantiation can be used to run code with the substituted type.
	// So for generic methods the validation has to happen basically always (since any access to the method can lead to the code
	// of the method executing eventually).
	// For generic types though the situation is different. Code on the type can only run if the type is instantiated (new)
	// or if static members are accessed on it (method calls, or fields accesses both can lead to static .cctor execution).
	// Others usages of the type cannot themselves lead to code execution in the type, and thus don't need to be validated.
	// Currently linker and analyzer both validate every time there's a type occurrence in the code.
	// NativeAOT on the other hand only validates the cases which can lead to code execution (this is partially because the compiler
	// doesn't care about the type in other situations really).
	// So for example local variables of a given type, or method parameters of that type alone will not cause code execution
	// inside that type and thus won't be validated by NativeAOT compiler.
	//
	// Below this explanation/fact is referred to as "NativeAOT_StorageSpaceType"
	// Storage space - declaring a storage space as having a specific type doesn't in itself do anything with that type as per
	// the above description.

We should figure out if analyzer needs to produce the warning or not.

Another situation is a static field access:

    TRequires<TUnknown>.Field;  // Should warn

Yet another situation - expression trees:

Expression<Func<string>> a = () => TRequires<TUnknown>.Field

And then type checks (NativeAOT doesn't warn here, see the link above):

if (value is TRequires<TUnknown>) {}

var a = value as TRequires<TUnknown>;

Catch and filter expressions (NativeAOT doesn't warn):

try {}
catch (TRequires<TUnknown>) {}
catch (Exception ex) when (ex is TRequires<TUnknown>) {}

Nested generic for methods:

GenericMethod<Generic1<Generic2<TRequires<TUnknown>>>();

Similarly nested generics don't cause DAM marking and thus diagnostics:

GenericMethod<Generic1<Generic2<TRequires<TypeWIthRUCMethod>>>(); // Should produce IL2026

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

Issue Details

---

Method<string>();

Type Method<TUnknown>()
{
    // Should WARN - TUnknown does have All annotation
    return typeof(TRequires<TUnknown>);
}

class TRequires<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>
{
}

The able produces IL2091 in the trimmer, but doesn't produce it in the analyzer. The bug seems to be that we don't process generic parameter annotations in a typeof.

Author:	vitek-karas
Assignees:	-
Labels:	area-Tools-ILLink
Milestone:	-

[sbomer]

Thanks a lot for writing this down!

• typeof: I'd like to nail down the model we want to follow for NativeAot and trimming and do the same in the analyzer.

  Trimming warns because the generic parameters are processed as part of MarkType, which is called for typeof - basically I think it's an implementation detail.
  It's not clear to me why NativeAot warns for typeof since my impression is that we try to limit warnings to patterns that have "unsafe" runtime behavior. Is that also an implementation detail?

• local variables: I don't see why the analyzer needs to warn, for the same reason that NativeAot doesn't warn. Unless something creates an instance of the type I can't think of a problem.

• type checks and filter expressions: again, I think we should follow the NativeAot behavior.

The remaining cases look like bugs we need to fix.

I also have one more for the list:

• Warning on generics in method parameters and return type for reflectable methods.

  Trimming and analyzer currently warn whether or not the method is reflectable, but NativeAot specifically only warns for these on reflectable methods, as a workaround for an incorrect suppression in DI: NativeAOT Data flow annotations are not applied to DI created generic types #81358. If we move generic validation into the analyzer's dataflow analysis this will no longer warn.

[vitek-karas]

Good point - If we can get the same behavior as NativeAOT I think that would be great (it warns less). Also if we can clearly define the way to do that in the type system (NativeAOT implementation kind of fell out of the way the compiler works), we could apply it to trimmer.

I don't know the typeof behavior right now - the thing about typeof is that it's considered a "reflection access" in NativeAOT - so maybe that's the reason. I would have to dig into that. It could be an implementation detail. In general I agree that I don't see why it would need to warn - having the type accessible doesn't run code on it (yet). Accessing statics of instantiating it would do that - but we should see all of that when it's done via reflection anyway. But it's possible this was one of the "this just doesn't work well for real world if don't do it" (like the DI thing).