Fixed IL Trimming warnings on DesktopGL/WindowsDX#8915
Conversation
|
@Lucasharskamp can you point to the Android/iOS methods that have an issue? I can reach out to the .net teams and see what we can do about it. |
|
I don't understand this part. There are already mechanisms in place to warn users what to do should this happen, and the documentation you're pointing to already states to enable trim analyzers and fix warnings to that effect. Also, mobile platforms are not limited to .NET 4.5, feel free to switch any code to .NET 8 if that makes sense and don't break platforms that need the compatibility. |
My suggestion is to add an additional warning in there "watch out, make sure your own custom |
|
Please feel free to propose enhancements in that regard. Another idea of improvement that we had, would be to be smarter with how type constructors are retrieved by All things considered, this PR looks good to me as-is. |
If the documentation site is based on Sandcastle/MAML (I think it is?), please add the following at the end of https://docs.monogame.net/articles/getting_started/preparing_for_consoles.html#no-use-of-runtime-reflection (it's displayed as a yellow alert warning box) |
Upgraded in #8921 |
I could've handled that, I just didn't because I didn't want to violate the ".NET Framework 4.5" rule. (Otherwise I would've done so) |
|
I'm approving these changes but we'll need an intermediate |
Hopefully the @SimonDarksideJ has the NuGet Testing step in place we can do a Aot check as part of the build test phase. |
Agreed. I've naturally tested it in a local build, but that's inviting a "but it works on my machine!" issue. (Also given I've only tested it on a single platform output) |
|
Let's merge and validate on another |
|
Added [Task] @Lucasharskamp to validate this in the next |
Both Android and iOS using old .net 4.5 methods for `Enum.GetValues` and `Marshal.GetFunctionPointerForDelegate`. This commit updates them to use the modern `Enum.GetValues<T>()` and `Marshal.GetFunctionPointerForDelegate<T>()` methods. We also update iOS to use `AppContext.BaseDirectory` instead of `typeof(AL).Assembly.Location`, which is the correct property to use in .NET 6+. Context #8915
Fixes # The goal is simple: Publish builds that are AOT/Trimmed will not break randomly anymore at build/trim/publish time. All IL Warnings in DesktopGL/WindowsDX are resolved. (Not Android/iOS) They are gone from the build and resolved by either tackling the problem or applying a "good enough" bandaid. Fixing IL trimmer warnings boils down to the API Contract; if we know at the time trimming happens (during AOT compilation) what shouldn't be trimmed from a specific type, we're golden. However, if a type's elements are needed because it gets dynamically invoked during runtime, and the trimmer doesn't know the type will be utilized for that, it can break. And yes, I am aware `[DynamicallyAccessedMembers(..)]` isn't magic; it doesn't check if X is in a type, it only ensures X won't be trimmed. (if it knows which type it shouldn't perform that action on). So, this PR can be broken down into several sections where different trimming problems get resolved: **1) "Pass the parcel" (VertexBuffer.cs, VertexDeclaration.cs,)** These are simple; just send up the callstack what we need from the generic type `T` or `Type type` that is being utilized; it usually becomes the problem of the assembly that is consuming the project now (e.g. through the NuGet Package). In most cases, the compiler of the consumer will take the hint and not trim the required bits of the type the user puts in there. In other cases, the compiler will throw a warning the user can resolve themselves. This should resolve any problem anyone may have regarding VertexBuffers. **2) ContentReaders** These are a bit trickier, but basically boil down to "anyone writing custom readers must ensure nothing gets trimmed from these readers". Unfortunately, we can't really throw warnings like "if you use `ContentTypeReader<T>`, please ensure nothing gets trimmed". So that's just a warning to put in the manual. I recommend adding a section to the following document to reflect that: https://docs.monogame.net/articles/getting_started/preparing_for_consoles.html **3) TypeConverters** The problem with `TypeConverter`s is that they were designed to be utilized during runtime by default, not set up during compile time. Which creates issues like: `TypeConverter conv = TypeDescriptor.GetConverter(typeof(Foo));` Of course the trimmer won't have a clue that it's not supposed to trim `FooConverter` (hence why all converters have a "retain everything" sticker on them), but _also_ that it shouldn't trim the bits and pieces of `Foo` the converter needs. In the case of MonoGame, this problem emerges because the converters try to convert from/to types that are derived from `IPackedVector`. Meaning: types derived from `IPackedVector` must still retain their interfaces and their public parameterless constructors. To resolve this, I've placed ''retain interfaces/public parameterless constructor'' on the types that derive from `IPackedVector` to ensure it wouldn't break that function. (But only if XNADESIGNPROVIDED is set, given that the type converters won't be used otherwise anyway) _(But so many of the types derived from `IPackedVector` don't even have public parameterless constructors for use by `Activator.CreateInstance(typeof(T))` in VectorConversions.cs->ConvertToFromVector4? But, naturally, anyone who wants to produce AOT/Trimmed versions of their product in MonoGame wouldn't likely even touch TypeConverters, but here we are, just in case)_ **4) Obsolete methods** A few method calls are obsolete, stating they shouldn't be used, in part because of the trimming problem of the actions inside the method. I just put a "disable the warning" on there - it's not a problem, it'll get dealt with anyway. Given that the `[Obsolete(...)]` throws a warning anyway, we can blindfold the compiler for the IL warnings without issue. **Android** Android has 2 unique IL warnings which boil down to the fact it has to utilize .NET Framework 4.5 methods and not their cleaner, better alternatives which were introduced in 4.5.1. Better to leave them there. There is nothing gained by blindfolding the compiler. **iOS** iOS has the same issue as Android; 2 unique IL warnings, but ''More modern alternatives exist, but .NET Framework 4.5 requirements''. (`AppContext.BaseDirectory` was introduced in .NET Framework 4.6, `Marshal.GetDelegateForFunctionPointer` has a better alternative in 4.5.1, as a comment above the method call laments this fact)
Fixes # The goal is simple: Publish builds that are AOT/Trimmed will not break randomly anymore at build/trim/publish time. All IL Warnings in DesktopGL/WindowsDX are resolved. (Not Android/iOS) They are gone from the build and resolved by either tackling the problem or applying a "good enough" bandaid. ### Description of Change Fixing IL trimmer warnings boils down to the API Contract; if we know at the time trimming happens (during AOT compilation) what shouldn't be trimmed from a specific type, we're golden. However, if a type's elements are needed because it gets dynamically invoked during runtime, and the trimmer doesn't know the type will be utilized for that, it can break. And yes, I am aware `[DynamicallyAccessedMembers(..)]` isn't magic; it doesn't check if X is in a type, it only ensures X won't be trimmed. (if it knows which type it shouldn't perform that action on). So, this PR can be broken down into several sections where different trimming problems get resolved: **1) "Pass the parcel" (VertexBuffer.cs, VertexDeclaration.cs,)** These are simple; just send up the callstack what we need from the generic type `T` or `Type type` that is being utilized; it usually becomes the problem of the assembly that is consuming the project now (e.g. through the NuGet Package). In most cases, the compiler of the consumer will take the hint and not trim the required bits of the type the user puts in there. In other cases, the compiler will throw a warning the user can resolve themselves. This should resolve any problem anyone may have regarding VertexBuffers. **2) ContentReaders** These are a bit trickier, but basically boil down to "anyone writing custom readers must ensure nothing gets trimmed from these readers". Unfortunately, we can't really throw warnings like "if you use `ContentTypeReader<T>`, please ensure nothing gets trimmed". So that's just a warning to put in the manual. I recommend adding a section to the following document to reflect that: https://docs.monogame.net/articles/getting_started/preparing_for_consoles.html **3) TypeConverters** The problem with `TypeConverter`s is that they were designed to be utilized during runtime by default, not set up during compile time. Which creates issues like: `TypeConverter conv = TypeDescriptor.GetConverter(typeof(Foo));` Of course the trimmer won't have a clue that it's not supposed to trim `FooConverter` (hence why all converters have a "retain everything" sticker on them), but _also_ that it shouldn't trim the bits and pieces of `Foo` the converter needs. In the case of MonoGame, this problem emerges because the converters try to convert from/to types that are derived from `IPackedVector`. Meaning: types derived from `IPackedVector` must still retain their interfaces and their public parameterless constructors. To resolve this, I've placed ''retain interfaces/public parameterless constructor'' on the types that derive from `IPackedVector` to ensure it wouldn't break that function. (But only if XNADESIGNPROVIDED is set, given that the type converters won't be used otherwise anyway) _(But so many of the types derived from `IPackedVector` don't even have public parameterless constructors for use by `Activator.CreateInstance(typeof(T))` in VectorConversions.cs->ConvertToFromVector4? But, naturally, anyone who wants to produce AOT/Trimmed versions of their product in MonoGame wouldn't likely even touch TypeConverters, but here we are, just in case)_ **4) Obsolete methods** A few method calls are obsolete, stating they shouldn't be used, in part because of the trimming problem of the actions inside the method. I just put a "disable the warning" on there - it's not a problem, it'll get dealt with anyway. Given that the `[Obsolete(...)]` throws a warning anyway, we can blindfold the compiler for the IL warnings without issue. ### Not changed **Android** Android has 2 unique IL warnings which boil down to the fact it has to utilize .NET Framework 4.5 methods and not their cleaner, better alternatives which were introduced in 4.5.1. Better to leave them there. There is nothing gained by blindfolding the compiler. **iOS** iOS has the same issue as Android; 2 unique IL warnings, but ''More modern alternatives exist, but .NET Framework 4.5 requirements''. (`AppContext.BaseDirectory` was introduced in .NET Framework 4.6, `Marshal.GetDelegateForFunctionPointer` has a better alternative in 4.5.1, as a comment above the method call laments this fact)
)" This reverts commit 18e37a7.
Fixes #
The goal is simple: Publish builds that are AOT/Trimmed will not break randomly anymore at build/trim/publish time.
All IL Warnings in DesktopGL/WindowsDX are resolved. (Not Android/iOS) They are gone from the build and resolved by either tackling the problem or applying a "good enough" bandaid.
Description of Change
Fixing IL trimmer warnings boils down to the API Contract; if we know at the time trimming happens (during AOT compilation) what shouldn't be trimmed from a specific type, we're golden. However, if a type's elements are needed because it gets dynamically invoked during runtime, and the trimmer doesn't know the type will be utilized for that, it can break.
And yes, I am aware
[DynamicallyAccessedMembers(..)]isn't magic; it doesn't check if X is in a type, it only ensures X won't be trimmed. (if it knows which type it shouldn't perform that action on).So, this PR can be broken down into several sections where different trimming problems get resolved:
1) "Pass the parcel" (VertexBuffer.cs, VertexDeclaration.cs,)
These are simple; just send up the callstack what we need from the generic type
TorType typethat is being utilized; it usually becomes the problem of the assembly that is consuming the project now (e.g. through the NuGet Package). In most cases, the compiler of the consumer will take the hint and not trim the required bits of the type the user puts in there. In other cases, the compiler will throw a warning the user can resolve themselves. This should resolve any problem anyone may have regarding VertexBuffers.2) ContentReaders
These are a bit trickier, but basically boil down to "anyone writing custom readers must ensure nothing gets trimmed from these readers". Unfortunately, we can't really throw warnings like "if you use
ContentTypeReader<T>, please ensure nothing gets trimmed". So that's just a warning to put in the manual. I recommend adding a section to the following document to reflect that: https://docs.monogame.net/articles/getting_started/preparing_for_consoles.html3) TypeConverters
The problem with
TypeConverters is that they were designed to be utilized during runtime by default, not set up during compile time. Which creates issues like:TypeConverter conv = TypeDescriptor.GetConverter(typeof(Foo));Of course the trimmer won't have a clue that it's not supposed to trim
FooConverter(hence why all converters have a "retain everything" sticker on them), but also that it shouldn't trim the bits and pieces ofFoothe converter needs.In the case of MonoGame, this problem emerges because the converters try to convert from/to types that are derived from
IPackedVector. Meaning: types derived fromIPackedVectormust still retain their interfaces and their public parameterless constructors. To resolve this, I've placed ''retain interfaces/public parameterless constructor'' on the types that derive fromIPackedVectorto ensure it wouldn't break that function. (But only if XNADESIGNPROVIDED is set, given that the type converters won't be used otherwise anyway)(But so many of the types derived from
IPackedVectordon't even have public parameterless constructors for use byActivator.CreateInstance(typeof(T))in VectorConversions.cs->ConvertToFromVector4? But, naturally, anyone who wants to produce AOT/Trimmed versions of their product in MonoGame wouldn't likely even touch TypeConverters, but here we are, just in case)4) Obsolete methods
A few method calls are obsolete, stating they shouldn't be used, in part because of the trimming problem of the actions inside the method. I just put a "disable the warning" on there - it's not a problem, it'll get dealt with anyway. Given that the
[Obsolete(...)]throws a warning anyway, we can blindfold the compiler for the IL warnings without issue.Not changed
Android
Android has 2 unique IL warnings which boil down to the fact it has to utilize .NET Framework 4.5 methods and not their cleaner, better alternatives which were introduced in 4.5.1. Better to leave them there. There is nothing gained by blindfolding the compiler.
iOS
iOS has the same issue as Android; 2 unique IL warnings, but ''More modern alternatives exist, but .NET Framework 4.5 requirements''. (
AppContext.BaseDirectorywas introduced in .NET Framework 4.6,Marshal.GetDelegateForFunctionPointerhas a better alternative in 4.5.1, as a comment above the method call laments this fact)