Skip to content

Failure to preserve types enhancements #34734

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

Merged
merged 9 commits into from
Feb 18, 2025
Merged

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Feb 13, 2025

Fixes #34654

Pavel ... Could you take a look at this one?

Two of the approaches work as I describe.

One approach, the Root Descriptor approach, I can't get to work using several permutations of assembly, type, and method ... assembly only ... assembly and method only ... or all three. If it won't be easy to figure out why this approach isn't working, should we merge with just the [DynamicDependency] attribute approach and the custom type approach? If so, I'll clip the Root Descriptor approach out of this, and I'll make a tracking item entry to try again to cover it some other time.


Internal previews

📄 File 🔗 Preview link
aspnetcore/blazor/host-and-deploy/configure-trimmer.md aspnetcore/blazor/host-and-deploy/configure-trimmer
aspnetcore/blazor/javascript-interoperability/index.md aspnetcore/blazor/javascript-interoperability/index

@guardrex guardrex self-assigned this Feb 13, 2025
@guardrex guardrex requested a review from pavelsavara February 14, 2025 14:47
@pavelsavara
Copy link
Member

pavelsavara commented Feb 17, 2025

You can find many examples of ILLink.Descriptors.xml in the runtime repo.
It may make sense to have full path via $(MSBuildThisFileDirectory) in the <TrimmerRootDescriptor Include="$(MSBuildThisFileDirectory)ILLink.Descriptors.xml" />

To see if your file was noticed or not, you can make it invalid xml and you should get fail. If nothing happened, somehow your TrimmerRootDescriptor didn' work.

I don't know if TrimmerRootDescriptor in MSbuild also works well for library projects and transitive dependencies of the trimmed app. Worth exploring.

Trimmer team prefers attributes over XML, because they don't have those problems.

The last option of using custom type instead is probably poor workaround, do we know why we say that ?
It would only work if the type was in assembly which doesn't have trimming enabled, which I guess is a (bad) default for app assemblies.
If we really want to propose that, I would treat it as very last resort.

@guardrex
Copy link
Collaborator Author

guardrex commented Feb 18, 2025

@pavelsavara ... No luck here this morning.

Let's take these one at a time ...

Breaking the XML

The first thing is breaking the XML to get an error to know if the file is being found. I took the opening <linker> tag out of the file; and either with or without $(MSBuildThisFileDirectory), I don't see an error anywhere at compile time or runtime. The app publishes normally AFAICT. Where would the error appear?

Method signature

I'm not sure what it should be. Nothing I've tried has worked. I'll leave it on the PR for now as ...

<method signature="System.Void .ctor(System.String,System.String)" />

... and that might be incorrect. However, we don't even know if my descriptor file is being picked up yet. If not, then all of my testing on method sigs was a waste of time.

Custom type

The last option of using custom type instead is probably poor workaround, do we know why we say that ?

It came from Javier here ...

dotnet/aspnetcore#52947 (comment)

I'm going to update that section to say that it will work but that we don't recommend the approach.

@pavelsavara
Copy link
Member

I don't see an error anywhere at compile time

It means it's not being noticed by the msbuild scripts.

Do you have it in <ItemGroup> </ItemGroup> ?

@guardrex
Copy link
Collaborator Author

I do ...

<ItemGroup>
  <TrimmerRootDescriptor Include="$(MSBuildThisFileDirectory)ILLink.Descriptors.xml" />
</ItemGroup>

@guardrex
Copy link
Collaborator Author

guardrex commented Feb 18, 2025

... with a broken file ...

  <assembly fullname="System.Runtime">
    <type fullname="System.Collections.Generic.KeyValuePair">
      <method signature="System.Void .ctor(System.String,System.String)" />
    </type>
  </assembly>
</linker>

... and when I publish, I have my publish profile set up to delete all files before publishing ...

image

If you want, I can put the project up in a GH repo.

@pavelsavara
Copy link
Member

If you want, I can put the project up in a GH repo.

yes

@guardrex
Copy link
Collaborator Author

🐀🍔 Roasted Rat Burgers! 🐀🍔

Ok ... I just made a major discovery here. The project that I'm using is a per-component BWA. I had the descriptor file set up in the server project 🙈 and just discovered that it has to be in the .Client project.

Let me try again now that I know what I was doing wrong. I'll get back to you in a bit .....................

@guardrex
Copy link
Collaborator Author

guardrex commented Feb 18, 2025

Yeah ... that seems ok now. I did get the broken XML file warning, fixed the file, and now it seems to be picking it up.

Next quesiton tho ... what is the correct method signature for the KeyValuePair example to make it work?

It isn't ...

<method signature="System.Void .ctor(System.String,System.String)" />

💥

crit: Microsoft.AspNetCore.Components.WebAssembly.Rendering.WebAssemblyRenderer[100]
Unhandled exception rendering component: ConstructorContainsNullParameterNames, System.Collections.Generic.KeyValuePair2[System.String,System.String]
System.NotSupportedException: ConstructorContainsNullParameterNames, System.Collections.Generic.KeyValuePair2[System.String,System.String]

... and not ...

<method signature="System.Void .ctor(TKey,TValue)" />

I can keep trying with brute force, but I'll wait for you to advise me the format of the signature for this.

@pavelsavara
Copy link
Member

    <type fullname="System.Collections.Generic.KeyValuePair`2">
        <method signature="System.Void .ctor()" />
        <method signature="System.Void .ctor(System.String,System.String)" />
    </type>

Try which of them works, with or without params.

@guardrex
Copy link
Collaborator Author

They both fail with ...

ILLink.Descriptors.xml(4,8): Warning IL2009: Could not find method 'System.Void .ctor()' on type 'System.Collections.Generic.KeyValuePair<TKey,TValue>'.

ILLink.Descriptors.xml(5,8): Warning IL2009: Could not find method 'System.Void .ctor(System.String,System.String)' on type 'System.Collections.Generic.KeyValuePair<TKey,TValue>'.

@pavelsavara
Copy link
Member

<method signature="System.Void .ctor(TKey,TValue)" /> ?

@guardrex
Copy link
Collaborator Author

The error during build went away, but the component is still throwing ......

crit: Microsoft.AspNetCore.Components.WebAssembly.Rendering.WebAssemblyRenderer[100]
Unhandled exception rendering component: ConstructorContainsNullParameterNames, System.Collections.Generic.KeyValuePair2[System.String,System.String] System.NotSupportedException: ConstructorContainsNullParameterNames, System.Collections.Generic.KeyValuePair2[System.String,System.String]

@pavelsavara
Copy link
Member

ConstructorContainsNullParameterNames

Why this is not working for you dotnet/runtime#102850
You are testing Net9 right ?

Maybe for Net8 and older @javiercn 's advice to use custom type is necessary fallback.

@guardrex
Copy link
Collaborator Author

Correct ... 9.0 app.

Do you want me to place the custom type back for <=8.0, or should this only have the [DynamicDependency] attribute approach for <=8.0?

@guardrex
Copy link
Collaborator Author

guardrex commented Feb 18, 2025

... so the fix is really going to be for .NET 10? ... I say so because the original issue is still open at ...

dotnet/aspnetcore#52947

... marked for .NET 10 and apparently covered by ...

dotnet/runtime#112216

If so, then we only have the [DynamicDependency] attribute approach and the custom type approach, and those two are valid for all versions. Then, we'll add this file descriptor approach at .NET 10's release (but it will work back to 8.0)?

This is very confusing! 😆

@pavelsavara
Copy link
Member

pavelsavara commented Feb 18, 2025

Now I understand more: There are 2 levels to this problem

  1. trimming the class/constructor
  2. trimming the parameter names

In dotnet/runtime#112216 Larry suggest to make trimming much less aggressive, which will probably solve both issues. This is only for Net 8

I believe that after dotnet/runtime#102850 rooting the ctor 1) will also keep names 2)
This was merged in May, so I assume it landed in Net9.
If it's still broken, we need to open issue and provide clear steps to reproduce.

@guardrex
Copy link
Collaborator Author

guardrex commented Feb 18, 2025

I see.

Should I put my test app up just so that you can check and make sure I have everything correct?

I just put it up here ....

https://github.com/guardrex/BlazorTrimmerTesting

You say a new issue, but dotnet/aspnetcore#52947 is still open. Seems like they just wouldn't close that one and keep working on it.

UPDATE: I added a comment to the open issue.

@guardrex
Copy link
Collaborator Author

I've noted in the article text using my tracking moniker ("UPDATE 10.0") to circle back around later for more work to bring back the file descriptor approach and drop the custom types approach.

@pavelsavara
Copy link
Member

Does the DynamicDependency solve the ConstructorContainsNullParameterNames issue ?

@guardrex
Copy link
Collaborator Author

Yes, it does.

@guardrex guardrex merged commit b7eb0c8 into main Feb 18, 2025
3 checks passed
@guardrex guardrex deleted the guardrex/blazor-trimmer-example branch February 18, 2025 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update complex type trimming remarks
2 participants