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

[MONO][WASM] Fix System.ComponentModel.TypeConverter tests by adding missing ILLink dependencies #50645

Merged
merged 4 commits into from
Apr 15, 2021

Conversation

naricc
Copy link
Contributor

@naricc naricc commented Apr 2, 2021

When EnableAggressiveTrimming=true, these types get trimmed out, which causes the System.ComponentModel.TypeConverter to fail. This adds the missing assemblies/types to ILLink.Descriptor.xunit.xml, preserving them during the trimming process, and also introduces a project specific ILLink descriptor.

@naricc naricc requested a review from mdh1418 April 2, 2021 14:34
@ghost ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Apr 2, 2021
@ghost
Copy link

ghost commented Apr 2, 2021

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

When EnableAggressiveTrimming=true, these types get trimmed out, which causes the System.ComponentModel.TypeConverter to fail. This adds the missing assemblies/types to ILLink.Descriptor.xunit.xml, preserving them during the trimming process.

Author: naricc
Assignees: -
Labels:

linkable-framework

Milestone: -

@naricc naricc requested review from LBynum77 and lewing April 2, 2021 14:46
@naricc naricc added the arch-wasm WebAssembly architecture label Apr 2, 2021
@ghost
Copy link

ghost commented Apr 2, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

When EnableAggressiveTrimming=true, these types get trimmed out, which causes the System.ComponentModel.TypeConverter to fail. This adds the missing assemblies/types to ILLink.Descriptor.xunit.xml, preserving them during the trimming process.

Author: naricc
Assignees: -
Labels:

arch-wasm, area-Infrastructure-libraries, linkable-framework

Milestone: -

@naricc naricc added the test-bug Problem in test source code (most likely) label Apr 2, 2021
@naricc naricc removed the request for review from LBynum77 April 2, 2021 14:47
@mdh1418
Copy link
Member

mdh1418 commented Apr 2, 2021

Is it known why parts of Castle.Core are getting linked out? Does it look like a linker bug or is it some reflection related reason that the linker will never be able to detect the assembly is needed?

For either reason, I think it is worth noting either in this PR or possibly as a comment above the assembly inclusion in the ILLink.Description.xunit.xml file.

On another note, maybe it is worth renaming ILLink.Description.xunit.xml since it will not be xunit specific assemblies. Or maybe we can add another .xml file that will include test suite needed assemblies (that are not xunit) and add that to

<TrimmerRootDescriptor Include="$(MSBuildThisFileDirectory)ILLink.Descriptor.xunit.xml" />

    <ItemGroup>
      <TrimmerRootDescriptor Include="$(MSBuildThisFileDirectory)ILLink.Descriptor.xunit.xml" />
      <TrimmerRootDescriptor Include="$(MSBuildThisFileDirectory)ILLink.Descriptor.<some-name>.xml" />
    </ItemGroup>

@naricc
Copy link
Contributor Author

naricc commented Apr 2, 2021

@mdh1418

Adding another ILLink file makes sense; should I do it in this PR, or a follow up?

The actual place that ends up needing theses constructors is here (and similar things): https://github.com/castleproject/Core/blob/d88e94447150996973d947a150ffda5efc0c3c1c/src/Castle.Core/DynamicProxy/Tokens/InvocationMethods.cs#L28

This is using reflection, but it guess it is conceivable that some sufficiently smart static analysis could figure that out. How clever is the linker expected to be?

@mdh1418
Copy link
Member

mdh1418 commented Apr 2, 2021

@naricc I think adding it to this PR would make sense in that in addition to adding xunit.runner.utility.netcoreapp10 in ILLink.Descriptor.xunit.xml to get System.ComponentModel.TypeConverter tests to pass, non xunit assemblies need to be linked in as well, by being introduced in a file alongside ILLink.Descriptor.xunit.xml.

But it seems like Castle.Core cannot be resolved on the runtime pipeline Build Browser wasm Release AllSubsets_Mono lane

How clever is the linker expected to be?

I don't know in detail, but I understand there is general issues with reflection. @marek-safar may know better for this situation.

@vitek-karas
Copy link
Member

The sample you noted above https://github.com/castleproject/Core/blob/d88e94447150996973d947a150ffda5efc0c3c1c/src/Castle.Core/DynamicProxy/Tokens/InvocationMethods.cs#L28 is something linker should be able to figure out just fine. You can try setting SuppressTrimAnalysisWarnings=false and see all the warnings coming from the "castle code" - linker generates these warnings everytime it sees a pattern it can't handle.

@naricc
Copy link
Contributor Author

naricc commented Apr 6, 2021

@vitek-karas So I ran it with SuppressTrimAnalysisWarnings=false, and it seems like it is not actually lossing track of it where I thought it was. Here are all the castle related errors: https://gist.github.com/naricc/f3cb83c71bb804bce8548331b62a220c

There isn't one involving CompositionInvocation invocation constructor. But I am then unsure why including those assemblies in TrimmerRootDescriptor makes any difference?

@naricc naricc marked this pull request as draft April 6, 2021 04:06
@naricc naricc marked this pull request as ready for review April 10, 2021 03:33
@vitek-karas
Copy link
Member

Found the issue - the problem is that Castle.Core uses special extension methods over reflection - they look identical in C# and they added them into System.Reflection namespace, so it's practically invisible in source file. But in IL they don't call Type.GetConstructor, instead they call NetCoreReflectionExtensions.GetConstructor which is not annotated.

Running with SuppressTrimAnalysisWarnings=false produces lot of warnings, one of which is:

ILLink: Trim analysis error IL2067: System.Reflection.NetCoreReflectionExtensions.GetConstructor(Type,BindingFlags,Object,Type[],Object[]): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors', 'DynamicallyAccessedMemberTypes.NonPublicConstructors' in call to 'System.Reflection.TypeExtensions.GetConstructors(Type,BindingFlags)'. The parameter 'type' of method 'System.Reflection.NetCoreReflectionExtensions.GetConstructor(Type,BindingFlags,Object,Type[],Object[])' 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.

Which is the root of the problem. The fix in this case would be to annotate the method in Castle, but honestly, Castle has so many other warnings... not sure if this one alone would fix the larger problem.

@naricc naricc requested a review from radical April 13, 2021 17:13
Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

LGTM 👍
In a follow up PR (wip in #50885), this will get used in other tests also.

@SamMonoRT
Copy link
Member

@naricc @radical ---- LGTM - Anything blocking merging this PR ?

@naricc naricc merged commit c2f2a1c into dotnet:main Apr 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 15, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Infrastructure-libraries linkable-framework Issues associated with delivering a linker friendly framework test-bug Problem in test source code (most likely)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants