[New API] Expose a way to identify custom attributes

[davidwengier]

Background and Motivation

Hot Reload is one of the big features of .NET 6 and a key scenario for it is the ability to create new route end points in ASP.NET Core web applications and APIs. These end points are denoted by methods that are annotated with an attribute (eg HttpGet) however currently Edit and Continue, which is the system that powers Hot Reload, doesn't support modifying attributes.

#52986 solves the broad issue and allows Edit and Continue to modify attributes, but we don't have a way to enforce that only attributes emitted in the CustomAttributes metadata table are able to be modified at run time. The compiler currently has this information, for some attributes, in the AttributeData.ShouldEmitAttribute method:
https://github.com/dotnet/roslyn/blob/main/src/Compilers/CSharp/Portable/Symbols/Attributes/AttributeData.cs#L687

This proposal is to add a public API for exposing the information in that method.

Proposed API

namespace Microsoft.CodeAnalysis
{
     public abstract class AttributeData
     {
+        public bool IsEmitted { get; }
     }

Usage Examples

var methodSymbol = semanticModel.GetDeclaredSymbol(methodSyntax);
foreach (var attribute in methodSymbol.GetAttributes())
{
	if (!attribute.IsEmitted)
	{
		ReportRudeEdit(...);
	}
}

Alternative Designs

I went with the approach of just exposing a method that already exists, but the "Should Emit" part of the name is a little odd for a public API. I considered IsCustomAttribute, but wasn't sure if the custom attribute part of the name would be confusing in this context, and thought the return type parameter made it odd too, because surely an attribute is either custom or not, irrelevant of whether used on a return type?

The other option of course is to have this new API be much more simple, and remove the parameters altogether, and just have a list of attributes that are known to not be emitted in any circumstance.

Risks

By exposing an existing method the risks are minimal. Using a different name, or getting rid of the parameters, introduces a risk in terms of having multiple things to maintain.

[333fred]

@AlekseyTs as API owner.

[tmat]

I think a better API would be bool IsPseudoCustomAttribute { get; } on a symbol that represents the attribute type.

IsReturnType and emittingAssemblyAttributesInNetModule should not be needed to determine whether an attribute is real custom attribute or pseudo-custom attribute.

For EnC purposes we can ignore netmodules. Those are legacy consturcts and already not supported by EnC.

[AlekseyTs]

I think a better API would be bool IsPseudoCustomAttribute { get; } on a symbol that represents the attribute type.

I think I disagree with that. Whether the attribute is going to be translated into an alternative representation in metadata depends on the application target as well.

[tmat]

We have attributes that are emitted to CustomAttribute table when applied on one target and to metadata flags when on a different target?

The ECMA spec defines pseudo-custom attributes like so:

II.21.2 Attributes used by the CLI
There are two kinds of custom attributes, called genuine custom attributes, and pseudo custom
attributes. Custom attributes and pseudo custom attributes are treated differently, at the time they are
defined, as follows:

• A custom attribute is stored directly into the metadata; the‘blob’ which holds its
  defining data is stored as-is. That ‘blob’ can be retrieved later.
• A pseudo custom attribute is recognized because its name is one of a short list.

...

Pseudo custom attributes therefore serve to capture user directives, using the same familiar syntax the
compiler provides for genuine custom attributes, but these user directives are then stored into the more
space-efficient form of metadata tables.

[davidwengier]

IsReturnType and emittingAssemblyAttributesInNetModule should not be needed to determine whether an attribute is real custom attribute or pseudo-custom attribute.

I agree, and this is what I meant in one of my sentences under "Alternative Designs" but you worded it better than me :)

In fact, I would imagine that for EnC it would just always pass false for emittingAssemblyAttributesInNetModule, and if its too hard to work out isReturnType (which, granted, it probably isn't) it could just call the function twice with both true and false and it would have the desired effect.

[AlekseyTs]

I believe the ECMA spec is only concerned by scenarios where runtime behavior is affected by the attributes and where metadata provide an alternative way to capture the information (through special flags, etc.), which is target specific. When the same attribute is applied to a target that doesn't have that special metadata encoding defined, we have no other choice but emit the attribute as is, which I believe we do.

[AlekseyTs]

It feels like AttributeData can simply have a property "IsEmitted", which can cover conditional attributes and pseudo attributes. Or we can have two separate properties.

[333fred]

@AlekseyTs I've applied api-needs-work for now. When you're satisfied with the current proposal, please remove it and apply api-ready-for-review

[AlekseyTs]

@davidwengier It doesn't look like you made any adjustments to the proposal based on the discussion. You "voted" for #53410 (comment). would you like to adjust the proposal based on that?

[davidwengier]

@AlekseyTs Apologies, I was unclear on the process. Thank you for the reminder, I've updated the issue description.

[333fred]

API Review

API Approved.