dotnet / csharplang

The official repo for the design of the C# programming language
11.53k stars 1.03k forks source link

Champion "Allow Generic Attributes" #124

Open gafter opened 7 years ago

gafter commented 7 years ago

See

AlphaDriveLegal commented 7 years ago

I've wanted this for a long time!

Mafii commented 7 years ago

I would be happpy to have this, too!

weitzhandler commented 7 years ago

Very compelling.

Here's my use case, one among many:

public class ValidateIf<TValidationAttribute> : ValidationAttribute
{
  public TValidationAttribute Attribute { get; private set; }
  //other properties to resemble condition
  public ValidateIf(Action<TValidationAttribute> configure)
  {
    Attribute = configure();
  }
  protected override ValidationResult IsValid(object value, ValidationContext validationContext)
  {
    var shouldValidate = this.ShouldValidate();
    if(!shouldValidate) return ValidationResult.Success;
    else return Attribute.GetValidationResult(value, validationContext);
  }
}

Usage:

public class Person
{
  [ValidateIf<RangeAttribute>(() => new RangeAttribute(params), Condition set up...)]
  public string Name { get; set; }
}

Short in time now, sorry for the poor pseudo, but I'm sure I'm understood.

Joe4evr commented 7 years ago

@weitzhandler Your example is pretty flawed since lambdas can't be passed as an attribute parameter; only primitives, string, typeof() expressions, and DateTime literals (VB).

A more sensible example of where a generic attribute would be useful:

// This concerns a bot command system that can parse strings into typed arguments,
// but sometimes the "default" reader for something may be too narrow,
// so this on a parameter would override which class does the reading for that argument.
[AttributeUsage(AttributeTargets.Parameter)]
public sealed class OverrideTypeReaderAttribute<TReader> : Attribute
    where TReader : TypeReader
{
    // We're only interested in the Type info at this point,
    // but we would really like the type-safety of the generic constraint
    public Type TypeReaderType { get; }

    public OverrideTypeReaderAttribute()
    {
        TypeReaderType = typeof(TReader);
    }
}
// Usage:
[Command("foo")]
public Task Foo([OverrideTypeReader<AlternativeBoolReader>] bool switch)
{
    //.....
}

Currently we imitate the constraint by taking a Type parameter in the constructor and checking IsAssignableFrom() at runtime, but pushing the check to compile-time via constraint would be neat and save us from needing to throw an exception if it fails.

weitzhandler commented 7 years ago

@Joe4evr I thought part of this proposal was allowing complex params I attribute initialization, which I saw in several issues, maybe in the Roslyn repo. And that's why I was going for an example that utilizes both, and I do vote for both genetic attributes and better and more flexible attribute initialization and setup options.

Joe4evr commented 7 years ago

I thought part of this proposal was allowing complex params I attribute initialization

I have not seen this proposal before, not to mention that such a thing would require quite a hefty spec change since as it stands, the compiler needs to be able to serialize the arguments directly in the metadata.

It is possible, if wanted for whatever reason, to instantiate an attribute as just a normal class, and that could use any type of parameter:

var attr = new SomethingSpecialAttribute(GetSomeData());

But that should only be a secondary constructor, otherwise it's totally useless to have that class be an attribute in the first place. (I have an example of where this is used in the same codebase I described above, but that's getting off-topic.)

gafter commented 7 years ago

I thought part of this proposal was allowing complex params I attribute initialization,

No.

alrz commented 6 years ago

We need a mechanism to understand which target runtime it works on. We need that for many things, and are currently looking at that. Until then, we can't take it.

I suppose that's on the use-site, whereas an abstract generic attribute doesn't seem to be harmful,

abstract class SomeAttribute<T> : Attritbue {}

Could we permit the above without (any) runtime support?

ymassad commented 5 years ago

Would this feature support this?

public class GenericClass<T>
{
    [SomeAttribute<T>]
    public void DoSomething(T input)
    {
    }
}

public class SomeAttribute<T> : Attribute
{

}
roydukkey commented 5 years ago

@ymassad According to the proposal, your example is correct but possibly not for the reasons that you expect. Here is an example that might make things a little clearer.

[SomeAttribute<A>]
public class GenericClass<A>
{
    [SomeAttribute<B>]
    public void DoSomething(B input) { }
}

public class SomeAttribute<C> : Attribute
{
    // `C` is either `A` or `B` according to its target.
}

As you can see from the example, the generic context comes from the attributes target.

ymassad commented 5 years ago

@roydukkey , I am not sure I understand. Can you elaborate?

Is B a concrete type?

roydukkey commented 5 years ago

@ymassad Sorry. I meant to write this.

[SomeAttribute<B>]
public void DoSomething<B>(B input) { }

I believe this is the spec.

AviAvni commented 5 years ago

The attribute can't have open generic type it must be instantiated at compile time

jnm2 commented 5 years ago

@ymassad What attribute instance would you expect typeof(GenericClass<>).GetMethod("DoSomething").GetCustomAttributes() to contain in your example?

ymassad commented 5 years ago

@jnm2 ,

Thanks. I see now why this is not possible.

what about this:

public class GenericClass<T>
{
    [SomeAttribute(typeof(T)]
    public void DoSomething(T input)
    {
    }
}

public class SomeAttribute : Attribute
{
    public SomeAttribute(Type type)
    {
        this.Type = type;
    }

    public Type Type {get;}
}

typeof(GenericClass<>).GetMethod("DoSomething").GetCustomAttributes() could return a SomeAttribute instance with Type containing the parameter type T.

jnm2 commented 5 years ago

@ymassad The problem there (if I remember the metadata format correctly) is that attribute arguments of type System.Type are stored as serialized strings containing the assembly-qualified name. What string can be deserialized (similar to Type.GetType) in such a way that it resolves to a generic type parameter? I think this would require a runtime change.

Edit: !0 (referring to the generic type parameter with index 0) and !!0 (same but for generic method parameters) are how this would work.

jnm2 commented 5 years ago

Yep, https://www.ecma-international.org/publications/files/ECMA-ST/ECMA-335.pdf, section II.23.3 Custom attributes, page 268:

  • If the parameter kind is System.Type, (also, the middle line in above diagram) its value is stored as a SerString (as defined in the previous paragraph), representing its canonical name. The canonical name is its full type name, followed optionally by the assembly where it is defined, its version, culture and public-key-token. If the assembly name is omitted, the CLI looks first in the current assembly, and then in the system library (mscorlib); in these two special cases, it is permitted to omit the assembly-name, version, culture and public-key-token.
ymassad commented 5 years ago

@jnm2 , thanks.

jnm2 commented 5 years ago

@ymassad TypeDescriptor will inject the type as a constructor parameter when it instantiates a type converter specified via TypeConverterAttribute, and maybe you can follow a similar pattern in the meantime?

https://github.com/dotnet/corefx/blob/bfb4ba1e398273cfd3bf15bf25c535f64bf072ea/src/System.ComponentModel.TypeConverter/src/System/ComponentModel/MarshalByValueComponent.cs#L16

https://github.com/dotnet/corefx/blob/bfb4ba1e398273cfd3bf15bf25c535f64bf072ea/src/System.ComponentModel.TypeConverter/src/System/ComponentModel/ComponentConverter.cs#L13-L18

https://github.com/dotnet/corefx/blob/bfb4ba1e398273cfd3bf15bf25c535f64bf072ea/src/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs#L223-L232

rhyous commented 5 years ago

The main use case I have is to make the attribute know the type I am placing it on.

public class EntityExamplesAttribute<T> : Attribute
{
    public abstract List<T> Examples {get;set;}
}

Or

public class EntitySeedDataAttribute<T> : Attribute
{
    public abstract List<T> Entities {get;set;}
}

Then I can do inheritance.

public class Entity1ExamplesAttribute : EntityExamplesAttribute<Entity1>
{
    public Entity1Attribute() 
    {
        Examples = new List<Entity1> { 
            new Entity1 { /*   first example entity */ },
            new Entity1 { /*   second example entity */ }
        }
    }
}

And

public class Entity1SeedDataAttribute : EntitySeedDataAttribute<Entity1>
{
    public Entity1Attribute() 
    {
        Examples = new List<Entity1> { 
            new Entity1 { /*   first seed entity */ },
            new Entity1 { /*   second seed entity */ }
        }
    }
}

And then apply to the entity

[Entity1SeedData]
[Entity1Examples]
public class Entity1
{
    // Entity stuff
}

Then I have code that seeds the database on first creation. Then I have code that shows example entities in a documentation page.

I already have it working with this feature, using object boxing and casting as opposed to generics.

dzmitry-lahoda commented 5 years ago

Will it be part of C# 8.X release? Can I start using it with some preview version?

Sorry for by broken C#, as it is not my native language. But I hope to write next kind of DSL with Roslyn generator

// union which stores some stucture so that no need to box with explict layout
[OverlappedUnion(Size = 64)] // analyar will thorw if any size is more than 64
//[OverlappedUnion(Size = 64, ReadOnly = true)]
enum X
{
  [Case<String>]
  A,
    [Case<(String z, int k>>] // x.B.zm x.B.k
    B,

    // no data
    [Case]
    [Case<Unit>]
    C,

    // fixed(see C# keyword) allocated 23 length char
    [Case<Memory<char>>(23)]
    E,

    [Case("CustomPropertyName")]
    D,
}

// generator generates all possible cases
XCase x;

switch (x.Tag)
{
   case X.A:
     // in DEBUG runtime checks A and then does not thorows A
     // in analyzer also checks
     // should work well with descruting
      var v1 = x.A;
     var v2 = x.Value; // object of Union or XStruct for OverlappedUnion
    break;
   case X.C
     break;
   // analyzer ensure default or all cases
}

// design with internal explicit stuct or base class or what ever,
// same API but may require boxing
[Union]
enum X
{
  [Case<String>]
  A,
    [Case<(String z, int k>>]
    B,

    // no data
    [Case]
    [Case<Unit>]
    C,

    [Case<Memory<char>>(23)]
    E,    
}
Joe4evr commented 5 years ago

Will it be part of C# 8.X release? Can I start using it with some preview version?

Sadly, despite the Roslyn PR being ready for literally a year now, it doesn't seem like it's gonna make 8.0 unless the language team decides to push the Merge button at literally the last minute.

Which is really sad, because I really want it ASAP. :anger:

thiagomajesk commented 5 years ago

@Joe4evr Just came across this today and I have to say that this would indeed be awesome to have right away with C# 8. BTW, it seems that something is going int this PR since your comment. Do you guys (@jcouv @AviAvni) mind bringing some information over here about this feature's availability?

ufcpp commented 5 years ago

https://github.com/dotnet/roslyn/blob/master/docs/Language%20Feature%20Status.md

image

jnm2 commented 5 years ago

@ymassad The problem there (if I remember the metadata format correctly) is that attribute arguments of type System.Type are stored as serialized strings containing the assembly-qualified name. What string can be deserialized (similar to Type.GetType) in such a way that it resolves to a generic type parameter? I think this would require a runtime change.

I reread this (https://github.com/dotnet/csharplang/issues/124#issuecomment-444965617) and have to correct myself: !0 (referring to the generic type parameter with index 0) and !!0 (same but for generic method parameters) are how this has worked.

TonyValenti commented 4 years ago

Any chance at all this will make it into C# 9?

Joe4evr commented 4 years ago

@TonyValenti Given that this issue is labeled as "10.0 Candidate", the answer is "No".

YairHalberstadt commented 4 years ago

Another use case for this:

I'm writing a compile time IOC framework for C# using source generators: https://github.com/YairHalberstadt/stronginject

Registrations are done via attributes, but because generics aren't allowed in attributes you can't do this: [RegisterGeneric(typeof(List<>))]

Instead you have to explicitly pass in the parameters using a generic factory method, e.g:

[Factory] public static List<T> CreateList<T>(IEnumerable<T> ts) => new List<T>(ts);

Whilst this is more flexible it's less concise, and requires you to update the method whenever the parameters change, which is against the spirit of using an IOC container in the first place.

Another thing that might be useful is partially open typeofs, allowing you to register e.g. [RegisterGeneric(typeof(ServiceImpl<string, >), typeof(IService<>))].

johnscott999 commented 3 years ago

While the use cases for this are not broad, it would have a big impact on certain applications.

For example, MVC's ProducesResponseTypeAttribute(Type type, int statusCode) can specify different result types based on the HTTP status. Currently, the following code does not build:

    internal abstract class BaseCrudController<TEntity> : Controller
        where TEntity: class
    {
        private DbContext context;

        //...

        // This next line throws an error
        [ProducesResponseType(typeof(TEntity), 200)]
        //                    ^^^^^^^^^^^^^^^ 
        //                    CS0416: An attribute argument cannot use type parameters
        [ProducesResponseType(400)]
        public virtual async Task<IActionResult> GetById(int id)
        {
            var instance = await this.context.Set<TEntity>().FindAsync(id);
            return instance is null 
                ? (IActionResult)NotFound()
                : (IActionResult)Ok(instance);
        }

        //...

    }

With this feature the annotation could be rewritten as

        [ProducesResponseType<TEntity>(200)]
        [ProducesResponseType(400)]
        public virtual Task<IActionResult> GetById(int id)
        {
            //...

Inheritors of this base could specify a closed generic, if compile time typing was needed, like for a swagger doc or api code gen.

    public class UserController: BaseCrudController<User>
    {
        //...

I would expect reflection calls returning that attribute to behave similarly to reflection calls on generic properties: If the generic is defined in the originating type definition, then return the closed generic. If the originating type is open, return the open definition.

image

julealgon commented 3 years ago

@johnscott999 you can achieve identical behavior by leveraging ActionResult<T> instead of IActionResult. There is no need to explicitly specify a ProducesResponseType in that case.

Having said that, I can see your suggestion being very useful in other contexts.

mrtristan commented 3 years ago

@julealgon not so if you have many instances of ProducesResponseType such as for cases where a bad request responds with a different model.

julealgon commented 3 years ago

@julealgon not so if you have many instances of ProducesResponseType such as for cases where a bad request responds with a different model.

@mrtristan I was specifically talking about the example provided, where it was being used with the entity type and the 200 response (a common thing). ActionResult<T> solves that particular issue without needing generic types in attributes.

Rekkonnect commented 3 years ago

dotnet/roslyn#16814 was closed, the new PR is dotnet/roslyn#26337

hez2010 commented 3 years ago

@johnscott999 Even with generic attributes feature you still cannot write code like that. The runtime doesn't allow to use open generics in attributes.

johnscott999 commented 3 years ago

@hez2010 the last code sample was my expectation on how reflection calls would interact with generic attributes. I'd expect generic attributes to be defined as either a defined type, or, if decorating an generic class or it's properties, one of that class's generic arguments.

//Valid
[GenericClassAttr<T>]
public class GenericClass<T> {
    [GenericPropAttr<T>]
    public string Property { get; set; }
    [GenericPropAttr<string>]
    public string Property { get; set; }
    // ...
}

// Invalid
[GenericClassAttr<TAnother>]
public class GenericClass<TOne> {
    // ...
}

// Invalid
public class GenericClass<TOne> {
    [GenericPropAttr<TAnother>]
    public string Property { get; set; }
    // ...
}

@julealgon You can work around it, but that also means you can't, for example, use the controller methods for non-success codes return NotFound() if you have a bad id on a get. MVC is flexible enough to work with and around, but not all use cases are as versatile.

julealgon commented 3 years ago

@johnscott999

that also means you can't, for example, use the controller methods for non-success codes return NotFound() if you have a bad id on a get.

Why not? All action results in AspNetCore can be converted implicitly to ActionResult<T>. Please try it yourself. You can definitely have a ActionResult<MyEntity> returning controller action and do a return NotFound() from it.

Of course, you have to then explicitly add the [ProducesResponseType] for those other cases too (as one would expect).

johnscott999 commented 3 years ago

@julealgon Huh, I'd always assumed that IActionResult<T> ineherited from IActionResult, but that's not it at all. You learn something every day. Regardless, this isn't the an aspnetcore issue, I'm was just trying to use that library to illustrate a use case.

Ultimately my argument is that having generic types in attributes would be helpful when using a abstract base class with inheritance that has attribute decorations. And since you would need to add the produces response type, having a crud controller for mvc webapi is still demonstrative, if not ideal.

RikkiGibson commented 3 years ago

One thing that I haven't seen discussed in this issue is how AllowMultiple = false should be handled.

using System;

[AttributeUsage(AttributeTargets.All, AllowMultiple = false)]
class Attr<T> : Attribute { }

[Attr<string>]
[Attr<string>] // error
[Attr<object>] // error?
class Program
{
}

The question is: if [Attr<string>] and [Attr<object>] are both applied to something, should it be considered a violation of the AllowMultiple setting?

Tentatively I am inclined to go with the most restrictive possible interpretation, and make it so different constructions of the same attribute is an error with AllowMultiple = false. Back in 2015 this was suggested (emphasis mine):

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.

TonyValenti commented 3 years ago

@RikkiGibson - I think that in the scenario you mentioned, if you want to prohibit '''MyAttribute''' and '''MyAttribute''', it would be best to introduce a base MyAttribute (which both generics inherit from) class and apply the AllowMultiple(false) attribute to it.

RikkiGibson commented 3 years ago

I don't expect that to introduce the expected restriction because the following currently compiles just fine: SharpLab

using System;

[AttributeUsage(AttributeTargets.All, AllowMultiple = false)]
class Attr : Attribute { }

class Attr1 : Attr { }
class Attr2 : Attr { }

[Attr1]
[Attr2]
public class C {
    public void M() {
    }
}
Rekkonnect commented 3 years ago

My best idea is to have AllowMultiple only prevent using multiple of the same attribute including the type arguments. For example, Attr<string> and Attr<object> will be usable with the AllowMultiple property set to true.

What I would instead use is a new property in AttributeUsage that determines whether to prohibit the same attribute with any type arguments, which I'd call AllowMultipleGenericUnbound (there surely is a better name for that). Defaults to true, when true it allows using that Attr with the same type arguments multiple times, and when false, it prohibits using the attribute more than once regardless of its type arguments. It obviously has no effect on non-generic attributes.

HaloFour commented 3 years ago

I'm curious as to if/how the runtime enforces that restriction today given IIRC this was already shipped in the runtime.

jnm2 commented 3 years ago
Shadowblitz16 commented 3 years ago

I like this idea but it only solves part of my problem


[RequireComponent<Transform, Renderer, Collider>()]
public class MyEntity : Entity
{

}

// ??? params on generics isn't supported

I think something that would be better for my case would be a generic type class

public MyAttribute(params Type<Component>[] components)
{

}
HaloFour commented 3 years ago

@Shadowblitz16

??? params on generics isn't supported

The runtime has no concept of variable generic arguments. You would need to define multiple versions of that attribute for each possible arity of generic arguments. Or you could just use a params Type[] constructor as you aren't really taking advantage of generics here except to have a different syntax:

[RequireComponent(typeof(Transform), typeof(Renderer), typeof(Collider))]
public class MyEntity : Entity
{

}
Shadowblitz16 commented 3 years ago

@HaloFour the issue is disallowing anything that is not a component to be a compile time error.

HaloFour commented 3 years ago

@Shadowblitz16

the issue is disallowing anything that is not a component to be a compile time error.

Right, if you want to use generic type constraints then you'll need to define multiple versions of that attribute, one for each number of generic type arguments you can accept:

SharpLab

One can argue that variadic generics would make your use case easier, but it gets substantially more complicated for any use case that would want to use those generic type arguments for anything other than reflection purposes.

Shadowblitz16 commented 3 years ago

@HaloFour this would work very well with variadic generic paramaters. still now sure how it would be passed to a generic array though

RikkiGibson commented 3 years ago

dotnet/roslyn#55577

We will make this feature available in the "preview" language version in .NET 6 RC1. We found that a large number of runtimes and tools (Mono, C++/CLI, etc.) may still have issues consuming generic attributes, so we are going to take an additional release cycle to try and iron out the issues, and give ourselves some time to make breaking changes with runtime APIs.

For example, if a user calls CustomAttributeData.GetCustomAttributes(someType, typeof(GenericAttr<>)) there is an open question about whether it should give all the constructions of the attribute which were used on the type. dotnet/runtime#56887

using System;

// what should the value of 'attrs' be here?
var attrs = CustomAttributeData.GetCustomAttributes(typeof(C), typeof(Attr<>));

[AttributeUsage(AttributeTargets.All, AllowMultiple = true)]
class Attr<T> : Attribute { }

[Attr<int>]
[Attr<string>]
class C { }
johnscott999 commented 3 years ago

It’s a bit cheeky, but why not Attribute[]

Attribute[] attrs = //…
attrs[0].GetType() // => typeof(Attr<int>)

from there you can get what you need with reflection.

practically I’d probably, inherit from a non generic base type that exposed the generic types of derived as properties, but that’s a design decision.

as for allow multiple, I think you’ve got to treat each different generic as a different class, the same way you would static properties in a generic class. I’d support adding an attribute usage for AllowMultipleGeneric (name pending), that would only allow one Attr<T> decorator if set. That would cover all the use cases that come to mind