NativeAOT difference in behavior to ILLink with descriptor preserved code and constant propagation

[vitek-karas]

This is basically an issue to make NativeAOT pass the test CompilerGeneratedCodeInPreservedAssembly

The scenario is an assembly which is entirely preserved via a descriptor XML and it contains unreachable code in constant condition branch which is compiler generated and triggers a data flow warning. This should not produce a warning, but it still does.

		public static void WithLocalFunction ()
		{
			if (AlwaysFalse) {
				LocalWithWarning ();
			}

			[ExpectedWarning ("IL2026", ProducedBy = Tool.Analyzer | Tool.NativeAot)] // Bug
			void LocalWithWarning ()
			{
				Requires ();
			}
		}

[MichalStrehovsky]

Why this one doesn't generate a warning with IL Link? Do we trim the method even though the user said they're going to reflect on the entire assembly?

[vitek-karas]

In ILLink we only process compiler generated (cg) methods as part of the closure of the user defined method. We don't run dataflow on them separately (this is the reason we have the "reflection access to cg method" warnings, which we recently decided to not produce, and that's why it's a hole).

So in this case, we should actually never run dataflow on the LocalWithWarning because its only reference should be removed by const-prop/branch-removal. I don't know why this happens in NativeAOT yet...

[MichalStrehovsky]

Doesn't preservation via XML descriptor mean "consider this to be reflection-accessed"?

If ILLink believed LocalWithWarning is reflection-accessed, it should trigger the warnings since ILLink specifically wants to generate warnings when compiler generated stuff is reflection-accessed because it considers it a legit scenario that we should warn about. It feels like the IL Link behavior is the bug. Am I missing something?

[vitek-karas]

You're not missing anything really - it's a mess :-)

ILLink is full of weird behaviors like this (history), which we should probably fix. In this case I personally don't know what the desired behavior should be.
On one hand you're right that the local function is "reflection accessed" because it's part of the descriptor, and as such we should see if it's potentially problematic and warn. (per current linker behavior, which will likely change).
On the other hand, this code looks perfectly fine for normal (full) trimming, since const-prop/branch-removal will remove the call and thus references to the local function. But it will suddenly become a problem if somebody roots the entire assembly (for example via a descriptor). That is weird... and we generally try to avoid that - that's one of the reasons why we don't warn if a RUC method is rooted due to a descriptor.

But I guess the "correct" way to handle this is to put a RUC onto the local function - the full trim behavior will be the same, and rooting will not warn either.

[MichalStrehovsky]

Makes sense - I read this issue as wanting to replicate the ILLink behavior, but the fix could also be an ILLink change or test change.

On the other hand, this code looks perfectly fine for normal (full) trimming, since const-prop/branch-removal will remove the call and thus references to the local function. But it will suddenly become a problem if somebody roots the entire assembly (for example via a descriptor). That is weird...

On the other hand, this is the behavior for non-compiler-generated code: if it's trimmed, there's no warning, but if it's a target of reflection (and kept because of that), we do warn.

[vitek-karas]

The more I think about this, the more I lean towards illink being wrong here. Another reason is the analyzer - it also warns in this case, because it doesn't treat local functions "special" - nor it should. And anlyzer is a good indication of what should happen for rooted assemblies - since it also effectively roots all code.
(and there's also the consistency argument that analyzer and illink should agree on which warnings to produce).

[ghost]

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

Issue Details

---

This is basically an issue to make NativeAOT pass the test CompilerGeneratedCodeInPreservedAssembly

The scenario is an assembly which is entirely preserved via a descriptor XML and it contains unreachable code in constant condition branch which is compiler generated and triggers a data flow warning. This should not produce a warning, but it still does.

		public static void WithLocalFunction ()
		{
			if (AlwaysFalse) {
				LocalWithWarning ();
			}

			[ExpectedWarning ("IL2026", ProducedBy = Tool.Analyzer | Tool.NativeAot)] // Bug
			void LocalWithWarning ()
			{
				Requires ();
			}
		}

Author:	vitek-karas
Assignees:	-
Labels:	untriaged, area-NativeAOT-coreclr, area-Tools-ILLink
Milestone:	-

[sbomer]

Here's an example that might help us decide on the behavior:

    static void UserCode() {
        var all = GetAll();

        if (AlwaysFalse)
            LocalFunction();

        void LocalFunction() {
            RequireAll(all);
        }
    }

    [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
    static Type GetAll() => typeof(int);

    static void RequireAll([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type t) {}

Currently, for compiler-generated code we only do dataflow analysis within the context of the calling user code. If we analyze this as if LocalFunction is reachable from reflection, technically the RequireAll callsite should warn because the reflection access doesn't necessarily guarantee the All requirements are met.

Since we decided not to warn on reflection access to compiler-generated code, it seems ok for the same reason to not produce dataflow warnings from compiler-generated code that's only accessed through reflection. And if it is accessed directly (from user code), then we can analyze it with the right context from that callsite.

I take that as an argument that the ILLink behavior is correct. But another option is: whenever compiler-generated code is accessed through reflection, analyze using the context from its reference in user code, without taking constant propagation into account.

[MichalStrehovsky]

Given that we now consider reflection-accessing compiler generated members undefined behavior, it sounds good to me not to warn and take ILLink behavior.

I think the only reason why ILC warns is that the constant propagation doesn't actually happen - ILC will not attempt to inline const-returning methods unless they are feature switches. If we adjust the test in a way that constant propagation happens with ILC too, the test will probably pass, I don't think we have differences here.

[sbomer]

Thanks, indeed it looks like the difference in the testcase was just due to not seeing a constant.

The next question is whether the analyzer should match this behavior too. For ILC/ILLink it falls out of dead code elimination, which the analyzer doesn't do. But for local functions that are only called behind feature guards, the analyzer could conceivably take the feature guards into account. I don't think it's worth doing this so I would consider this fixed by #105042.