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.

Sign up for GitHub

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

Jump to bottom

Add class as a target for RequiresUnreferencedCodeAttribute #50064

Merged
merged 2 commits into from
Apr 1, 2021
Merged

Add class as a target for RequiresUnreferencedCodeAttribute #50064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2317dfd
Add class as a target for RequiresUnreferencedCodeAttribute
Mar 22, 2021
ad29a72
Merge branch 'main' into AddRucOnClass
tlakollo Mar 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ....Private.CoreLib/src/System/Diagnostics/CodeAnalysis/RequiresUnreferencedCodeAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace System.Diagnostics.CodeAnalysis
/// This allows tools to understand which methods are unsafe to call when removing unreferenced
/// code from an application.
/// </remarks>
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor, Inherited = false)]
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Class, Inherited = false)]
Copy link
Member

Choose a reason for hiding this comment

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

Why just class, rather than also, say, struct?

We should review the API change as part of API review, though mainly just to ensure we've properly discussed exactly what targets we want to enable, not because anyone would have a real problem with it.
cc: @terrajobst

Copy link
Member

Choose a reason for hiding this comment

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

Structs are problematic because they can be default-instantiated. We would like this to be considered unsafe (and warn) only if a constructor or a static method on the type is called to limit the amount of noise (and additional suppressions needed) when a type is annotated as such.

If we allow this on structs, we would have to warn on instance methods too, plus it would introduce problems when unsafe instance methods get called indirectly through interfaces (the tool would not be able to see that at all).

It's likely not worth the trouble.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for skipping the process, I created #50122 to discuss the API change

#if SYSTEM_PRIVATE_CORELIB
public
#else
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Runtime/ref/System.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6010,7 +6010,7 @@ public RequiresAssemblyFilesAttribute() { }
public string? Message { get { throw null; } set { } }
public string? Url { get { throw null; } set { } }
}
[System.AttributeUsageAttribute(System.AttributeTargets.Constructor | System.AttributeTargets.Method, Inherited=false)]
[System.AttributeUsageAttribute(System.AttributeTargets.Class | System.AttributeTargets.Constructor | System.AttributeTargets.Method, Inherited=false)]
public sealed partial class RequiresUnreferencedCodeAttribute : System.Attribute
{
public RequiresUnreferencedCodeAttribute(string message) { }
Expand Down
5 changes: 3 additions & 2 deletions src/libraries/shims/ApiCompatBaseline.PreviousNetCoreApp.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@ CannotRemoveAttribute : Attribute 'System.Diagnostics.CodeAnalysis.AllowNullAttr
Compat issues with assembly System.Net.Primitives:
CannotRemoveAttribute : Attribute 'System.Diagnostics.CodeAnalysis.AllowNullAttribute' exists on parameter 'value' on member 'System.Net.Cookie.Name.set(System.String)' in the contract but not the implementation.
Compat issues with assembly System.Runtime:
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute' changed from '[AttributeUsageAttribute(AttributeTargets.Constructor | AttributeTargets.Method, Inherited=false)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class | AttributeTargets.Constructor | AttributeTargets.Method, Inherited=false)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute' changed from '[AttributeUsageAttribute(AttributeTargets.Field | AttributeTargets.GenericParameter | AttributeTargets.Method | AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue, Inherited=false)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class | AttributeTargets.Field | AttributeTargets.GenericParameter | AttributeTargets.Interface | AttributeTargets.Method | AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue | AttributeTargets.Struct, Inherited=false)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'System.Runtime.Versioning.SupportedOSPlatformAttribute' changed from '[AttributeUsageAttribute(AttributeTargets.Assembly | AttributeTargets.Class | AttributeTargets.Constructor | AttributeTargets.Enum | AttributeTargets.Event | AttributeTargets.Field | AttributeTargets.Method | AttributeTargets.Module | AttributeTargets.Property | AttributeTargets.Struct, AllowMultiple=true, Inherited=false)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Assembly | AttributeTargets.Class | AttributeTargets.Constructor | AttributeTargets.Enum | AttributeTargets.Event | AttributeTargets.Field | AttributeTargets.Interface | AttributeTargets.Method | AttributeTargets.Module | AttributeTargets.Property | AttributeTargets.Struct, AllowMultiple=true, Inherited=false)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'System.Runtime.Versioning.UnsupportedOSPlatformAttribute' changed from '[AttributeUsageAttribute(AttributeTargets.Assembly | AttributeTargets.Class | AttributeTargets.Constructor | AttributeTargets.Enum | AttributeTargets.Event | AttributeTargets.Field | AttributeTargets.Method | AttributeTargets.Module | AttributeTargets.Property | AttributeTargets.Struct, AllowMultiple=true, Inherited=false)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Assembly | AttributeTargets.Class | AttributeTargets.Constructor | AttributeTargets.Enum | AttributeTargets.Event | AttributeTargets.Field | AttributeTargets.Interface | AttributeTargets.Method | AttributeTargets.Module | AttributeTargets.Property | AttributeTargets.Struct, AllowMultiple=true, Inherited=false)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'System.Runtime.CompilerServices.AsyncMethodBuilderAttribute' changed from '[AttributeUsageAttribute(AttributeTargets.Class | AttributeTargets.Delegate | AttributeTargets.Enum | AttributeTargets.Interface | AttributeTargets.Struct, Inherited=false, AllowMultiple=false)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class | AttributeTargets.Delegate | AttributeTargets.Enum | AttributeTargets.Interface | AttributeTargets.Method | AttributeTargets.Struct, Inherited=false, AllowMultiple=false)]' in the implementation.
Copy link
Member

Choose a reason for hiding this comment

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

Does this change have to happen? Or was this generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a conflicting change in main

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that answers the question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, it was generated, I had to make some changes because the count is wrong since a while back, and the conflicting attribute was under Compat issues with assembly System.Security.Cryptography instead of Compat issues with assembly System.Runtime

Compat issues with assembly System.Security.Cryptography.Algorithms:
CannotRemoveAttribute : Attribute 'System.Runtime.Versioning.UnsupportedOSPlatformAttribute' exists on 'System.Security.Cryptography.CryptoConfig' in the contract but not the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'System.Runtime.CompilerServices.AsyncMethodBuilderAttribute' changed from '[AttributeUsageAttribute(AttributeTargets.Class | AttributeTargets.Delegate | AttributeTargets.Enum | AttributeTargets.Interface | AttributeTargets.Struct, Inherited=false, AllowMultiple=false)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class | AttributeTargets.Delegate | AttributeTargets.Enum | AttributeTargets.Interface | AttributeTargets.Method | AttributeTargets.Struct, Inherited=false, AllowMultiple=false)]' in the implementation.
Total Issues: 16
Total Issues: 18
Copy link
Member

Choose a reason for hiding this comment

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

Why is this 18 instead of 17? Didn't we just add 1 new warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just count the number of warnings after the merge with main