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

Disable and trim 'Marshaler<T>' on Native AOT #1907

Open
wants to merge 16 commits into
base: staging/2.3
Choose a base branch
from

Conversation

Sergio0694
Copy link
Member

This type is super cursed and causes a ton of code to be rooted. It's also not really needed anymore on Native AOT. This PR makes its static constructor throw right away on Native AOT, allowing all its code to be removed. This should save a fair amount 🤞

@Sergio0694 Sergio0694 requested a review from manodasanW January 29, 2025 07:55
@Sergio0694 Sergio0694 changed the base branch from master to staging/2.3 January 29, 2025 07:55
@dongle-the-gadget
Copy link
Contributor

Should we make this pay-for-play instead (using a feature flag, for instance) rather than a complete breaking change?

@Sergio0694
Copy link
Member Author

@dongle-the-gadget there's two considerations for why we're not doing that:

  • There should be no "play" here. Things should just keep working the same, because pretty much all marshalling code paths that used to rely on Marshaler<T> now rely on generated code from the AOT generator. Those who don't, we should just fix.
  • Disabling this makes us add proper AOT annotations, so we get diagnostics to further help enforce not using them.

@@ -83,9 +145,18 @@ public static object FromAbi(IntPtr ptr)

public static unsafe object GetValue(IInspectable inspectable)
{
#if NET
Copy link
Member

Choose a reason for hiding this comment

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

I need to double check what our behavior here is today when boxed arrays are marshaled from native side.

Copy link
Member Author

Choose a reason for hiding this comment

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

This type should never be used on AOT (I left some comments above the type explaining it). Unless I'm missing some way it could still somehow end up being referenced on NAOT? I couldn't find any possible entry points though 🤔

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

Successfully merging this pull request may close these issues.

3 participants