Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NativeAOT and remove warnings #171

Merged
merged 1 commit into from
Jul 23, 2024
Merged

Fix NativeAOT and remove warnings #171

merged 1 commit into from
Jul 23, 2024

Conversation

xoofx
Copy link
Contributor

@xoofx xoofx commented Jul 22, 2024

Fixes #156

Hello there ☺️

This PR is attempting to fix the crash/incompatibility with NativeAOT:

  • Breaking change for IZLoggerFormatter which shouldn't be an issue. In all the call sites of FormatLogEntry, TEntry is always IZLoggerEntry and not an actual value type, so forcing IZLoggerEntry on the interface will avoid the crash at NativeAOT runtime Failed to create generic virtual method implementation (In general, it's better to avoid generic method with generic parameters in interfaces):
      public interface IZLoggerFormatter
      {
          bool WithLineBreak { get; }
    -      void FormatLogEntry<TEntry>(IBufferWriter<byte> writer, TEntry entry)
    -           where TEntry : IZLoggerEntry;
           void FormatLogEntry(IBufferWriter<byte> writer, IZLoggerEntry entry);
      }
  • Add several [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] to generics mostly to help with the usage of Json behind the scene.
  • Added a few [UnconditionalSuppressMessage] for IL3050 and IL2026 due to the usage of several methods from JsonSerializer.Serialize which takes an object. The ones that are using JsonSerializerOptions should be fine as the user can set its own instance to SystemTextJsonZLoggerFormatter.JsonSerializerOptions and initialize it with a NativeAOT/Source generator version. There are other places where JsonSerializer.Serialize is used, but without the options. I'm not sure exactly if it will be a problem (I'm not using JsonSerializer for now). Ideally the same JsonSerializerOptions should be propagated to all these places, but it looks like JsonSerializer.Serialize in these cases might be only used for primitives.

@neuecc
Copy link
Member

neuecc commented Jul 23, 2024

Thank you, I appreciate your in-depth knowledge!
I am glad that I could not have made this fix myself.
I will study it.

We will release it as soon as possible!

@neuecc neuecc merged commit 83c3c26 into Cysharp:master Jul 23, 2024
@MIRIMIRIM
Copy link
Contributor

ZLogger.Internal.MagicalBox.ReaderCache.EnumStringWrite<T> and ZLogger.Internal.MagicalBox.ReaderCache.EnumUtf8Write<T> maybe also need [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)]?

@xoofx
Copy link
Contributor Author

xoofx commented Jul 28, 2024

ZLogger.Internal.MagicalBox.ReaderCache.EnumStringWrite<T> and ZLogger.Internal.MagicalBox.ReaderCache.EnumUtf8Write<T> maybe also need [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)]?

They don't because the functions used behind (e.g var name = EnumDictionary<T>.GetStringName(v);) are resolved in the EnumDictionary constructor, and EnumDictionary should be tagged there already on the generic parameter.

@MIRIMIRIM
Copy link
Contributor

When building a project with ZLogger using PublishAot, warnings related to EnumStringWrite<T> and EnumUtf8Write<T> appear. These seems to be related to MagicalBox.TryRead<T>, but warnings don't occur when building ZLogger enable IsAotCompatible.

@xoofx
Copy link
Contributor Author

xoofx commented Jul 28, 2024

MagicalBox.TryRead<T> is not doing anything particular, mainly using RuntimeHelpers.IsReferenceOrContainsReferences<T>(), Unsafe.ReadUnaligned<T> and Unsafe.SizeOf<T>() and don't need DynamicallyAccessedMembers.

That being said, these warnings might surface in a conservative way, so these methods might require indeed DynamicallyAccessedMembers to avoid warnings during PublishAot. Please make a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some AOT warning
3 participants