dotnet / runtime

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

Extend usage of DisplayNameAttribute #29867

Open HermanEldering opened 5 years ago

HermanEldering commented 5 years ago

DisplayNameAttribute currently only has the UsageAttribute targets shown below. This limits how the DisplayNameAttribute can be used. The targets should be extended to allow specifying display names for enums and structs.

[AttributeUsage(AttributeTargets.Property | AttributeTargets.Event | AttributeTargets.Class | AttributeTargets.Method)]

Rationale and usage

It can be useful to define DisplayNames for enum elements, like this:

public enum FriendlyColorsEnum
{
    [DisplayName("Blanched Almond Color")]
    BlanchedAlmond = 1,
    [DisplayName("Dark Sea Green Color")]
    DarkSeaGreen = 2,
    [DisplayName("Deep Sky Blue Color")]
    DeepSkyBlue = 3,
    [DisplayName("Rosy Brown Color")]
    RosyBrown = 4
}

The example was taken from someone else who used the DescriptionAttribute instead.

While it is possible to use the DescriptionAttribute it has a different meaning, and it could make sense to use both the DisplayNameAttribute and the DescriptionAttribute on the same elements.

The benefit of using an attribute is that the display name is not limited by the naming rules of the programming language. For instance it can include spaces, special characters, start with a number. It is possible to implement a custom attribute, but that makes the code more complex.

In addition to enabling the use of DisplayNameAttribute on enums it seems sensible to also enable it for structs.

Proposed change

My proposal is to change the targets to All, since that is what is used for DescriptionAttribute. It might not make sense to apply the DisplayName on every target, but I am also not aware of drawbacks to allowing it for all targets.

[AttributeUsage(AttributeTargets.All)]

A more limited target could suffice but that would need a more detailed look at the trade-offs for each target.

cunningham74 commented 4 years ago

It looks like this might have been broken in .net core 3.0. I'm trying to upgrade a project from .net core 2.2 to 3.0 and I'm getting the following error Attribute 'DisplayName' is not valid on this declaration type. It is only valid on 'class, method, property, indexer, event' declarations. Here is my enum declaration for reference.

public enum ComparisonType { Undefined = 0, National = 1, [DisplayName("Peer Group")] PeerGroup = 2, }

yahorsi commented 4 years ago

We've got that issue as well.

OpenSpacesAndPlaces commented 4 years ago

Seeing the same on a migration from 2.2 to 3.1.1

om3n07 commented 4 years ago

Also encountering this issue when migrating from 2.1 to 3.1.1.

HermanEldering commented 4 years ago

For those interested, a possible workaround might be using the DisplayAttribute instead of DisplayNameAttribute.

ericstj commented 4 years ago

Looks like DisplayNameAttribute inadvertently omitted its AttributeUsageAttribute in the reference assembly prior to 3.x. https://github.com/dotnet/corefx/blob/4681a68852b4e790c1d743ec7d1825bcd5ff9260/src/System.ComponentModel.Primitives/ref/System.ComponentModel.Primitives.cs#L139

The attribute was missing in .NETCoreApp, but present in .NETStandard, https://github.com/dotnet/standard/blob/52777e1506d39f64d12550e0703c4bd5e7e9f218/netstandard/ref/System.cs#L1638

I brought this back with https://github.com/dotnet/runtime/commit/a58e4fb6481228b02e2c44f3acd54efa578a81de#diff-66eed886fd01ffb17d8e43424a30a0d3R145 which made 3.x consistent with .NETFramework, .NETStandard, and the .NETCore implementation.

It seems reasonable for us to bring this back.

What's the consuming scenario that will read the attribute from enum values? TypeDescriptor? I want to make sure the end-to-end is considered and tested (to avoid future regressions and ensure a complete solution).

ghost commented 4 years ago

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

OpenSpacesAndPlaces commented 4 years ago

What's the consuming scenario that will read the attribute from enum values? TypeDescriptor? I want to make sure the end-to-end is considered and tested (to avoid future regressions and ensure a complete solution).

For me the most common use case would be scenarios where an enum makes the most sense for code flow, but then also needing "user friendly" text. Granted this could be handled in a mapping object, but the syntax sugar here allows for cleaner field management (IMO).

@ericstj

HermanEldering commented 4 years ago

What's the consuming scenario that will read the attribute from enum values?

My use case is that I have a custom generic type converter to convert enums that I use in data binding. My code uses reflection (GetCustomAttributes<>) to look for the attribute and convert to/from other data types (in this case strings).

Using the existing DisplayName attribute makes it easier to work with compared to implementing a third party attribute, mostly because the programmer only has to learn a single attribute and it can be shared between different projects/libraries instead of each rolling their own.

ericstj commented 4 years ago

To be clear, no-one here is expecting the runtime (EG: TypeDescriptor) to read this attribute in the new scenarios?

danmoseley commented 3 years ago

It's not a regression from "last" release anymore

eerhardt commented 2 years ago

Moving to 8.0 as this issue has been around for a long time and is not critical to fix in 7.0.

steveharter commented 1 year ago

We need to investigate the work involved here to make this work for enum and structs. I don't think just adding the AttributeTargets for enum and struct make much sense without a default implementation.

HermanEldering commented 1 year ago

@steveharter what is the rationale for having both DisplayAttribute and DisplayNameAttribute in the framework?

Are there places where only one is used and not the other? Shouldn't all places that look for DisplayNameAttribute also look for DisplayAttribute?

If that is the case wouldn't it be better to deprecate DisplayNameAttribute?

steveharter commented 1 year ago

@steveharter what is the rationale for having both DisplayAttribute and DisplayNameAttribute in the framework?

The attributes in the S.ComponentModel ([DisplayName], [Description]) work with TypeDescriptor but the ones in S.ComponenentMode.DataAnnotations ([Display]) do not. DataAnnotations is also a bit newer and used in more places including ASP.NET and Entity Framework for binding, validation and views.

It is interesting that the two areas also have different targets. DisplayAttribute targets Class + Event + Method + Property (Class was added in .NET Core 2.1) while DisplayNameAttribute targets Class + Field + Method + Parameter + Property

Neither supports events.

Are there places where only one is used and not the other? Shouldn't all places that look for DisplayNameAttribute also look for DisplayAttribute?

The framework doesn't expose a way to do this automatically, so it would be up to the app.

If that is the case wouldn't it be better to deprecate DisplayNameAttribute?

Since DisplayNameAttribute is tied to classes like TypeDescriptor and works with other other attributes in that S.ComponentModel namespace it would mean deprecating entire sets of classes which is not normally done.

steveharter commented 1 year ago

The attribute System.ComponenentModel.DataAnnotations.DisplayAttribute has a different set of targets. Someone could ask the same questions about extending those attributes. Those various attributes, and the ones in System.ComponentModel as well, were designed for specific scenarios and existing tooling was geared towards supporting the known targets. So if we expand the targets, there may be "downstream" feature requests and will be various inconsistencies for those tools.

Similarly, the framework may have "downstream" work -- if using TypeDescriptor instead of reflection to obtain attributes, what additional work (if any) is necessary (if any). Sample:

internal class Program
{
    static void Main(string[] args)
    {
        PropertyDescriptorCollection pdc = TypeDescriptor.GetProperties(new MyClass());
        Console.WriteLine($"DisplayName:{pdc[0].DisplayName}"); // DisplayName:DESC1

        // Below should be DisplayName: DESC2 once enums allow [DisplayName]
        Console.WriteLine($"DisplayName:{pdc[1].DisplayName}"); // DisplayName: foo
    }
}

public class MyClass
{
    public MyClass2 myClass2 { get; set; } = new MyClass2();
    public MyEnum foo { get; set; } = MyEnum.Green;
}

[DisplayName("DESC1")]
public class MyClass2 { }

// Not allowed today: CS0592
// [DisplayName("DESC2")]
public enum MyEnum
{
    Red = 1,
    Green = 2
}