Do not trim private methods used by the designer

[Tanya-Solyanik]

Description

Copied from dotnet/winforms#11314

Private methods whose names follow the pattern ShouldSerialize<PropertyName> or Reset<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 from IComponent. Property should not be hidden from serializers via SerializerVisibility attribute. Property should be public.

TypeDescriptor accesses these methods here

Suggested fix from @ericstj

The fix is to add a file here runtime/src/libraries/System.Data.Common/src/ILLink at bad00cf23ec49a2607776ffd6e1810c4dbf540b3 · dotnet/runtime (github.com)

That looks similar to runtime/src/libraries/System.ComponentModel.TypeConverter/src/ILLink/ILLink.Descriptors.LibraryBuild.xml at bad00cf23ec49a2607776ffd6e1810c4dbf540b3 · dotnet/runtime (github.com)

Search pattern for ShouldSerialize

Search pattern for Reset

Searches yeld some false positives if the class is not designable, not an IComponent, or class does not have the 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.

Reproduction Steps

This works on 4.8.1 but doesn't on net6

using System;
using System.ComponentModel;
using System.Data;

namespace ConsoleApp1
{
    internal class Program
    {
        static void Main(string[] args)
        {
            DataColumn dataColumn1 = new DataColumn
            {
                ColumnName = "dataColumn1",
                DataType = typeof(DateTime)
            };

            var properties = TypeDescriptor.GetProperties(dataColumn1);
            var property = properties[nameof(DataColumn.DefaultValue)];
            if (property != null)
            {
                bool shouldSerialize = property.ShouldSerializeValue(dataColumn1);
                Console.WriteLine($"ShouldSerialize default? expected {false} actual {shouldSerialize}");

                dataColumn1.DefaultValue = DateTime.MinValue;
                shouldSerialize = property.ShouldSerializeValue(dataColumn1);
                Console.WriteLine($"ShouldSerialize changed? expected {true} actual {shouldSerialize}");

                // Reset method is not available
                property.ResetValue(dataColumn1);
                Console.WriteLine($"Reset? expected {DateTime.MinValue} actual {dataColumn1.DefaultValue}");
            }

            property = properties[nameof(DataColumn.Caption)];
            if (property != null)
            {
                bool shouldSerialize = property.ShouldSerializeValue(dataColumn1);
                Console.WriteLine($"ShouldSerialize default? expected {false} actual {shouldSerialize}");

                dataColumn1.Caption = "Caption";
                shouldSerialize = property.ShouldSerializeValue(dataColumn1);
                Console.WriteLine($"ShouldSerialize changed? expected {true} actual {shouldSerialize}");

                // Reset method is available
                property.ResetValue(dataColumn1);
                Console.WriteLine($"Reset? expected {nameof(dataColumn1)} actual {dataColumn1.Caption}");
            }

            property = properties[nameof(DataColumn.Namespace)];
            if (property != null)
            {
                bool shouldSerialize = property.ShouldSerializeValue(dataColumn1);
                Console.WriteLine($"ShouldSerialize default? expected {false} actual {shouldSerialize}");

                dataColumn1.Namespace = "Namespace";
                shouldSerialize = property.ShouldSerializeValue(dataColumn1);
                Console.WriteLine($"ShouldSerialize changed? expected {true} actual {shouldSerialize}");

                // Reset method is available
                property.ResetValue(dataColumn1);
                Console.WriteLine($"Reset? expected '' actual `{dataColumn1.Namespace}`");
            }

            _ = Console.ReadLine();
        }
    }
}

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.

[Tanya-Solyanik]

FYI: @LakshanF @steveharter

[dotnet-policy-service]

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

[Tanya-Solyanik]

System.Data has the most problems, but all assemblies should be examined for this pattern.

[merriemcgaw]

@steveharter @ericstj can we have this triaged soon? This is super important for WinForms customers.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-componentmodel
See info in area-owners.md if you want to be subscribed.

[ericstj]

Marking this one as ComponentModel - the caller of private reflection here is ComponentModel.

[steveharter]

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 :

• System.Data.Common.DataAdapter
• System.Data.Odbc.OdbcCommand
• System.Data.Odbc.OdbcParameter
• System.Data.OleDb.OleDbCommand
• System.Data.OleDb.OleDbParameter

[ericstj]

@steveharter did you also look for the Reset pattern? I see that might be on a couple additional types. Cool that we can do this with attributes - that's much better than the XML config.

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]

@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.

[steveharter]

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.

[jkotas]

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.

[Tanya-Solyanik]

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?

[jkotas]

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.)