Linker should warn when reflection-accessing annotated methods

[MichalStrehovsky]

When we have a method that is an intrinsic for flow analysis purposes, or has parameters marked as [DynamicallyAccessedMembers], or is marked [RequiresUnreferencedCode], linker should warn when we see that method being reflection accessed.

The problem this is trying to avoid is someone e.g. locating MakeGenericMethod (that currently warns if used statically) and reflection-invoking it with some parameters. The initial reflection access should be a warning since illink is not capable of figuring out the dataflow into the method. Someone could be violating the annotations, resulting in a broken app without warnings.

See https://github.com/dotnet/runtime/pull/47134/files#diff-e09e7f6880244829d1c3b16e19b451ab04d059a7df0d8a367cd3fd08230c9096R64.

We should probably extend this to delegates pointing to such methods (although that one has a natural extension - we could require the delegate invoke method itself to be annotated same as the target).

[vitek-karas]

Isn't this basically a dupe of #1607 ?

[MichalStrehovsky]

It wasn't clear to me what the scope of that issue is - the original issue talks about warning when a RequiresUnreferencedCode-annotated method somehow ends up in the final output.

I added the "methods that have DynamicallyAccessedMembers annotations" as a comment, but the original issue already had a pretty big scope and there was no reaction to that comment, so I filed a new one to make sure that aspect is not missed either. In that case we don't want to warn whenever such method ends up in the output - we specifically want to warn if we see it gets added as "visible to reflection". The place where it gets implemented might be different.

[vitek-karas]

I'm fine keeping it as separate issues.

Why do you think it's important to recognize "visible to reflection"? Let's say I have a method CallEverything([DAM(All)] Type type). And I root it in my descriptor, because it's going to be called from a place linker can't see (for example native code - mono can do this and probably does in some interop code). I think we should warn in this case - since linker can't reason about its usage and thus it's potentially dangerous.

[MichalStrehovsky]

Would we warn if such method is in a copy or copyused assembly? We don't know why someone made it copy or copyused - it's possible they did it because there will be native code reflection-invoking everything in the assembly. Or maybe they just did it because they are doing some reflection and they can't be bothered to specify what exactly they reflect on.

I don't see copy/copyused as different from rooting a type with all members from ILLinkTrim, or rooting just a specific method from ILLinkTrim.

Linker is not in a position to reason about "reflection" happening from native code. I would put it into the "not our problem" bin. Linker doesn't know it's even happening in the first place (we could issue a warning otherwise).

If this is actual "regular" reflection that the user couldn't be bothered to exactly specify, users are already suppressing a warning somewhere. Since they're suppressing warnings, things might not work. The reason why they won't work is either that the member is missing, or the member had annotations that illink can't validate at runtime. But there is a warning, so this is not a illink bug.

[vitek-karas]

The copy and copyused is a good point - that would be messy.
So basically we need to figure out that "we're marking this because of reflection" and only then raise the warning...

[MichalStrehovsky]

I think these all go through MarkIndirectlyCalledMethod so maybe that's a good choke point although it might need tweaks so that we can report good warnings. Once #1682 is fixed this will naturally get extended to delegates to these methods which is great.

[vitek-karas]

I think for methods we will have to use the same logic we currently use for RequiresUnreferencedCode attribute: https://github.com/mono/linker/blob/c739a81ba553b00df1cb2f5b9974deae996b757a/src/linker/Linker.Steps/MarkStep.cs#L2697

It doesn't use MarkIndirectlyCalledMethod currently as it needs to react to more cases. I think it would be good to do the same for DAM for consistency (they should behave similarly).

For fields it makes sense to use MarkFieldVisibleToReflection as the choke point.

There are couple of "interesting" questions/behaviors:

• Reading from a field is not a dangerous operation, but currently linker won't be able to tell if the reflection access is for read or write - so it will warn even if accessing the field via reflection for reading.
• Should we warn if only the return value of a method is annotated? Calling a method with annotated return value is not dangerous. What could be dangerous is using reflection emit to generate a method like that. On its own that would not matter, unless the reflection emit is used to override a base method with annotation on return value. Warning on the reflection access to the base method would "cover" this.