Add class as a target for RequiresUnreferencedCodeAttribute #50064
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. |
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
tlakollo
added
the
linkable-framework
|
Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding, @sbomer Issue DetailsBackground and MotivationAs explained in dotnet/linker#1742 some types are considered not trim compatible leading to mark all its member methods with RequiresUnreferencedCode. This could lead to hundreds of members annotated with RequiresUnreferencedCode. The proposal is to only annotate the type with RequiresUnreferencedCode, then the linker will treat this as if we annotated constructors and static methods with RequiresUnreferencedCode.
|
| @@ -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)] | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sorry for skipping the process, I created #50122 to discuss the API change
stephentoub
added
the
NO-MERGE
|
Since #50122 got approved this can now be reviewed |
| 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 |
There was a problem hiding this comment.
Why is this 18 instead of 17? Didn't we just add 1 new warning?
There was a problem hiding this comment.
I just count the number of warnings after the merge with main
| 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. |
There was a problem hiding this comment.
Does this change have to happen? Or was this generated?
There was a problem hiding this comment.
It was a conflicting change in main
There was a problem hiding this comment.
I don't think that answers the question.
There was a problem hiding this comment.
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
ghost
locked as resolved and limited conversation to collaborators
eerhardt
removed
the
NO-MERGE
Background and Motivation
As explained in dotnet/linker#1742 some types are considered not trim compatible leading to mark all its member methods with RequiresUnreferencedCode. This could lead to hundreds of members annotated with RequiresUnreferencedCode. The proposal is to only annotate the type with RequiresUnreferencedCode, then the linker will treat this as if we annotated constructors and static methods with RequiresUnreferencedCode.