-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Annotate unsupported APIs in .Net core as Obsolete - Reflection area #50941
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to 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. |
src/libraries/System.Private.CoreLib/src/System/Reflection/StrongNameKeyPair.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, assuming that this has gone through API review and that the breaking change has been approved.
There was no API review, but we had an offline discussion that we want to Obsolete APIs not supported in .Net core for all platforms so that developers get warnings at design time instead of getting PNSE at runtime (from that perspective i think it is not breaking change). We filed issues for each area so that area owners handled it appropriately, main thing the area owners need to consider/make sure is the APIs are really not supported on any platform and not intended/planned to get supported in the future. For the APIs in this PR APIs in |
src/libraries/System.Private.CoreLib/src/System/Reflection/StrongNameKeyPair.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/StrongNameKeyPair.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/AssemblyName.cs
Outdated
Show resolved
Hide resolved
bfdf1ea
to
50def75
Compare
According to source.dot.net, there are only two instantiations of https://github.com/dotnet/msbuild/blob/9bcc06cbe19ae2482ab18eab90a82fd079b26897/src/Tasks/StrongNameUtils.cs#L68-L84 |
d7f9ae7
to
94de4b3
Compare
src/libraries/System.Private.CoreLib/src/System/Reflection/StrongNameKeyPair.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/Assembly.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A missing UrlFormat, but otherwise LGTM. Thanks!
src/libraries/System.Private.CoreLib/src/System/Reflection/AssemblyName.cs
Outdated
Show resolved
Hide resolved
34edc80
to
e20427e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there's a check failing in src/mono/System.Private.CoreLib/src/System/RuntimeTypeHandle.cs
where another pragma will be needed. But I preemptively approve based on that getting resolved.
What course of action should the downstream consumers affected by this change take? E.g. dotnet/winforms is currently broken because
/cc: @terrajobst |
You could also just suppress the obsoletion. I doubt anyone would care :-) |
I didn't consider it, that's an option too :) |
Fixes #50529
As part of #47228 we are annotating APIs not supported in .Net Core with [Obsolete] attribute