Implement ExperimentalAttribute

[RussKie]

Resolves #77869

/cc: @geeknoid @joperezr @stephentoub

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[RussKie]

Build failures don't seem related to my change, as other PRs fail on the same CI legs...

[stephentoub]

Resolves #77869

If this resolves that issue, what is tracking the tooling side of this, either in an analyzer or the compiler (wherever we landed)?

And when do we expect that tooling to land? We don't want to ship the attribute without associated tooling.

[RussKie]

If this resolves that issue, what is tracking the tooling side of this, either in an analyzer or the compiler (wherever we landed)?

And when do we expect that tooling to land? We don't want to ship the attribute without associated tooling.

@terrajobst is working on that AFAIK.
/cc: @jaredpar

[ghost]

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Resolves #77869

/cc: @geeknoid @joperezr @stephentoub

Author:	RussKie
Assignees:	RussKie
Labels:	area-System.Diagnostics, new-api-needs-documentation
Milestone:	-

[joperezr]

@terrajobst or @stephentoub, can you remind me what is the plan for using this attribute downlevel? Is the idea to just define the attribute as part of your assembly as an internal type or were we planning on having a Microsoft.BCL.* package with this?

[stephentoub]

can you remind me what is the plan for using this attribute downlevel?

@terrajobst can correct me if I'm wrong, but I believe the plan is:

• We ship a public ExperimentalAttribute in-box in .NET 8.
• We ship either an analyzer / compiler feature in .NET 8 / C# 12 that recognizes any definition of ExperimentalAttribute regardless of visibility.
• Anyone who needs this downlevel can add an internal copy of the attribute to their own assembly.

Immo?

[azure-pipelines]

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

[RussKie]

/azp run runtime

[geeknoid]

I'm with Jose here, I don't value the custom diag IDs and after a year or two of using an experimental attribute + analyzer in our code base, this has just not been an issue.

[joperezr]

Ok, I've re-watched the API Proposal meeting where this was discussed and understand the scenario now. The answer to my question above seems to be that the 1% case is for library authors that want to use a custom diagnostic ID for their experimental APIs. So for example, I build the FooBar library and want to introduce an experimental API, and I want to specify that people calling this API should get a diagnostic warning Foo0001 when using that API. This way, I may group all of my experimental APIs to all use the same daignostic (Foo0001) so that consumers of my library that are okay with using my experimental APIs can simply add a <NoWarn>Foo0001</NoWarn> to their project in order to remove all of the warnings coming from uses of my experimental APIs, but not the ones coming from other experimental APIs in different libraries.

That scenario does seem niche, and seems like it was mostly inspired by the Obsolete attribute, which has the same concept. I propose we do an incremental approach here where we start with an attribute that doesn't support custom diagnostic IDs and can be merged now and we can have a temporary analyzer on the roslyn-analyzers repo for this as @geeknoid suggests, and then if we think it's necessary and @jaredpar thinks we have time to get to this in .NET 8, we later re-add the DiagnosticId support and move the analyzer to be part of the compiler instead. Does that sound reasonable?

[RedworkDE]

(I only have limited experience with this and didn't try this idea) While analyzers have to declare the diagnostics they want to emit, source generators do not. So shouldn't it be possible to implement this as a very weird source generator, that doesn't emit any sources and just raises the experimental diagnostics with the custom ids?

[jaredpar]

I'll give you a firm yes / no by end of week on this.

[jaredpar]

We're going to try and get our end done this week / early next. The change itself is small but the testing is not. @jcouv will be doing the work.

[joperezr]

That's great, thanks @jaredpar for confirming. Apologies if this is a very dumb question, but in order for you to start the work, do you need this PR to be merged first or can you start the work in parallel?

[jaredpar]

We can start the work without this PR.

[stephentoub]

If the tooling is going to happen soon, whether in compiler or analyzer as a backup, I have no problem merging the attribute now. I just wanted to make sure it was still in the works, as I hadn't heard any update for literally months :)

[joperezr]

Sounds good to me. Merging this in then, thanks folks!

[jcouv]

@joperezr @stephentoub Let's not include "Assembly" and "Module" in the list. I don't think those targets are valid for other attributes of this kind (Obsolete, Windows.Foundation.Metadata.Experimental, Windows.Foundation.Metadata.Deprecated, ...).

[joperezr]

cc: @terrajobst @geeknoid

Thanks for the feedback @jcouv. Aside from the fact that other attributes don't support this, is there a particular reason why you don't think we should allow Assembly or Module use? Seems to me that there are valid scenarios where people developing new assemblies might opt to use it at that level, in the past this has been done mostly via using prerelease package versions but I think that not everybody uses that and there are some scenarios where it might be better to have stable p ackages with assemblies marked as experimental in them.

[jcouv]

The main reason for my question is precedent. The secondary reason is implementation cost (deviating from existing attributes means additional complexity and testing).
I'm not saying it's a bad idea, but since the previous design worked, I think we'd need a strong reason to revise it (why was it sufficient for those other attributes but not this one).

[joperezr]

fair enough. @bartonjs @terrajobst @stephentoub Do you know off the top of your head why the Obsolete attribute doesn't support Assembly or Module targets? I would have expected the same arguments should be made for both Obsolete and Experimental, since they are essentially the same attribute but just tracking the two opposing ends of an API lifecycle.

[RussKie]

I believe dotnet/extensions has assembly-level applications of the attribute. E.g., https://github.com/dotnet/extensions/blob/d558f8de86adf93044bc5386bd4367b3d509ead3/Directory.Build.targets#L68-L72

[terrajobst]

For the platform compat attributes we allowed assembly/module and we use it quite a bit.

[terrajobst]

I'm fine with excluding them for now if we want to match ObsoleteAttribute. We can revisit then we run into limitations.

[RussKie]

@geeknoid FYI

[jcouv]

FYI, the compiler/roslyn change was merged.

[jcouv]

@joperezr @stephentoub I just want to confirm the expected user experience with an example. I'm re-using a message we already have for APIs marked as experimental.

Let me know if you have any feedback.

    [Fact]
    public void Test()
    {
        var src = """
C.M();

[System.Diagnostics.CodeAnalysis.Experimental("DiagID1", UrlFormat = "https://example.org/{0}")]
class C
{
    public static void M() { }
}
""";

        var comp = CreateCompilation(new[] { src, experimentalAttributeSrc });
        comp.VerifyDiagnostics(
            // 0.cs(1,1): warning DiagID1: 'C' is for evaluation purposes only and is subject to change or removal in future updates. (https://example.org/DiagID1)
            // C.M();
            Diagnostic("DiagID1", "C").WithArguments("C").WithLocation(1, 1)
            );

        var diag = comp.GetDiagnostics().Single();
        Assert.Equal("DiagID1", diag.Id);
        Assert.Equal(ErrorCode.WRN_Experimental, (ErrorCode)diag.Code);
        Assert.Equal("https://example.org/DiagID1", diag.Descriptor.HelpLinkUri);
    }

[joperezr]

Yup, the above looks right to me. You can then also write a compilation which passes in the NoWarn for your diagnostic which should not show it any more, or alternatively suppress at each call site by using pragma.

[terrajobst]

Same