Open alexrp opened 5 months ago
Hi @alexrp , DotNex.Metaprogramming
doesn't support trimming for obvious reasons: runtime code generation and heavy use of Reflection.
Is there any reason in principle why it couldn't work? Obviously there would be some responsibility on my own part to root some Type
s/MethodInfo
s that I pass into DotNext.Metaprogramming, but as far as the library's use of reflection goes, wouldn't it just be a matter of annotating with the right attributes?
Almost every method in the library should be marked with RequiresUnreferencedCodeAttribute
. It means that IL trimmer is unable to determine which methods should not be trimmed from your code because DotNext.Metaprogramming
uses LINQ Expression Trees and runtime code generation. I considered this work as useless because you need to inform IL trimmer manually through configuration about all program elements used internally in the library through Reflection.
Hmm, I don't quite follow.
Wouldn't you just need to annotate the API surface of the library with [DynamicallyAccessedMembers(...)]
, [DynamicDependency(...)]
, etc as necessary? (And then plumb those through wherever the analyzer demands it.) It seems to me like the dependencies shouldn't be too hard to reason about, unless the library internally reflects over entire assemblies or something?
I think System.Linq.Expressions
is fully supported with trimming. If you browse the various factory and helper methods in the library, you can see that these annotations are being used there. Now, they still annotate the public API surface with [RequiresUnreferencedCode]
, but AFAIK, this is just to notify the user that they may be relying on members to be present that the linker can't catch. (This same principle is applied to other things like Assembly.DefinedTypes
too, where it may be fine if you know what you're doing, but the warning is there just to make you take an extra look and audit the usage.) In general, as long as your reflection dependencies are obvious to the trimmer's dataflow analysis, you can just suppress the warning about the [RequiresUnreferencedCode]
attribute.
So, in other words, I think there is value in DotNext.Metaprogramming adding these annotations because they would help the trimmer's analysis. But also, adding [RequiresUnreferencedCode]
to the top-level public API surface makes sense too. That way, the trimmer's dataflow analysis will work as expected when the user isn't doing anything crazy (like e.g. passing generated, non-literal property names in), and the [RequiresUnreferencedCode]
will prompt the user to audit their code, after which they can disable that specific warning.
Does this make sense?
Wouldn't you just need to annotate the API surface of the library with [DynamicallyAccessedMembers(...)]
Because it doesn't work in general. For instance, AwaitExpression
requires reflection over Task<T>
and T
is not known at compile time. It means that I cannot declare dependency statically using attribute.
[DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(Task<>))]
This is a valid declaration of open type in C#, however I'm 95% sure that IL trimmer doesn't understand that. I mean it doesn't understand that it need to keep all public methods for any instantiation of type Task<>
.
It seems there was some work done recently related to open generics: https://github.com/dotnet/runtime/pull/96648
That said, I don't actually know if this at all addresses your specific example of preserving Task<T>
members. But at the very least, it seems to be an indication that the trimmer team cares about use cases related to open generics?
In the worst case, I think DotNext.Metaprogramming could embed a root descriptor file that preserve Task<>
. You can see that the docs for the format even have examples for open generics.
By the way, the log in the original issue is with TrimMode=partial
. Here is one with TrimMode=full
which limits the analysis to code that I actually use:
I wonder why AwaitExpression
is even included? I don't use it anywhere in my code. :thinking: The code I generate with DotNext.Metaprogramming is fully sync.
It seems there was some work done recently related to open generics
It's done for DynamicallyAccessedMembers
, not for DynamicDependency
.
There are lot of complex cases not covered with attributes. For instance, ForEachExpression
and many other classes override Reduce
method. It has conversion from custom Expression Tree to well-known expression types. It requires reflection. However, the method cannot be marked with one of those attributes aimed for IL trimming because it's virtual. Otherwise, it produces a warning. In this case all I have is to mark entire class with RequiresUnreferencedCode
attribute and delegate configuration of IL trimming to the library consumer.
In this case all I have is to mark entire class with
RequiresUnreferencedCode
attribute
I suppose applying dependency attributes to the ForEachExpression
class itself would not work?
and delegate configuration of IL trimming to the library consumer.
I am not actually opposed to doing this, by the way. The fundamental problem here is that, even if I do this, I am still left with all these trim analysis warnings coming from DotNext.Metaprogramming. AFAIK, the only way I can silence them is by suppressing all trimmer warnings, even from my own assemblies and other third-party dependencies, which is not good. :slightly_frowning_face:
class itself would not work?
No. It works with Expression.Type
property which is not controlled by ForEachExpression
and others. There is no way to mark the property declared in external assembly.
Ok, I see what you mean. Thinking a bit more about it, I can't think of a good solution there either.
What do you think about best-effort annotations? I.e.:
[RequiresUnreferencedCode]
to the public API surface where unannotated reflection is done underneath (e.g. ForEachExpression
), to notify the user that they must audit their code and the trimming result to make sure everything necessary is preserved.[UnconditionalSuppressMessage]
attributes as necessary for situations like Reduce()
where reflection happens, but the user has already been notified through [RequiresUnreferencedCode]
on the relevant factory method(s).It seems like this would bring the best user experience here. Depending on APIs used, there would still be some work for the user to do to preserve reflected-upon types/members, but at least there would no longer be any unactionable/unsuppressible trim analysis warnings. The warnings that would be there for [RequiresUnreferencedCode]
can just be suppressed locally in code when the user has audited each case.
What do you think about best-effort annotations?
I tried this approach several times and it requires a lot of work to analyze each case accurately.
Sad to hear that <TrimMode>partial</TrimMode>
doesn't work.
I tried this approach several times and it requires a lot of work to analyze each case accurately.
If I can find the time to go through the library and do this, would you take a PR for it? No promises that I can get the approach to work either, but I can at least try.
Sad to hear that
<TrimMode>partial</TrimMode>
doesn't work.
Yeah, I initially thought that this mode was what I wanted for dependencies like this that aren't marked trimmable. But then you just get unsuppressible analysis warnings as shown here. It seems that, either way, there's just no winning unless you annotate the library. It's honestly a pretty annoying design on the part of the trimming infrastructure.
would you take a PR for it?
I'll submit initial PR so you can be a co-author to continue this work. Anyway, this library probably will never be AOT compatible.
Anyway, this library probably will never be AOT compatible.
That's fine. I wouldn't even try to use S.L.E with AOT; that seems like a recipe for a bad time. I'm just hoping we can get basic trimming somewhat working (even if it still requires some effort from the user side) so I can reduce the size of my published (JIT) app.
Done, draft PR is opened (see above).
I just tried turning on trimming for my app, and noticed some warnings coming from DotNext.Metaprogramming:
At first glance, it seems like most of these can be fixed by plumbing the right attributes through.