-
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
Add obsolete attribute to DisablePrivateReflectionAttribute #49550
Add obsolete attribute to DisablePrivateReflectionAttribute #49550
Conversation
It seems to me that a more appropriate tag is |
cc: @stephentoub @jkotas |
@@ -3,6 +3,7 @@ | |||
|
|||
namespace System.Runtime.CompilerServices | |||
{ | |||
[Obsolete("DisablePrivateReflectionAttribute has no effect in .NET Core and .NET 5.0+ applications.")] |
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.
Thanks for working on this.
Per #43247 (comment), this needs an ID set on it. That ID will need to be recorded in https://github.com/dotnet/runtime/blob/0c3cc15f90f0e3b81ce3199be1ac217564043635/src/libraries/Common/src/System/Obsoletions.cs. You can see other examples in that file and how they're used in call sites.
In addition, this is editing the src file, but the class in the reference assembly file https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/System.Runtime/ref/System.Runtime.cs will also need to have the attribute applied.
@@ -3,6 +3,7 @@ | |||
|
|||
namespace System.Runtime.CompilerServices | |||
{ | |||
[Obsolete(Obsoletions.EscapeUriStringMessage, DiagnosticId = Obsoletions.EscapeUriStringDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] |
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.
This needs a new message and new diagnostic ID for this particular obsoletion added to that file, and then those new values used here. It shouldn't use the existing ones, which aren't relevant to this API.
@@ -9381,6 +9381,7 @@ public sealed partial class DependencyAttribute : System.Attribute | |||
public string DependentAssembly { get { throw null; } } | |||
public System.Runtime.CompilerServices.LoadHint LoadHint { get { throw null; } } | |||
} | |||
[System.ObsoleteAttribute("DisablePrivateReflectionAttribute has no effect in .NET Core and .NET 5.0+ applications.", DiagnosticId = "SYSLIB0013", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")] |
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.
SYSLIB0013 is already taken. A new obsoletion ID is needed.
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 was mentioned in the commentary on this issue, so I decided that it was necessary to use it. Thanks for the explanation. Can I take SYSLIB0015 instead?
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.
I hope I did it right :)
@stephentoub should I contribute the new obsoletion ID here https://github.com/dotnet/runtime/blob/main/docs/project/list-of-obsoletions.md? |
Yes, please. Thanks. |
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.
Thanks.
@stephentoub I've finished with docs. Thank you too. |
I see that some of the checks failed (exactly half). It is necessary to somehow mark in which places there should be no warnings that the attribute is now obsolete. How should this be done? |
@@ -48,5 +48,8 @@ internal static class Obsoletions | |||
|
|||
internal const string WebRequestMessage = "Use HttpClient instead."; | |||
internal const string WebRequestDiagId = "SYSLIB0014"; | |||
|
|||
internal const string DisablePrivateReflectionAttributeMessage = "DisablePrivateReflectionAttribute has no effect in .NET Core and .NET 5.0+ applications."; |
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.
This attribute still has effect in a few corner cases in CoreCLR. Search for DisablePrivateReflection
under src\coreclr
and delete everything found.
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.
Also, we may want to say .NET 6.0+
in the error message.
@jkotas I've finished with changes that you requested. But there is some trouble with building libs:
How can this be fixed? |
Could you please also delete this: https://github.com/dotnet/runtime/blob/main/src/coreclr/inc/corhdr.h#L1824-L1827 |
Try to add |
Yes, I've tried that. This cleared the error from netstandard, but followed in turn by the same error from mscorelib ( |
Ok, then try this central place instead: https://github.com/dotnet/runtime/blob/main/src/libraries/Directory.Build.targets#L20 |
@jkotas I've tried to add |
I think it is ok. |
@jkotas That works nice and looks better than adding NoWarn to every project file. I will commit this change. Thank you! |
@jkotas I think I have definitely finished working on this now. The checks are going well. Anything else? |
@@ -48,5 +48,8 @@ internal static class Obsoletions | |||
|
|||
internal const string WebRequestMessage = "Use HttpClient instead."; | |||
internal const string WebRequestDiagId = "SYSLIB0014"; | |||
|
|||
internal const string DisablePrivateReflectionAttributeMessage = "DisablePrivateReflectionAttribute has no effect in .NET Core and .NET 6.0+ applications."; |
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.
internal const string DisablePrivateReflectionAttributeMessage = "DisablePrivateReflectionAttribute has no effect in .NET Core and .NET 6.0+ applications."; | |
internal const string DisablePrivateReflectionAttributeMessage = "DisablePrivateReflectionAttribute has no effect in .NET 6.0+ applications."; |
@@ -9381,6 +9381,7 @@ public sealed partial class DependencyAttribute : System.Attribute | |||
public string DependentAssembly { get { throw null; } } | |||
public System.Runtime.CompilerServices.LoadHint LoadHint { get { throw null; } } | |||
} | |||
[System.ObsoleteAttribute("DisablePrivateReflectionAttribute has no effect in .NET Core and .NET 6.0+ applications.", DiagnosticId = "SYSLIB0015", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")] |
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.
[System.ObsoleteAttribute("DisablePrivateReflectionAttribute has no effect in .NET Core and .NET 6.0+ applications.", DiagnosticId = "SYSLIB0015", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")] | |
[System.ObsoleteAttribute("DisablePrivateReflectionAttribute has no effect in .NET 6.0+ applications.", DiagnosticId = "SYSLIB0015", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")] |
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.
Thank you!
Close #43247