Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IL trimming support for DotNext.Metaprogramming #219

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

alexrp
Copy link

@alexrp alexrp commented Jan 29, 2024

Continuing from #218 (see #218 (comment)).

@alexrp alexrp marked this pull request as draft January 29, 2024 20:10
@sakno sakno linked an issue Jan 30, 2024 that may be closed by this pull request
@sakno sakno added this to Opened in Metaprogramming via automation Jan 30, 2024
@sakno sakno added the enhancement New feature or request label Jan 30, 2024
Metaprogramming automation moved this from Opened to In Progress Jan 30, 2024
@alexrp alexrp force-pushed the feature/il-trimming-metaprogramming branch from e1e1790 to 9c731ea Compare February 1, 2024 07:06
@alexrp
Copy link
Author

alexrp commented Feb 1, 2024

I just did some testing in a simple console app:

static class Program
{
    [DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(MyClass<>))]
    static void Main()
    {
        Console.WriteLine(Type.GetType(Console.ReadLine()?.Trim() ?? string.Empty));
    }
}

class MyClass<T> // "MyClass`1" in Type.GetType()
{
}

Good news: It works! If you remove the attribute, publish with trimming, and run the program, it will fail to find the type. With the attribute, it will be there as expected. (Note: I'm getting the type name from user input to make 100% sure that it isn't just being preserved by the linker's dataflow analysis.)

So, going by this, it should be safe to sprinkle [DynamicDependency] where appropriate to get rid of some cases of [RequiresUnreferecedCode].

@sakno
Copy link
Collaborator

sakno commented Feb 1, 2024

Does it mean that [DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(MyClass<>))] preserve all type instantiations of MyClass<T> found in the code?

@alexrp
Copy link
Author

alexrp commented Feb 1, 2024

Does it mean that [DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(MyClass<>))] preserve all type instantiations of MyClass<T> found in the code?

I asked Michal on Discord and he said that it won't do that. But this should only matter for AOT. For JIT use cases, just preserving the uninstantiated generic class should be enough for any instantiation of it to work.

@alexrp
Copy link
Author

alexrp commented Feb 15, 2024

Just noting that I haven't forgotten about this. 🙂 I just have some other items on my to-do list that I need to check off before I get back to this.

@alexrp
Copy link
Author

alexrp commented Feb 19, 2024

Okay. I think I'm starting to come around to your position that it might just be too much work to do this accurately. I've run into multiple cases where I thought "oh, I'll just slap on [DynamicDependency] here", only to realize that it wouldn't actually cover all possible cases - e.g. places that deal with Task/TaskAwaiter-like types. It's increasingly looking like it's just going to be [RequiresUnreferencedCode] for 95% of the library. I suppose that's still better than nothing, since it at least bubbles the analysis warnings up to the user code and makes them actionable, but it's still not quite what I had hoped might be possible.

I'll keep poking at it for a bit, but it's not looking good for [DynamicDependency].

@alexrp
Copy link
Author

alexrp commented Mar 26, 2024

@sakno I've mulled this over some more, and I think I might throw in the towel. Even for our use case of DotNext.Metaprogramming, we ended up deciding that writing a source generator made more sense for other reasons than trimming -- namely, startup performance, compile-time error detection, and debuggability -- so the primary motivation for this work is gone anyway. 😔

That said, if you like, I can split the addition of the DotNext.Trimming project into a separate PR as that ought to still be useful for more complete trimming analysis of the other DotNext libraries that are actually trimming-compatible. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Metaprogramming
  
In Progress
Development

Successfully merging this pull request may close these issues.

Various trimming warnings in DotNext.Metaprogramming
2 participants