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

Maui's usage of Reflection to find op_Implicit operators is not trim-compatible #5023

Closed
eerhardt opened this issue Mar 2, 2022 · 5 comments
Labels
area-publishing Issues with the app packaging/publishing process (ipk/apk/msix/trimming) area-xaml XAML, CSS, Triggers, Behaviors fixed-in-9.0.0-preview.3.10457 platform/android 🤖 platform/iOS 🍎 s/try-latest-version Please try to reproduce the potential issue on the latest public version t/app-size Application Size / Trimming t/enhancement ☀️ New feature or request
Projects

Comments

@eerhardt
Copy link
Member

eerhardt commented Mar 2, 2022

Description

We are getting ILLink trim warnings in the following code:

internal static MethodInfo GetImplicitConversionOperator(this Type onType, Type fromType, Type toType)
{
#if NETSTANDARD1_0
var mi = onType.GetRuntimeMethod("op_Implicit", new[] { fromType });
if (mi == null) return null;
if (!mi.IsSpecialName) return null;
if (!mi.IsPublic) return null;
if (!mi.IsStatic) return null;
if (!toType.IsAssignableFrom(mi.ReturnType)) return null;
return mi;
#else
var bindingAttr = BindingFlags.Public | BindingFlags.Static | BindingFlags.FlattenHierarchy;
IEnumerable<MethodInfo> mis = null;
try
{
mis = new[] { onType.GetMethod("op_Implicit", bindingAttr, null, new[] { fromType }, null) };
}

Here are the warnings:

C:\git\maui\src\Controls\src\Core\Xaml\TypeConversionExtensions.cs(270,5): Trim analysis warning IL2070: Microsoft.Maui.Controls.Xaml.TypeConversionExtensions.GetImplicitConversionOperator(Type,Type,Type): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.Type.GetMethod(String,BindingFlags,Binder,Type[],ParameterModifier[])'. The parameter 'onType' of method 'Microsoft.Maui.Controls.Xaml.TypeConversionExtensions.GetImplicitConversionOperator(Type,Type,Type)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\git\WeatherTwentyOne\src\WeatherTwentyOne\WeatherTwentyOne.csproj]
C:\git\maui\src\Controls\src\Core\Xaml\TypeConversionExtensions.cs(275,24): Trim analysis warning IL2070: Microsoft.Maui.Controls.Xaml.TypeConversionExtensions.GetImplicitConversionOperator(Type,Type,Type): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.Type.GetMethods(BindingFlags)'. The parameter 'onType' of method 'Microsoft.Maui.Controls.Xaml.TypeConversionExtensions.GetImplicitConversionOperator(Type,Type,Type)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\git\WeatherTwentyOne\src\WeatherTwentyOne\WeatherTwentyOne.csproj]

This issue is that if the trimmer doesn't see these op_Implicit operators being used in IL, it will trim them. This will cause the above code to behave differently in a trimmed application, since the operator is removed. This code won't find it, and it won't be used to do the conversions that it is supposed to do.

Options

I can only think of 2 high-level options to fix this:

  1. Remove this logic from Maui and only allow TypeConverters to be used to convert values. This means anything that relies on these op_Implicit operators from working would need to be fixed. But at least the behavior would be consistent with or without trimming.
  2. Teach the trimmer (somehow, either intrinsically or a custom step) to preserve these op_Implicit operators. We did something similar for System.Linq.Expressions in Preserving operator methods necessary for System.Linq.Expressions linker#1821.

cc @mattleibow @PureWeen @StephaneDelcroix @vitek-karas

Steps to Reproduce

(These steps are fuzzy, let me know if you need more detailed steps)

  1. Implement a BindableProperty that uses an op_Implicit operator to do a conversion from a string.
  2. Use that BindableProperty in an app.
  3. Trim the app

Notice that the conversion through the op_Implicit operator doesn't happen in the trimmed app because it was trimmed away.

Version with bug

Preview 13 (current)

Last version that worked well

Unknown/Other

Affected platforms

iOS, Android

Affected platform versions

All

Did you find any workaround?

No response

Relevant log output

No response

@eerhardt eerhardt added the t/bug Something isn't working label Mar 2, 2022
@jsuarezruiz jsuarezruiz added this to New in Triage Mar 3, 2022
@vitek-karas
Copy link
Member

Ideally we would pursue 1. if possible - remove this code from Maui.

If that doesn't work, we could extend the fix we did for Linq.Expression to also handle MAUI. It's kind of a hack, but I don't really see any other option. I think it would be good to measure the impact of this - what it would mean to include all operators everywhere in MAUI. Technically we could do a targeted fix and only include implicit operators, but that would mean slightly different (and yet another) hacky behavior compared to Linq.Expressions in the linker - would love to avoid that.

@Eilon Eilon added the t/app-size Application Size / Trimming label Mar 3, 2022
@mattleibow
Copy link
Member

Is this an improvement? #5073

Should we do the whole thing differently? What happens if we have a non - generic base or something? We cast anyways, so we can maybe get the value and then cast. It will throw us out was wrong.

@StephaneDelcroix StephaneDelcroix removed the t/bug Something isn't working label Mar 7, 2022
@StephaneDelcroix
Copy link
Contributor

the reason op_implicit is there isn't to convert from string (we have TypeConverters for that), but for the other cases, like when you provide a Color to a Brush. Iirc it was added for Shell support, so participants in the Shell can be promoted to their parent (you declare a ShellContent when it expects a ShellSection).

As with everything else XAML and Binding, we don't know what we'll be used til runtime unless we enforce XamlC everywhere. That's something I'd want, but won't happen right away

@mattleibow mattleibow added legacy-area-perf Startup / Runtime performance area-xaml XAML, CSS, Triggers, Behaviors t/enhancement ☀️ New feature or request labels Feb 10, 2023
@mattleibow mattleibow added this to the Backlog milestone Feb 10, 2023
@ghost
Copy link

ghost commented Feb 10, 2023

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@mattleibow mattleibow removed the legacy-area-perf Startup / Runtime performance label Feb 10, 2023
@Eilon Eilon added the area-publishing Issues with the app packaging/publishing process (ipk/apk/msix/trimming) label May 10, 2024
@samhouts samhouts added the s/try-latest-version Please try to reproduce the potential issue on the latest public version label Jul 1, 2024
Copy link
Contributor

Hi @eerhardt. We have added the "s/try-latest-version" label to this issue, which indicates that we'd like you to try and reproduce this issue on the latest available public version. This can happen because we think that this issue was fixed in a version that has just been released, or the information provided by you indicates that you might be working with an older version.

You can install the latest version by installing the latest Visual Studio (Preview) with the .NET MAUI workload installed. If the issue still persists, please let us know with any additional details and ideally a reproduction project provided through a GitHub repository.

This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

@dotnet-policy-service dotnet-policy-service bot removed this from the Backlog milestone Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-publishing Issues with the app packaging/publishing process (ipk/apk/msix/trimming) area-xaml XAML, CSS, Triggers, Behaviors fixed-in-9.0.0-preview.3.10457 platform/android 🤖 platform/iOS 🍎 s/try-latest-version Please try to reproduce the potential issue on the latest public version t/app-size Application Size / Trimming t/enhancement ☀️ New feature or request
Projects
No open projects
Development

No branches or pull requests

6 participants