Support `ExperimentalAttribute.Message`

[terrajobst]

There was a request to apply experimental attribute with a message, just like [Obsolete]:

namespace System.Diagnostics.CodeAnalysis;

[AttributeUsage(/* … */)]
public sealed class ExperimentalAttribute : Attribute
{
    // Already exists:
    // public ExperimentalAttribute(string diagnosticId);
    // public string DiagnosticId { get; }
    // public string UrlFormat { get; set; }

    // New with .NET 10:
    public string Message { get; set; }
}

We don't see a problem with that but it would require feature work in the compiler to support it.

Any thoughts on this?

[jaredpar]

@deepakrathore33 please do not triage compiler / language bugs. Your movement caused it to completely fall off my radar

[AlekseyTs]

From compiler's perspective the following shape will be simpler to support (a constructor argument vs. a property set):

namespace System;

public partial class ExperimentalAttribute
{
    public ExperimentalAttribute() {}
    public ExperimentalAttribute(string message)
    {
         Message = message;
    }

    public string? Message { get; }
}

[jaredpar]

@tannergooding ^

[tannergooding]

@AlekseyTs the signature there isn't quite possible due to the existing public ExperimentalAttribute(string diagnosticId) signature.

The is the current public API surface is:

sealed class ExperimentalAttribute : Attribute
{
    public ExperimentalAttribute(string diagnosticId)
    {
        DiagnosticId = diagnosticId;
    }

    public string DiagnosticId { get; }

    public string? UrlFormat { get; set; }
}

We could potentially also expose public ExperimentalAttribute(string diagnosticId, string message) and public string? Message { get; } instead of public string? Message { get; set; }, would that work?

[terrajobst]

Apologies, I used the partial syntax to just focus on the addition. The type currently looks like this:

namespace System.Diagnostics.CodeAnalysis;

[AttributeUsage(AttributeTargets.Assembly |
                AttributeTargets.Module |
                AttributeTargets.Class |
                AttributeTargets.Struct |
                AttributeTargets.Enum |
                AttributeTargets.Constructor |
                AttributeTargets.Method |
                AttributeTargets.Property |
                AttributeTargets.Field |
                AttributeTargets.Event |
                AttributeTargets.Interface |
                AttributeTargets.Delegate,
                Inherited = false)]
public sealed class ExperimentalAttribute : Attribute
{
    public ExperimentalAttribute(string diagnosticId);
    public string DiagnosticId { get; }
    public string UrlFormat { get; set; }
}

We could do a constructor, but then it would look like this:

namespace System.Diagnostics.CodeAnalysis;

[AttributeUsage(AttributeTargets.Assembly |
                AttributeTargets.Module |
                AttributeTargets.Class |
                AttributeTargets.Struct |
                AttributeTargets.Enum |
                AttributeTargets.Constructor |
                AttributeTargets.Method |
                AttributeTargets.Property |
                AttributeTargets.Field |
                AttributeTargets.Event |
                AttributeTargets.Interface |
                AttributeTargets.Delegate,
                Inherited = false)]
public sealed class ExperimentalAttribute : Attribute
{
    public ExperimentalAttribute(string diagnosticId, string message);
    public string DiagnosticId { get; }
    public string Message { get; }
    public string UrlFormat { get; set; }
}

We thought of Message to be rare and we generally use properties for optional attribute parameters. That said, you're not the first to suggest a constructor parameter. I think I'm coming around to that.

@jeffhandley, @tannergooding /cc @dotnet/fxdc

[AlekseyTs]

It turned out we already have a property that compiler pays attention to (UrlFormat), adding another settable property is not a big deal.

[bartonjs]

FDG says (with regard to Attribute types)

DO NOT provide constructor parameters to initialize properties corresponding to the optional arguments.

As there is an overload that does not take message, message is (by definition) optional. So should be done in a get/set property, not a ctor parameter.

[jaredpar]

FYI: FDG == framework design guidelines.

That term isn't used as much in this repo ;)

[terrajobst]

@jaredpar

That term isn't used as much in this repo ;)

Heathens! ;-)

@bartonjs

Yes. But there is plenty of prior art in the BCL where we violated that. For example [Obsolete]. I see this more of a question of ergonomics, rather than a hard rule.

The question is whether you would prefer this:

[Experimental("SYSLIB123", Message = "The FancyPants IA is experimental"]
public void FancyPantsOpCodeFromTannerThatLikeFivePeopleWant();

over this:

[Experimental("SYSLIB123", "The FancyPants IA is experimental"]
public void FancyPantsOpCodeFromTannerThatLikeFivePeopleWant();

I don't have a strong preference, but multiple folks asked for a constructor parameter now.

[jodydonetti]

Nitpicking probably (the intent was clear), but should this:

We could do a constructor, but then it would look like this:

namespace System.Diagnostics.CodeAnalysis;

[AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Module | AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Enum | AttributeTargets.Constructor | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Event | AttributeTargets.Interface | AttributeTargets.Delegate, Inherited = false)] public sealed class ExperimentalAttribute : Attribute { public ExperimentalAttribute(string diagnosticId, string message); public string DiagnosticId { get; } public string Message { get; } public string UrlFormat { get; set; } }

be this (multiple ctors):

namespace System.Diagnostics.CodeAnalysis;

[AttributeUsage(AttributeTargets.Assembly |
                AttributeTargets.Module |
                AttributeTargets.Class |
                AttributeTargets.Struct |
                AttributeTargets.Enum |
                AttributeTargets.Constructor |
                AttributeTargets.Method |
                AttributeTargets.Property |
                AttributeTargets.Field |
                AttributeTargets.Event |
                AttributeTargets.Interface |
                AttributeTargets.Delegate,
                Inherited = false)]
public sealed class ExperimentalAttribute : Attribute
{
    public ExperimentalAttribute(string diagnosticId);
    public ExperimentalAttribute(string diagnosticId, string message);
    public string DiagnosticId { get; }
    public string Message { get; }
    public string UrlFormat { get; set; }
}

not to remove an existing ctor?

[terrajobst]

Right :-)

[AlekseyTs]

@terrajobst I will be able to start the work on compiler's side as soon as the shape of the API is finalized. At this point, I really do not have a personal preference regarding the shape.

[terrajobst]

Sounds good. Will close on this via email and report back.

[terrajobst]

We decided to stick to the approved API shape, which means no ctor argument for message and instead have a settable property for it. I updated the issue description to reflect that.

[tannergooding]

@jaredpar, does the Roslyn support need to be merged before the libraries side PR?

Or are we fine to go ahead and merge the libraries side and it just won't light up until Roslyn also finishes its work?

[jaredpar]

This is just changing the message that the compiler produces so I don't forsee it being a problem.

[AlekseyTs]

does the Roslyn support need to be merged before the libraries side PR?

I think it doesn't. An extra property should have no impact on the current compiler behavior.