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

[Trimming] Fix remaining trimming warnings related to XAML parsing #20474

Merged

Conversation

simonrozsival
Copy link
Member

Description of Change

This PR fixes 5 trimming warnings in 2 scenarios which are related to runtime XAML parsing:

  • BindablePropertyConverter - the converter is completely unused when all XAML is compiled because there is a compiled version of the converter (Microsoft.Maui.Controls.XamlC.BindablePropertyConverter). This code is analyzed by ILC because it is linked through the [TypeConverter] attribute on BindableProperty. The exceptions will be thrown in the scenario when XAML runtime parsing is disabled and the developer calls LoadFromXaml().
  • XamlParser - the XamlParser.GetElementType is called (only) from XamlServiceProvider through IXamlTypeResolver. The IXamlTypeResolver is only used when parsing XAML at runtime. Code generated by the XAML compiler which uses XamlServiceProvider doesn't need to use IXamlTypeResovler.

Issues Fixed

Contributes to #19397

/cc @StephaneDelcroix

@rmarinho
Copy link
Member

/rebase

@jsuarezruiz jsuarezruiz removed their request for review February 12, 2024 14:04
@@ -23,6 +24,13 @@ public override bool CanConvertTo(ITypeDescriptorContext context, Type destinati

object IExtendedTypeConverter.ConvertFromInvariantString(string value, IServiceProvider serviceProvider)
{
if (!RuntimeFeature.IsXamlRuntimeParsingSupported)
{
throw new NotSupportedException("XAML runtime parsing is not supported. " +
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a localized message ?

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 think this should be localized but I'm not sure what our guidelines for localization are. This exception shouldn't be reached under normal circumstances and it's mostly there to guide the trimmer. Also, maybe the NotSupportedException isn't the right exception here and I should change it to InvalidOperationException (to keep it in line with https://github.com/dotnet/maui/blob/net9.0/src/Controls/src/Core/ResourceDictionary.cs#L65 ?)

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't localize runtime exception. only compile time ones

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed, InvalidOperationException is probably better

@jonathanpeppers jonathanpeppers self-requested a review February 13, 2024 16:39
Comment on lines 25 to +30
object IExtendedTypeConverter.ConvertFromInvariantString(string value, IServiceProvider serviceProvider)
{
if (!RuntimeFeature.IsXamlRuntimeParsingSupported)
{
throw new InvalidOperationException(RuntimeFeature.XamlRuntimeParsingNotSupportedErrorMessage);
}
Copy link
Member

@jonathanpeppers jonathanpeppers Feb 13, 2024

Choose a reason for hiding this comment

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

It looks like the converter has a XamlC (build-time) replacement:

[Xaml.ProvideCompiled("Microsoft.Maui.Controls.XamlC.BindablePropertyConverter")]
public sealed class BindablePropertyConverter : TypeConverter, IExtendedTypeConverter

So it seems we should be fine to throw here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, XamlC uses the other class to generate the IL directly. The code that I added to throw won't be ever called for any XAML that's compiled. It can only be reached if the developer manually calls LoadFromXaml.

@simonrozsival
Copy link
Member Author

Test failures seem unrelated to this PR to me.

@rmarinho rmarinho merged commit 4023296 into dotnet:net9.0 Feb 14, 2024
41 of 47 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants