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

Hide reflection additions introduced for Windows Store #111079

Merged
merged 3 commits into from
Jan 21, 2025

Conversation

pedrobsaila
Copy link
Contributor

Fixes #89975

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 3, 2025
@jkoritzinsky
Copy link
Member

Based on #53217 (comment) (the comment after the one linked in the issue), the CustomAttributeExtensions class shouldn't be marked EB.Never.

@pedrobsaila
Copy link
Contributor Author

pedrobsaila commented Jan 4, 2025

Based on #53217 (comment) (the comment after the one linked in the issue), the CustomAttributeExtensions class shouldn't be marked EB.Never.

It's just a wrapper around static calls of Attribute class, CustomAttributeExtensions doesn't provide anything additional comparing to the Attribute class

@jkoritzinsky
Copy link
Member

It provides the generic GetCustomAttribute<T> methods that no one else provides. It's not just a wrapper like the other types.

@pedrobsaila
Copy link
Contributor Author

pedrobsaila commented Jan 4, 2025

It provides the generic GetCustomAttribute<T> methods that no one else provides. It's not just a wrapper like the other types.

This method does just an explicit cast for the Attribute.GetCustomAttribute method result:

(T)Attribute.GetCustomAttribute(element, attributeType);

@jkoritzinsky
Copy link
Member

Yes, but this API shape is more convenient and this is the only type that provides it. The other APIs are requested to be hidden as they're the exact same API shape as an existing API and not unique API shapes.

@@ -12677,18 +12677,30 @@ public enum ResourceLocation
ContainedInAnotherAssembly = 2,
ContainedInManifestFile = 4,
}
[System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)]
[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]

ref sources should use the full type name for attributes including Attribute suffix. It is the style used by the ref sources auto-generator.

(multiple places)

@tannergooding
Copy link
Member

Doesn't this need to goto API review for approval first?

We typically treat new attributes, particularly ones like EditorBrowsable, Obsolete, etc as part of the "public API surface" and get explicit sign-off (even where its a clear thing we want to do).

@pedrobsaila
Copy link
Contributor Author

Doesn't this need to goto API review for approval first?

We typically treat new attributes, particularly ones like EditorBrowsable, Obsolete, etc as part of the "public API surface" and get explicit sign-off (even where its a clear thing we want to do).

I think it was done as part of #53217

@ericstj
Copy link
Member

ericstj commented Jan 7, 2025

I see in #53217 (comment) that @steveharter mentioned there were some open questions remaining about scope. I'd like his review here.

@steveharter
Copy link
Member

I updated #89975 to add the affected APIs and marked that issue as "api-ready-for-review". Once approved, this PR can go in.

@steveharter
Copy link
Member

I see in #53217 (comment) that @steveharter mentioned there were some open questions remaining about scope. I'd like his review here.

Yeah based on that I created #89975 to track which I just updated (see above).

@steveharter
Copy link
Member

API was approved

@steveharter steveharter merged commit 4365cd2 into dotnet:main Jan 21, 2025
137 of 139 checks passed
@pedrobsaila pedrobsaila deleted the 89975 branch January 21, 2025 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Reflection community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide TypeInfo and references to it
6 participants