Enable generic attributes

[AviAvni]

Implements feature proposal #953

Succesfully compile and run https://gist.github.com/AviAvni/00831abbddf4ceca198236de362ed449

Depend on: dotnet/coreclr#9189

[jcouv]

Thanks for the PR!
As a heads up, we may have to move the PR to another branch due to scheduling considerations. We'll let you know. I don't think that should block reviewing the change though.
Also, I need to figure out how similar dependencies between compiler and runtime were previous handled.

FYI @mattwar

[AviAvni]

@jcouv
Ok I need to do something?
I think it will take some time utill the coreclr pr will be merged
And this pr can't be merged without it
Also I think instead of deleting the condition I need to make it work only where the target runtime allow it

[jcouv]

@AviAvni Let me get back to you.
The trouble with the runtime change is that there is more than one runtime. There is CoreCLR, which you have a PR for. But there is also CoreRT, desktop?, mono.
I need to find out how such dependencies where previously handled. For instance, can the compiler behave differently for different runtimes?

[jcouv]

From discussion with @jaredpar and @VSadov, the only way to have a compiler feature be conditional on runtime is to have some kind of API marker (type or method) in corlib, which the compiler can test for.
As part of your CoreCLR runtime, you'll need to identify such a marker and then use it here.

A couple of language questions that need to be discussed/resolved with LDM (@mattwar will drive):

1. If MyAttribute is generic, will [MyAttribute(1)] infer the generic type parameters?
2. Are type arguments allowed, and if so when? For instance, [MyAttribute<T>].

[AviAvni]

@jcouv

1. I think it's same as constructors so no type inference
2. The coreclr don't enable this

[AviAvni]

@jcouv thanks for the help
currently there is no changes in the mscorlib api so I don't know how to marker it
after the pr in the coreclr will merge I can try to implement it in CoreRT and maybe in mono
for the desktop I can't do anything
another question do i need to implement it also for VB.NET in this pr?
I'll work on this also for f# compiler

[jaredpar]

currently there is no changes in the mscorlib api so I don't know how to marker it

Perhaps @jkotas wants to weigh in here. There are a number of new runtime features we are looking at adding. For every one of them that has compiler support we're going to need an API marker in order for the compiler to enable support.

[jkotas]

I think that the nature of this issue is similar to ref locals and returns. ref locals and returns are not keyed by any API. They should work just fine with existing runtimes, tools and libraries in theory. In practice, they do not work just fine because of they expose pre-existing bugs and limitations because of the paths handling them were never exercised, or they were explicitly not expected. The bugs and issues related to byref returns are just starting to show up as people are starting to use the feature and finding places where it does not work. I think that is fine and expected. We should acknowledge it, and not react to it by pulling back the feature like we have done it for default valuetype constructors some time ago.

This feature is pretty similar. There is nothing in ECMA that prohibits generic attributes, and they should work just fine in theory. In practice, they do not. Number of parts throughout the system have to be updated to make them work well:

• There is more than one runtime to worry about as you have said (Mono, CoreRT, full framework)
• Some the tools that operate on IL are likely going to be affected too (compilers, Cecil-based or CCI2-based tools)
• New APIs for accessing the generic attributes maybe needed (e.g. the existing reflection APIs do not allow you to access the generic attributes efficiently - you have to always enumerate).

I would not hurt to run this feature through API review to get peoples opinions on how to deal with this. It is not strictly a new API right now, but it affects number of existing APIs, there may be new APIs needed, and we will need rules on where to use the generic attributes vs. the classic non-generic ones. cc @terrajobst @KrzysztofCwalina @karelz

[jaredpar]

I think that is fine and expected. We should acknowledge it, and not react to it by pulling back the feature like we have done it for default valuetype constructors some time ago.

Definitely don't want to pull back the feature. I'm more looking for a way to enable it more reliably.

ref locals and returns are not keyed by any API. They should work just fine with existing runtimes, tools and libraries in theory.

Is there any existing runtime though where generic attributes work?

[jkotas]

Is there any existing runtime though where generic attributes work?

It depends on which reflection API you use. If you use Type.CustomAttributes to inspect the custom attributes, the generic ones work fine in both .NET Framework and .NET Core. If you use Type.GetCustomAttributes(), it does not work for generic ones. It does not work because of the implementation has explicit block for them, but it seems to work just fine when the block is removed ... so it is fair to say that it is just a bug in this particular API.

It would be useful to find out what works and what does not accross different runtimes.

[jcouv]

@AviAvni Have you tried the compiler change in combination with any other runtimes than the CoreCLR (Full framework, Mono, or CoreRT)?
Also, could you describe what happens when using CoreCLR runtime prior to your fix? I'm still trying to gauge whether a marker for runtime support would make sense here.

Regarding your question about VB, yes, it is good to add support across the board. But let's make sure we iron out the issues with C# and runtime first.

[AviAvni]

@jcouv full framework I get exception you can see https://github.com/dotnet/coreclr/pull/9189/files#diff-1f1dd1552f4465a1fff8bd2460eab007L701 coreclr same

[jcouv]

Thanks @AviAvni for the information.

@jkotas Given that this doesn't work on full framework or coreclr, I agree with Jared that we need to shield this feature with some kind of runtime marker.

For this purpose, we could simply use an enum type. This would be extensible, as we expect we will need such markers as more compiler/runtime features ship. What do you think?

[jkotas]

this doesn't work on full framework or CoreCLR

This is not 100% correct statement. It works if you use Type.CustomAttributes reflection API, or raw metadata readers. It does not work if you use a particular reflection APIs because of that API has a bug. I do not think we need active roadblocks that prevent people from accidentally hitting bugs exposed by new language features.

However, I agree that similar marker may be useful in general. So if you believe that we really need it here, feel free to turn in into API proposal: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md

[jcouv]

FYI, I made an API proposal for an enum listing features supported by the runtime: https://github.com/dotnet/corefx/issues/17116

[AviAvni]

@jcouv it's look like I can't use RuntimeFeature class now do I need to update something?
It's look like Roslyn now users Assembly System.Runtime, Version=4.0.20.0 that don't have this class
And I found it on Assembly System.Runtime, Version=4.2.0.0
How to do the update?

[jcouv]

@AviAvni I thought we already have a dependency on the API, but I'll double-check (https://github.com/dotnet/corefx/issues/19788#issuecomment-363235102).

[jcouv]

Confirmed that Roslyn isn't using RuntimeFeatures yet.

Going back to the design notes: to check statically the compiler needs to probe for well-known member RuntimeFeatures.XYZ where "XYZ" is a property created for the feature of interest.
This doesn't need to change the version referenced by Roslyn, but rather to look for a well-known type in a compilation (which contains its own set of references).
This means adding new WellKnownType and WellKnownMember then calling Compilation.GetWellKnownMember and checking that it isn't an error type.

[AviAvni]

@jcouv Thanks so just to be sure only checking the member exists and without calling the Is supported method? So why need this method?

[AviAvni]

@jcouv And I finished the PR Of coreclr it will be merge after version 2.1

[jcouv]

The method is there for runtime detection, like runtime code generation. We expect that to be uncommon scenario.

[AviAvni]

@jcouv thanks

[AlekseyTs]

It doesn't look like dotnet/csharplang#124 has been approved by LDM. Also, I believe this work should be done in a feature branch. See https://github.com/dotnet/roslyn/blob/master/docs/contributing/Developing%20a%20Language%20Feature.md for more information.

[jcouv]

Created feature branch features/generic-attributes (forked off features/compiler branch which has 15.7 compiler work) and I'm working to enable it with CI.
Started thread with feature champion to get the proposal stamped as Aleksey noted.

[jcouv]

@jaredpar reminded me that there was pushback from @jkotas on adding a runtime feature flag for this, as most runtimes support it and it is more like a bug. I'll try to dig up the context, but if that's the case, then it's ok not to gate on flag from compiler side.

[AviAvni]

@jcouv I changed the PR target to dotnet:features/generic-attributes

[AviAvni]

@jcouv the coreclr pr just merged how we can continue?

[jcouv]

@AviAvni I'm trying to reload some context. Here's where I think we stand:

• we don't need to gate the feature on runtime detection (because the runtime was broadly working, with a bug, rather than the reverse)
• we should get LDM to approve prototyping the feature (Champion "Allow Generic Attributes" csharplang#124) so that we (compiler team) can start reviewing the PR in details. LDM is off session for next 10 days. I'll set myself a reminder to push this to agenda at first opportunity.

If I were you, I'd consider closing this PR and opening a new one, as we don't often look at PRs at the tail of the list.

[AviAvni]

@jcouv Thanks I'll open new pr soon