Allow DynamicallyAccessedMembersAttribute on methods

[vitek-karas]

We need a way to add DynamicallyAccessedMembersAttribute to the "this" parameter for calls like Type.GetFields. There's no way in C# to do this (and in fact even IL is a bit tricky in that regard), so the linker supports recognizing the attribute on a method itself, but only for System.Type class. It then treats that attribute as if it's put on the "this" parameter.

Fixes #36708

[ghost]

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

[MichalStrehovsky]

We should put comments around this explaining why we need this in CoreLib but we don't want this to be exposed through contracts.

[eerhardt]

Agreed.

One concern I have is if/when someone regenerates the ref assembly, this new AttributeTarget will get generated into the ref assembly, and it might not get caught during code review.

[jkotas]

+1 We want to be getting rid of the differences between autogenerated ref assembly, not adding new ones.

[vitek-karas]

Given the feedback - alternate solution is to introduce an internal only attribute specifically for this, basically DynamicallyAccessedMembersOnThisAttribute. Since it would be internal to corelib it could not be used outside of it. And we can teach linker about it.

Thoughts?

[stephentoub]

Can you help me understand what's so special about corelib in this regard? I'm wondering if we have this use-case why others may not as well? It's specific not just to corelib but more so to Type itself?

[vitek-karas]

The DynamicallyAccessedMembersAttribute only applies to "data" of type System.Type or System.String. So vast majority of its use cases are annotating method parameters, method return values or fields. Since it only applies to System.Type and System.String nobody but CoreLib will ever be in a situation to try to put it onto the "this" parameter - simply because nobody but CoreLib implements instance methods on the System.Type or System.String classes (I ignore the very special case of somebody deriving from System.Type - if that happens we can look into this again, but I doubt we'll ever hit that).

So yes - it's basically specific to System.Type itself as I don't think we will ever need it on System.String for anything. We might want to use it internally on RuntimeType as well.

[eerhardt]

Would another alternative be to special-case Type in the linker? Or would that be too invasive to the linker?

[vitek-karas]

Depends on what kind of "Special case" you have in mind. We already do special case some of the API on Type in the linker. But there's a lot of them, so the idea was to avoid special-casing all of them. The other consideration is maintenance. If we add/tweak something in CoreLib it's much closer to the change to have it in the attributes then in the linker.

And another idea was that the system would work without the special casing in the linker, that just makes it "nicer" (less warnings for things linker can figure out) - but that requires annotations in the CoreLib itself - which is what this is about.

I think that internal only attribute is basically a special case in the linker - just such that there's still reasonable separation between the tool and the library.

[MichalStrehovsky]

Maybe an example would be helpful to explain what this is about:

class Type
{
    public ConstructorInfo[] GetConstructors() => ...
}

The above API is only safe to call with linker in the picture if the System.Type instance that we're calling it on has all public constructors kept. We could special case it in linker, or annotate it in the class library. Unfortunately, the public dataflow annotations that we're adding in .NET 5 cannot express this, because we don't have a way to annotate this parameter on instance methods. Placing the attribute on the method is a hacky but kind of understandable way to achieve that.

class Type
{
    // When placed on a method, this annotation annotates `this`
    [DynamicallyAccessedMembers(MemberTypes.PublicConstructors)]
    public ConstructorInfo[] GetConstructors() => ...
}

[vitek-karas]

I changed it to a separate internal only attribute - does that look better?

[stephentoub]

a separate internal only attribute

It's public rather than internal; was that intentional? I know it's not in the ref, but it's not clear why this would need to be exposed as public even with that.

Even so, it seems there are two aspects of "public" here: the visibility of the attribute itself in .NET 5, but also the recognition of it with the linker. If the linker is recognizing this attribute by name, isn't it effectively then something folks could rely on by defining their own version of the attribute?

That said, if adding an internal (not public) attribute fixes the problem, that seems fine until something better can be devised.

[vitek-karas]

That's my bad - it should be internal.

Linker actually ignores the attribute (even the public one, but it will do this for the new one as well) if it's applied to anything else then System.Type and System.String. So even if somebody would try to use it, it would have no effect. (as mentioned above we may let linker recognize it on couple more types like RuntimeType`, but that doesn't change the publicly observable behavior).

[eerhardt]

I'm not super excited about the name, but I can't think of anything better.

• DynamicallyAccessedMembersOnCurrent
• DynamicallyAccessedMembersOnInstance
• DynamicallyAccessedMembersInternal
• DynamicallyAccessedMembersForType

None of those are significantly better IMO. So I think what we have is fine. It is internal, so if we really want to change it, we could.

[jkotas]

Would it make sense to just allow DynamicallyAccessedMembersAttribute on Methods, unconditionally?

One has to be careful to apply DynamicallyAccessedMembersAttribute on Type values only. The attribute restrictions do not prevent it from being applied incorrectly. So having one more way how the attribute can be applied incorrectly does not seem to be a big deal.

[vitek-karas]

@jkotas In the API review the allowed targets for the attribute were actually discussed quite a bit - I got the feeling that we try to reduce it to the very minimal set possible. Mainly because it gives people direct feedback in VS/build if they got it wrong.

In this case I don't like that solution mainly because it's basically always wrong (with this one little internal exception). I do agree that in theory it doesn't make sense to put the attribute on the method directly, but I can easily see people getting confused by this pretty quickly.

[jkotas]

Is the linker going to warn when this attribute is applied in places where it does not make sense? It would fix the confusion for this case, and all other cases.

DynamicallyAccessedMembersInternal

Another option is to put the internal version into the Internal namespace. We do that in number of cases where we have special internal version of the public type, e.g. Internal.Runtime.CompilerServices.Unsafe.

[vitek-karas]

Currently linker doesn't warn if otherwise recognized attribute is used in a place where it's ignored. The main reason typically is that if somebody puts an attribute in the wrong place in a NuGet, when I build the app I would constantly get a warning without a good way to suppress it (we're building the ability to maybe suppress it now, but it's still relatively bad experience).

I can definitely move this to other namespace if it makes more sense - and maybe even change the name to something including "Internal" or so.

[jkotas]

The main reason typically is that if somebody puts an attribute in the wrong place in a NuGet, when I build the app I would constantly get a warning without a good way to suppress it

If the NuGet author is adding these attributes, they are presumably going to verify they work. If they depend on their customers to verify it, it is their problem.

My default stance would be to warn about misplaced attributes always. It is the good diagnostic experience. Only worry about the noise if/once we find that people are shipping NuGets with misplaced attributes and not correcting the bugs fast enough.

[vitek-karas]

@marek-safar for more details on the current linker behavior (of not reporting any issues with input XMLs and most attributes).

[marek-safar]

I'm fine with warnings for publicly available attributes as they are highly unlikely to change but not sure it's worth the effort to add warnings for internal attributes which could change in the future.

[vitek-karas]

I moved the attribute to Internal.Diagnostics.CodeAnalysis namespace to make it very clear that's meant to be internal only.

[jkotas]

Nit: We are keeping this list alphabetically sorted. It should be in the Internal* block.

[vitek-karas]

I just realized that we will need to make this public (in whichever way we decide to do it). We need to be able to annotate the public APIs in ref assemblies as well (if only to make the libraries linker runs see the attributes). So even this special case will have to be public.

So the two options are:

• Introduce a new attribute for this special case - basically next to the existing DynamicallyAccessedMembersAttribute, just different name but otherwise basically the same. Pro: We can give it a more specific name which should convey that it's not to be used externally really.
• Just change the existing attribute to allow it on methods. This has two subcases:
  • Allow it on methods for everybody - make the change in impl. and ref assemblies as well. This is clean in the sense that we don't have differences between impl and ref assemblies. But it's a bit confusing to users as the attribute looks like it's valid on methods, but linker would complaint if used (outside of System.Type).
  • Allow it on methods only in impl assembly and not allow it in ref assembly (the original proposal in this PR). This would make it less confusing to users, but it would still let us use it in the ref assembly for CoreLib.

From end user perspective the best option is the original proposal - only make the change in impl. assembly and leave it out of ref assembly. No observable difference from the outside, but we get a publicly visible attribute to annotate the special methods with. So I personally prefer this one.

If we think that introducing a new difference between impl. and ref. assemblies is a big problem, then I would probably vote for just allowing it on methods for everybody. I don't like introducing a new public attribute just for this.

[jkotas]

introducing a new difference between impl. and ref. assemblies is a big problem

Yes, it is.

vote for just allowing it on methods for everybody

+1

[vitek-karas]

#36708 has been approved - making the attribute applicable to methods all up (publicly).

I updated the description of this PR to match the new solution.
I also pushed a new change which implements this as per #36708 .

[eerhardt]

I think these changes can be merged. The failures are below:

There are 2 test failures which aren't related to this code change:

  Discovering: System.Threading.Tasks.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Threading.Tasks.Tests (found 540 of 720 test cases)
  Starting:    System.Threading.Tasks.Tests (parallel test collections = on, max threads = 2)
   System.Threading.Tasks.Tests: [Long Running Test] 'System.Threading.Tasks.Tests.YieldAwaitableTests.RunAsyncYieldAwaiterTests', Elapsed: 00:02:11
   System.Threading.Tasks.Tests: [Long Running Test] 'System.Threading.Tasks.Tests.YieldAwaitableTests.RunAsyncYieldAwaiterTests', Elapsed: 00:04:11
   System.Threading.Tasks.Tests: [Long Running Test] 'System.Threading.Tasks.Tests.YieldAwaitableTests.RunAsyncYieldAwaiterTests', Elapsed: 00:06:11
   System.Threading.Tasks.Tests: [Long Running Test] 'System.Threading.Tasks.Tests.YieldAwaitableTests.RunAsyncYieldAwaiterTests', Elapsed: 00:08:11
   System.Threading.Tasks.Tests: [Long Running Test] 'System.Threading.Tasks.Tests.YieldAwaitableTests.RunAsyncYieldAwaiterTests', Elapsed: 00:10:11
   System.Threading.Tasks.Tests: [Long Running Test] 'System.Threading.Tasks.Tests.YieldAwaitableTests.RunAsyncYieldAwaiterTests', Elapsed: 00:12:11
   System.Threading.Tasks.Tests: [Long Running Test] 'System.Threading.Tasks.Tests.YieldAwaitableTests.RunAsyncYieldAwaiterTests', Elapsed: 00:14:11

...
[EXECUTION TIMED OUT]
Exit Code:-3Executor timed out after 900 seconds and was killed

  Discovering: System.Net.Sockets.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Net.Sockets.Tests (found 801 of 1201 test cases)
  Starting:    System.Net.Sockets.Tests (parallel test collections = on, max threads = 4)
    System.Net.Sockets.Tests.OSSupportTest.IOControl_SIOCATMARK_Unix_Success [FAIL]
      Assert.Equal() Failure
      Expected: 1
      Actual:   0
      Stack Trace:
        /_/src/libraries/System.Net.Sockets/tests/FunctionalTests/OSSupport.cs(108,0): at System.Net.Sockets.Tests.OSSupportTest.IOControl_SIOCATMARK_Unix_Success()
    System.Net.Sockets.Tests.CreateSocket.Ctor_Raw_Supported_Success [SKIP]
      Condition(s) not met: "SupportsRawSockets"
  Finished:    System.Net.Sockets.Tests
=== TEST EXECUTION SUMMARY ===
   System.Net.Sockets.Tests  Total: 1071, Errors: 0, Failed: 1, Skipped: 1, Time: 6.876s

The dotnet-runtime-perf (Performance Linux x64 release mono net5.0) leg failed with:

fatal: update_ref failed for ref 'HEAD': cannot update the ref 'HEAD': Trying to write ref HEAD with nonexistent object 865805a5267c1366fb5f9fa328185cd2b19d74b2

[vitek-karas]

I filed #37181 and #37183 for the test failures.