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

Add trim/AOT annotations to make code trim/AOT-safe #1222

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

Sergio0694
Copy link

@Sergio0694 Sergio0694 commented Apr 14, 2024

Closes #1218

This PR adds all trimming and AOT annotations to the code to make the codebase trim/AOT-safe. There's no functional changes introduced in this PR, other than removing the fast column accessor for all value types on NAOT, as that would otherwise throw (and generally just be unsupported code due to the AOT warning we'd have to suppress otherwise). It should still work, just take the slower reflection-based fallback path in that case. In order to allow all annotations to correctly flow to consumers, all projects have also been switched to multi-targeting, with the .NET 8 TFM being added as well.

src/SQLite.cs Outdated Show resolved Hide resolved
@mattleibow
Copy link

mattleibow commented Apr 23, 2024

To reduce all these changes and the sheer number of #if blocks, would it be worth adding these attribute types as internal types just for netstandard?

I have been doing for Skia and MAUI: https://github.com/mono/SkiaSharp/blob/main/source/Common/NullableAttributes.generated.cs

The attributes are ignored anyway on netstandard.

@Sergio0694
Copy link
Author

@mattleibow I thought about that (in fact I was just going to use PolySharp), but I decided not to because from my understanding SQLite-net also supports source-only mode. In that case if you were to define local attributes you could also get into weird situations if consumers already have polyfills for those (which is not uncommon), and you could get anything from warnings to build errors due to amboguous type references. Thoughts?

@mattleibow
Copy link

hmm, ok yeah, the source only mode will be more annoying. You could wrap the polyfills in a new #if !INCLUDE_POLY or something. But then if you are using plain netstd you will have to set this as well as if you have some of the items.

So yeah, I see the better choice really is these #if on each one.

@@ -752,7 +953,18 @@ public CreateTableResult CreateTable (Type ty, CreateFlags createFlags = CreateF
/// <returns>
/// Whether the table was created or migrated for each type.
/// </returns>
public CreateTablesResult CreateTables<T, T2> (CreateFlags createFlags = CreateFlags.None)
#if NET8_0_OR_GREATER
[UnconditionalSuppressMessage ("Trimming", "IL2026", Justification = "This method preserves metata for all type arguments.")]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor spelling mistake

Comment on lines +223 to +233
#if NET8_0_OR_GREATER
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
#endif
T,
#if NET8_0_OR_GREATER
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
#endif
T2,
#if NET8_0_OR_GREATER
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
#endif

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to have it all inside of a single #ifdef?

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.

NativeAOT support plans
5 participants