Remove generated locals in RegexCompiler.LoadSearchValues

[stephentoub]

Rely instead on improvements to JIT's recognition of Unsafe.As.

Depends on #85954
cc: @EgorBo, @MihaZupan

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Rely instead of improvements to JIT's recognition of Unsafe.As.

Depends on #85954
cc: @EgorBo, @MihaZupan

Author:	stephentoub
Assignees:	stephentoub
Labels:	area-System.Text.RegularExpressions
Milestone:	-

[eerhardt]

Suggested change

	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2060:MakeGenericMethod", Justification =
	"Only uses Unsafe.As<T> with reference types <T>, which it also uses in this method to ensure it's rooted. Unsafe.As is an intrinsic with a nop " +
	"implementation supplied at codegen time. And this is also part of the regex compiler that's only used when dynamic code compilation is supported.")]
	static MethodInfo MakeUnsafeAs(Type type)
	{
	Debug.Assert(!type.IsValueType);
	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2060:MakeGenericMethod", Justification =
	"Calling Unsafe.As<T> is safe since the T doesn't have trimming annotations.")]
	static MethodInfo MakeUnsafeAs(Type type)
	{

[stephentoub]

I thought this was warning that it doesn't know whether it'll have the code for Unsafe.As<Foo> for a given Foo. What warning is that? My justification was based on assuming that's what it's warning about.

[eerhardt]

In the other reply (#86010 (comment)) I explained what the current warning means. (I added this comment so I could suggest the justification message and remove the Assert). The current warning is a "trimming IL" warning, not an AOT warning. All "Trimming" warnings start with IL2xxx while AOT warnings start with IL3xxx

The warning you get for Native AOT when using MakeGenericType/Method is IL3050 - https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot/warnings/il3050

Note: you aren't getting AOT warnings in this code because the whole class is annotated with [RequiresDynamicCode], which disables all AOT warnings in the class, and moves the warnings to the callers.

[stephentoub]

The current warning is a "trimming IL" warning

Ok. I see the details now in https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/trim-warnings/il2060.

[RequiresDynamicCode]

It's strange to me that we lump MakeGenericMethod in as part of RequiresDynamicCodeAttribute. There's a meaningful difference between something that uses reflection emit and must be able to compile (or interpret) at execution time, and something that just needs the code to be available. MakeGenericMethod doesn't require dynamic code if you only use instantiations that are known to already be available. The JIT can create such code on-demand, but in an AOT model where all the code is created ahead of time, it's not that MakeGenericMethod requires dynamic code, it's that it requires the code to have already been compiled, and it fails if it wasn't.

[eerhardt]

It's strange to me that we lump MakeGenericMethod in as part of RequiresDynamicCodeAttribute.

The way I think of it is that MakeGenericMethod has correctness concerns for both trimming and when not being able to JIT code (i.e. Native AOT). So when you use it, you will typically get 2 warnings - one for trimming (IL2060) and one for AOT (IL3050). The IL3050 warning is automatically suppressed here because this code already says it requires dynamic code, so there's no point in warning about it.

But the trimming warning IL2060 is still emitted because this code can be used in a trimmed app. Even if we never did Native AOT, you'd still get the warning - but like I said in the other thread, I think it may be a bug in the trimmer here because it should be able to statically see that there are no trimming annotations on Unsafe.As.

[vitek-karas]

I'll let @MichalStrehovsky correct me, but I think MakeGenericMethod is AOT incompatible because it might need runtime code gen. If all of the generic arguments are reference types, then it should not need code gen, but if one of them is a value type, it needs to have the code for it pre-generated. Kind of similar to List<string> and List<object> will share the same code and thus you can at runtime create a new List<MyRefType>. But List<int> and List<bool> don't share code, so you can't create a List<double> at runtime (unless it was compiled into the app).
To my understanding that is the reason why MakeGenericMethod can produce IL3050.

That is all assuming it can statically figure out which generic method is being instantiated, if it can't even tell that, then it should warn always.

[stephentoub]

Yes, understood. Maybe I'm splitting hairs here, but to me "requires dynamic code" suggests it will always require dynamic code, and that's not the case. If you have SomeMethod<T> and you use MakeGenericMethod(typeof(double)), that should work just fine as long as your app includes the code for SomeMethod<double>. This is really [NeedsPrecompiledCode], such that it will fail if such code isn't available.

[vitek-karas]

That's true (I will restrain from discussing the attribute name ;-)). In theory the compiler could figure lot of this out and warn much less (it can know that it generated code for SomeMethod<double>, so it doesn't need to warn if it sees it being constructed via MakeGenericMethod). I'm definitely open to suggestions on which parts of this are more important to improve on (make the compiler more clever).

There's a question of how much we want to make the compiler clever in general though since the analyzer will not be able to do the same - and then deciding what is the right balance between diagnostics from the analyzer and the compiler.

[MichalStrehovsky]

The RequiresDynamicCode work similar to RequiresUnreferencedCode. Both of them can work under some circumstances and that's why it's sometimes possible to suppress the warning.

The fact that RDC on MakeGenericType is sometimes suppressible and RDC on Ref.Emit is never suppressible is a thing that could change. E.g. if there's enough of a business justification, we could recognize that some Ref.Emit uses are just about building "fast field accessors" and intrinsically convert them to the fast field accessors we're planning to add. Then such Ref.Emit use is also technically safe and suppressible.

The attribute really only captures that "unless otherwise specified, this will require runtime code generation".

[stephentoub]

The attribute really only captures that "unless otherwise specified, this will require runtime code generation".

That's what I'm struggling with, from a naming/messaging perspective. There is no runtime code generation with native aot, so saying something requires runtime code generation to work suggests it'll never work on native aot, but obviously that's not the case. The real condition in this case isn't that it "requires runtime code generation" but rather that it "requires certain code to be available", which in the case of native aot means it had to generate that code at build time.

Anyway, I'll drop it. I think we're making this confusing, though.

[eerhardt]

Thanks!