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]: Add GetDeclaringType to PropertyDefinition and EventDefinition #60380

Open
MichalStrehovsky opened this issue Oct 14, 2021 · 7 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection.Metadata
Milestone

Comments

@MichalStrehovsky
Copy link
Member

Background and motivation

S.R.Metadata provides a way to get the owning type of fields and methods. It doesn't provide an API to get owning type of properties and events. I think it should be exposed for parity.

The lookup strategy is similar (a binary search in one of the tables).

API Proposal

namespace System.Reflection.Metadata
{
    public struct PropertyDefinition
    {
        public TypeDefinitionHandle GetDeclaringType();
    }

    public struct EventDefinition
    {
        public TypeDefinitionHandle GetDeclaringType();
    }
}

API Usage

PropertyDefinition someProperty = ...;
var owningType = someProperty.GetDeclaringType();

Risks

No response

@MichalStrehovsky MichalStrehovsky added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Reflection.Metadata labels Oct 14, 2021
@ghost
Copy link

ghost commented Oct 14, 2021

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

Issue Details

Background and motivation

S.R.Metadata provides a way to get the owning type of fields and methods. It doesn't provide an API to get owning type of properties and events. I think it should be exposed for parity.

The lookup strategy is similar (a binary search in one of the tables).

API Proposal

namespace System.Reflection.Metadata
{
    public struct PropertyDefinition
    {
        public TypeDefinitionHandle GetDeclaringType();
    }

    public struct EventDefinition
    {
        public TypeDefinitionHandle GetDeclaringType();
    }
}

API Usage

PropertyDefinition someProperty = ...;
var owningType = someProperty.GetDeclaringType();

Risks

No response

Author: MichalStrehovsky
Assignees: -
Labels:

api-suggestion, area-System.Reflection.Metadata

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Oct 14, 2021
@ghost ghost added this to Needs triage in Triage POD for Meta, Reflection, etc Oct 14, 2021
@MichalStrehovsky
Copy link
Member Author

Cc @tmat for a S.R.Metadata new API opinion.

@buyaa-n buyaa-n removed the untriaged New issue has not been triaged by the area owner label Oct 14, 2021
@buyaa-n buyaa-n added this to the 7.0.0 milestone Oct 14, 2021
@buyaa-n buyaa-n moved this from Needs triage to 7.0 in Triage POD for Meta, Reflection, etc Oct 14, 2021
@buyaa-n
Copy link
Member

buyaa-n commented Jul 7, 2022

Moving this to 8.0, @MichalStrehovsky please let us know if you need it in 7.0

@buyaa-n buyaa-n modified the milestones: 7.0.0, 8.0.0 Jul 7, 2022
@tmat
Copy link
Member

tmat commented Jul 7, 2022

API looks good to me.

@MichalStrehovsky MichalStrehovsky added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 8, 2022
@terrajobst
Copy link
Member

terrajobst commented Aug 16, 2022

Video

  • Looks good as proposed
namespace System.Reflection.Metadata;

public partial struct PropertyDefinition
{
    public TypeDefinitionHandle GetDeclaringType();
}

public partial struct EventDefinition
{
    public TypeDefinitionHandle GetDeclaringType();
}

@terrajobst terrajobst 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 Aug 16, 2022
@deeprobin
Copy link
Contributor

I took the liberty to implement this issue (see issue-60380). In retrospect, I realized that it would probably be useful to know a little better about the IL format.

After the implementation quite a few tests fail. I think I did something wrong with the offset calculation. Can anyone help me out here, or does anyone see where the error lies?

https://github.com/deeprobin/runtime/blob/de24b71d687975e8dff8cc03391807372dcedb81/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Internal/Tables.cs#L152-L168

I am grateful for any help.

/cc @MichalStrehovsky

@MichalStrehovsky
Copy link
Member Author

I didn't look in detail yet, but at minimum there are two copy-paste bugs in internal TypeDefinitionHandle GetDeclaringType(EventDefinitionHandle eventDef) (using stuff related to properties on events).

@steveharter steveharter modified the milestones: 8.0.0, 9.0.0 Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection.Metadata
Projects
No open projects
Development

No branches or pull requests

6 participants