Erroneous linker error when using DAM attributes + primary constructors

[captainsafia]

Description

The linker does not respect DynamicallyAccessedMembers attributes on parameters within a primary constructor.

internal sealed class TypeBasedOpenApiDocumentTransformer([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type transformerType) : IOpenApiDocumentTransformer
{
	private readonly ObjectFactory _transformerFactory = ActivatorUtilities.CreateFactory(transformerType, []);
}

Note: this issue appears to repro specifically on the linker configuration used by ASP.NET Core's LinkabilityChecker tool. A more simplified repro on a console app doesn't share the same issue.

Reproduction Steps

1. git clone --recursive https://github.com/dotnet/aspnetcore
2. git checkout origin/safia/linker-primconstructors-bug
3. ./restore.sh
4. cd src/Tools/LinkabilityChecker
5. dotnet run
6. Observe linker exception.

Expected behavior

No linker exception is raised.

Actual behavior

Trim analysis warning IL2077: Microsoft.AspNetCore.OpenApi.TypeBasedOpenApiDocumentTransformer.TypeBasedOpenApiDocumentTransformer(Type):
 'instanceType' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors' in call to 
'Microsoft.Extensions.DependencyInjection.ActivatorUtilities.CreateFactory(Type, Type[])'. The field 
'Microsoft.AspNetCore.OpenApi.TypeBasedOpenApiDocumentTransformer.<transformerType>P' 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.

Regression?

No response

Known Workarounds

Explicitly defining a transformerType field with the DAM annotations resolves the issue.

Configuration

$ dotnet --info                                                                                                                                     11:38:54
.NET SDK:
 Version:           9.0.100-preview.5.24229.2
 Commit:            d301a122c4
 Workload version:  9.0.100-manifests.90bb8117
 MSBuild version:   17.11.0-preview-24222-11+9cdb3615a

Other information

No response

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[sbomer]

Simple repro:

using System;
using System.Diagnostics.CodeAnalysis;

new C(typeof(int)).UseField();

class C([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type t) {
    public void UseField() => Require(t); // warning IL2077

    static void Require([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type t) {}
}

This happens when the primary constructor parameter is turned into a field (for example, when it's accessed from an instance method). The field doesn't carry the annotation from the constructor parameter.

It feels like another case similar to dotnet/roslyn#46646, but the compiler can't assume that the attribute supports AttributeTargets.Field. We may be able to figure out the mapping back to the constructor parameter from the compiler-generated field name, which is '<t>P' in this example.

@agocke @MichalStrehovsky

[MichalStrehovsky]

First of all, I'm surprised that C# doesn't allow this to compile:

class C([field: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type t)
{
}

Unless we get a feature where Roslyn would "flow" these annotations for us in some way to backing fields, the best thing we could do is to allow people to do all this manually:

class C([param, field: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type t)
{
}

Allowing the param, field part would need new syntax, but is probably a lot cheaper than whatever feature to flow this to compiler-generated members by Roslyn. I don't know how realistic that request really is.

I'm not a fan of reverse engineering Roslyn's mangling for this. I was not a huge fan for lambdas/async state machines either. I don't know if we really want to be on a threadmill to reverse engineer manglings for all the new things C# introduces in the coming years (and whenever the manglings change, supporting the old version of the mangling forever, with the testability nightmares it comes with).

[CyrusNajmabadi]

It's an implementation detail if there is a field at all. And the language is very careful to not make it a requirement that a field be used to back a primary constructor.

[MichalStrehovsky]

It's an implementation detail if there is a field at all. And the language is very careful to not make it a requirement that a field be used to back a primary constructor.

If that's the case, it's not fixable on the trimming analysis side - unless some things around this become contractual. Not being able to use primary constructors when dataflow annotations are needed doesn't seem like such a huge loss IMHO.

[agocke]

• @jaredpar

I agree with Michal, we should try to move forward with discussions on how to lift the attributes in compiler generated code.

@CyrusNajmabadi this isn’t really a language question. The core problem is that IL carries extra semantics when doing trimming. In this case the compiler is unaware of those semantics, but that breaks an end to end user feature. We need to find some way to indicate to the compiler that these attributes are important at the IL level, similar to MethodImpl or DllImport