Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Get info about generic attributes #64169

Closed
Tracked by #44655
VBAndCs opened this issue Jan 23, 2022 · 13 comments · Fixed by #68158
Closed
Tracked by #44655

[API Proposal]: Get info about generic attributes #64169

VBAndCs opened this issue Jan 23, 2022 · 13 comments · Fixed by #68158
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection Priority:3 Work that is nice to have
Milestone

Comments

@VBAndCs
Copy link

VBAndCs commented Jan 23, 2022

Background and motivation

Make it easier to handle generic attributes.

API Proposal

  • Allow use open generic as a param to Attribute.GetCustomAttributes Method.. It currently returns an empty array.
  • Add an the instance property TypeArgs to the Attribute class that returns all type args if any.

API Usage

var Attrs = Attribute.GetCustomAttributes(this.GetType(), typeof(TestAttribute<>));
var type = Attrs[0].TypeArgs[0];

Alternative Designs

No response

Risks

No response

@VBAndCs VBAndCs added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 23, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Reflection untriaged New issue has not been triaged by the area owner labels Jan 23, 2022
@ghost
Copy link

ghost commented Jan 23, 2022

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Make it easier to handle generic attributes.

API Proposal

  • Allow use open generic as a param to Attribute.GetCustomAttributes Method.
  • Add an the instance property TypeArgs to the Attribute class that returns all type args if any.

API Usage

var Attrs = Attribute.GetCustomAttributes(this.GetType(), typeof(TestAttribute<>));
var type = Attrs[0].TypeArgs[0];

Alternative Designs

No response

Risks

No response

Author: VBAndCs
Assignees: -
Labels:

api-suggestion, area-System.Reflection, untriaged

Milestone: -

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jan 24, 2022

Add an the instance property TypeArgs to the Attribute class that returns all type args if any.

I'm not certain why this is needed. Why not just do .GetType().GenericTypeArguments?

@buyaa-n
Copy link
Contributor

buyaa-n commented Jan 28, 2022

For adding a new property i agree with @CyrusNajmabadi, doesn't seem we need to add property for TypeArgs

Allow use open generic as a param to Attribute.GetCustomAttributes Method.. It currently returns a null.

Makes sense to me, will discuss with the team. As this is not a new API suggestion and we don't need the other API suggestion removing the api-suggestion label

@buyaa-n buyaa-n removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Jan 28, 2022
@buyaa-n buyaa-n added this to the 7.0.0 milestone Feb 2, 2022
@buyaa-n buyaa-n added the help wanted [up-for-grabs] Good issue for external contributors label Feb 11, 2022
@madelson
Copy link
Contributor

Just to clarify, is the proposal that calling any GetCustomAttributes() method with an open generic type deriving from Attribute will return all attributes which are or derive from generic instantiations of that type? So if we had:

class GenericAttribute<T> : Attribute { }
class DerivesFromGenericAttribute : GenericAttribute<string> { }

[GenericAttribute<int>, GenericAttribute<string>, DerivesFromGenericAttribute]
class HasAttributes { }

Then typeof(HasAttributes).GetCustomAttributes(typeof(GenericAttribute<>)) would return all 3 attributes?

@buyaa-n I'm potentially interested in working on this but I'd want to be sure of the expected behavior first.

@steveharter
Copy link
Member

Then typeof(HasAttributes).GetCustomAttributes(typeof(GenericAttribute<>)) would return all 3 attributes?

That is my understanding of the ask. I think it makes sense, although I don't know where this "open generic" convention is used elsewhere for similar filtering or scoping.

Since the workaround is not that difficult (get all custom attributes and manually filter) this feature is lower-priority.

@steveharter steveharter added the Priority:3 Work that is nice to have label Feb 15, 2022
@steveharter
Copy link
Member

@GrabYourPitchforks if we add a new convention for open generic types to be used to mean all closed, does this rise to the level of discussing it in an API review meeting?

@buyaa-n
Copy link
Contributor

buyaa-n commented Feb 16, 2022

Then typeof(HasAttributes).GetCustomAttributes(typeof(GenericAttribute<>)) would return all 3 attributes?

I believe that should depend on if the bool inherit parameter set, as it is false by default, in this case DerivesFromGenericAttribute should not be included.

@buyaa-n
Copy link
Contributor

buyaa-n commented Apr 13, 2022

@GrabYourPitchforks if we add a new convention for open generic types to be used to mean all closed, does this rise to the level of discussing it in an API review meeting?

@GrabYourPitchforks suggested to go through API review

@buyaa-n buyaa-n added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed help wanted [up-for-grabs] Good issue for external contributors labels Apr 13, 2022
@terrajobst
Copy link
Member

terrajobst commented Apr 14, 2022

Yes, reflection should support retrieving attribute by open generic types. And yes, using a base type should also work, assuming inherit is set to true.

@buyaa-n buyaa-n added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 14, 2022
@buyaa-n
Copy link
Contributor

buyaa-n commented Apr 14, 2022

I'm potentially interested in working on this but I'd want to be sure of the expected behavior first.

Thanks @madelson, the expected behavior is approved, do you still interested in working on this?

@madelson
Copy link
Contributor

@buyaa-n sure I'll try to take a look at it this weekend to get a sense of what is involved.

@madelson
Copy link
Contributor

@buyaa-n :

I believe that should depend on if the bool inherit parameter set, as it is false by default, in this case DerivesFromGenericAttribute should not be included.

I'm a bit confused. Are we talking about the inherit parameter on MemberInfo.GetCustomAttributes(bool)? That parameter is supposed to determine whether to to search this member's inheritance chain to find the attributes; I don't think it affects type-based filtering of the attributes.

For example:

typeof(RegexOptions).GetCustomAttributes(typeof(Attribute), inherit: false) // returns the FlagsAttribute

What am I missing?

@buyaa-n
Copy link
Contributor

buyaa-n commented Apr 18, 2022

You are right, I have mistaken about the parameter.

In this case the behavior should be same as how it works for non generic attributes, and Attribute.GetCustomAttributes(,typeof(TestType), typeof(NonGenericAttribute)) returns all derived attribute by default

madelson added a commit to madelson/runtime that referenced this issue Apr 18, 2022
Filtering on an open generic type retrieves all attributes that are or
are derived from constructed instances of that type.

fix dotnet#64169
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 18, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 22, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection Priority:3 Work that is nice to have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants