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

Don't trim ShouldSerializeXXX and ResetXXX members that may be called with reflection #102780

Merged
merged 5 commits into from
Jun 12, 2024

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented May 28, 2024

Fixes #102244

This change makes sure these members exist in the inbox version of the System.Data.Common assembly.

The TypeDescriptor reflection-based model, the XML serializer, and the Json.NET serializer use these naming conventions to call the members with reflection.

Copy link
Contributor

Tagging subscribers to this area: @roji, @ajcvickers
See info in area-owners.md if you want to be subscribed.

@steveharter steveharter requested a review from buyaa-n June 10, 2024 20:16
@steveharter steveharter added this to the 9.0.0 milestone Jun 10, 2024
@steveharter steveharter changed the title Don't trim members called by TypeDescriptor's use of reflection Don't trim ShouldSerializeXXX and ResetXXX members that may be called with reflection Jun 10, 2024
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

This doesn't look like the right fix. #102244 (comment) and the subsequent discussion discuss why.

I think this should use ILLink.LibraryBuild file like the original issue was suggesting so that these get preserved when we build the repo. After repo build it can be trimmed like anything else. ILLink.LibraryBuild is designed for these cases.

These DynamicDependency will keep things unconditionally in trimmed apps even if nobody uses TypeDescriptor/Json.NET/etc. It's a global fix for a local problem. It will regress the size for everyone, including the well behaved apps that don't use trim unsafe code this tries to fix. Everything that could need this should already be marked as trim unsafe. We don't need trim unsafe code to work in trimmed apps, it is okay and expected that trim unsafe code will be broken after trimming. It is the responsibility of whoever is turning on trimming on trim unsafe code to author XML descriptors if they wish to take the hard path.

The TypeDescriptor reflection-based model and the XML and Json.NET serializers use these conventions.

These are also trim unsafe and not expected to work after trimming. The issue this PR tries to address is a drop in the bucket, there are more fundamental trimming problems with these serializers.

@steveharter
Copy link
Member Author

This doesn't look like the right fix. #102244 (comment) and the subsequent discussion discuss why.This doesn't look like the right fix. #102244 (comment) and the subsequent discussion discuss why.

I assume the change requested here is to move the DynamicDependency from source to XML like this:

    <type fullname="System.Data.DataSet">
      <method signature="System.Void .ctor()">
        <attribute fullname="System.Diagnostics.CodeAnalysis.DynamicDependencyAttribute">
          <argument>ResetRelations</argument>
        </attribute>

however, when I do that it appears trimming already occurred, meaning in the example above I'll get this error when building:

C:\git\ComponentModel\src\libraries\System.Data.Common\src\System\Data\DataSet.cs(47,9): error IL2037: System.Data.DataSet.DataSet(): No members were resolved for 'ResetRelations' on type 'System.Data.DataSet'. [C:\git\ComponentModel\src\libraries\sfx.proj]

@jkotas
Copy link
Member

jkotas commented Jun 11, 2024

I assume the change requested here is to move the DynamicDependency from source to XML like this:

Yes, it should be in ILLink.Descriptors.LibraryBuild.xml.

however, when I do that it appears trimming already occurred, meaning in the example above I'll get this error when building:

I am not sure where this error is coming from. Could you please apply the change so that we can see it?

@steveharter
Copy link
Member Author

Yes, it should be in ILLink.Descriptors.LibraryBuild.xml.

I saw that right after I posted; I was using the wrong xml file. I added src\libraries\System.Data.Common\src\ILLink which is working.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

I'm not reviewing whether this list is complete, but the approach looks good to me! Thanks!

@steveharter steveharter requested a review from SamMonoRT June 12, 2024 14:41
@steveharter steveharter merged commit 75e4544 into dotnet:main Jun 12, 2024
81 of 83 checks passed
@steveharter steveharter deleted the FixTrimmingForTypeDescriptor branch June 12, 2024 18:13
@steveharter
Copy link
Member Author

steveharter commented Jun 12, 2024

This should be considered for servicing once verified.

@SamMonoRT
Copy link
Member

@roji @AndriySvyryd fyi --

@github-actions github-actions bot locked and limited conversation to collaborators Jul 18, 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.

Do not trim private methods used by the designer
4 participants