[Proposal] Allow generic attributes

[ashmind]

Problem

For some attributes (e.g. TypeConverterAttribute) that are used to reference types, the current usage looks like this:

[TypeConverter(typeof(X))]

This has two disadvantages:

1. You can't guarantee that the type matches the requirements of the attribute -- e.g. has an empty constructor or inherits TypeConverter.
2. The syntax is a bit verbose.

Suggestion

I suggest that the generic attributes should be supported, including any generic constraints.
Usage example:

[TypeConverter<X>]

Pros

1. Support for type constraints in type-referencing attributes
2. Shorter syntax
3. As far as I know, generic attributes are already supported in IL

Cons

The set of attributes that benefit from this might not be that large.

Updates

Removed pro 4 as it wasn't a pro.

[gafter]

3 and 4 are not pros, nor are they cons. 2 is marginal. 1 is the only justification that makes enough sense to consider it as a language feature. Do you have any use cases?

[ashmind]

To me 3 is important -- I don't see a reason why CS0698 exists in the first place, and removal of special cases should make language easier to learn and understand (especially since actual implementation can be done outside of the Roslyn team now).

I agree on 2 and 4.

Example attributes that could benefit from this (e.g. by defining a generic attribute subclass for old cases, and by defining the attribute as generic in new cases):

1. TypeConverterAttribute (new(), TypeConverter)
2. TypeDescriptionProviderAttribute (new(), TypeDescriptionProvider)
3. SettingsProviderAttribute (new(), SettingsProvider)
4. DesignerAttribute (new(), IDesigner, TDesigner: TDesignerBase)
5. InstallerTypeAttribute (new, IDesigner)
6. Newtonsoft.Json.JsonConverterAttribute (new(), JsonConverter)
   7... (Basically any *Provider/*Converter attribute I could find)

In fact ReSharper provides a special BaseTypeRequired annotation just for these cases -- allowing developer to specify the expectation.

[sharwell]

Ignore my previous comment (now deleted). The generic type arguments for a generic attribute type can be encoded in the MemberRef metadata table, by representing the "Class" field as a TypeSpec.

[Przemyslaw-W]

Glad I'm not the only one. https://roslyn.codeplex.com/discussions/542178

2015-03-02 0:29 GMT+01:00 Sam Harwell notifications@github.com:

Ignore my previous comment (now deleted). The generic type arguments for a
generic attribute type can be encoded in the MemberRef metadata table, by
representing the "Class" field as a TypeSpec.

—
Reply to this email directly or view it on GitHub
#953 (comment).

[mikedn]

(e.g. by defining a generic attribute subclass for old cases, and by defining the attribute as generic in new cases):

Problem is, the main advantage of this proposal is the support for generic constraints but the constraints can be easily circumvented by using the base attribute. You'd need to obsolete the TypeConverter's Type constructor to convince people to use the generic TypeConverter attribute and that's unlikely to happen. Besides, for some of these attributes (DesignerAttribute especially) it's more common to specify the type name rather than the type.

There's also the question of performance, do you really want to instantiate tens or maybe even hundreds of such generic attribute type just to get those generic constraints?

[sharwell]

Problem is, the main advantage of this proposal is the support for generic constraints but the constraints can be easily circumvented by using the base attribute.

A long discussion on a closely related scenario with the opposite conclusion is in dotnet/corefx#271.

[GeirGrusom]

Problem is, the main advantage of this proposal is the support for generic constraints but the constraints can be easily circumvented by using the base attribute.

How is that an issue that concerns the C# compiler? How is it an issue at all? Changing existing usage of attributes is not worthwhile, and the old attributes will continue to work, so why stop anyone?

You'd need to obsolete the TypeConverter's Type constructor to convince people to use the generic TypeConverter attribute and that's unlikely to happen

Why? It literally has no benefit to anyone to obsolete the old attributes. It would be counter-productive.

Besides, for some of these attributes (DesignerAttribute especially) it's more common to specify the type name rather than the type.

How does that concern generic attributes?

There's also the question of performance, do you really want to instantiate tens or maybe even hundreds of such generic attribute type just to get those generic constraints?

Why is that the compiler's problem?

[mikedn]

Why is that the compiler's problem?

Who says it the compiler's problem? This is a language feature, not some obscure compiler implementation detail that isn't part of the language specification. And as with all language features it's worth considering the pros and cons.

How does that concern generic attributes?

Use cases have been requested to support this language feature. Uses cases have been provided and I pointed out that some of these uses cases are not representative.

Why? It literally has no benefit to anyone to obsolete the old attributes. It would be counter-productive.

Yes, it would be counter productive to obsolete the old attributes and that's one of the reasons I already said that it won't happen. The side effect of not being able to do that is that the main selling point of this language feature (ability to use type constraints) is not very valuable for the presented use cases. For example, the consumer of TypeConverterAttribute still needs to deal with the fact that the specified type might not be a type converter after all.

How is that an issue that concerns the C# compiler? How is it an issue at all? Changing existing usage of attributes is not worthwhile, and the old attributes will continue to work, so why stop anyone?

Again, this is not about the C# compiler but about the C# language. A language would do well not to add features on the basis that "it is possible so why not?". A language feature should be useful, useful enough to pay for its cons.

[ashmind]

The side effect of not being able to do that is that the main selling point of this language feature (ability to use type constraints) is not very valuable for the presented use cases.

I don't really see it as a feature that would directly benefit those attributes (though even subclasses would make those somewhat nicer to use). The thing is -- there are a lot of APIs created for .NET all the time, including MS obsoleting some APIs, creating new APIs, etc.

Based on the attributes we see now, it's not hard to see that those APIs are likely to use similar approaches -- and in those cases there will be no legacy parts to care about.

A language feature should be useful, useful enough to pay for its cons.

I am still not fully clear on how it works with new, open-sourced Roslyn. I think I read somewhere that the team had -100 points as a starting cost of any feature.

However, what if the feature has no obvious cons and is implemented completely through contributors? Of course there would be still some work required to review it, but is it -100 anymore?

[gafter]

The -100 points don't come primarily from implementation effort. It comes from increased complexity of the language.

[ashmind]

But doesn't removal of CS0698 decrease complexity? I can't speak for all developers, but for me language feels simpler when a feature such as generic classes works everywhere -- instead of breaking for edge cases (such as attributes).

[mikedn]

But doesn't removal of CS0698 decrease complexity?

I suppose that depends on what exactly language complexity is. I could say that this feature adds an alternative way of doing things and that means that the developer has to make a choice. TypeConverterAttribute with a constructor taking a Type parameter or TypeConverterAttribute<T>?

Which choice is better and why? Is the generic version really "nicer"? Nice is rather subjective, for example I can say that the non-generic version is nicer because it clearly shows what the code is doing while the generic version is hiding that.

What bugs me the most about the generic attribute approach is that it encourages creation of generic types that don't really have to be generic. Let's look at how a generic type converter attribute would look:

public sealed class TypeConverterAttribute<T> : TypeConverterAttribute {
    public TypeConverterAttribute() : base(typeof(T)) {
    }
}

What exactly is generic about this type? Does it have a field of type T? No. Does it use T in a method signature? No. So, why generic?

And it isn't true that there are no cons. There is certain overhead associated with generic instantiations. I believe that this overhead would be fairly small for typical attributes but it is certainly higher than 0 and it deserves at least to be mentioned.

[ashmind]

@mikedn Your points are all very valid.

I don't see choice here as a bad thing, mostly because it is consistent with the same choice around type-accepting methods (type.IsDefined(Type) or type.IsDefined<T>(), etc) and classes.

What exactly is generic about this type? Does it have a field of type T? No. Does it use T in a method signature? No. So, why generic?

I think that logically it can easily be using T in type signature though:

public sealed class MyConverterAttribute<T> /* : probably some interface */
    where T : MyConverter, new()
{
    public MyConverterAttribute() {
    }

    public T CreateConverter() {
        return new T();
    }
}

It's just that TypeConverterAttribute offloads creation to other parts of the framework.

[gafter]

I agree that removing otherwise arbitrary language restrictions would not trigger the -100 complexity penalty. Of course we still need to measure the bang-for-the-buck, by weighing the complexity of changing the language specification and implementation (of both C# and VB, and probably F# as well) versus the benefits of the change, and compare that to other things we could be doing.

[Przemyslaw-W]

@mikedn https://github.com/mikedn

public sealed class TypeConverterAttribute : TypeConverterAttribute {
public TypeConverterAttribute() : base(typeof(T)) {
}
}

This is based on provided use case, but it is a bit contrieved
example. Consider situation where you need to introduce completely new
attribute which needs to take some type as its input. Then you have
two choices - go with typeof or use generic. At least for me generic
is obvious choice.

2015-03-04 2:43 GMT+01:00 Neal Gafter notifications@github.com:

I agree that removing otherwise arbitrary language restrictions would not
trigger the -100 complexity penalty. Of course we still need to measure the
bang-for-the-buck, by weighing the complexity of changing the language
specification and implementation (of both C# and VB, and probably F# as
well) versus the benefits of the change.

—
Reply to this email directly or view it on GitHub
#953 (comment).

[ashmind]

Of course we still need to measure the bang-for-the-buck, by weighing the complexity of changing the language specification and implementation (of both C# and VB, and probably F# as well) versus the benefits of the change, and compare that to other things we could be doing.

That where the main question comes in -- considering that the change can be done by community -- how should that be weighted?

At this point you probably know the approach better than me, but just in case it is useful:

My first thought would be to put it on some low priority up for grabs list.

Items there would never be implemented by Roslyn team because of other more important work. However, a full implementation can be accepted from the community.

This isn't perfect as it still implies effort spent on reviewing the PR and defining acceptance criteria. However that would allow improvements that might otherwise never get in due to workload -- and people who want to improve things would be able to do actual coding and not just discussion.

[mikedn]

I don't see choice here as a bad thing

Yes, in general choice is good. Unfortunately not all developers make the best choices and sometimes it makes sense to limit choice.

I think that logically it can easily be using T in type signature though:

You could do that but in most cases it won't be useful. The consumer of the attribute doesn't usually know the type at compile time so it would be impossible to use T CreateConverter(). It has to be TypeConverter CreateConverter() and it has to be accessed via the non-generic base class or interface that is used to find the attribute. It's nice that you can do a simple return new T(); but that's ultimately syntactic sugar for Activator.CreateInstance and a cast.

[MrJul]

While the attribute usage is nice, retrieving them seems cumbersome.
To use the sample example as yours:

before:

Attribute.GetCustomAttribute(typeof(SomeType), typeof(TypeConverter))

after:

Attribute.GetCustomAttributes(typeof(SomeType))
  .Where(attr => attr.IsGenericType && attr.GetGenericTypeDefinition() == typeof(TypeConverter<>))
  .SingleOrDefault()

Even with a simple extension method to avoid repeating this logic over and over, you still need some base class or interface implemented (to cast your attribute to) to use it. This applies to every *Converter/*Provider attribute you mentioned.

Off the top of my head, the only use case I see for generic attributes is when the generic type is used like an enum or as a filter:

class DiagnosticAttribute<TLanguage> : Attribute { }

[Diagnostic<CSharpLanguage>]
class MyDiagnosticA { }

[Diagnostic<VBLanguage>]
class MyDiagnosticB { }

// no need to filter diagnostics, I can get C# ones directly!
csharpDiagnostics = assembly.GetTypes()
  .Where(type => type.IsDefined(typeof(Diagnostic<CSharpLanguage>))

Nothing that couldn't be achieved without a non-generic one and further filtering.

That said, I do agree that not being able to use generics in attributes feels weird, and language consistency is important (same thing with nullable properties in attributes).

[MrJul]

How does AttributeUsage.AllowMultiple = false work with generic attributes: does it limit the usages to one and only one attribute instance, or one instance per generic type?

[mikedn]

While the attribute usage is nice, retrieving them seems cumbersome.

Well, yes, that's why the generic TypeConverter I have shown above inherits from the non generic TypeConverter, so you can retrieve (and use) the generic attribute the same way you can retrieve the non generic one. Ashmind mentions "some interface" implementation in his MyConverterAttribute<T> which I presume it serves the same purpose.

How does AttributeUsage.AllowMultiple = false work with generic attributes: does it limit the usages to one and only one attribute instance, or one instance per generic type?

Probably it doesn't because the rules for this case haven't been written. FooAttribute<int> and FooAttribute<string> are different types so they would probably be both allowed without a special rule. The rule should be that the generic definition - FooAttribute<> - is considered rather than its instantiations.

[gafter]

@ashmind

That where the main question comes in -- considering that the change can be done by community -- how should that be weighted?

At this point you probably know the approach better than me, but just in case it is useful:

My first thought would be to put it on some low priority up for grabs list.

The first step here would be a proposed update to the C# and VB language specifications. This isn't something we're planning, so we are not creating an issue for it beyond this one. But if someone does propose detailed specification changes, we would consider those specs and decide if a community-submitted implementation would be welcome.

[ashmind]

The first step here would be a proposed update to the C# and VB language specifications.

How is this done? As a pull request against the spec document?

[gafter]

@ashmind The language spec documents are not open-source at the moment, so you'd have to write diffs (by hand) from some recent version of the published specs.

[dsaf]

Relevant discussion:

http://stackoverflow.com/questions/294216/why-does-c-sharp-forbid-generic-attribute-types

I would personally appreciate this feature.

[leppie]

Reflection API support would also need to be added for this to work, currently GetCustomAttributes will fail trying to get a generic custom attribute ie [Foo<int>].

[alrz]

will this be supported?

void F<T>([Foo<T>] T t){}

does it make sense at all?

[dsaf]

It's funny how attributes were made to look like classes but then suddenly have lots of unexpected (but motivated) limitations. Might've as well introduced an attribute keyword.

[alrz]

I don't know about regular attributes, but it does make sense to support it through #6671

void F<T>([[Foo<T>]] T t){}

And of course, we should be able to use it in constraints (#2944):

[FooAttribute<Bar>] class Foo {}
void F<T, U>(T obj) where T : [FooAttribute<U>] {}
F<Foo, Bar>(new Foo());

[AviAvni]

I Implement(removed the validation code) this feature in the roslyn source
and I compiled successfuly simple code but when running I get exception

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApplication21
{
    class MyAttribute<T> : Attribute
    {

    }

    [MyAttribute<int>()]
    class Program
    {
        static void Main(string[] args)
        {
            var a = typeof(Program).GetCustomAttributes(false);
            foreach(var att in a)
            {
                Console.WriteLine(att);
            }
        }
    }
}

Unhandled Exception: System.NotSupportedException: Generic types are not valid.
at System.RuntimeTypeHandle.CreateCaInstance(RuntimeType type, IRuntimeMethodInfo ctor)
at System.Reflection.CustomAttribute.GetCustomAttributes(RuntimeModule decoratedModule, Int32 decoratedMetadataToken, Int32 pcaCount, RuntimeType attributeFilterType, Boolean mustBeInheritable, IList derivedAttributes, Boolean isDecoratedTargetSecurityTransparent)
at System.Reflection.CustomAttribute.GetCustomAttributes(RuntimeType type, RuntimeType caType, Boolean inherit)
at System.RuntimeType.GetCustomAttributes(Boolean inherit)
at ConsoleApplication21.Program.Main(String[] args) in C:\Users\Avi.Avni\documents\visual studio 2015\Projects\ConsoleApplication21\ConsoleApplication21\Program.cs:line 19

Source: https://github.com/AviAvni/roslyn/commit/9cf2b3a9b67bd4a554fdac71ccbcb47af2133169

I'm looking at coreclr and corefx

[TylerBrinkley]

For me I'd like to see at least support for defining abstract generic attribute classes. My use case for this would be to allow defining a generic validator attribute.

abstract class EnumValidatorAttribute<TEnum> : Attribute
{
    public abstract IsValid(TEnum value);
}

class DayTypeValidatorAttribute : EnumValidatorAttribute<DayType>
{
    public override bool IsValid(DayType value) => value == DayType.Weekday || value == DayType.Weekend || value == (DayType.Weekday | DayType.Holiday) || value == (DayType.Weekend | DayType.Holiday);
}

[Flags]
[DayTypeValidator]
enum DayType
{
    Weekday = 1,
    Weekend = 2,
    Holiday = 4
}

Currently this can be done through an interface instead but would be much cleaner to support defining an abstract generic attribute class directly.

[leppie]

It would be better to define that enum as:

enum DayType
{
    Weekday = 0,
    Weekend = 1,
    Holiday = 2
}

Thus the LSB it is a toggle, with optional holiday flag, and you only need to check < 4.

[TylerBrinkley]

That enum was just an example and doesn't have any bearing on the use case.

[Pzixel]

@AviAvni any news? What's about coreclr/corefx?

[AviAvni]

@Pzixel no sorry I'll again

[AviAvni]

@Pzixel https://github.com/dotnet/coreclr/blob/565efeadebc4e57adda634396933e958ffd51bb0/src/vm/customattribute.cpp#L670 here the code where the exception happened need to remove the condition for generic type and look if it's OK or need to find way to instantiate generic type instance

[AviAvni]

@Pzixel I successfully run the code tomorrow night I'll send PR for coreclr and roslyn

[AviAvni]

@Pzixel Just open the PRs

[gafter]

This is now tracked at dotnet/csharplang#124