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

VisualTypeConverter loops over all assemblies looking for IVisual types #4937

Closed
eerhardt opened this issue Feb 25, 2022 · 5 comments
Closed
Labels
area-publishing Issues with the app packaging/publishing process (ipk/apk/msix/trimming) fixed-in-9.0.0-preview.2.10293 platform/android 🤖 platform/iOS 🍎 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/app-size Application Size / Trimming (sub: perf) t/bug Something isn't working

Comments

@eerhardt
Copy link
Member

Description

We are getting trimmer warnings here:

C:\git\maui\src\Controls\src\Core\Visuals\VisualTypeConverter.cs(75,26): Trim analysis warning IL2026: Microsoft.Maui.Controls.VisualTypeConverter.Register(Assembly,Dictionary<String,IVisual>): Using member 'System.Reflection.Assembly.GetExportedTypes()' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Types might be removed. [C:\DotNetTest\MauiTest\MauiTest.csproj]
C:\git\maui\src\Controls\src\Core\Visuals\VisualTypeConverter.cs(77,7): Trim analysis warning IL2062: Microsoft.Maui.Controls.VisualTypeConverter.Register(Assembly,Dictionary<String,IVisual>): Value passed to parameter 'visual' of method 'Microsoft.Maui.Controls.VisualTypeConverter.Register(Type,Dictionary<String,IVisual>)' can not be statically determined and may not meet 'DynamicallyAccessedMembersAttribute' requirements. [C:\DotNetTest\MauiTest\MauiTest.csproj]

The issue is that the VisualTypeConverter looks at all the assemblies in the process looking for types that implement IVisual:

void InitMappings()
{
var mappings = new Dictionary<string, IVisual>(StringComparer.OrdinalIgnoreCase);
Assembly[] assemblies = AppDomain.CurrentDomain.GetAssemblies();
// Check for IVisual Types
foreach (var assembly in assemblies)
Register(assembly, mappings);
if (Internals.Registrar.ExtraAssemblies != null)
foreach (var assembly in Internals.Registrar.ExtraAssemblies)
Register(assembly, mappings);

static void Register(Assembly assembly, Dictionary<string, IVisual> mappings)
{
if (assembly.IsDynamic)
return;
try
{
foreach (var type in assembly.GetExportedTypes())
if (typeof(IVisual).IsAssignableFrom(type) && type != typeof(IVisual))
Register(type, mappings);

This is not trim-safe, since Types that are not referenced by IL can be trimmed by the trimmer. But in the converter code, it takes a string object and then tries looking up in the "registered mapping" for the IVisual by the string name. This can break trimmed apps because the Type being referenced by that string may have been trimmed.

In talking with @Redth and @PureWeen this approach is out-of-date, and we should remove the assembly scanning. Instead, we should add a "trim-compatible" method for doing this registration. For example:

  1. Using an assembly-level attribute (which we already have: lower in that InitMapping method).
  2. Adding a new builder.RegisterVisual<TVisual>() method that people can add to their CreateMauiApp() method. This will register the Visual type statically, and the Types that aren't referenced can be safely trimmed.

Steps to Reproduce

  1. dotnet new maui
  2. dotnet restore
  3. dotnet build -f net6.0-android -r android-arm64 -t:Run -c Release --no-restore -p:SuppressTrimAnalysisWarnings=false -p:TrimmerSingleWarn=false /bl
  4. Inspect the trimmer warnings for mentions of VisualTypeConverter.

Version with bug

Unknown/Other (please specify)

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 Feb 25, 2022
@PureWeen PureWeen self-assigned this Feb 25, 2022
@eerhardt eerhardt added the t/app-size Application Size / Trimming (sub: perf) label Mar 8, 2022
@Redth
Copy link
Member

Redth commented Mar 23, 2022

@PureWeen is this done? can this be closed now? I see a PR linked that was closed but not merged.

@Redth Redth added this to the 6.0.300 milestone Mar 23, 2022
@PureWeen
Copy link
Member

PureWeen commented Mar 23, 2022

@Redth not yet

The solution for this changed once we decided to move a bunch of stuff around between Controls and Core. A new PR still need to be put together but getting this fully working is dependent on unwinding the Registrar code and getting that moved into Compatibility

@samhouts samhouts added the s/verified Verified / Reproducible Issue ready for Engineering Triage label May 2, 2022
@samhouts samhouts modified the milestones: 6.0.300, 6.0.300-servicing May 2, 2022
@PureWeen PureWeen removed their assignment Aug 3, 2022
@Redth Redth modified the milestones: 6.0-servicing, Backlog Aug 30, 2022
@ghost
Copy link

ghost commented Aug 30, 2022

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.

@samhouts samhouts removed the s/verified Verified / Reproducible Issue ready for Engineering Triage label Apr 5, 2023
@jinxinjuan jinxinjuan added s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage labels Jun 14, 2023
@jinxinjuan
Copy link

Verified this issue with Visual Studio Enterprise 17.7.0 Preview 1.0. Can repro with provided repro steps in terminal.

@rmarinho
Copy link
Member

rmarinho commented Feb 9, 2024

Fixed by #20417

@rmarinho rmarinho closed this as completed Feb 9, 2024
@rmarinho rmarinho modified the milestones: Backlog, .NET 9 Planning Feb 9, 2024
@rmarinho rmarinho moved this from Todo to Done in MAUI SDK Ongoing Feb 9, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 11, 2024
@Eilon Eilon added the area-publishing Issues with the app packaging/publishing process (ipk/apk/msix/trimming) label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-publishing Issues with the app packaging/publishing process (ipk/apk/msix/trimming) fixed-in-9.0.0-preview.2.10293 platform/android 🤖 platform/iOS 🍎 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/app-size Application Size / Trimming (sub: perf) t/bug Something isn't working
Projects
Status: Done
Development

No branches or pull requests

7 participants