Enable generic attributes #9189

AviAvni · 2017-01-28T18:51:04Z

Succesfully compile and run https://gist.github.com/AviAvni/00831abbddf4ceca198236de362ed449

Merge branch 'master'

merge from master

jkotas · 2017-01-29T01:51:47Z

src/vm/customattribute.cpp

@@ -695,10 +694,6 @@ FCIMPL2(Object*, RuntimeTypeHandle::CreateCaInstance, ReflectClassBaseObject* pC
        PRECONDITION(
            (!pCtor && gc.refCaType->GetType().IsValueType() && !gc.refCaType->GetType().GetMethodTable()->HasDefaultConstructor()) || 
            (pCtor == gc.refCaType->GetType().GetMethodTable()->GetDefaultConstructor()));
-
-        // If we relax this, we need to insure custom attributes construct properly for Nullable<T>


How does this work for Nullable types that this comment is talking about?

If you mean that the generic argument is nullable so I tried with int? and it works great too

AviAvni · 2017-01-30T15:16:13Z

@jkotas Do I need to do more things to get this PR merged?

ghost · 2017-01-30T15:46:43Z

src/vm/customattribute.cpp

@@ -671,7 +671,6 @@ FCIMPL2(Object*, RuntimeTypeHandle::CreateCaInstance, ReflectClassBaseObject* pC
    CONTRACTL {
        FCALL_CHECK;
        PRECONDITION(CheckPointer(pCaTypeUNSAFE));
-        PRECONDITION(!pCaTypeUNSAFE->GetType().IsGenericVariable()); 


I believe this precondition is still valid. This is guarding against something like this:

class G<T> { [T] // <-- still disallowed, correct? class Inner {} }

@atsushikan You right sorry I revert this delete and validated it's still work

jkotas · 2017-01-30T15:57:37Z

@atsushikan Could you please take a look at this?

There will need to be tests added or updated in corefx.

ghost · 2017-01-30T16:11:13Z

What I see here LGTM but the framework has a bunch of overlapping custom-attribute fetching apis and the code isn't as factored for sharing as it could be.

Attribute.GetCustomAttribute(*), IsDefined, etc.
CustomAttributeExtensions.GetCustomAttributes() (mostly wraps Attribute, but should check.)
ICustomAttributeProvider.GetCustomAttributes() (implemented on MemberInfo, Assembly, Module, etc. - this is largely its own codepath, I recall.)
CustomAttributeData.GetCustomAttributes(),
MemberInfo.CustomAttributes properties (and on Assemblies, Modules)

Can you test some of these as well to make sure all our code paths have been updated?

AviAvni · 2017-01-30T16:17:39Z

@atsushikan what is the best place to write this tests?
I'll check them
@jkotas do you mean the tests of System.Reflection.* on CoreFX repository I'll check them too

ghost · 2017-01-30T16:23:42Z

System.Runtime\tests is where you'd add the tests, but this assumes that this new language feature is already enabled in the corefx toolset. If not, you should leave an issue open for writing tests once the feature is available.

For now, I just meant test it privately as you've been doing...

AviAvni · 2017-01-30T16:25:29Z

@atsushikan I also made PR for Roslyn dotnet/roslyn#16814 so when this will merge hope also roslyn merge the PR

jkotas · 2017-01-30T17:06:42Z

Should I be able to say myType.GetCustomAttribute(MyCustomAttribute<>) and expect it to work for any T?

AviAvni · 2017-01-30T17:09:20Z

@jkotas I'll check this too

ghost · 2017-01-30T18:30:38Z

Should I be able to say myType.GetCustomAttribute(MyCustomAttribute<>) and expect it to work for any T?

This sounds like appropriate material for an api proposal. It's a bit iffy - it conflates the concept of assignability with instantiation.

Note that this would be a breaking change if we don't do it in NS2.0 as in some instances (the GetCustomAttribute() apis off MemberInfo (the ones not "added" by CustomAttributeExtensions) don't do any validation on the passed in attribute type, so this kind of call today would return an empty array (as in "Yessir, none of the attributes on this member are assignable to MyCustomAttribute<>")

MichalStrehovsky · 2017-01-30T19:45:26Z

How will this work for serialized custom attribute data? Suppose we have:

class FooAttribute<T> : Attribute
{
    public T Field;
}

Now, I would expect this to work:

[Foo<int>(Field = 123)]
class X { }

But this shouldn't:

[Foo<int?>(Field = 123)]
class Y { }

Will the class library fail appropriately? Will Roslyn be checking for this too?

AviAvni · 2017-01-30T20:45:56Z

@MichalStrehovsky roslyn already check this

MichalStrehovsky · 2017-01-30T20:50:55Z

@MichalStrehovsky roslyn already check this

But the runtime needs to check for this too. There could be a buggy compiler that emits such metadata blob. We need to make sure it fails as expected (and make sure we have test coverage for that).

It's possible the runtime already does handle it, but since this PR is removing some validation that would previously cover it, we need to ensure the new failure modes the removed validation opens us up to are covered as well.

AviAvni · 2017-01-30T20:56:57Z

@MichalStrehovsky I'm working on tests I'll add this scenario thanks

AviAvni · 2017-01-31T14:02:02Z

@atsushikan Please look at my tests https://gist.github.com/AviAvni/3ee62f4c93d33485db880cf01011e0b6

@MichalStrehovsky I tried to make tests that you mentioned by taking the IL from the tests exe with ildasm
https://gist.github.com/AviAvni/388b8a3023181bd78f9d56e85c2a4b31
and change it and compile back with ilasm but I need help what to change I tried to make line 138 value like 137 value not sure if this correct

ghost · 2017-01-31T16:38:08Z

Be careful about distinguishing beween the instance member GetCustomAttributes() and the extension method CustomAttributes() - they're called using the same C# syntax but for reasons that escape me, aren't the same at all internally.

It's probably safest to write the tests so that the instance member version is always invoked through the ICustomAttributeProvider interface rather directly from the member, while the extension method is invoke without using the extension method syntax, i.e. rather than writing

memberInfo.GetCustomAttribute(typeof(D), inherit: true),

write

((ICustomAttributeProvider)memberInfo).GetCustomAttributes(typeof(D), inherit: true)

and

CustomAttributeExtensions.GetCustomAttribute(memberInfo, typeof(D), inherit: true).

That way, it's always clear which one you mean.

AviAvni · 2017-01-31T18:25:07Z

@atsushikan thanks I updated the gist
now I'm investigating why https://gist.github.com/AviAvni/3ee62f4c93d33485db880cf01011e0b6#file-genericattributetests-cs-L171 have different behavior

AviAvni · 2017-01-31T18:54:28Z

@atsushikan now the difference is in line https://gist.github.com/AviAvni/3ee62f4c93d33485db880cf01011e0b6#file-genericattributetests-cs-L187
and still don't understand why it's happend

ghost · 2017-01-31T19:21:25Z

CustomAttributeExtensions.GetCustomAttributes() should never return null. If there are no matching attributes, it should return a 0-length enumeration.

jkotas · 2018-02-04T15:54:25Z

tests/src/reflection/GenericAttribute/GenericAttributeMetadata.cs

@@ -0,0 +1,72 @@
+using System;


Could you please add license header?

AviAvni · 2018-02-04T16:54:36Z

@jkotas I added the license header and the missing spaces sorry

jkotas · 2018-02-05T03:11:52Z

@AviAvni The change looks good to me now. Thanks!

We are too close to snapping 2.1 release and I do not think there is runway to verify this together with the matching C# changes. This will need to wait after 2.1.

ghost · 2018-02-07T17:40:25Z

tests/src/reflection/GenericAttribute/GenericAttributeTests.cs

+        AssertAny(b8, a => (a as MultiAttribute<bool?>)?.Value == null);
+
+        Assert(CustomAttributeExtensions.GetCustomAttributes(programTypeInfo, typeof(MultiAttribute<>), false) == null);
+        Assert(CustomAttributeExtensions.GetCustomAttributes(programTypeInfo, typeof(MultiAttribute<>), true) == null);


If you pass in an open attribute type to this api (typeof(MultiAttribute<)), I think it should either:

Throw a NotSupportedException saying explicitly that the attribute type cannot be an open type (and for consistency, make Attribute.GetCustomAttributes work the same way) I think this is the preferable option as I can't think of a good use case for passing in an open type here.

or

Return a zero-length array of type Attribute[].

We shouldn't be returning null - there is no precedent for this api returning null.

Edit: Changed ArgumentException to NotSupportedException since that's the behavior you see today with the api.

@atsushikan I implemented this on Attribute.GetCustomAttributes and with CustomAttributeExtensions api it throws NotSupportedException as expected
but it not work for ICustomAttributeProvider api is this api need also to throw?

ghost

Passing in an open type as the attribute type should continue to throw NotSupportedException. See my last comment.

Edit: My bad - returning null is the NETFX compatible behavior. It was CoreRT that was incompatible.

ghost · 2018-02-08T14:51:42Z

We should implement the compatible behavior. I just checked on .NETFx – the ICustomAttribute api returns a zero-length array of type Object[] when passed an open type. That’s a bit unfortunate – I’d have preferred a consistent response from all three variants, but compatibility is more important than reporting an obvious programmer error the same way everywhere. So let’s have that variant continue to return an empty Object array.

AviAvni · 2018-02-08T15:14:59Z

@atsushikan I made the change you asked

ghost · 2018-02-08T15:23:39Z

src/mscorlib/src/System/Attribute.cs

@@ -459,6 +459,9 @@ public static Attribute[] GetCustomAttributes(MemberInfo element, Type type, boo
            if (type == null)
                throw new ArgumentNullException(nameof(type));

+            if (type.IsGenericTypeDefinition)


The api you want is Type.ContainsGenericParameters. This will catch open types such as G<int, T> in addition to G<>.

ghost · 2018-02-08T15:24:56Z

tests/src/reflection/GenericAttribute/GenericAttributeTests.cs

+{
+    static int Main(string[] args)
+    {
+        new MultiAttribute<string>("a");


This line looks like an accidental leftover.

ghost · 2018-02-08T15:34:55Z

Ah - I just checked the behavior of Attribute.GetCA and CustomAttributeExtensions.GetCA on NETFX and it seems they do return null when passed in an open generic type. It was the CoreRT framework that was throwing NotSupportedException. As much as returning null feels wrong to me, we should stay compatible with NETFX. Your original version (11c28a8) is fine as is. I'll make the change to CoreRT so it matches.

Sorry about the bother.

…bute" This reverts commit 69603dc.

jkotas · 2018-04-16T18:10:02Z

@AviAvni Could you please resolve the conflict? This can be merged now.

AviAvni · 2018-04-17T04:47:44Z

@jkotas conflict resolved build is green

jkotas · 2018-04-17T04:52:26Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

AviAvni · 2018-04-17T07:28:59Z

@jkotas build is green

jkotas · 2018-04-17T14:16:05Z

@AviAvni Thank you for your persistence in pushing this through!

AviAvni · 2018-04-17T14:19:07Z

@jkotas thank you and the team very much for helping me I'll follow if there is more things from me
If there are more things needed on this ping me

AviAvni · 2019-06-07T14:00:39Z

Hiii @jkotas I'm working on the Roslyn PR and testing with .net core sdk preview 6
I found that in case the generic attribute parameter is String and the ctor is not the default one then there is a bug that the attribute generic parameter that return from GetCustomAttribute is System.__Canon and not String I'll try to debug this on monday and check what is missing in the tests

jkotas · 2019-06-07T15:19:19Z

@AviAvni Thank you! If you won't be able to get to this soon, could you please at least file an issue in CoreCLR repo so that we have it tracked?

AviAvni added 3 commits July 22, 2016 13:08

Merge pull request #1 from dotnet/master

257a880

Merge branch 'master'

Merge pull request #2 from dotnet/master

f531ffe

merge from master

enable generic attributes

0182671

dnfclas added the cla-already-signed label Jan 28, 2017

AviAvni mentioned this pull request Jan 28, 2017

Enable generic attributes dotnet/roslyn#16814

Closed

jkotas reviewed Jan 29, 2017

View reviewed changes

jkotas added the area-Reflection label Jan 29, 2017

jkotas assigned ghost and DnlHarvey Jan 29, 2017

ghost reviewed Jan 30, 2017

View reviewed changes

revert delete check for attribute as generic variable

dc0e9c3

AviAvni closed this Jan 31, 2017

jkotas reviewed Feb 4, 2018

View reviewed changes

tests/src/reflection/GenericAttribute/GenericAttributeMetadata.cs

@@ -0,0 +1,72 @@

using System;

Copy link

Member

jkotas Feb 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add license header?

fix review issues

11c28a8

ghost reviewed Feb 7, 2018

View reviewed changes

ghost suggested changes Feb 7, 2018

View reviewed changes

throw NotSupportedException when reqesting open generic attribute

69603dc

ghost reviewed Feb 8, 2018

View reviewed changes

Update GenericAttributeTests.cs

4305967

Revert "throw NotSupportedException when reqesting open generic attri…

0a4df91

…bute" This reverts commit 69603dc.

ghost approved these changes Feb 8, 2018

View reviewed changes

Merge branch 'master' into generic-attributes

166860f

jkotas merged commit 1d406b1 into dotnet:master Apr 17, 2018

AviAvni mentioned this pull request Apr 23, 2018

Enable generic attributes dotnet/roslyn#26337

Merged

NickCraver mentioned this pull request Dec 10, 2018

Fix for #21456 (Regressions in attribute allocations for non-generic attributes) #21462

Merged

AviAvni unassigned DnlHarvey Dec 31, 2018

jcouv mentioned this pull request Apr 24, 2020

Test plan for "generic attributes" dotnet/roslyn#36285

Closed

25 tasks

robertmclaws mentioned this pull request Jul 26, 2022

Allow common types in attribute values (nullable, decimal) dotnet/runtime#4525

Open

Enable generic attributes #9189

Enable generic attributes #9189

Conversation

AviAvni commented Jan 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AviAvni commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Jan 30, 2017

ghost commented Jan 30, 2017

AviAvni commented Jan 30, 2017

ghost commented Jan 30, 2017

AviAvni commented Jan 30, 2017

jkotas commented Jan 30, 2017

AviAvni commented Jan 30, 2017

ghost commented Jan 30, 2017

MichalStrehovsky commented Jan 30, 2017

AviAvni commented Jan 30, 2017

MichalStrehovsky commented Jan 30, 2017

AviAvni commented Jan 30, 2017

AviAvni commented Jan 31, 2017 • edited Loading

ghost commented Jan 31, 2017

AviAvni commented Jan 31, 2017

AviAvni commented Jan 31, 2017

ghost commented Jan 31, 2017

Choose a reason for hiding this comment

AviAvni commented Feb 4, 2018

jkotas commented Feb 5, 2018

ghost Feb 7, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment • edited by ghost Loading

Choose a reason for hiding this comment

ghost commented Feb 8, 2018 via email

AviAvni commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 8, 2018

jkotas commented Apr 16, 2018

AviAvni commented Apr 17, 2018

jkotas commented Apr 17, 2018

AviAvni commented Apr 17, 2018

jkotas commented Apr 17, 2018

AviAvni commented Apr 17, 2018 • edited Loading

AviAvni commented Jun 7, 2019

jkotas commented Jun 7, 2019

AviAvni commented Jan 31, 2017 •

edited

Loading

ghost Feb 7, 2018 •

edited by ghost

Loading

ghost left a comment •

edited by ghost

Loading

AviAvni commented Apr 17, 2018 •

edited

Loading