Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Library-Level Preview Features #77869

Closed
geeknoid opened this issue Nov 3, 2022 · 39 comments · Fixed by #85444
Closed

Enable Library-Level Preview Features #77869

geeknoid opened this issue Nov 3, 2022 · 39 comments · Fixed by #85444
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime blocking Marks issues that we want to fast track in order to unblock other important work

Comments

@geeknoid
Copy link
Member

geeknoid commented Nov 3, 2022

Edited by @terrajobst. Original request at the end. The spec for this feature is here.

Background and motivation

In .NET 6, we've added a way to ship preview features in otherwise stable releases. It works by tagging APIs (and language features) as preview features and requiring the user's project to explicitly opt-into using them via the <EnablePreviewFeatures> property.

The feature was primarily designed to accommodate platform-level preview features, that is for features that span runtime, library, and language, such as generic math. However, we also believed that the feature would work for libraries that want to offer preview functionality in otherwise stable NuGet packages.

This feature is about providing a model that works great for library authors. For details, see the design document.

API Proposal

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; }
}

API Usage

Shipping an experimental API in a stable package

Martin builds a library that integrates with ASP.NET Core and provides features to make it easier to build services. The library is distributed as a NuGet package. The package is already in a version well past 1.0 and is considered stable.

He ships the package often and doesn't want to maintain multiple flavors of the package, one marked as prerelease and one marked as stable. However, he wants to expose new APIs without making the promise that these APIs are stable. He designs the new APIs as if they would be stable, that is, he puts them in the namespace they belong and names the types and members without any naming convention that implies that they aren't stable yet. If customer feedback is positive, the API will become stable as-is, otherwise he'll make changes to address the feedback.

In order to convey to the consumers that these APIs aren't stable yet, he puts the Experimental attribute on them:

namespace MartinsLibrary.FancyLogging
{
    public static class FancyLoggerExtensions
    {
        [Experimental("ML123", UrlFormat="https://martinslibrary.net/diagnostics/{0}")]
        public static IServiceCollection AddFancyLogging(this IServiceCollections services)
        {
            // ...
        }
    }
}

Consuming an experimental API

David builds a new version of Jabber.NET and consumes Martin's library. He very much wants to play with the new fancy logging Martin added. When David calls AddFancyLogging he gets a compilation error:

error ML123: FancyLoggerExtensions.AddFancyLogging() is not a stable API. In order to consume this experimental API, suppress this error from the call site.

David is happy to accommodate future breaking changes around the fancy logging feature. He understands that the diagnostic ID is specific to the fancy logging feature of Martin's library, so he decides to add a suppression to the project file:

<Project>
    <PropertyGroup>
        <!-- Fancy logging is an experimental APIs -->
        <NoWarn>$(NoWarn);ML123</NoWarn>
    </PropertyGroup>
</Project>
Original Request ### Background and motivation

Library developers often want to experiment with APIs before committing to long-term support. This is true within an organization, as well as within the open-source community. It would be very useful for the .NET infrastructure to support experimental interfaces in a first class way.

The idea is to flag the use of experimental APIs in calling code, to make developers aware that they're calling into an API which is not yet settled and is subject to change outside of the normal support model for the component.

The attribute would be supplemented by a Roslyn analyzer that reports use of experimental APIs. You can also imagine special treatment in IntelliSense and in DocFx for experimental APIs.

API Proposal

namespace System.Diagnostics.CodeAnalysis;

/// <summary>
/// Indicates that an API element is experimental and subject to change without notice.
/// </summary>
[AttributeUsage(
    AttributeTargets.Class |
    AttributeTargets.Struct |
    AttributeTargets.Enum |
    AttributeTargets.Interface |
    AttributeTargets.Delegate |
    AttributeTargets.Method |
    AttributeTargets.Constructor |
    AttributeTargets.Property |
    AttributeTargets.Field |
    AttributeTargets.Event |
    AttributeTargets.Assembly)]
public sealed class ExperimentalAttribute : Attribute
{
    /// <summary>
    /// Gets a human readable explanation for marking API as experimental.
    /// </summary>
    public string? Message { get; }

    /// <summary>
    /// Initializes a new instance of the <see cref="ExperimentalAttribute"/> class.
    /// </summary>
    public ExperimentalAttribute()
    {
        // Intentionally left empty.
    }

    /// <summary>
    /// Initializes a new instance of the <see cref="ExperimentalAttribute"/> class.
    /// </summary>
    /// <param name="message">Human readable explanation for marking experimental API.</param>
    public ExperimentalAttribute(string message)
    {
        Message = Throws.IfNull(message);
    }
}

API Usage

[Experimental("Trying this out, maybe committed in the 3.1 release")]
public void DoSomethingNew();

[assembly: Experimental("This whole assembly is subject to change without notice")]

Alternative Designs

No response

Risks

No response

@geeknoid geeknoid added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 3, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 3, 2022
@geeknoid geeknoid changed the title [API Proposal]: Introduce an [Experimental] attribute to complete [Obsolete] [API Proposal]: Introduce an [Experimental] attribute to complement [Obsolete] Nov 3, 2022
@jkotas
Copy link
Member

jkotas commented Nov 3, 2022

Is this any different from RequiresPreviewFeaturesAttribute?

@stephentoub
Copy link
Member

Is this any different from RequiresPreviewFeaturesAttribute?

I don't believe so in any meaningful way. @geeknoid, this is the attribute I mentioned we already have, with an associated analyzer, shipped in .NET 6.

I don't see us shipping an [Experimental] in addition to that.

@Clockwork-Muse
Copy link
Contributor

Duplicate #31542

@stephentoub
Copy link
Member

stephentoub commented Nov 4, 2022

Duplicate #31542

Right, where the proposed [Experimental] became [RequiresPreviewFeatures].

I'm going to close this one. If there are additional ways RequiresPreviewFeatures should be improved, we can certainly look into that, but at present I'm not aware of any significant deficiencies.

Thanks, Martin.

@stephentoub stephentoub closed this as not planned Won't fix, can't repro, duplicate, stale Nov 4, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Nov 4, 2022
@geeknoid
Copy link
Member Author

geeknoid commented Nov 4, 2022

As a third party developer, can I use [RequiresPreviewFeatures]? I thought this was coupled to the .NET runtime's idea of preview.

So in my foo nuget package, I can mark stuff with this attribute, and get warnings when I use these APIs?

@stephentoub
Copy link
Member

As a third party developer, can I use [RequiresPreviewFeatures]

Yes

So in my foo nuget package, I can mark stuff with this attribute, and get warnings when I use these APIs?

Yes

@ghost ghost locked as resolved and limited conversation to collaborators Dec 4, 2022
@danmoseley
Copy link
Member

@geeknoid you mentioned offline that [RequiresPreviewFeatures] doesn't fulfil your project's needs. Can you enumerate the gaps? We should discuss whether we can improve RPF. It would be unfortunate to bifurcate.

@danmoseley danmoseley reopened this Dec 6, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 6, 2022
@danmoseley
Copy link
Member

Reopening for that conversation.

@tannergooding
Copy link
Member

@danmoseley, worth noting this is still locked. I believe an admin will need to unlock it

@dotnet dotnet unlocked this conversation Dec 6, 2022
@danmoseley
Copy link
Member

Hmm, that's odd, at least for discussions, only triage rights are required. I don't see mention of issues though: https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/repository-roles-for-an-organization

@geeknoid
Copy link
Member Author

geeknoid commented Dec 7, 2022

@danmoseley The main problem is that RequiresPreviewFeature does funny things for different TFMs:

https://github.com/dotnet/designs/blob/main/accepted/2021/preview-features/preview-features.md#meaning-of-property-in-multi-targeted-projects

I just want to be able to mark my specific API as being experimental, and that's across any TFMs. So when I add a feature, I'm going to add to it to both my net7.0 build and to my netstandard2.0 build and I want customers to be notified when they use this experimental API in both cases.

@stephentoub
Copy link
Member

@terrajobst, can you comment?

I don't want to see us introduce yet another "this is a preview / experimental feature" attribute. If the current one doesn't currently suffice, from my perspective we should bend over backwards to make it suffice rather than introduce something additional.

@danmoseley
Copy link
Member

I agree - is there a technical issue preventing us extending ours, or philosophical?

@geeknoid
Copy link
Member Author

geeknoid commented Dec 8, 2022

I'm all for reusing. I had started ripping [Experimental] out of our code base and then noticed that [RequiresPreviewFeatures] wasn't working right for us, so I backed out...

@terrajobst
Copy link
Member

terrajobst commented Dec 9, 2022

@KrzysztofCwalina asked for this attribute to be usable for the Azure SDK. I believe his scenarios are similar to the one raised by @geeknoid. The primary challenge for the Azure SDK is that it needs to work in projects targeting .NET Framework, .NET Standard, and .NET Core. Projects targeting .NET 7.0 or greater can just use the attribute. For projects targeting older frameworks they can in principle define an internal attribute under the same name but the challenge then is we provide the diagnostic via an analyzer that is built into the SDK. That means consumers would only get the diagnostic if they are using the .NET 7.0 SDK or greater (currently our analyzer is also only used if the TFM is high enough, but we could relax that which would probably bump the SDK to 8.0). .NET Standard users might or might not (depending on the SDK they are using) and .NET Framework users most like would not not, assuming they are using old-school MSBuild projects instead of SDK-style projects (at which point they are like .NET Standard, which means it still depends on the SDK version).

So in order to provide this experience sensibly, we would have to ship the analyzer as a NuGet package that the Azure SDK can depend on. We would make it so that we'd only inject the package provided analyzer for .NET Framework, .NET Standard, and .NET Core <= 6.0. We could decide to include the attribute in this package and have it type forward on .NET Core 7.0 -- or -- we could just tell users for down-level platforms to ship an internal version themselves, which would be my preference as it doesn't create yet-another assembly.

Regarding the behavior in multi-targeting: we could decide to relax that. The rub is of course that if people turn on preview features, they turn on preview features across the entire stack. This includes runtime preview features, language preview features, and core-library preview features. As the spec suggests, this feature wasn't designed to support third party preview features, but the design also explicitly doesn't disallow it, so modulo the downlevel issue, it would just work for anyone else. However, they have to accept that their customers don't just turn on preview features for their libraries, they turn them on for the entire .NET stack.

If we don't want this, there are two options around this:

  1. We can revive the original design that allows for multiple preview features.
  2. We can tell people to take our analyzer as the base for building their own. This can either be down via copy & pasting our analyzer and modifying it, or we could enable third parties to define their own attributes and project setting, that our analyzer can also provide diagnostics for.

My preference would be to start with a simple model, which is accepting the single preview dimension. If that is proven to not be workable, we can still add the additional levels of preview-ness but we deliberately scoped it out for good reasons.

@geeknoid
Copy link
Member Author

geeknoid commented Dec 9, 2022

The model we use at the moment is that:

  • We define our own attribute and analyzer that flags uses of marked APIs
  • The [Experimental] attribute is shown in the reference docs
  • Users don't need to opt in to using experimental features, we rely on the warnings as the "protector"

It really sounds like the semantics of the existing RequiresPreviewFeatures isn't right for what I want. The fact an app uses a single experimental feature from package A does not by any stretch of the imagination mean that they are OK using experimental features in all other packages, and in the runtime or language.

It sounds to me like the right answer all around is to define a new attribute and analyzer, in a dedicated package targeting netstandard2.0. For extra bonus points, improve IntelliSense to clearly identity experimental features as it pops up suggestions as you're typing.

@stephentoub
Copy link
Member

we would have to ship the analyzer as a NuGet package that the Azure SDK can depend on

We already do this. The https://github.com/dotnet/roslyn-analyzers/blob/main/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DetectPreviewFeatureAnalyzer.cs is part of Microsoft.CodeAnalysis.NetAnalysis, which is included both in the .NET SDK and as its own NuGet package:
https://www.nuget.org/packages/Microsoft.CodeAnalysis.NetAnalyzers

And it works fine in a .NET Framework app. I just created a new one, added a reference to this package, added an internal copy of the attribute, and voila:
image

I don't see why we'd create yet another analyzer or attribute.

this feature wasn't designed to support third party preview features

That's not how I remember it and I'm really surprised it shows up like that in the spec. This feature closed multiple community requests for something the community could use, and saying it only works for our own libraries doesn't meet that need.

The fact an app uses a single experimental feature from package A does not by any stretch of the imagination mean that they are OK using experimental features in all other packages, and in the runtime or language.

You're suggesting every library that wants its own experimental features should define its own new attribute (in its own namespace), write its own analyzer, and ship those specific to the library? That sounds very undesirable.

If instead you're suggesting we create a new attribute and new analyzer just to be able to ship it in a nuget, we already have the nuget for the analyzer and an attribute that meets that need. If we want to extend the attribute/analyzer to support fine-grained disablement, fine. If we want to ship a nuget for the attribute, ok, but it really seems like something someone that wants this can just copy/paste the 5 lines, and the number of libraries that want this will be limited... if that changes in the future, we could ship it in a nuget then.

@geeknoid
Copy link
Member Author

geeknoid commented Dec 9, 2022

@stephentoub That's not what I'm looking for.

I'm not looking for a way to enable an experimental feature. I'm looking for a way to signal to the customer that they are using an experimental feature.

I want to mark an API as experimental anywhere in any of our assemblies, and have customers be able to use them as-is without enabling. An analyzer then tells them at the point of use that they are using an experimental feature. IntelliSense tells them too. And the experimental attribute is shown in the reference doc page, maybe the reference doc page even has special coloring to notify the user the API is experimental. If the user wants to in fact use the specific experimental feature in their code, they can suppress or ignore the warning.

I don't want to just say "my assembly wants to use all experimental features of all dependencies, of the language, and of the runtime" by virtue of setting a single property in my assembly's project file. That's an anti-pattern I think. I want to instead have an ecosystem of dependencies, each of which can have distinct experimental features and as the author of the assembly I can chose specifically which experimental feature I will use, and will get notified if I accidentally try to use some experiemental feature I'd rather not use.

@stephentoub
Copy link
Member

stephentoub commented Dec 9, 2022

I'm not understanding the distinction. You ship a method marked with [TheAttribute]. The analyzer warns that they're using something annotated with that attribute, and they have to do some gesture to make the warning go away. How is this different from what [RequiresPreviewFeatures] does?

I want to instead have an ecosystem of dependencies, each of which can have distinct experimental features and as the author of the assembly I can chose specifically which experimental feature I will use, and will get notified if I accidentally try to use some experiemental feature I'd rather not use.

There are two options for this, both of which Immo outlined and both of which I referenced. Either:

  • Everyone who has experimental features ships their own attribute and their own analyzer both specific to their library, or
  • We augment the one true attribute/analyzer to support multiple dimenstions, as Immo put it

How is what you're talking about different from that?

@jkotas
Copy link
Member

jkotas commented Dec 16, 2022

Assigned correct area label.

@dakersnar dakersnar added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Dec 16, 2022
@terrajobst terrajobst changed the title [API Proposal]: Introduce an [Experimental] attribute to complement [Obsolete] Enable Library-Level Preview Features Dec 16, 2022
@terrajobst
Copy link
Member

terrajobst commented Dec 16, 2022

I have convinced myself that we need to make some changes to [RequiredPreviewFeatures] in order to make it work well for libraries (i.e. non-platform level components). I have drafted a spec here. Feedback welcome!

@terrajobst terrajobst added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Feb 27, 2023
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 9, 2023
@terrajobst
Copy link
Member

terrajobst commented Mar 9, 2023

Video

Looks good as proposed

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; }
}

@RussKie
Copy link
Member

RussKie commented Apr 26, 2023

I'm happy to dip my toes with the runtime changes and implement this. Just to clarify before I start working on this. Will the code go under https://github.com/dotnet/runtime/tree/main/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis?

@CodeBlanch
Copy link
Contributor

This feature seems like it would really help us over on OpenTelemetry .NET. The OpenTelemetry Specification routinely defines things as experimental (ex: RegisterProducer) and we have been struggling to find ways to give customers stable SDKs with sub-APIs in preview.

@stephentoub
Copy link
Member

stephentoub commented Apr 27, 2023

Will the code go under https://github.com/dotnet/runtime/tree/main/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis?

Yes, the attribute would be added there (and the path to it added to https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems). You'll also need to update the https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime/ref/System.Runtime.cs ref assembly file with the shape of the API, and add some simple tests in https://github.com/dotnet/runtime/tree/main/src/libraries/System.Runtime/tests/System/Diagnostics/CodeAnalysis.

I don't remember if we landed on the compiler itself doing the validation or an analyzer doing the validation. If the latter, that would be done in dotnet/roslyn-analyzers.

@RussKie
Copy link
Member

RussKie commented Apr 27, 2023

I believe it's meant to be a compiler feature (https://youtu.be/-MBZGdnYPGA?t=364).

@RussKie RussKie self-assigned this Apr 27, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 27, 2023
RussKie added a commit to RussKie/runtime that referenced this issue May 16, 2023
joperezr pushed a commit that referenced this issue Jun 15, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 15, 2023
@geeknoid
Copy link
Member Author

@terrajobst Would it be possible to maintain assembly-level support for this attribute? In dotnet/extensions, we have the notion of dev-stage packages. Those are packages which are in active development and thus the entire API surface of the package should be considered experimental. Using our home-grown original ExperimentalAttribute, we've been able to set a single attribute to mark the whole API surface of the package as experimental. But without assembly-level support, we'd have to mark each API independently as experimental.

A similar argument can be made when making a whole package as obsolete, it would be reasonable to be able to apply the obsolete attribute to the assembly level.

CC: @danmoseley @joperezr

@joperezr
Copy link
Member

Agreed here. If we were only enabling the attribute for members, then I'd agree that it doesn't make sense to have it on the assembly level, but the whole reason why we allow adding it to higher level concepts like classes and structs is for convenience to not have to add it to all members of them, so I don't understand why that exact same reasoning doesn't apply for the assembly level.

@geeknoid
Copy link
Member Author

@terrajobst Ping.

@teo-tsirpanis
Copy link
Contributor

@geeknoid what are you referring to? The attribute already can be applied to assemblies.

@stephentoub
Copy link
Member

He's referring to the compiler's analysis, which doesn't pay attention to the attribute on the assembly. @jcouv is looking into it.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.