Annotate is native aot compatible #1532
Conversation
|
Hi I think this looks promising. I don't have any experience with AOT myself, but at a glance it looks good. Any thoughts @lipchev ? @OleRoss Could you please target |
|
I did a quick test on my unreleased branch- and as far as I can tell there was only a single .cs file that needed to change: private static TAttribute? GetAttribute<
#if NET
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor | DynamicallyAccessedMemberTypes.PublicFields)]
#endif
TAttribute>(ITypeDescriptorContext? context) where TAttribute : UnitAttributeBase
{I would probably annotate directly the class itself, as from the above code I can barely tell where's the start and end of the parameter definition. 😄 I also did a quick refactoring on my I haven't got the C++ stuff installed, so wasn't able to actually test if it really works, but my |
- Add a PolyFill class to InternalHelpers to allow for usage of attributes in netstandard - Add EnumHelpers class to InternalHelpers to support a single API for different target frameworks - Update generation of RegisterDefaultConversions to not use reflection but generate necessary bindings directly - Update generations of Units to use EnumHelpers - Add generic overloads where a type is passed in a method - Annotate methods where I did not find a simple fix with RequiresDynamicCode attributes
OleRoss
force-pushed
the
annotate-is-native-aot-compatible
branch
from
18518f0 to
be0b245
Compare
|
Hi @angularsen, @lipchev, thanks for taking a look at the pr. I rebased the PR on the release/v6 branch. @lipchev did you do some changes I just purged? Additionally, could you elaborate |
I merged #1507 an hour or so ago, but I think your merge conflicts are probably coming from the file changes in the
I'm no expert, but this appears to work (I copy pasted it from the #if NET
[RequiresDynamicCode("The native code for this instantiation might not be available at runtime.")]
[RequiresUnreferencedCode("If some of the generic arguments are annotated (either with DynamicallyAccessedMembersAttribute, or generic constraints), trimming can't validate that the requirements of those annotations are met.")]
#endif
public class JsonQuantityConverter : JsonConverterFactory |
…ative-aot-compatible # Conflicts: # UnitsNet/CustomCode/UnitAbbreviationsCache.cs # UnitsNet/CustomCode/UnitsNetSetup.cs # UnitsNet/UnitConverter.cs
| // TODO I think we should either return empty or throw QuantityNotFoundException here | ||
| var enumValues = Enum.GetValues(unitEnumType).Cast<Enum>(); | ||
| var all = GetStringUnitPairs(enumValues, formatProvider); | ||
| return all.Select(pair => pair.Item2).ToList(); | ||
| // var enumValues = Enum.GetValues(unitEnumType).Cast<Enum>(); | ||
| // var all = GetStringUnitPairs(enumValues, formatProvider); | ||
| // return all.Select(pair => pair.Item2).ToList(); | ||
| throw new QuantityNotFoundException("No quantity information was found for the type."); |
There was a problem hiding this comment.
@lipchev I updated the branch to include your merged PR.
Unfortunatelly, I cannot use Enum.GetValues(unitEnumType) with AOT, but because of the todo comment I see two options:
- Return empty or throw an exception (would make the Enum.GetValues call redundant
- Split the logic into two methods, one with a generic overload and one with an attribute
For now, I throw an exception (one idea from the comment) but that leads to failing unit tests. I would wait until the todo is resolved before doing more.
There was a problem hiding this comment.
Yeah, that's one of the few TODO's I've got left.
@angularsen In #1507 You kind a left it at "Reasonable, but out of scope of this PR" - how do you think we should handle this, throw or empty?
There was a problem hiding this comment.
I think we should definitely throw, with some helpful exception message instructing to add the QuantityInfo to the UnitAbbreviationsCache. I guess this normally only happens for custom quantities, or when creating a blank new abbreviation cache for whatever reason.
It seems we don't yet support adding QuantityInfo to an existing cache, so we probably also need a way add custom quantities to the default singleton cache:
UnitAbbreviationsCache.Default.AddQuantity(HowMuch.Info); // QuantityInfo
UnitAbbreviationsCache.Default.MapUnitToAbbreviation(HowMuchUnit.AShitTon, culture, "my shiny new abbreviation");There was a problem hiding this comment.
Just to add, although we probably could continue mapping units to abbreviations without a QuantityInfo, it feels more coherent to reuse the QuantityInfo and UnitInfo structures for this. A unit always belongs to a quantity, and I think it's fair to require both.
Also, this decision will probably affect approximately zero people out there.
|
@OleRoss |
Hi, I did not find any information about AOT compatibility on this project, but I got some IL warnings when building a project of mine.
I annotated both
UnitsNetandUnitsNet.NumberExtensionsprojects. I skippedUnitsNet.Serialization.JsonNetas this would probably need bigger changes.Things done:
I tried to make minimal changes only, but possibly breaking changes are
structconstraint to generic enum-constrained methodsRegisterDefaultConversionsFeel free to merge or reject, but I would be happy to have those annotations included in the project to make life easier when working with NativeAOT.