Consider PreserveDependencyAttribute to help linker

[stephentoub]

We currently use ILLinkTrim.xml files to express APIs, whether public or non-public, that should not be trimmed away by the linker, whether during the first trimming phase when we build the assemblies or the second optional trimming phase when an app is deployed with trimming.

However, these declarations are causing us to keep around a lot more state than is necessary, for two reasons:

1. The presence of such an .xml file is forcing the binary to be preserved, even if nothing from it ends up being used.
2. It doesn't provide the ability to say "only keep XYZ if ABC is used".

For example, in System.Data.Common, we have an ILLinkTrim.xml file that says the DataTableTypeConverter's .ctor() should be preserved. That's because the DataView.Table property is annotated with a TypeConverterAttribute:
https://github.com/dotnet/corefx/blob/19b304f7815894b13cb61e87e1c9eac49a474c7e/src/System.Data.Common/src/System/Data/DataView.cs#L473
and the type converter infrastructure needs access to that ctor to instantiate it via reflection. However, because that .ctor is mentioned in the .xml file, it'll never be trimmed away, even if DataView.Table itself isn't used. And DataTableTypeConverter's ctor references DataTable, which in turn references a whole bunch of stuff.

The same goes for other assemblies, e.g. System.Text.Json, System.Private.DataContractSerialization, etc.

We should consider instead exposing and using the PreserveDependencyAttribute that the linker already has some support for (though coreclr and corefx are currently using too old a version), e.g.

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Field, AllowMultiple = true, Inherited = false)]
    internal sealed class PreserveDependencyAttribute : Attribute
    {
        public PreserveDependencyAttribute(string? memberSignature)
        {
            MemberSignature = memberSignature;
        }

        public PreserveDependencyAttribute(string? memberSignature, string typeName)
        {
            MemberSignature = memberSignature;
            TypeName = typeName;
        }

        public PreserveDependencyAttribute(string? memberSignature, string typeName, string assemblyName)
        {
            MemberSignature = memberSignature;
            TypeName = typeName;
            AssemblyName = assemblyName;
        }

        public string? MemberSignature { get; set; }
        public string? TypeName { get; set; }
        public string? AssemblyName { get; set; }

        public string? Condition { get; set; }
    }
}

This, or something like it, has the ability to specify not just that something is needed, but why that thing is needed, in a way where if the reason for it needing to be there gets trimmed away, so too does the need. It also has the benefit that the dependency is expressed in the code rather than in a separate asset.

cc: @sbomer, @jkotas, @marek-safar

[jkotas]

I agree that this is a good idea.

The PreserveDependencyAttribute as it exists today is not compatible with per-assembly trimming mode that we use to remove unnecessary code from CoreLib and CoreFX today. We would need to do something about it. Options I can think of:

• Stop doing per-assembly trimming for CoreLib and CoreFX, and live with the shared framework size regression.
• Do the trimming in Roslyn: Tech Roslyn how to trim unnecessary code like private constants from the output. This would give us most of the benefits.
• Make the internals accessed by reflection across assemblies public so that they are not trimmed in single assembly mode (this is what we used to do for ProjectN).
• Add a new attribute or flag to existing attribute that will tell linke that the type or member should be kept, but only in a single assembly mode.

[stephentoub]

The PreserveDependencyAttribute as it exists today is not compatible with per-assembly trimming mode that we use to remove unnecessary code from CoreLib and CoreFX today.

Yes, I just had a discussion with Marek about that.

I think there's another options as well:

• Add an option to the linker to not strip PreserveDependency or its usage. We would specify that when building the assemblies, but not when publishing. There'd be the ever so small size increase from the attribute usage, but that's a drop in the bucket, and we're already paying for the .xml file being embedded as a resource, which we wouldn't then have.

[jkotas]

Add an option to the linker to not strip PreserveDependency or its usage. We would specify that when building the assemblies,

The PreserveDependency is specified on the consuming side, but the trimming needs to be suppressed on the producing side. These two often live in different assemblies. How would the linker get to know about the consuming assembly when trimming the producing assembly?

[stephentoub]

How would the linker get to know about the consuming assembly when trimming the producing assembly?

The two most common cases I've seen are:

• Both the PreserveDependency and the thing it's preserving are in the same assembly, e.g. all of the cases where something in the assembly depends on something else in the assembly to be constructible via reflection.
• The thing being preserved is public and thus wouldn't be trimmed as part of building the assembly, e.g. corelib using Process.WorkingSet.

Both of those would be unaffected.

There is the third case you mention, where there's a cross-assembly dependency to something non-public. I've seen very few cases of that, but they do exist. For example, for better or worse System.Net.HttpListener today accesses Cookie.ToServerString, which is internal, via reflection. If we put PreserveDependency in HttpListener, it would have the problem you mention. We could instead put PreserveDependency on the Cookie's ctor, such that ToServerString only need be kept if it's possible it could be called. That's certainly not ideal, as then ToServerString is kept around even if HttpListener isn't used, but at least thus far I haven't seen this to be a widespread issue, since we actively discourage private reflection across assembly boundaries, for others and for ourselves.

This case could also be addressed by just adding a parameterless ctor to [PreserveDependency] and saying that anything attributed with it will be kept. I think this is basically your option 4, albeit your option 4 could be more flexible.

The other mentioned options also have some downsides:

• Stop doing per-assembly trimming => as you say, shared framework size regression
• Do the trimming in Roslyn => it would help with eliminating things like private/internal consts, but it would likely lead to less maintainable code or shared framework size regressions, as we currently have files that are pulled into multiple assemblies but where not everything in those files are used by every consuming assembly
• Make the internals accessed by reflection across assemblies public => impacts our tooling around ref assemblies, impacts (very few?) cases where the implementation and ref assembly are one in the same, leads to some confusion when looking at the source / maintenance issues, and means that such functionality is available via public reflection

[marek-safar]

The PreserveDependency is specified on the consuming side, but the trimming needs to be suppressed on the producing side.

@jkotas the proposed command line option would be global for the linker run, basically not removing this attribute anywhere.

[MichalStrehovsky]

In light of dotnet/linker#805 we should look for a different name:

Some proposed names from the issue:

• HasDependencyOnAttribute
• ImplicitDependencyAttribute
• MemberDependencyAttribute
• Throwing one more name DynamicDependencyAttribute

[vitek-karas]

I like the DynamicDependency.

[marek-safar]

This is more like ExplicitDependencyAttribute than ImplicitDependencyAttribute especially when used for dependencies channelled via native code

[MichalStrehovsky]

especially when used for dependencies channelled via native code

Is this a customer scenario? It feels like the use cases where we need to preserve something because native code is essentially going to "reflect" on this are more our internal/Unity use cases and not something mainstream customers would hit. (I want to make sure we optimize the name for customer scenarios and not our internal use cases.)

It feels that the callbacks from native code still fit the "dynamic" name because they're all essentially just invoked through a different kind of reflection (get looked up using string names, called in a funny way, etc.).

But I don't have a very strong preference for this attribute in particular.

[terrajobst]

Video

• It seems wild cards are also supported, which raises a few questions:
  • Does it include private and public APIs or just public?
  • How hard is it for upstream dependencies to implement (e.g. ILSpy or VS Find All Usages)
  • Do we need them? We'd like because it seems there are places where we have a lot.
• The linker should at least warn when the strings can't be matched
• typeName and assemblyName are optional because they default to same and same assembly
• Looks like memberSignature should be non-nullable
• Condition is some specific string that is interpreted by the linker, today that's only DEBUG (which unfortunately doesn't map to the C# notion of compiling a debug build)

namespace System.Diagnostics.CodeAnalysis
{
    [AttributeUsage(AttributeTargets.Method |
                    AttributeTargets.Constructor |
                    AttributeTargets.Field,
                    AllowMultiple = true,
                    Inherited = false)]
    public sealed class DynamicDependencyAttribute : Attribute
    {
        public DynamicDependencyAttribute(string memberSignature);
        public DynamicDependencyAttribute(string memberSignature, string typeName, string assemblyName);
        public DynamicDependencyAttribute(DynamicallyAccessedMemberKinds memberKinds, string typeName, string assemblyName);

        public DynamicDependencyAttribute(string memberSignature, Type type);
        public DynamicDependencyAttribute(DynamicallyAccessedMemberKinds memberKinds, Type type);

        public DynamicallyAccessedMemberKinds? MemberKinds { get; }
        public string? MemberSignature { get; }
        public string? TypeName { get; }
        public string? AssemblyName { get; }
        public string? Condition { get; set; }
    }
}

[eerhardt]

Closed in #35387.