Skip to content

[Trimming] Use type converters instead of implicit cast operators (part 1/2) - #21050

Merged
rmarinho merged 8 commits into
dotnet:net9.0from
simonrozsival:type-converters-instead-of-implicit-cast-operators-part-1
Mar 26, 2024
Merged

rmarinho merged 8 commits into
dotnet:net9.0from
simonrozsival:type-converters-instead-of-implicit-cast-operators-part-1

Conversation

@simonrozsival

Copy link
Copy Markdown
Member

Description of Change

This is a first of two PRs that introduce the possibility to define type conversions using TypeConverters instead of relying on finding and invoking op_Implicit at runtime via reflection. The main reason for this change is that the way we're working with implicit operators is not trimming-compatible.

This PR introduces the internal TypeConversionHelper that contains all the updated conversion logic. The second PR will add type converters for all types that have implicit operators in MAUI.

Will this break any existing app?

No. This is an opt-in feature. Either the app is trying to run the app with NativeAOT and this feature is necessary for the app to work, or they manually opted into the feature by setting the feature switch to false (see FeatureSwitches.md).

What if a library or an app doesn't introduce type converters?

When MAUI tries to convert a value and there isn't a suitable type converter, it will try to look up the implicit operator and if it finds it, it will print a runtime warning. This is to give at least some feedback to the customer while making sure the app behaves the same way in Debug mode with Mono and in Release mode with NativeAOT.

I considered throwing an exception instead, but it seems common practice in MAUI to print warnings and fall back to some default value (for example if there's a binding where the path isn't compatible with the given binding context).

What if a library or an app needs to add a type converter for a third-party type?

There's an escape hatch in the form of the app builder extension method to globaly register a type converter for a type. See FeatureSwitches.md.

Why TypeConverter and not IValueConverter?

I chose TypeConverters over IValueConverters because they seem to be better suited for this use case. On the other hand, IValueConverters are easier to write. Feedback is welcome.

Issues Fixed

Fixes #19922
Fixes #5023
Fixes #19397 - we hit 0 trimming warnings in dotnet new maui on iOS! 🎉

/cc @jonathanpeppers @vitek-karas @StephaneDelcroix

@simonrozsival
simonrozsival requested a review from a team as a code owner March 6, 2024 13:07
@jsuarezruiz jsuarezruiz added the area-xaml XAML, CSS, Triggers, Behaviors label Mar 6, 2024
@jsuarezruiz
jsuarezruiz requested review from PureWeen and removed request for jsuarezruiz March 6, 2024 14:27
Comment thread docs/design/FeatureSwitches.md Outdated
Comment thread src/Controls/src/Core/TypeConversionHelper.cs Outdated
Comment thread src/Controls/src/Core/TypeConversionHelper.cs Outdated
?? toType.GetImplicitConversionOperator(fromType: value.GetType(), toType: toType);

if (opImplicit != null)
if (TypeConversionHelper.TryConvert(value, toType, out var convertedValue))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Technically this does change the existing behavior - if there was a TypeConverter, it would not have been used, but now it will be, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, if the existing type converter can convert between the two types, it would be used. But that's the same case as in BindableProperty.TryConvert. If we wanted to make sure that behavior doesn't change, we would have to never use type converters when the MauiImplicitCastOperatorsUsageViaReflectionSupport feature switch is not set.

This behavior would definitely make it even more in line with "don't break any existing app", but maybe it would make adoption of type converters instead of implicit casts slower? This is a good question and I'm not sure which option I prefer.

@StephaneDelcroix StephaneDelcroix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if this doesn't affect current behaviour unless NativeAOT is enabled, I'm good

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-xaml XAML, CSS, Triggers, Behaviors fixed-in-9.0.0-preview.3.10457

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants