graphql-dotnet
[DEMO] More AOT annotations
I tried to follow AOT annotations guidelines and I have a bad feeling about this all.
Problems:
- It is very toxic for code - even worse then async/await "infection" or NRT annotations.
- I do not understand why we should manually write annotations - the analyzer already knows what is missing in specific cases and demands to put it into code. It looks strange. I hope that in NET8 this will be fixed and it will not be necessary to manually do the work. Though it may be the same story as for NRT - analizer knows but it's a developer responsibility to say the last word.
- I do not understant what to do in 50% of cases when
Typecomes from some calculated variable. - It seems to me that we "excessive" demand trimmer to preserve some type's parts in methods with branch logic (for example, type.GetProperties() in the first branch and type.GetMethods() in the second).
I'm skeptical about such code annotation.
One more.
5. Analyzer require to have the same annotations in virtual-override methods - https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/trim-warnings/il2092. See IGraphTypeMappingProvider, AutoRegisteringGraphTypeMappingProvider and ManualGraphTypeMappingProvider. First I places annotations on AutoRegisteringGraphTypeMappingProvider - OK. Then on IGraphTypeMappingProvider - well... OK. But then I had to do it for ManualGraphTypeMappingProvider but there is no to do with clrType here - just a single compare operation!
- It is very toxic for code - even worse then async/await "infection" or NRT annotations.
I have no problem at all with async/await or NRT annotations, and on the contrary appreciate the explicitness of async/await and NRT annotations. In this case, the annotations are only needed where we are using reflection, and necessarily so if we want to provide the best experience for the user with trimming support. Other libraries that do not use trimming would not need such annotations. Honestly, there are no annotations within the majority of the codebase; nothing in DocumentExecuter, the execution strategies, schema printer, validation rules, and most other classes.
At the same time, we make extensive use of reflection, such that if we were to follow the suggested annotations based on build warnings, we would likely spread the 'infection' throughout the entire codebase, primarily due to a few specific methods -- namely ToObject, GetGraphTypeFromType, the SchemaTypes class and NameFieldResolver. So, what we really need to is determine where the suppressions and warnings need to be placed, and all will be well.
- I do not understand why we should manually write annotations - the analyzer already knows what is missing in specific cases and demands to put it into code. It looks strange. I hope that in NET8 this will be fixed and it will not be necessary to manually do the work. Though it may be the same story as for NRT - analizer knows but it's a developer responsibility to say the last word.
There certainly are a few annotations that the compiler is requiring that should not be necessary; namely static methods which calls other annotated methods. They are certainly not required. Perhaps for virtual methods they might be due to the methods being overridden. I assume this must be to ease analysis of libraries built on these methods; perhaps at design time. It is possible that with .NET 8 the analyzer rule will be updated to reduce this need. However, most of the build warnings addressed in this PR do not fall in this category, and make sense to me.
- I do not understant what to do in 50% of cases when
Typecomes from some calculated variable.
I can review. I think there is a logical answer, but often that answer might to simply mark a whole method or class as [RequiresUnreferencedCode] and suppress the warning elsewhere, for ease of development more than anything else.
- It seems to me that we "excessive" demand trimmer to preserve some type's parts in methods with branch logic (for example, type.GetProperties() in the first branch and type.GetMethods() in the second).
I think that is the case in a few places. I have no problem with that; better the trimmer trims to little than too much. Allowing the user to trim their assembly has a significant benefit, I believe, in certain circumstances, rather than requiring that the user's assembly be rooted for any use of GraphQL.NET.
- Analyzer require to have the same annotations in virtual-override methods
Nowhere would I add annotations simply because the analyzer suggests we do so. The analyzer may not be correct. We may need to suppress warnings in some cases, either because the warning is incorrect, or because it is not applicable in the scenario.
What we don't yet have are attributes indicating where it is proper to suppress warnings.
I agree. It's a bit tricky yet to figure out the right way to go with trimming, not much experience here.
If you like, I can create a PR targeting this PR with my suggested changes from this review, plus some of the suppression attributes that I suggest we add.
Actually I think that I'll close this one to continue with the series of some smaller PRs. I'm not sure about all my changes here. I would like first to add trimming annotations in those places that 100% safe to add.
100% safe to add
You could say that "everywhere" is "100% safe to add". We should be more concerned with "100% correct to add". We do not want to unnecessarily add warnings throughout the codebase where they instead should properly be suppressed.
In this case, the annotations are only needed where we are using reflection, and necessarily so if we want to provide the best experience for the user with trimming support.
I would say that the problem is about transitivity in trimming annotations. It is not obvious to me at what stage (method/type) these annotations should stop.
Other libraries that do not use trimming would not need such annotations.
use -> support I would say. Apps can use (can require) trimming on publish as I understand, not libs.
So, what we really need to is determine where the suppressions and warnings need to be placed, and all will be well.
I agree. From my side I still do not feel this edge.
but often that answer might to simply mark a whole method or class as [RequiresUnreferencedCode] and suppress the warning elsewhere, for ease of development more than anything else.
I see that you are constantly using a pair of something like "let's use [RequiresUnreferencedCode] here" immediately followed by something like "and suppress the warning elsewhere" 😄 . I don't get it. Why to add annotations at all if then suppress all usages of the marked APIs?
We should be more concerned with "100% correct to add".
Yes, I mean correct instead of safe.
@Shane32 I would not merge this. Let's keep it for some time as a reference/reminder. Instead I would like to add new AOT annotations one by one (1-5-10 APIs) so that each PR will only change a small part of the codebase.
I agree. I also tried to make some minor updates yesterday to annotate just ToObject but found that there was no way to properly annotate the method, since it is recursive and operates on list types. This restriction then applies to the calls to Field within InputObjectGraphType (inherited members from ComplexGraphType). I feel we will find this type of issue persists throughout the codebase. We have a choice of either suppressing all aberrations, "doing our best" and expecting it will work in most situations, or else in addition to "doing our best" we also mark the entry point(s) as [RequiresUnreferencedCode] similar to how the MS DI provider works.
@sungam3r As you know, .NET 8 has a heavy focus on AOT compatibility I've been reading up on their approach, and it seems that one technique they use often to provide an AOT-compatible package is to detect known incompatibilities and throw an exception. For example, one of their goals was to provide an AOT-compatible dependency injection provider. However, if a value type was registered (such as MyStruct), and then the enumerable type was requested (e.g. IEnumerable<MyStruct>), there would be a runtime error since static code analysis could not reveal that the array type of MyStruct was needed. In their case the solution was to throw an exception if any value-type was registered, considering that the likelyhood of this need was rare.
In a similar manner, we could check the IsDynamicCodeSupported property and throw an exception for certain situations where it is not practical to simply mark the method as [RequiresDynamicCode]. One such example is adding a schema-initialization check for any field resolvers that use NameFieldResolver. We can throw an exception in this case, so it is clear to a developer that the code will not operate correctly in production when they are debugging it. Another situation is input object CLR classes which include an IEnumerable<T> or IList<T> property of value types. While List<T> of a value type would be able to be analyzed by static code analysis, an interface would not.
Of course other methods that simply fail to run correctly can be marked as [RequiresDynamicCode]
Maybe, maybe. My general feeling is about trim analyzer should be smart enough to do 90% of things that we have to do now marking code with [DynamicallyAccessedMemberTypes(..)]. After all, the compiler "sees" all calls to reflection methods, invoking ctors, etc. Why should I as developer do all that things? It looks ridiculous. Maybe MS was not ready with that feature 100% for NET7 so they released it as is requiring 100500 hints from developers to guide trimmer.
I don't want to guide trimmer so much. This is a work for a compiler/trimmer itself, not a person.