search

microsoft / kiota-abstractions-dotnet

Abstractions library for the Kiota generated SDKs in dotnet
https://aka.ms/kiota/docs
MIT License
24 stars 21 forks source link

Reduce IParseNode Enum methods DynamicallyAccessedMembers scope #202

Closed hwoodiwiss closed 4 months ago

hwoodiwiss commented 4 months ago

This will need to be manually updated across the other repos that implement IParseNode

fixes #201

hwoodiwiss commented 4 months ago

Thanks for this @hwoodiwiss

Would you also be able to submit PRs for the serialization libs taking a dependency on IParseNode? If not, I can probably do that once we merge this one in.

I'm happy to update the Serialization libraries for this

hwoodiwiss commented 4 months ago

I'm going to need to take a deeper look at this, looking at the usage of GetEnumName we're always going to pass Enum as the generic type, as generic type information isn't available above ReplaceEnumValueByStringRepresentation so we'll have to rely on Runtime type information which will make this incompatible with AOT.

I'll revert this part of the change for now.

hwoodiwiss commented 4 months ago

I think fully supporting AOT compilation is going to need some deeper changes that will likely also require changes to the public API, specifically around RequestInformation.QueryParameters. If you think it makes sense, I'll open an issue that can run long to track and discuss fully supporting AOT.

baywet commented 4 months ago

Please go ahead with the issue. Do you think we should hold to merge this PR to get resolution? or do you believe this PR is already an improvement over the current situation?

hwoodiwiss commented 4 months ago

Please go ahead with the issue. Do you think we should hold to merge this PR to get resolution? or do you believe this PR is already an improvement over the current situation?

Cool, I'll open something this evening.

I think this is still an improvement, it gives a better base going forward, and this was by far the largest producer of trim warnings I saw when compiling AOT, just due to how many times it's used, plus nothing the serialization libraries are doing at the moment requires more than DynamicallyAccessedMemberTypes.PublicFields, and shouldn't need to.

MichalStrehovsky commented 4 months ago

Enumerating fields on enums is always trimming/AOT safe. If the T is restricted to be Enum, it's fine to access public static fields on it and suppress the warning. See https://github.com/dotnet/runtime/issues/97737.

I.e. even if this is GetNames<T>(T o) where T : Enum { o.GetType().GetFields(BindingFlags.Static | .Public) will work and the warning can be suppressed. This guarantees o is some enum and that's enough.

baywet commented 4 months ago

Alright, thanks everyone for the context! I'll hand it over to @andrueastman for a final review, merge and publish!

hwoodiwiss commented 4 months ago

Enumerating fields on enums is always trimming/AOT safe. If the T is restricted to be Enum, it's fine to access public static fields on it and suppress the warning. See dotnet/runtime#97737.

I.e. even if this is GetNames<T>(T o) where T : Enum { o.GetType().GetFields(BindingFlags.Static | .Public) will work and the warning can be suppressed. This guarantees o is some enum and that's enough.

Ahh, okay, that makes sense. I'll amend this to take that into account this evening. Thank you for the info. In which case, after this, the abstractions library should be trimming safe.