feature: Refit should be linker-friendly and support trimming

Sergio0694 commented 2 years ago

Is your feature request related to a problem? Please describe.

Yes. The issue is that despite Refit uses source generators, the generated code is just a thin proxy back to the reflection-powered functionality in the main library. This means that using Refit with trimming enabled completely breaks it. As it is, there's not much value in having source generators. They should be updated so that all the reflection-powered logic is executed at compile time, and the generated stubs will just have all the logic that is today done at runtime by the method builder APIs. This library should effectively require no runtime reflection at all (or, virtually none, but especially none that can break trimming).

Describe the solution you'd like

As mentioned, the source generator should be updated so that instead of just calling back into the method builder and invoking that reflection-based callback to perform the request, it'd instead emit all the same logic that's today done at runtime. That is, all the various marshalling of parameters, request building etc. should just be done in a statically-analyzeable way and generated at compile time. The library should just expose helper APIs for the generator to use to construct the individual bits of a request/response.

All the information that the method builder is using at runtime is there at compile time too anyway. That logic should just essentially be inverted, and the generator APIs should be used to achieve static reflection, instead of runtime reflection like today.

If there's any reflection left, additionally, all those APIs should be properly annotated.

Refit as a whole should have the trimming analyzers enabled when on the .NET 6+ target.

Describe alternatives you've considered

Runtime directives can be used to patch the problem. This is extremely cumbersome, hard to do especially for beginners, and causes an unnecessary binary size increased, given you'd be forced to preserve way more stuff than strictly needed, to be safe.

Describe suggestions on how to achieve the feature

Investigate all the individual bits that method builder uses to construct the request.
Expose the necessary helper APIs for them from that type.
Update the generator to do all that introspection at compile time.
Update the generator to perform the needed calls to those helpers to construct and execute the request right there.

Sergio0694 commented 2 years ago

Hey @clairernovotny! 👋 Can we coordinate with you to discuss, plan and eventually implement this feature, or would you be able to connect us with the right folks to talk about this? @Kant2002 has kindly offered to help with this (he has experience doing very similar work, and has contributed to several source generator and trimming related work on other projects in the past), and he was wondering how to go about this, given that the scope of the work involved for this really needs some discussion first 🙂

I was thinking we could do pretty much what we did back in #836 and following PRs, where we added System.Text.Json support and eventually published a new major release that split Newtonsoft.Json out in a separate package so consumers could avoid taking that extra dependency. The amount of changes here would also make this not be backwards compatible, so I feel we could do the same and do this in a new major release.

NOTE: this would only be a binary breaking change. Consumers bumping the NuGet reference and rebuilding from source should not actually encounter breaking changes, because the generators would just generate all updated code on the spot.

Let us know how we can proceed with this! We really think adding proper trimming and reflection-free support for Refit would be a huge win for a ton of consumers. Especially now that starting from .NET 7 (see here), NativeAOT builds will try to trim everything by default, which means people publishing and referencing an assembly using Refit will start to see more linker warnings now, because the compiler will attempt to trim everything. And unless they changed that back, they'd now also have runtime issues, because Refit just breaks down with trimming enabled.

Thank you! 🙂

crozone commented 2 years ago

This feature would be great when used in conjunction with a source generator based DI system like https://github.com/YairHalberstadt/stronginject. It would make Refit usable in embedded applications which benefit heavily from NativeAOT compilation.

GitHub
GitHub - YairHalberstadt/stronginject: compile time dependency injection for .NET
compile time dependency injection for .NET. Contribute to YairHalberstadt/stronginject development by creating an account on GitHub.

anaisbetts commented 2 years ago

Microsoft: "Please rewrite your entire library and spend hours of your limited free developer time, because we broke it. Again. Users will receive absolutely no new feature or benefit as a result of this work, they will simply have exactly what they did before. Oh, and we also plan to include breaking changes in your library."

I recommend users disable NativeAOT.

Sergio0694 commented 2 years ago

I think there's some misunderstanding here and some incorrect statements, and also some frustration towards Microsoft that I'm not really sure is warranted in this specific case. Let me try to clarify things here.

"Microsoft:"

This was just me proposing an improvement. I wasn't speaking for Microsoft, and I don't think I ever said that that was the case here. This was just something I wanted to start a conversation about, as I'm personally a fan of refit 🙂

"Please rewrite your entire library and spend hours of your limited free developer time"

Nobody said anyone from the refit team should rewrite the entire library. In fact, @kant2002 already said he'd like to help work on this, and I also said I'd be happy to provide him with assistance.

"because we broke it"

Nobody broke anything. Refit still works just fine, it's just very inefficient and doesn't support trimming, but that has always been the case.

"Users will receive absolutely no new feature or benefit as a result of this work, they will simply have exactly what they did before"

This is just clearly false and also very easy to disprove. I already outlined the improvements for end users that these changes would being. The library as a whole would be faster, it would give them the ability to manually inspect the code being used and improve their debugging experience, it would make the whole library trimmable and it would also make it much easier to use it on AOT scenarios. Those are valid benefits for end users.

"Oh, and we also plan to include breaking changes in your library"

I already talked with @clairernovotny internally and she said this would be fine, which is the reason why @kant2002 had already started some preliminary work. The breaking changes would go into a new major version, so customers could also choose to just remain on the previous one until they're ready to migrate. This is exactly what we did when I added the System.Text.Json support to refit and we later decided to make it the default and pull Newtonsof.Json out and in a separate package.

"I recommend users disable NativeAOT."

This is just poor advice, and it's unwarranted.

Let me clarify things some more as well. I really like refit, and as I've mentioned I've also already contributed to it several time in the past. The reason why I'm proposing this is precisely because I like this project, and I'd love to see it improve, address the architectural issues it currently has, and become the best HTTP REST library it can possibly be. I've opened this proposal because I wanted to start a conversation about this and also allow @kant2002 to get in touch with the maintainers and get assurances that if he had done work it wouldn't have been blocked after the fact (which is why we reached out to Claire). This post wasn't meant as a critique on anyone nor as a demand on anyone from the team to do work for free 🙂

kant2002 commented 2 years ago

As person who want this feature. Is this would be accepted if I implement this?

anaisbetts commented 2 years ago

The library as a whole would be faster

Do you have any data to prove that, in real-world scenarios involving actual network requests, that this makes any significant difference?

much easier to use it on AOT scenarios.

This is Curious because Refit actually uses source generators because of Xamarin AOT compilation requirements on iOS, and there it has worked for nearly a decade without issues. Why is this different?

I think it is more appropriate to take a wait-and-see approach on this one, rather than radically rewriting how Refit works (for the third time!), in order to chase the latest Microsoft feature. If this is still causing users grief in a year, I think it's worth converting it.

Sergio0694 commented 2 years ago

"Do you have any data to prove that, in real-world scenarios involving actual network requests, that this makes any significant difference?"

Yes, I'd you're doing few very long requests the relative impact would of course be minimal, but as I mentioned that wouldn't be the main point (though for apps doing tons of smaller requests it could still be noticeable). The main benefit would be supporting trimming, improving debugging, proper AOT support, and less impact on the IDE performance (this last one is a separate point, but it would be another benefit of a better source generation architecture).

"This is Curious because Refit actually uses source generators because of Xamarin AOT compilation requirements on iOS, and there it has worked for nearly a decade without issues. Why is this different?"

Using source generators alone is not a magic wand that automatically makes code trimmer-safe or AOT-friendly. Refit uses source generators, yes, but the architecture is pretty suboptimal: the generated code is just a few very thin stubs that just forward all the logic back into the library, which then uses some admittedly pretty extreme reflection (and comments in the code do say as much) to make things work. As in, refit is not using source generators in a way that a truly source generator first architecture would. As a result, the code can work in some AOT scenarios, but it's still fundamentally inefficient, not AOT-friendly and doesn't support trimming at all (that's both due to the architecture, and the fact it's not even annotated). For instance, if you use refit on AOT and enable trimming, it will not work at all and it will just crash at runtime.

"in order to chase the latest Microsoft feature"

To clarify: this is not something new per se. As I mentioned, refit has been broken with trimming on UWP for years already. The idea is just that now that better source generator APIs are available, trimming is more widespread, trimming annotations are available, NativeAOT is officially supported, it seems like a good time to start looking into whether this would be doable 🙂

(Which it is, it just requires some work).

anaisbetts commented 2 years ago

Refit uses source generators, yes, but the architecture is pretty suboptimal: the generated code is just a few very thin stubs that just forward all the logic back into the library, which then uses some admittedly pretty extreme reflection

....you know that I wrote this library right? Why are you explaining my own code to me

Sergio0694 commented 2 years ago

"This is Curious because Refit actually uses source generators because of Xamarin AOT compilation requirements on iOS, and there it has worked for nearly a decade without issues. Why is this different?

I... Just answered your question 🥲

I'm aware you wrote most of this library, but I'm not sure how you expected me to be able to answer that without having to underline some bits of how the codebase is structured versus what is being proposed here, which is what I did. If you meant that you already knew all of this too, well then I'm afraid I don't understand why you even asked that question in the first place.

kant2002 commented 2 years ago

My motivation to work on this issue is following. I would like to have better ecosystem which cares about NativeAOT + reflection-free mode. Reflection-free mode is experimental mode for NativeAOT, so you may think about it as niche within niche. Even if that's niche, whether possible I try to contribute back to existing libraries. Based on my observations, most code works just fine even in this mode, if augment things here and there with source generators (which I use like Reflection at compile time). Usually this results in the all dynamic code to be generated by sourcegen and visible to developers. That helps trimming technology within dotnet reason about things and end-user (developer) do not have to think about how to massage things to please trimming. I would say this also allows other developers to better reason about library, but that's maybe my opinion.

Who cares about this subniche. I would say game developers, hot-heads like me, embedded developers and quite possible cloud developers. Game developers and embedded developers obviously like squeezing water from stone, so this case is definitely good for them. Cloud developers would have faster startup time so for some workflows that's allow them save money.

I do not expect huge adoption of NativeAOT (default mode) in near future so if you drive this library purely based on demand, then I understand why you would like to wait and see. My experience is that unrestrained usage of reflection may cause issue for end-developers and complicate their lives. I do want that working with NativeAOT would be as pleasant as with CoreCLR, so that's why I'm going to libraries and try to fix things here and there.

If you think that's not important for you, I can understand that. No skin of my back. Then I have to create something similar but without reflection myself. Either way, you may see that I do not super speedy in writing code for this issue. If there chance that I may persuade you extend support of your for these niches I prefer that way.

crozone commented 2 years ago

Microsoft: "Please rewrite your entire library and spend hours of your limited free developer time, because we broke it. Again. Users will receive absolutely no new feature or benefit as a result of this work, they will simply have exactly what they did before. Oh, and we also plan to include breaking changes in your library."

I recommend users disable NativeAOT.

Well, this is a surprisingly hostile response to a feature request...

NativeAOT is an opt-in technology, but a valuable one in certain contexts where a JIT is just too slow or completely unavailable. For the moment, if an application requires NativeAOT, it simply cannot use Refit, which sucks once you're used to how nice Refit makes working with APIs. Going back to HttpClient boilerplate is a pill.

The reason this issue exists at all is because we like Refit and we want to use it in even more scenarios. It's a request to collaborate, not to ask you to do all the work, but to focus community effort towards this feature because it is an open source project and it makes all of our development experiences better. I really hope you consider re-opening this issue because true reflection-less Refit would be a massive boon to many.

anaisbetts commented 2 years ago

In isolation, I would not be so hostile to this request - it is Annoying, but Reasonable. However, having authored many C# libraries and having things like this happen over and over and over, it is frustrating, both as a user of C# who Wants It To Be Good, and as a maintainer.

.NET decides to break the world, and then eagerly rushes to convince everyone to get on board. The ecosystem gets left in a fractured mess over and over, as maintainers throw out all of their working, production-ready code and start again. Why is it okay to ship NativeAOT with such massive breaking changes? Xamarin shipped AOT almost a decade earlier and absolutely bent over backwards to still support existing code. Why? Because not breaking the ecosystem matters. And in fact, Refit works just fine with Xamarin AOT on iOS, arguably the most hostile AOT environment that exists!

Even the suggestion of "Just major version it bro" like it magically waives away all compatibility issues is really frustrating to me - now anyone who has a shared library using Refit, who tries to integrate the latest version to their app, is completely stuck. This is not an edge case, this happens all the time in corporate environments. But instead, we are now going to break all of those people. That seems Bad!

As a developer and maintainer, I am very much in the Raymond Chen camp of software design - Don't. Break. Software, unless it's really really really worth it. This is a part of why people, after all these years, still use things like Refit, and ReactiveUI, and Squirrel.Windows - because they don't constantly break people.

ScarletKuro commented 1 year ago

Hello everyone! I wanted to inquire about the current status of this matter. Previously, there seemed to be a significant community interest, with several individuals expressing their willingness to contribute by submitting pull requests. However, it has come to my attention that the pull request from @kant2002 has been closed. Does this indicate a lack of interest from the library owners?

Personally, I strongly believe that incorporating this feature and marking library with <IsTrimmable>true</IsTrimmable> would be highly beneficial as I utilize Blazor WASM. In this environment, trimming is crucial due to the importance of minimizing the libraries size and saving every byte.

crozone commented 1 year ago

My use case is getting Refit into industrial .NET applications which are often targeted towards low power ARM SoCs, usually single core and around 1GHz in speed. .NET is surprisingly performant in steady state, but JIT pauses and deployment size are troublesome. ReadyToRun is an alright stopgap solution but NativeAOT performs excellently and is really the end goal.

I'm also curious as to why https://github.com/reactiveui/refit/pull/1414 was closed. If implementing full source generation is considered too disruptive to the project goals, would it be more feasible to fork/branch off into a separate package which is a ground up source generator first implementation?

damidhagor commented 1 year ago

I'd also find AOT support extremely helpful. I'm deploying several web apis to Azure which use Refit to communicate with each other and other external apis. Since this is a private project and I need to keep my Azure bill low I've set the web apps to shut off when they're inactive. AOT would really help with the cold start experience when calling the apis the first time when they're inactive and need to start up as fast as possible. Also AOT (if I remember correctly) greatly reduces the file sizes of the published apps so that deploying to Azure would be faster and my Github action quotas for build artifact storage would not be exceeded too often.

It would be really awesome if Refit as such an integral tool for so many .Net projects would support this new feature.

ghostnguyen commented 9 months ago

I'd also find AOT support extremely helpful. I'm deploying several web apis to Azure which use Refit to communicate with each other and other external apis. Since this is a private project and I need to keep my Azure bill low I've set the web apps to shut off when they're inactive. AOT would really help with the cold start experience when calling the apis the first time when they're inactive and need to start up as fast as possible. Also AOT (if I remember correctly) greatly reduces the file sizes of the published apps so that deploying to Azure would be faster and my Github action quotas for build artifact storage would not be exceeded too often.

It would be really awesome if Refit as such an integral tool for so many .Net projects would support this new feature.

I have develop new Refit that support AOT. Nuget package name is HttpClientFiller

https://www.nuget.org/packages/HttpClientFiller

HttpClientFiller 0.1.1
HttpClient Filler Code Generate in C#

crozone commented 9 months ago

@ghostnguyen Where's the source code?

ghostnguyen commented 9 months ago

@ghostnguyen Where's the source code?

The source code/dll need to run your project are:

The generated code in your project.
HttpFillerAttribute and HttpFillerCore (https://github.com/ghostnguyen/HttpClientFillerIssue/)

GitHub
GitHub - ghostnguyen/HttpClientFillerIssue
Contribute to ghostnguyen/HttpClientFillerIssue development by creating an account on GitHub.

bugproof commented 9 months ago

@ghostnguyen Where's the source code?

Seems like the source code of the code generator is not available.

ghostnguyen commented 9 months ago

@ghostnguyen Where's the source code?

Seems like the source code of the code generator is not available.

This will not be required to build and to run your project.

bugproof commented 9 months ago

@ghostnguyen Where's the source code?

Seems like the source code of the code generator is not available.

This will not be required to build and to run your project.

It's not about if it will be required or not. It's about trust and safety. Some people might think it's closed source because it contains some malicious code inside/backdoor/whatever. I think that is the concern here. I mean sure you can still add malicious code with open source libs but it adds a bit of trust when there's a complete src available.

ghostnguyen commented 9 months ago

@ghostnguyen Where's the source code?

Seems like the source code of the code generator is not available.

This will not be required to build and to run your project.

It's not about if it will be required or not. It's about trust and safety. Some people might think it's closed source because it contains some malicious code inside/backdoor/whatever. I think that is the concern here.

It is safe because there is no some malicious code end up in your product. It is one of the advantage of Roslyn.

anaisbetts commented 9 months ago

@ghostnguyen @bugproof This conversation has nothing to do with Refit, please take it to a more appropriate venue.

Arlodotexe commented 7 months ago

Lack of trimmer support is the reason I stopped using Refit after my first .NET project years ago. For anything with a UI, my platform of choice is UWP/Uno Platform, and they both make good use of AoT:

.NET Native under UWP has never been optional for Microsoft Store apps. Like Sergio mentioned, we can use Resource Directives as a workaround for any non-linker-friendly code, but it's cumbersome for most and confusing for many.
Uno Platform under Wasm uses an IL Linker and AoT to improve performance and reduce final package sizes. It's a must for users on mobile browsers.

I'd like to use Refit again, but I can't until refit is trimmer-friendly.

ghostnguyen commented 7 months ago

Lack of trimmer support is the reason I stopped using Refit after my first .NET project years ago. For anything with a UI, my platform of choice is UWP/Uno Platform, and they both make good use of AoT:

* [.NET Native](https://learn.microsoft.com/en-us/windows/uwp/dotnet-native/) under UWP has never been optional for Microsoft Store apps. Like Sergio mentioned, we can use Resource Directives as a workaround for any non-linker-friendly code, but it's cumbersome for most and confusing for many.

* Uno Platform under Wasm uses an [IL Linker](https://platform.uno/docs/articles/features/using-il-linker-webassembly.html) and [AoT](https://platform.uno/docs/articles/external/uno.wasm.bootstrap/doc/runtime-execution-modes.html#profile-guided-aot) to improve performance and reduce final package sizes. It's a must for users on mobile browsers.

I'd like to use Refit again, but I can't until refit is trimmer-friendly.

You can try this library, this Refit like and AoT support. https://www.nuget.org/packages/HttpClientFiller

HttpClientFiller 0.1.4
HttpClient Filler Code Generate in C# - Refit like but support AOT

xljiulang commented 5 months ago

I would like to remind you that using [ModuleInitializer] can prevent the proxy class from being trimming, see HttpApiProxyClassInitializer.WebApiClient now has better performance in design and performance.

TimothyMakkison commented 5 months ago

Hey, would you still be open to me doing source generator rewrite? I feel like there are some major performance and usability gains to be made with generated code.

While it should be possible to remove all reflection from the internal logic, it looks like several configuration interfaces rely on reflection types ie GetFieldNameForProperty(PropertyInfo). Removing these would be a breaking change, and I don't know what could replace them, some logic might rely upon knowing which method a parameter belonged to and adjusting its logic accordingly.

Aside from that the only public changes would be the removal of requestBuilder.BuildRestResultFuncForMethod 🤞

public interface IGithubApi
{
    [Get("/users/{user}")]
    Task<User> GetUser(string user, [Header("Authorization")] string authorization, [Query(CollectionFormat.Multi)]int[] ages);
}

// Generated code
public Task<User> GetUser(string user, string authorization, int[] ages)
{
    var req = new HttpRequestMessage() { Method = HttpMethod.Get };
    req.Headers.Add("Authorization", $"Authorization: {authorization}");

    // could use `ValueStringBuilder` here with a little trickery
    var sb = new StringBuilder();
    sb.Append("/users/");
    sb.Append(settings.UrlParameterFormatter.Format(user, null, typeof(string)));
    sb.Append('?');

    var key = settings.UrlParameterKeyFormatter.Format("ages");
    Helpers.AddQueryCollection(sb, key, ages, settings);

    req.RequestUri = new Uri(sb.ToString());
    // send, handle errors and serialize etc
}

ScarletKuro commented 5 months ago

I would like to remind you that using [ModuleInitializer] can prevent the proxy class from being trimming, see HttpApiProxyClassInitializer.WebApiClient now has better performance in design and performance.

Of course, the PR you did (https://github.com/reactiveui/refit/pull/1710) is better than nothing, but it adds the entire generated code as an exception. This means that if there is an API call that you don't use, the generated code for it will still be preserved rather than trimmed.

The author suggested using the source code generator, not just to generate a thin reflection proxy, but to generate the code that is currently done at the runtime level. This would have a very positive effect on the library.

ghostnguyen commented 5 months ago

Hey, would you still be open to me doing source generator rewrite? I feel like there are some major performance and usability gains to be made with generated code.

While it should be possible to remove all reflection from the internal logic, it looks like several configuration interfaces rely on reflection types ie GetFieldNameForProperty(PropertyInfo). Removing these would be a breaking change, and I don't know what could replace them, some logic might rely upon knowing which method a parameter belonged to and adjusting its logic accordingly.

Aside from that the only public changes would be the removal of requestBuilder.BuildRestResultFuncForMethod 🤞
public interface IGithubApi
{
    [Get("/users/{user}")]
    Task<User> GetUser(string user, [Header("Authorization")] string authorization, [Query(CollectionFormat.Multi)]int[] ages);
}

// Generated code
public Task<User> GetUser(string user, string authorization, int[] ages)
{
    var req = new HttpRequestMessage() { Method = HttpMethod.Get };
    req.Headers.Add("Authorization", $"Authorization: {authorization}");

    // could use `ValueStringBuilder` here with a little trickery
    var sb = new StringBuilder();
    sb.Append("/users/");
    sb.Append(settings.UrlParameterFormatter.Format(user, null, typeof(string)));
    sb.Append('?');

    var key = settings.UrlParameterKeyFormatter.Format("ages");
    Helpers.AddQueryCollection(sb, key, ages, settings);

    req.RequestUri = new Uri(sb.ToString());
    // send, handle errors and serialize etc
}

You can check the generate code in this library to see its working https://www.nuget.org/packages/HttpClientFiller

HttpClientFiller 0.1.4
HttpClient Filler Code Generate in C# - Refit like but support AOT

ghostnguyen commented 5 months ago

I would like to remind you that using [ModuleInitializer] can prevent the proxy class from being trimming, see HttpApiProxyClassInitializer.WebApiClient now has better performance in design and performance.

Of course, the PR you did (#1710) is better than nothing, but it adds the entire generated code as an exception. This means that if there is an API call that you don't use, the generated code for it will still be preserved rather than trimmed.

The author suggested using the source code generator, not just to generate a thin reflection proxy, but to generate the code that is currently done at the runtime level. This would have a very positive effect on the library.

Yes, this library does fully support 'generate the code that is currently done at the runtime level' https://www.nuget.org/packages/HttpClientFiller

HttpClientFiller 0.1.4
HttpClient Filler Code Generate in C# - Refit like but support AOT

xljiulang commented 5 months ago

Born for AOT, but abandoned the support of many lower versions of .NET, and the scalability is almost zero, why bother?

Sergio0694 commented 4 months ago

I don't understand why this issue has been closed. Refit is, as of 7.1.1, still completely not trim-safe nor AOT-safe.

Just try it yourself by creating this minimal project:

using Refit;
using WebApis;

var gitHubApi = RestService.For<IGitHubApi>("https://api.github.com");

Console.WriteLine(await gitHubApi.GetUser("sergio"));
Console.WriteLine(await gitHubApi.GetUsers());

namespace WebApis
{
    public interface IGitHubApi
    {
        [Get("/users/{user}")]
        Task<User> GetUser(string user);

        [Get("/users/")]
        Task<User[]> GetUsers();
    }

    public sealed record User(string Name, string Id, string Email);
}

Then publish with AOT. You'll get 0 warnings in the IDE, but all the following warnings at publish time, from ILC:

/_/Refit/RestService.cs(81): Trim analysis warning IL2072: Refit.RestService.For(Type,HttpClient,IRequestBuilder): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors' in call to 'System.Activator.CreateInstance(Type,Object[])'. The return value of method 'System.Collections.Concurrent.ConcurrentDictionary`2<Type,Type>.GetOrAdd(Type,Func`2<Type,Type>)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/_/Refit/RestService.cs(179): Trim analysis warning IL2057: Refit.RestService.GetGeneratedType(Type): Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String)'. It's not possible to guarantee the availability of the target type.
/_/Refit/RequestBuilderImplementation.cs(202): AOT analysis warning IL3050: Refit.RequestBuilderImplementation.BuildRestResultFuncForMethod(String,Type[],Type[]): Using member 'System.Reflection.MethodInfo.MakeGenericMethod(Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime.
/_/Refit/RequestBuilderImplementation.cs(218): AOT analysis warning IL3050: Refit.RequestBuilderImplementation.BuildRestResultFuncForMethod(String,Type[],Type[]): Using member 'System.Reflection.MethodInfo.MakeGenericMethod(Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime.
/_/Refit/RequestBuilderImplementation.cs(32): Trim analysis warning IL2070: Refit.RequestBuilderImplementation.RequestBuilderImplementation(Type,RefitSettings): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.Interfaces' in call to 'System.Type.GetInterfaces()'. The parameter 'refitInterfaceType' of method 'Refit.RequestBuilderImplementation.RequestBuilderImplementation(Type,RefitSettings)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/_/Refit/RequestBuilderImplementation.cs(63): Trim analysis warning IL2070: Refit.RequestBuilderImplementation.AddInterfaceHttpMethods(Type,Dictionary`2<String,List`1<RestMethodInfoInternal>>): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Type.GetMethods(BindingFlags)'. The parameter 'interfaceType' of method 'Refit.RequestBuilderImplementation.AddInterfaceHttpMethods(Type,Dictionary`2<String,List`1<RestMethodInfoInternal>>)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/_/Refit/RequestBuilderImplementation.cs(159): Trim analysis warning IL2060: Refit.RequestBuilderImplementation.<>c__DisplayClass11_0.<CloseGenericMethodIfNeeded>b__0(CloseGenericMethodKey): Call to 'System.Reflection.MethodInfo.MakeGenericMethod(Type[])' can not be statically analyzed. It's not possible to guarantee the availability of requirements of the generic method.
/_/Refit/RequestBuilderImplementation.cs(159): AOT analysis warning IL3050: Refit.RequestBuilderImplementation.<>c__DisplayClass11_0.<CloseGenericMethodIfNeeded>b__0(CloseGenericMethodKey): Using member 'System.Reflection.MethodInfo.MakeGenericMethod(Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime.
/_/Refit/SystemTextJsonContentSerializer.cs(75): AOT analysis warning IL3050: Refit.SystemTextJsonContentSerializer.GetDefaultJsonSerializerOptions(): Using member 'System.Text.Json.Serialization.JsonStringEnumConverter.JsonStringEnumConverter(JsonNamingPolicy,Boolean)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. JsonStringEnumConverter cannot be statically analyzed and requires runtime code generation. Applications should use the generic JsonStringEnumConverter<TEnum> instead.
/_/Refit/RestMethodInfo.cs(546): Trim analysis warning IL2075: Refit.RestMethodInfoInternal.ParseHeaders(MethodInfo): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.Interfaces' in call to 'System.Type.GetInterfaces()'. The return value of method 'System.Reflection.MemberInfo.DeclaringType.get' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/_/Refit/RestMethodInfo.cs(238): Trim analysis warning IL2075: Refit.RestMethodInfoInternal.GetParameterProperties(ParameterInfo): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperties(BindingFlags)'. The return value of method 'System.Reflection.ParameterInfo.ParameterType.get' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/_/Refit/SystemTextJsonContentSerializer.cs(37): Trim analysis warning IL2026: Refit.SystemTextJsonContentSerializer.ToHttpContent<T>(!!0): Using member 'System.Net.Http.Json.JsonContent.Create<T>(T,MediaTypeHeaderValue,JsonSerializerOptions)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved.
/_/Refit/SystemTextJsonContentSerializer.cs(37): AOT analysis warning IL3050: Refit.SystemTextJsonContentSerializer.ToHttpContent<T>(!!0): Using member 'System.Net.Http.Json.JsonContent.Create<T>(T,MediaTypeHeaderValue,JsonSerializerOptions)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext.
/_/Refit/RequestBuilderImplementation.cs(1248): Trim analysis warning IL2075: Refit.RequestBuilderImplementation.DoNotConvertToQueryMap(Object): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.Interfaces' in call to 'System.Type.GetInterfaces()'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/_/Refit/RequestBuilderImplementation.cs(479): Trim analysis warning IL2075: Refit.RequestBuilderImplementation.BuildQueryMap(Object,String,RestMethodParameterInfo): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperties(BindingFlags)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/_/Refit/FormValueMultimap.cs(169): Trim analysis warning IL2070: Refit.FormValueMultimap.GetProperties(Type): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperties(BindingFlags)'. The parameter 'type' of method 'Refit.FormValueMultimap.GetProperties(Type)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/_/Refit/RefitSettings.cs(268): Trim analysis warning IL2075: Refit.DefaultUrlParameterFormatter.<>c__DisplayClass1_0.<Format>b__1(String): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors', 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.PublicFields', 'DynamicallyAccessedMemberTypes.PublicNestedTypes', 'DynamicallyAccessedMemberTypes.PublicProperties', 'DynamicallyAccessedMemberTypes.PublicEvents' in call to 'System.Type.GetMember(String)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/_/Refit/RefitSettings.cs(319): Trim analysis warning IL2075: Refit.DefaultFormUrlEncodedParameterFormatter.<>c__DisplayClass1_0.<Format>b__1(String): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors', 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.PublicFields', 'DynamicallyAccessedMemberTypes.PublicNestedTypes', 'DynamicallyAccessedMemberTypes.PublicProperties', 'DynamicallyAccessedMemberTypes.PublicEvents' in call to 'System.Type.GetMember(String)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/_/Refit/ValidationApiException.cs(49): Trim analysis warning IL2026: Refit.ValidationApiException.Create(ApiException): Using member 'System.Text.Json.JsonSerializer.Deserialize<ProblemDetails>(String,JsonSerializerOptions)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved.
/_/Refit/ValidationApiException.cs(49): AOT analysis warning IL3050: Refit.ValidationApiException.Create(ApiException): Using member 'System.Text.Json.JsonSerializer.Deserialize<ProblemDetails>(String,JsonSerializerOptions)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use System.Text.Json source generation for native AOT applications.
/_/Refit/SystemTextJsonContentSerializer.cs(48): Trim analysis warning IL2026: Refit.SystemTextJsonContentSerializer.<FromHttpContentAsync>d__4`1.MoveNext(): Using member 'System.Net.Http.Json.HttpContentJsonExtensions.ReadFromJsonAsync<T>(HttpContent,JsonSerializerOptions,CancellationToken)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved.
/_/Refit/SystemTextJsonContentSerializer.cs(48): AOT analysis warning IL3050: Refit.SystemTextJsonContentSerializer.<FromHttpContentAsync>d__4`1.MoveNext(): Using member 'System.Net.Http.Json.HttpContentJsonExtensions.ReadFromJsonAsync<T>(HttpContent,JsonSerializerOptions,CancellationToken)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext.
/_/Refit/SystemTextJsonContentSerializer.cs(106): Trim analysis warning IL2026: Refit.ObjectToInferredTypesConverter.Write(Utf8JsonWriter,Object,JsonSerializerOptions): Using member 'System.Text.Json.JsonSerializer.Serialize(Utf8JsonWriter,Object,Type,JsonSerializerOptions)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved.
/_/Refit/SystemTextJsonContentSerializer.cs(106): AOT analysis warning IL3050: Refit.ObjectToInferredTypesConverter.Write(Utf8JsonWriter,Object,JsonSerializerOptions): Using member 'System.Text.Json.JsonSerializer.Serialize(Utf8JsonWriter,Object,Type,JsonSerializerOptions)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use System.Text.Json source generation for native AOT applications.

@glennawatson to clarify: Refit is not at the time being trim-safe nor AOT-safe whatsoever. This is just completely undefined behavior. This issue should not have been closed (not sure why it was in the first place).

ChrisPulman commented 4 months ago

I believe Github automatically closed this issue so I have reopened it.

Let's come up with a solution for this, although the request is valid, the solution is not so simplistic. If there is a way to achieve this while maintaining the complete functionality so as not to break existing code, then a complete PR with examples of both being Trim and AOT safe while maintaining the existing features, functionality and API. All existing tests should pass and additional tests where possible should be added for any additional code. There's a high number of users reliant on Refit and I would rather leave Trim and AOT abilities out than force people to rewire their applications to meet a new API. So let's make the right solution. Thank you to all who have posted their thoughts.

Sergio0694 commented 4 months ago

"If there is a way to achieve this while maintaining the complete functionality so as not to break existing code"

It depends:

If you mean "truly make everything AOT-safe with 0 breaking API changes", likely not
If you mean "make the library AOT-annotated with 0 breaking API changes", yes

This is what I've mentioned before as well. If maintaining complete backwards compatibility is a primary concern and we don't want to introduce any breaking changes even in a new major upgrade, that's fine, however we should at least add the necessary annotations to the library so that consumers would get the right warnings right in the IDE, instead of only seeing them at publish time. That will result in a much better developer experience.

Put simply, we need to add a .NET 8 TFM to Refit, toggle IsAotCompatible in the project, and add the necessary [RequiresUnreferencedCode] and [RequiresDynamicCode] to all APIs until all warnings are gone. We'd also need to make sure to add the necessary annotations to APIs that would depend on trim/AOT-unsafe code that's only generated at build time by the source generator, if any. The goal has to be that developers should get all trim/AOT-warnings right in the IDE, and that if they can compile their code with no warnings in the IDE, they should get 0 warnings at publish time as well.

To be clear, this would not make the library trim/AOT-safe. As I said, I don't think that's doable without some breaking changes. But it would, however, make it clearly incompatible with trimming and AOT, which at least gives you well defined behavior.

ChrisPulman commented 4 months ago

Thank you @Sergio0694 the Net 8.0 TFM is already in place, next step AOT Annotation.

Once this is in place an AOT Safe version could be produced for Net 8, depending on how wildly different it would need to become we may need to consider a separate Refit.AOT package to ensure the existing features are maintained too; there's too many people using Refit to make a big breaking change that results in everyone having to rewire and retest their applications. @TimothyMakkison had an idea to implement this and if it's a possibility to get this done with minimal API changes with a Major version then let's consider this, otherwise a separate AOT package will most likely be the best way forward. I will discuss with the other Refit maintainers to see which approach would be best once there is something to make a decision on.

Sergio0694 commented 4 months ago

That sounds like a very reasonable plan to me 🙂 If I may offer a couple suggestions in case you do decide to also make an AOT-friendly version (as in, not just a version that is annotated but effectively not usable, but rather one that is actually designed to be AOT-friendly from the start):

Make sure to include a test project (can be a simple console app using Refit to make a few example API calls in the various scenarios, doesn't even necessarily have to run and be end-to-end tested if that's too complicated to setup), and have your CI also publish that app with NativeAOT and validate that there are no trim/AOT warnings (ie. no ILXXXX warnings). If there are any, that is undefined behavior and the library is not AOT-compatible. It is not sufficient to only verify no trim/AOT warnings in the IDE from the analyzers, as those do not have complete coverage. You must publish and have ILC validate your code.
Enforce a rule to not use any trim/AOT suppressions in your code (ie. no [UnconditionalSuppressMessage]).

ghostnguyen commented 4 months ago

I have created a fully support AOT, no reflection and almost identical to Refit's usage. Just wonder if it has any help? https://www.nuget.org/packages/HttpClientFiller

kant2002 commented 4 months ago

When I last time look at it make AOt compatible require a bit of refactoring. https://github.com/reactiveui/refit/pull/1414 That was first direction.

Basically you should extract AOT compatible subset of API or create new helpers in Refit so source generators can leverage that. I would start with having model completely independent from Relection

reactiveui / refit

feature: Refit should be linker-friendly and support trimming #1389