Add UnmanagedCallersOnly constructor that takes a single type (UnmanagedCallersOnly is not CLSCompliant)

[MichalStrehovsky]

When trying to use [UnamangedCallersOnly(CallConvs = new Type[] { typeof(CallConvStdcall) })] in a CLSCompliant assembly (CoreLib...) I'm getting this warning from Roslyn:

CS3016 Arrays as attribute arguments is not CLS-compliant

This warning is also emitted if the attributed thing is marked internal or private. I'm unclear why this would matter.

Filing this issue in the runtime repo since this is where the attribute is but we might want to move to Roslyn.

On an unrelated note - why is this an array in the first place?

Cc @AaronRobinsonMSFT

[jkotas]

cc @333fred

[AaronRobinsonMSFT]

@MichalStrehovsky I don't know the why for Arrays as attribute arguments is not CLS-compliant, but imagine it is because VB.NET has no way to express them? I don't know VB.NET well enough to say that is the case, just a guess. @333fred or @jaredpar should be able to confirm the real reason.

The reason it is a Type[] is based on a series of conversations with the Roslyn team and some goals for the runtime. The desire to expose this attribute was to provide symmetry with the C# function pointers feature. One of the goals of the design was to decouple knowledge of calling conventions and modifiers from the metadata and the runtime. The solution was to avoid reuse/expansion of the CallingConvention enumeration and instead leverage the System.Runtime.CompilerServices.CallConv* types for declaring the calling convention. This will allow the runtime to add support and avoid requiring Roslyn to respond to that support. Support is dictated by the runtime and as new types are added support in the runtime can be added or blocked.

At this point the argument could be made we only need a single type to describe that and at the moment that is true. However, there is some expectation that as we enrich the interop boundary the number of calling conventions and modifiers will grow - consider CallConvSuppressGCTransition. This attribute isn't really a calling convention but rather a modifier. Thus we wanted a variable slot to potentially grow the options. This can be observed in the syntax for C# function pointers where a variable number of these types can be placed - see dotnet/roslyn#39865 (comment). The fact that the attribute has an array is to mimic that fact.

It could be possible to handle a common case, single value, and create an override for that. I don't believe this is something we can do in .NET 5, but open to that if need be. It would require work in Roslyn to respond and that window is closing fast. I think adding this in .NET 6 is reasonable.

That is the story from the implementation side of things. What is the case for making a CoreLib fully CLS compliant? I will say this attribute in principle is really for a C# specific language feature. Of course it has other use cases in AOT, but the impetus for exposing it in .NET 5 was for C# function pointers. Understanding why this is a .NET 5 issue would also be helpful, is this limitation impacting specific work?

[333fred]

As to the why arrays are not permitted for CLS compliance in attribute arguments, I also have no idea. I suspect that this thread may end up enlightening me though :). I am curious why this attribute would need to be CLS compliant though.

[MichalStrehovsky]

CLS compliance is just something that we enable on CoreLib, for what I assume are masochistic reasons (Roslyn-generated IL/metadata hasn't conformed to the CLS for quite some time).

What prompted me to open this issue is that:

1. Roslyn complains about CLS compliance when this attribute is applied on internal/private methods. I thought CLS compliance only matters on the public surface area. I would expect the warning to show up on the attribute itself, but it doesn't look like we have CLSCompliant(false) on it.
2. The Type[] felt a bit unergonomic. I had to update about a dozen places that used the old CallingConvention = format and having to type new Type[] { typeof(Foo) } was a bit annoying. Feels like this attribute is not designed for the 90% use case (just a single calling convention and no "calling convention modifiers").

I fixed this by adding CS3016 to NoWarn.

[AaronRobinsonMSFT]

Feels like this attribute is not designed for the 90% use case (just a single calling convention and no "calling convention modifiers").

We did try to keep that in mind. Not supplying a calling convention uses the platform default and what we hope is the 90 % case. The only scenarios were this really should matter would be on x86 where the calling convention isn't stdcall on Windows or cdecl on non-Windows.

[jkotas]

CLS compliance is just something that we enable on CoreLib, for what I assume are masochistic reasons

I have tried to remove the CLS compliant attribute on CoreLib at one point, but it had very bad fallout. I do not remember the details, just remember that it is not easy to fix.

[stephentoub]

It could be possible to handle a common case, single value, and create an override for that.

This is what we did in this release for the new MemberNotNullAttribute. It has two ctors, one taking a single string, which is CLS compliant, and one taking an array of strings, which isn't.

[AaronRobinsonMSFT]

This is what we did in this release for the new MemberNotNullAttribute.

@stephentoub Nice! Prior art here pushes this much closer to the "it should happen" camp. Still not sure it is worth it for .NET 5 given the current conversation but it seems perfect for an early .NET 6 API update.

[333fred]

stephentoub Nice! Prior art here pushes this much closer to the "it should happen" camp. Still not sure it is worth it for .NET 5 given the current conversation but it seems perfect for an early .NET 6 API update.

From the compiler side, at least, I don't think it would be very hard to react to having a single parameter or multiple. Considering that the work for support is the next thing on my list, it's not really a hard change to make.

[AaronRobinsonMSFT]

@333fred As in making this happen for the .NET 5 release? The runtime work shouldn't be too difficult, but I would defer to @elinor-fung who performed the most recent update.

[333fred]

As in making this happen for the .NET 5 release?

Yes. There's not much to add from the roslyn side, especially since I haven't done any of the work for the multiple types bit yet.

[elinor-fung]

I don't think it'd be much from the runtime side either.

[elinor-fung]

Actually, I think any runtime updates would be a lot more involved than my initial reaction. We're only looking at named arguments right now, so we don't have to deal with parsing potentially different constructors at all. Updating to be able to do that seems non-trivial.

[ghost]

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of our issue cleanup automation.

[ghost]

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.