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

[XC] Avoid NativeAOT trim warnings for compiled bindings #25396

Conversation

simonrozsival
Copy link
Member

@simonrozsival simonrozsival commented Oct 19, 2024

Contributes to #25406

Description of Change

The BindingExtension class has an attribute that tells XamlC to include the XamlTypeResovler in the service provider when calling the ProvideValue method:

[RequireService([typeof(IXamlTypeResolver), typeof(IXamlDataTypeProvider)])]
public sealed class BindingExtension : IMarkupExtension<BindingBase>

The XamlTypeResovler is not trim and AOT compatible. Fortunately, it is NOT necessary when the binding is compiled. This PR adds a workaround that will prevent generating the faulty code in .NET 9. I think we should follow-up on this later in .NET 10 and properly compile the BindingExtension (see the referenced issue for more details).

@PureWeen PureWeen added this to the .NET 9.0 GA milestone Oct 19, 2024
@jsuarezruiz jsuarezruiz added the area-xaml XAML, CSS, Triggers, Behaviors label Oct 21, 2024
@simonrozsival simonrozsival changed the title [WIP] Avoid creating XamlTypeResolver for compiled bindings to avoid ILC warnings [XC] Avoid NativeAOT trim warnings for compiled bindings Oct 21, 2024
@simonrozsival simonrozsival marked this pull request as ready for review October 21, 2024 09:24
@simonrozsival simonrozsival requested a review from a team as a code owner October 21, 2024 09:24
@simonrozsival simonrozsival requested review from PureWeen and tj-devel709 and removed request for a team October 21, 2024 09:24
Comment on lines 432 to 435
if (context.ValidateOnly)
{
yield break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for reverting #24493 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this change, the test doesn't work (the service provider isn't instantiated at all so I cannot inspect the code that we generate). I'm considering letting the tests pass and then remove the test + the changes in this file and keep just the fix itself to avoid perf regressions (cc @jonathanpeppers).

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK to take the if (ValidateOnly) check out, if this fixes something. It's still in a couple other places.

The problem with it generally:

  • If there is some code path that needs to emit an error/warning
  • The if (ValidateOnly) would skip it

So, I was trying to put it places where we'd get free perf and not lose any errors or warnings.

@jonathanpeppers
Copy link
Member

/azp run

Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@@ -151,6 +151,8 @@ public class XamlCTask : XamlTask
/// </summary>
public bool ValidateOnly { get; set; }

internal bool GenerateFullILInValidateOnlyMode { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Does a .targets file need to set this? It could be a private property like $(_MauiGenerateFullILInValidateOnlyMode) for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is meant just for tests and so it's internal. I don't think this can even be set from the targets file.

@simonrozsival
Copy link
Member Author

Closing in favor of #25420

@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-xaml XAML, CSS, Triggers, Behaviors
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants