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

[API Proposal]: TypeDescriptor-related trimming support #101202

Closed
steveharter opened this issue Apr 17, 2024 · 3 comments · Fixed by #102094
Closed

[API Proposal]: TypeDescriptor-related trimming support #101202

steveharter opened this issue Apr 17, 2024 · 3 comments · Fixed by #102094
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.ComponentModel linkable-framework Issues associated with delivering a linker friendly framework
Milestone

Comments

@steveharter
Copy link
Member

steveharter commented Apr 17, 2024

Background and motivation

This feature updates the System.ComponentModel.TypeConverter assembly to make it trimmer compatible so that it can be used by WinForm apps in NativeAot. This feature is primarily focused on improving correctness of trimming, and not necessarily to reduce size. There will likely be optimizations later for size once correctness is achieved.

This area of ComponentModel primarily is the TypeDescriptor which is used to obtain the properties, events and attributes for classes. It has various "provider" extensibility points as an alternative to the default provider which is based on reflection. However, this framework was designed well before any trimming-related scenarios existed and thus today is not safely trimmable.

The need to avoid RUC in all cases, and DAM for most cases

The TypeConverter assembly has been annotated with trimming attributes over the last couple of releases, but depends heavily on the unactionable [RequiresUnreferencedCode] (or RUC) which essentially means the code is not safely trimmable.

The goal here is to avoid RUC and instead depend only on [DynamicallyAccessedMembers] (or DAM) which the linker understands. Unlike CoreClr, NativeAot does not necessarily support reflection on members even when the physical methods exist (after the linker is run due to code references that calls them); the proper use of DAM is necessary for the linker and NativeAot to preserve member metadata necessary for reflection introspection (e.g. obtaining the list of properties) and to add the reflection stubs which are necessary for invoke (e.g. calling a getter\setter).

RUC has been used here because the existing APIs do not always have a Type - they often take object, which is an issue for the linker since it can only rationalize on a static Type through typeof or generic <T> parameters. The cases that take object are not linker-compatible.

Some existing methods take Type (e.g. TypeDescriptor.GetProperties(Type)), but DAM is not always desired on these methods since they are often called several levels deep in a call stack, and not during initialization, for example, where the static Type is known. Callers of such DAM-based APIs would get linker warnings since the Type parameter would not be passed statically and thus the warning would need to be suppressed. Suppressing these can cause run-time issues, such as silently not showing any properties for a given Type.

Adding a registration API with DAM along with run-time checks to enforce

The proposal adds new APIs that mirror existing APIs but end with "RegisteredType" and where these new APIs will not have DAM.

In order to tell the linker to preserve the metadata\members for reflection, the proposal adds APIs to "register" a type where the API does have DAM. This is intended to be called during initialization, such as in the designer-generated InitializeComponent().

Run-time validation is also added to these new APIs that do not have DAM to verify they are only called with the "registered" types. This validation prevents incorrect results; for example, missing properties from TypeDescriptor.GetPropertiesFromRegisteredType(Type).

API Proposal

These may use the [Experimental] attribute pending discussion.

using System.ComponentModel;

public sealed class TypeDescriptor
{
    // The main registration API that will cause the linker\trimmer to preserve metadata\members for reflection.
+   public static void RegisterType<[DynamicallyAccessedMembers(
        DynamicallyAccessedMemberTypes.PublicConstructors |
        DynamicallyAccessedMemberTypes.PublicParameterlessConstructor |
        DynamicallyAccessedMemberTypes.PublicFields |
        DynamicallyAccessedMemberTypes.Interfaces |
        DynamicallyAccessedMemberTypes.PublicProperties |
        DynamicallyAccessedMemberTypes.PublicMethods |
        DynamicallyAccessedMemberTypes.PublicEvents)] T>();

    public static AttributeCollection GetAttributes([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]Type componentType);
+   public static AttributeCollection GetAttributesFromRegisteredType(Type componentType);

     [RequiresUnreferencedCode] public static TypeConverter GetConverter(object component);
+   public static TypeConverter? GetConverterFromRegisteredType(object component);

     [RequiresUnreferencedCode] public static TypeConverter GetConverter([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]Type type);
+   public static TypeConverter? GetConverterFromRegisteredType(Type type);

    public static EventDescriptorCollection GetEvents([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]Type componentType);
+   public static EventDescriptorCollection GetEventsFromRegisteredType(Type componentType);

    public static PropertyDescriptorCollection GetProperties([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]Type componentType);
+   public static PropertyDescriptorCollection GetPropertiesFromRegisteredType(Type componentType);

    [RequiresUnreferencedCode] public static PropertyDescriptorCollection GetProperties(object component);
+   public static PropertyDescriptorCollection GetPropertiesFromRegisteredType(object component);
}

// Uses Default Interface Methods to add new members.
// Alternative approach is to add 'ICustomTypeDescriptorForRegisteredType : ICustomTypeDescriptor'
public interface ICustomTypeDescriptor
{
    AttributeCollection GetAttributes();
+   AttributeCollection GetAttributesFromRegisteredType() { // DIM }

    [RequiresUnreferencedCode] TypeConverter? GetConverter();
+   TypeConverter? GetConverterFromRegisteredType() { // DIM };

    EventDescriptorCollection GetEvents();
+   EventDescriptorCollection GetEventsFromRegisteredType() { // DIM };

    PropertyDescriptorCollection GetProperties();
+   PropertyDescriptorCollection GetPropertiesFromRegisteredType() { // DIM };

    // Defaults to 'null' which then the framework defers to the new feature switch as to validate or not.
+   public virtual bool? RequireRegisteredTypes { get { // DIM } }

    // The overloads that take attributes were purposely omitted for now.
}

public abstract class CustomTypeDescriptor : ICustomTypeDescriptor
{
    public virtual AttributeCollection GetAttributes();
+   public virtual AttributeCollection GetAttributesFromRegisteredType();

    [RequiresUnreferencedCode]public virtual TypeConverter? GetConverter();
+   public virtual TypeConverter? GetConverterFromRegisteredType();

    public virtual EventDescriptorCollection GetEvents();
+   public virtual EventDescriptorCollection GetEventsFromRegisteredType();

    public virtual PropertyDescriptorCollection GetProperties();
+   public virtual PropertyDescriptorCollection GetPropertiesFromRegisteredType();

    // Defaults to 'null' which then the framework defers to possible new feature switch as to validate or not.
+   public virtual bool? RequireRegisteredTypes { get; }
}

public abstract class PropertyDescriptor : MemberDescriptor
{
    public virtual TypeConverter Converter {[RequiresUnreferencedCode] get; }
+   public virtual TypeConverter ConverterFromRegisteredType { get; }
}

public abstract class TypeDescriptionProvider
{
+   public virtual void RegisterType<[DynamicallyAccessedMembers(
        DynamicallyAccessedMemberTypes.PublicConstructors |
        DynamicallyAccessedMemberTypes.PublicParameterlessConstructor |
        DynamicallyAccessedMemberTypes.PublicFields |
        DynamicallyAccessedMemberTypes.Interfaces |
        DynamicallyAccessedMemberTypes.PublicProperties |
        DynamicallyAccessedMemberTypes.PublicMethods |
        DynamicallyAccessedMemberTypes.PublicEvents)] T>();

 [RequiresUnreferencedCode] public ICustomTypeDescriptor? GetTypeDescriptor(object? instance);
+ public ICustomTypeDescriptor GetExtendedTypeDescriptorFromRegisteredType(object instance);

 [RequiresUnreferencedCode] public ICustomTypeDescriptor? GetTypeDescriptor(Type objectType);
+  public ICustomTypeDescriptor? GetTypeDescriptorFromRegisteredType(Type objectType);

 [RequiresUnreferencedCode] public virtual ICustomTypeDescriptor? GetTypeDescriptor(GetTypeDescriptorFromRegisteredType(Type objectType, object instance);
+  public virtual ICustomTypeDescriptor? GetTypeDescriptorFromRegisteredType(Type objectType, object instance);

+   public virtual bool IsRegisteredType(System.Type type);
    // Defaults to 'null' which then the framework defers to the new feature switch as to validate or not.
+   public virtual bool? RequireRegisteredTypes { get; }
}

public class ComponentResourceManager
{
    [RequiresUnreferencedCode] public virtual void ApplyResources(object value, string objectName, CultureInfo? culture)
+   public virtual void ApplyResourceToRegisteredType(object, string objectName, CultureInfo? culture)
}

API Usage

  public class ExampleClass
  {
      public string Property1 { get; set; }
      public int Property2 { get; set; }
  }

  public class Program
  {
      public static void Main()
      {
          Test(typeof(ExampleClass));
          Console.ReadLine();
      }

      private static void Test(Type type)
      {
          // When publishing self-contained + trimmed, we get warning IL2026 and IL2067 on GetProperties():
          // Warning IL2026: TypeDescriptorExample.Program.Test(Type): Using member 'System.ComponentModel.TypeDescriptor.GetProperties(Type)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code.PropertyDescriptor's PropertyType cannot be statically discovered.
          // Warning IL2067: TypeDescriptorExample.Program.Test(Type): 'componentType' argument does not satisfy 'DynamicallyAccessedMemberTypes.All' in call to 'System.ComponentModel.TypeDescriptor.GetProperties(Type)'.The parameter 'type' of method 'TypeDescriptorExample.Program.Test(Type)' 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.
          PropertyDescriptorCollection properties = TypeDescriptor.GetProperties(type);

          // The property count will be 0 here instead of 2 when publishing self-contained + trimmed.
          Console.WriteLine($"Property count: {properties.Count}");
          foreach (PropertyDescriptor prop in properties)
          {
              string propName = prop.Name;
              Console.WriteLine($"Property: {propName}");
          }

          // To avoid the warning and ensure reflection can see the properties, we register the type:
          TypeDescriptor.RegisterType<ExampleClass>();
          // To avoid the warnings and ensure validation we call the new API;
          properties = TypeDescriptor.GetPropertiesFromRegisteredType(type);
      }
  }

Validation Modes

The new "FromRegisteredType" APIs will validate the type is registered, at least for the reflection provider which requires the DAM attribute via the registration API. Registering a Type also registers base classes, but does not register referenced types such as from properties.

A type provider can have a parent; if a "RegisteredType" API is called on a provider with a parent, the request goes to the parent. The parent provider, if a custom provider, may or may not have implemented this (our shipping providers will). If the provider is unconfigured (SupportsRegisteredTypes returns false) we will assume the provider does not use reflection and will forward to the legacy API instead. This helps people to migrate to the new APIs (if they want to avoid trimmer suppressions) but not worry about whether the provider is configured or not to use registered types. It is valid to register a type even though the provider may ignore it.

Existing APIs will have no changes. Various RUC\DAM etc overrides would be suppressed by caller. Most likely this will not work with trimming (as today).

Pending prototyping and the first set of functionality, we may want to enforce that unconfigured providers (that return null from RequiresRegisteredType), which have the "RegisteredType" members called on them, are validated at runtime. To do this, we may add a feature switch "System.ComponentModel.TypeDescriptor.RequireRegisteredTypes". When this switch is enabled, there will additional validation that will ensure a trimmed application behaves as expected. It is expected that WinForms enables this feature switch. We may also decide to disable certain APIs if they are incompatible with trimming (have RUC or require DAM for low-level methods) and if the new APIs can be used instead.

Alternative Designs

Remove existing RUC\DAM

  1. Keep the RegisterType and RequireRegisteredTypes members.
  2. Remove the parallel "FromRegisteredType" APIs
  3. Remove all RUC\DAM annotations on the appropriate existing members that would be safe when using registration.
  • Avoids the parallel APIs.
  • A large breaking change and would require the use of the new feature switch in order to safely trimming.

Remove existing RUC\DAM if feature switch is on

Same as the alternative above, but for (3) add a property to the RUC\DAM attributes that specifies a feature switch as a string. The linker will ignore these annotations if the feature switch is on.

This avoids the breaking change and the parallel APIs. However, it makes a non-trivial application with external dependencies more difficult to migrate since code that is not owned, and still uses unregistered types, will no longer get linker warnings for RUC\DAM and thus difficult to determine what types need to be registered.

Risks

No response

@steveharter steveharter added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 17, 2024
@steveharter steveharter self-assigned this Apr 17, 2024
@steveharter steveharter added this to the 9.0.0 milestone Apr 17, 2024
Copy link
Contributor

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

@steveharter steveharter added the linkable-framework Issues associated with delivering a linker friendly framework label Apr 18, 2024
@steveharter steveharter added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 25, 2024
@MichalStrehovsky
Copy link
Member

// Instead of .All, we may use more narrow values (public properties, fields, events, members, constructors) can this start with the more narrow definition?

.All has size implications, but more importantly, we found out that .All increases the chances the end user will get trimming warnings. For example:

class Program
{
    static void Require<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>() { }

    static void Main()
    {
        Require<Other>();
    }
}

class Other
{
    static IEnumerable<MethodInfo> Walk()
    {
        // Just some random ***trim safe*** reflection, it's not too important what this does.
        Type? t = typeof(AttributeUsageAttribute);
        foreach (var m in t.GetMethods())
            yield return m;
    }
}

This warns on dotnet publish because .All says we want to reflect on everything, including the state machine type that implements the yield return: Trim analysis warning IL2118: Program.Main(): Compiler-generated member 'Other.<Walk>d__0.MoveNext()' is accessed via reflection. Trimmer can't guarantee availability of the requirements of the member..

If you change it to .PublicMethods | .NonPublicMethods, the warning will be gone because we no longer make it appear we intend to reflect on the compiler generated state machine type.

If we're defining a new API, we should try to design it so that it doesn't run into these issues.

@bartonjs
Copy link
Member

bartonjs commented May 2, 2024

Video

  • We discussed the implications of using DIMs in ICustomTypeDescriptor, and it seems OK.
  • TypeDescriptor.GetPropertiesFromRegisteredType (and others) has other overloads, they're intentionally omitted.
  • The DynamicallyAccessedMemberTypes value on RegisterType is going to be something less than All, will be corrected during implementation.
  • TypeDescriptorProvider.RegisterType<T> doesn't seem to need the DynamicallyAccessedMembers attribute, the use is covered by the static wrapper.
using System.ComponentModel;

public sealed partial class TypeDescriptor
{
    // The main registration API that will cause the linker\trimmer to preserve metadata\members for reflection.
    // Instead of .All, we may use more narrow values (public properties, fields, events, members, constructors)
    public static void RegisterType<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>();

    public static AttributeCollection GetAttributesFromRegisteredType(Type componentType);
    public static TypeConverter? GetConverterFromRegisteredType(object component);
    public static TypeConverter? GetConverterFromRegisteredType(Type type);
    public static EventDescriptorCollection GetEventsFromRegisteredType(Type componentType);
    public static PropertyDescriptorCollection GetPropertiesFromRegisteredType(Type componentType);
    public static PropertyDescriptorCollection GetPropertiesFromRegisteredType(object component);
}

// Uses Default Interface Methods to add new members.
// Alternative approach is to add 'ICustomTypeDescriptorForRegisteredType : ICustomTypeDescriptor'
public partial interface ICustomTypeDescriptor
{
    AttributeCollection GetAttributesFromRegisteredType() => DIM;
    TypeConverter? GetConverterFromRegisteredType() => DIM;
    EventDescriptorCollection GetEventsFromRegisteredType() => DIM;
    PropertyDescriptorCollection GetPropertiesFromRegisteredType() => DIM;

    // Defaults to 'null' which then the framework defers to the new feature switch as to validate or not.
    public virtual bool? RequireRegisteredTypes => null;

    // The overloads that take attributes were purposely omitted for now.
}

public abstract class CustomTypeDescriptor : ICustomTypeDescriptor
{
    public virtual AttributeCollection GetAttributesFromRegisteredType();
    public virtual TypeConverter? GetConverterFromRegisteredType();
    public virtual EventDescriptorCollection GetEventsFromRegisteredType();
    public virtual PropertyDescriptorCollection GetPropertiesFromRegisteredType();

    // Defaults to 'null' which then the framework defers to possible new feature switch as to validate or not.
    public virtual bool? RequireRegisteredTypes => null;
}

public abstract class PropertyDescriptor : MemberDescriptor
{
    public virtual TypeConverter ConverterFromRegisteredType { get; }
}

public abstract class TypeDescriptionProvider
{
    public virtual void RegisterType<T>();
    public virtual ICustomTypeDescriptor GetExtendedTypeDescriptorFromRegisteredType(object instance);

    public ICustomTypeDescriptor? GetTypeDescriptorFromRegisteredType(object instance);

    public virtual bool IsRegisteredType(System.Type type);
    public virtual bool? RequireRegisteredTypes => null;
}

public class ComponentResourceManager
{
    public virtual void ApplyResourceToRegisteredType(object value, string objectName, CultureInfo? culture);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 2, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.ComponentModel linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants