dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.97k stars 4.66k forks source link

[API Proposal]: An attribute to indicate boxed value type is not expected for certain API #91191

Open huoyaoyuan opened 1 year ago

huoyaoyuan commented 1 year ago

Background and motivation

There are APIs that accepts object and operates with object identity, thus boxed structs will not work.

Examples including:

ArgumentNullException.ThrowIfNull (#85154) is similar, but the motivation is different, and will have different meaning with nullable value type.

With such an attribute, we can have one analyzer for all these APIs, without recognizing them one by one, and also works for user code. Can it be reviewed together?

API Proposal

namespace System.Diagnostics.CodeAnalysis;

[AttributeUsage(AttributeTargets.Parameter, Inherited = true)]
public class ObjectIdentyExpectedAttribute : Attribute
{
}

The analyzer will warn if the declared type of the parameter is value type.

API Usage

// Moitor.Enter annotated with ObjectIdentyExpected

lock (myStruct) // analyzer warns here
{

}

Alternative Designs

Should it be named as what's expected, or what's unexpected?

Should it be inherited?

Should we include a custom diagnostic id for analyzer? Users are unlikely to suppress such warnings though.

Risks

No response

teo-tsirpanis commented 1 year ago

It is valid to pass a struct to GC.KeepAlive if you want to keep its object fields alive, and I heard that the JIT optimizes it to not box.

jakobbotsch commented 1 year ago

and I heard that the JIT optimizes it to not box.

Correct -- #54412.

ghost commented 1 year ago

Tagging subscribers to this area: @tommcdon See info in area-owners.md if you want to be subscribed.

Issue Details
### Background and motivation There are APIs that accepts `object` and operates with object identity, thus boxed structs will not work. Examples including: - GC.KeepAlive (For keeping the collectible type alive, will `KeepAlive(value.GetType())` be better?) - Monitor.Enter ArgumentNullException.ThrowIfNull (#85154) is similar, but the motivation is different, and will have different meaning with nullable value type. With such an attribute, we can have one analyzer for all these APIs, without recognizing them one by one, and also works for user code. Can it be reviewed together? ### API Proposal ```csharp namespace System.Diagnostics.CodeAnalysis; [AttributeUsage(AttributeTargets.Parameter, Inherited = true)] public class ObjectIdentyExpectedAttribute : Attribute { } ``` ### API Usage ```csharp // Moitor.Enter annotated with ObjectIdentyExpected lock (myStruct) // analyzer warns here { } ``` ### Alternative Designs Should it be named as what's *expected*, or what's *unexpected*? Should it be inherited? Should we include a custom diagnostic id for analyzer? Users are unlikely to suppress such warnings though. ### Risks _No response_
Author: huoyaoyuan
Assignees: -
Labels: `api-suggestion`, `area-System.Diagnostics`, `untriaged`, `needs-area-label`
Milestone: -