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] Add feature switch to enable compiling bindings with source #24924

Conversation

simonrozsival
Copy link
Member

Description of Change

This PR adds a new feature switch IsXamlCBindingWithSourceCompilationEnabled which is disabled by default unless the app is built with <PublishAot>true</PublishAot> or with <TrimMode>full</TrimMode>.

This feature switch should make it easier to migrate existing apps to .NET 9. In some apps, bindings that previously worked would now stop working (see #23711 for customer feedback).

To make it possible to enable this feature by default in the future, I added a new warning XC0025 that will be shown by XamlC for all bindings that are not compiled because this feature is disabled.

I also modified the error message when there is a mismatch between the declared x:DataType and the actual binding context. This is based on feedback in #24349. The error message will now contain the actual conflicting types and it should help developers identify which binding is failing more easily. I also added this warning to TypedBinding which also rejects inputs that don't match the expected source type. This change should make it easier to enable the new feature switch in existing apps or when migrating to NativeAOT/full trimming.

/cc @jonathanpeppers @StephaneDelcroix @PureWeen @vitek-karas

Issues Fixed

Fixes #23711
Fixes #24349

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. 👍 I just had the one minor comment.

=> methodDef.Body.Instructions.Any(instruction =>
instruction.OpCode == OpCodes.Newobj
&& instruction.Operand is MethodReference methodRef
&& methodRef.DeclaringType.Name == "TypedBinding`2");
Copy link
Member

Choose a reason for hiding this comment

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

Would the 2 change if a new type was introduced? Would this be more future proof?

Suggested change
&& methodRef.DeclaringType.Name == "TypedBinding`2");
&& methodRef.DeclaringType.Name.StartsWith("TypedBinding`"));

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see why we would change TypedBinding to have a different number of generic arguments. It hasn't changed in a long time and it is (internal) public API. I would personally keep the test as is for now.

@@ -175,6 +175,7 @@
DebugSymbols = "$(DebugSymbols)"
DebugType = "$(DebugType)"
DefaultCompile = "true"
CompileBindingsWithSource = "$(MauiEnableXamlCBindingWithSourceCompilation)"
Copy link
Contributor

Choose a reason for hiding this comment

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

changing the API will cause problems and force clean/rebuild while changing version, but there's nothing we can do about it

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is OK since this should be part of RC2 and not a servicing release. Maybe it would be worth mentioning in release notes?

@PureWeen PureWeen added this to the 9.0-rc2 milestone Sep 25, 2024
@samhouts
Copy link
Member

do we need to highlight this as a compatibility change?

@rmarinho rmarinho merged commit 0d0f25a into dotnet:net9.0 Sep 27, 2024
121 checks passed
@PureWeen
Copy link
Member

PureWeen commented Oct 1, 2024

do we need to highlight this as a compatibility change?

This used to be a breaking change that wasn't fully documented. This PR ensures that users maintain more compatibility now when going from net8 to net9.

Users can now only enable this path via a feature switch.

@simonrozsival and @davidbritch are working on notes related to this and NativeAOT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-xaml XAML, CSS, Triggers, Behaviors fixed-in-net9.0-nightly This may be available in a nightly release!
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants