Mono does not support querying for and open generic type when searching for Attributes

[davidwrighton]

Description

While fixing issue #56492 in coreclr, I discovered that the generic attribute test was not running on any platform. When I enabled it, not only did mono have issue #56492, it also didn't appear to support generic attribute at all. This may be a blocker for dotnet/roslyn#55190. As part of the fix for the coreclr bug, in PR #56879 I will be fixing the build of the generic attribute test, but disabling it on mono platforms.

@RikkiGibson, please comment on this issue and its importance.
@AlekseyTs, fyi, please make sure the right person in mono is aware of this.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area:
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

While fixing issue #56492 in coreclr, I discovered that the generic attribute test was not running on any platform. When I enabled it, not only did mono have issue #56492, it also didn't appear to support generic attribute at all. This may be a blocker for dotnet/roslyn#55190. As part of the fix for the coreclr bug, in PR #56879 I will be fixing the build of the generic attribute test, but disabling it on mono platforms.

@RikkiGibson, please comment on this issue and its importance.
@AlekseyTs, fyi, please make sure the right person in mono is aware of this.

Author:	davidwrighton
Assignees:	-
Labels:	area-System.Reflection-mono, untriaged
Milestone:	-

[SamMonoRT]

@lambdageek @vargaz fyi

[vargaz]

The test fails at this line:
Assert(CustomAttributeExtensions.GetCustomAttributes(programTypeInfo, typeof(MultiAttribute<>), false) == null);

because it calls Array.CreateInstance () with the open generic type.
Looking at the custom attribute code, I don't see the place where it checks for generic type definitions and returns null on coreclr.

[RikkiGibson]

I am looking into whether the compiler team considers it a ship blocker for the feature. cc @jaredpar @jcouv

[davidwrighton]

Ah, if its just the issue around the open type... see internal static object[] GetCustomAttributes(RuntimeType type, RuntimeType caType, bool inherit) in src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs. there is a check bool useObjectArray = (caType.IsValueType || caType.ContainsGenericParameters); and typeof(MultiAttribute<>) ContainsGenericParemters I believe. So in that case instead of creating an array of type MultiAttribute<> the array is of type object.

[davidwrighton]

I do agree though, where is the null coming from? That just seems odd. I'll take a look at the coreclr behavior, and try and figure out what is supposed to trigger that to happen..

[davidwrighton]

Ah. The null comes from

runtime/src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs

Line 468 in 1fb151f

_ => (element.GetCustomAttributes(attributeType, inherit) as Attribute[])!,

The internal custom attribute functions return object[] and then that line converts the object[] into an Attribute[] or null. I can't explain why anyone would write the code this way, but that's where the null comes from.

[RikkiGibson]

Speculatively I think the compiler sees a few options from our POV:

1. We are able to get generic attributes support in Mono in the .NET 6 timeframe and the compiler ships "generic attributes" in C# 10. This is the happy path.
2. Mono won't be able to support generic attributes in .NET 6, and the compiler ships "generic attributes" under a preview flag with the caveat that it may not currently work depending on the target runtime.
3. (not sure if this would work) We introduce a RuntimeFeature for "generic attributes" and make the compiler detect its presence during compilation, ensuring that compilation fails against platforms which don't fully support the feature.

Feel free to correct me here @jcouv @jaredpar if these options don't seem right.

[davidwrighton]

@RikkiGibson I've spent a bit of time looking into the issue (and figuring out how to run the tests under Mono), and I can say that while the Generic Attributes test as written fails, it ONLY fails for the case of attempting to look up an attribute of an open type. As that particular query could exist outside of any actual usage of generic attributes I believe it is irrelevant to general generic attribute support.

As such, I think that there is no risk to the C# feature at this time even if this particular issue isn't fixed in Mono, and that I'd like @vargaz/@lambdageek to determine what priority/approach they wish to take on fixing the remaining issue identified via this test.

[vargaz]

Is the GetCustomAttribute () call supposed to return null for an open type ? According to the docs, it should return a (possible empty) array, or throw an exception.

[davidwrighton]

Well that's a good question. I'd like to defer to @steveharter for what should this thing be doing, and then lets make the two runtimes implement the same logic. I'm generally of the opinion that the particular scenario we're talking about here is probably just wrong.

[Joe4evr]

I can say that while the Generic Attributes test as written fails, it ONLY fails for the case of attempting to look up an attribute of an open type.

Just to confirm, that means in a case of:

public sealed MyGenericAttribute<T> : NonGenericBaseAttribute { }

asking GetCustomAttributes<NonGenericBaseAttribute>() does include any instances of MyGenericAttribute<> with whatever type arguments were at the use-site?

I might be biased, but I'm under the impression that's going to be the majority case. Of course, if Mono can fix the problematic case in time, that's all the better, but maybe it doesn't need to be a full blocker.

[vargaz]

If we want to make GetCustomAttributes () work with an open type, then it needs to return an object[], since the return array needs to contain different instantiations of the same type.

[davidwrighton]

I don't think we have any consensus on what the behavior should be for anything involving open generics and the GetCustomAttributes api. Obviously we can't return an array of open types. I'm not entirely comfortable with just returning null as CoreCLR does, and I'm pretty sure an exception around bad array type creation isn't what should happen either. Also we can't change the signature of any public apis. That is why I pinged @steveharter because he's nominally in charge of reflection, and might be able to provide a more reasoned opinion on what should happen. However as this is a fairly edge case, I'm ok with shipping what we do have. However, if we decide to do that, I'd like to know so I can modify the existing coreclr runtime test to get rid of the current tests around open generics and re--enable it for mono, as we don't have any coverage for generic attributes in mono at all right now.

[Joe4evr]

If we want to make GetCustomAttributes () work with an open type, then it needs to return an object[], since the return array needs to contain different instantiations of the same type.

Doesn't it already return Attribute[]? That's already enough.

[vargaz]

Returning Attribute[] could work.

[SamMonoRT]

Is there any work to be done here for 6.0 ?

[davidwrighton]

No, we'll deal with this in 7.0. I quite like the idea of consolidating on the idea that a request for the open type yields an Attribute[] filled with the closed instances that are actually on the metadata object, but I don't think that will happen now. Also, from some internal discussion about the thicket of runtime/metadata problems that have been identified it looks like the C# feature will be pushed into preview, so we can both get the code out there, and fix the rest of the ecosystem before we make this a fully supported C# feature.

[madelson]

Also related: #64169

[SamMonoRT]

@davidwrighton @vargaz - do we need any work towards this for 7.0 ? Want to make sure we have enough time for that if needed.

[SamMonoRT]

Moving to 8.0.0 for any new Mono support needed.

[marek-safar]

Can this issue be now closed?

[vargaz]

Fixed in #78091.