Closed Tanya-Solyanik closed 2 months ago
FYI: @LakshanF @steveharter
Tagging subscribers to this area: @roji, @ajcvickers See info in area-owners.md if you want to be subscribed.
System.Data has the most problems, but all assemblies should be examined for this pattern.
@steveharter @ericstj can we have this triaged soon? This is super important for WinForms customers.
Tagging subscribers to this area: @dotnet/area-system-componentmodel See info in area-owners.md if you want to be subscribed.
Marking this one as ComponentModel - the caller of private reflection here is ComponentModel.
The fix is likely to use [DynamicDependency(nameof(ShouldSerializeCaption)
on the appropriate DataColumn
ctor to preserve ShouldSerializeCaption
, etc.
In addition to DataColumn
, this appears to also affect DataSet
and DataTable
plus these :
@steveharter did you also look for the Reset
Another possible fix here would be to just make these public so that it's clear that they're externally called. I don't have much preference so long as we can ensure we preserve the members in the framework assemblies we ship, as well as preserve them in a self-contained trimmed application that might depend on them.
@steveharter did you also look for the Reset pattern?
Yes the regex I used is (private|internal) void Reset|bool ShouldSerialize[A-Z].*\(\)
plus some manual filtering to see if was intended for TypeDescriptor such as whether or not there a property of the same name.
Draft PR is opened, but I'd like feedback on how deep we should go. There are two trimming issue categories:
1) The inbox assemblies are automatically trimmed, but not the OOB ones. This affects the types in System.Data.Common.dll
but not System.Data.Odbc
and System.Data.OleDb
. Also, this only affects private
and internal
members.
2) Assemblies are trimmed by the user via csproj settings for building a self-contained application. These include the same inbox assemblies as well as the OOB ones (e.g. System.Data.Odbc
).
Should we only focus on (1) for now? Note that the PR currently includes all of (1) and only part of (2) - in particular there are additional classes including DataSet
and DataAdapter
that have public Reset\ShouldSerialize methods that are not yet covered in the PR. Also, if we do (2) we should add formal trimming tests, but the are some infrastructures issues with process for OOB assemblies - I'll log an issue for that.
Is the title of this issue "Do not trim private methods used by the designer" correct ?
If it is correct, the trimming done by the user (2) does not impact designers. The trimming in of inbox assemblies should be fixed by ILLink.Descriptors.LibraryBuild.xml
files as suggested above.
The second case might impact winforms applications that expose IDE-like functionality. The simplest scenario I can think of is an application with property grid, and the user trying to reset property value to the default. However, if such an application is trimmed, developer has to "register" types that are displayed in the property grid using the new feature Steve introduced to get TypeDescriptor infrastructure working. Will such types from System.Data.Odbc preserve the Reset/ShouldSerialize methods?
The second case might impact winforms applications that expose IDE-like functionality.
Applications that expose IDE-like functionality are typically incompatible with trimming for many different reasons. (If we hear about applications like that and the TypeDescriptor being one of the last things preventing them from being trimmable, we can always fix this for them by adding a feature switch.)
Description
Copied from https://github.com/dotnet/winforms/issues/11314
Private methods whose names follow the pattern
ShouldSerialize<PropertyName>
orReset<ProppertyName>
should not be trimmed by the linker because they are used by the TypeDescriptor via reflection. Additional conditions: Winforms designer is interested in types derived fromIComponent
. Property should not be hidden from serializers via SerializerVisibility attribute. Property should be public.TypeDescriptor accesses these methods here
Suggested fix from @ericstj
Searches yeld some false positives if the class is not designable, not an public, serializable property, or method is actually called directly. For each property we need to examine metadata in the trimmed assembly to see if the methods were removed.
IComponent
, or class does not have theReproduction Steps
This works on 4.8.1 but doesn't on net6
Expected behavior
ShouldSerialize, Reset methods are present in the assembly
Actual behavior
ShouldSerialize, Reset methods are trimmed
Regression?
The end scenario regressed between the InProc and OOP designers.
Known Workarounds
none
Configuration
WIndows
Other information
Trimmed ShouldSerialize causes serialization using BinaryFormatter. The fix should be serviced.