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

Experimental APIs #285

Merged
merged 13 commits into from
Feb 27, 2023
Merged

Experimental APIs #285

merged 13 commits into from
Feb 27, 2023

Conversation

terrajobst
Copy link
Member

@terrajobst terrajobst commented Dec 16, 2022

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 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.

Since then, we've seen requests from partners who have tried using it and hit limitations that come from guardrails we put into place due to the complex nature of platform-level preview features.

The goal of this feature is to find a design that works for both, platform-level preview features and library-level preview features.

@jeffhandley
Copy link
Member

When we first introduced preview features, we considered adding some sort of a name so that consumers could enable/disable preview features by name; we decided it was overkill at that time. Should we reconsider that approach here instead of introducing a differentiation between "platform" and "library"?

We could have the nameless (null name) attributes denote platform, but then allow the name to be viral the way is described here. That would allow libraries to even have different sets of preview features within their libraries. Imagine:

  1. Library Foo introduces preview feature set "FooBar" in version 34.0.0
    • There are many APIs annotated as "FooBar"
  2. Consumers consume Foo version 34.1.0 and enable "FooBar" features to be used
  3. Library Foo introduces preview feature set "QuxBaz" in version 35.0.0
    • There are many APIs annotated with "QuxBaz"
    • The "FooBar" APIs are refined based on consumer feedback, but they remain in preview
  4. A consumer wants to upgrade to 35.0.0, compensate for "FooBar" breaking changes, but disallow "QuxBaz" usage

Of course, even just allowing or disallowing preview feature usage at the library level also could benefit from a name. But, ugh, would we want it to be a fully-qualified name? I submit that usability is more important than name collision avoidance, and therefore just passing through whatever name the library author applies to the attribute is fine. In fact, unqualified names make the viral nature much easier to manage anyway.

In summary though, I propose that instead of differentiating between library and platform, we allow the attribute to include a name--and then consumers can specify the list of names that are enabled. A value of true indicates that platform-level preview features are enabled and all named preview features are enabled too. And only when set to true do we require the SDK version match as we do today.

Co-authored-by: Buyaa Namnan <buyankhishig.namnan@microsoft.com>
@terrajobst
Copy link
Member Author

terrajobst commented Jan 3, 2023

When we first introduced preview features, we considered adding some sort of a name so that consumers could enable/disable preview features by name; we decided it was overkill at that time.

I'm not opposed to that (shocker, since I proposed the name-based approach before); the primary pushback about this design was that it creates combinatorial complexity. For example, what happens if these features depend on each other (explicitly or inadvertently)? Are we going to test combinations for stuff we as the .NET platform ship (runtime, core libraries, compilers, ASP.NET, EF etc)? Do we need to track the list of these names to avoid conflicts?

I have a negative knee-jerk reaction whenever we build facilities that differentiate between platform- and ecosystem use but I think in this case this would be acceptable because there is a stark difference between the features that require runtime/language support and the ones that don't, which are very visible (and as proven by the conversations we had with our partners) surprising in its limitations.

By having the feature distinguish between them, I think we can deliver a better UX than if we build a generic name-based system because we then can no longer track which features lock you into a matching SDK and which ones don't -- which in my opinion is the biggest issue in the UX.

@333fred
Copy link
Member

333fred commented Jan 3, 2023

I also don't think we're locking ourselves out of providing a more granular approach in the future: we could retconn platform and library as just two different names.

@marcpopMSFT
Copy link
Member

The original trigger for this update was customers who were using preview features with the 6.0 SDK and targeting net6.0 were upgrading their SDK and being broken without any other changes (ie still targeting net6.0). This proposal sounds like those customers would be working if they were only using library level features as preview features would still be enabled at the library level but still be broken if they were using net6.0 platform level preview features. Am I reading that correctly?

The customer would still be targeting net6.0 and using the 7.0 SDK so the platform level preview features would be disabled. Question though, breaking them in this config makes sense if they wanted to use 7.0 platform preview features (as they aren't targeting 7.0 yet) but what about 6.0 platform preview features when using 7.0?

@akoeplinger
Copy link
Member

Regarding the name-based approach: I guess this would also be handy for eventually extending beyond preview features to regular features (e.g. "threading" for the wasm case)

@terrajobst
Copy link
Member Author

terrajobst commented Jan 9, 2023

@marcpopMSFT

This proposal sounds like those customers would be working if they were only using library level features as preview features would still be enabled at the library level but still be broken if they were using net6.0 platform level preview features. Am I reading that correctly?

Yes, that is precisly the intent.

@akoeplinger

Regarding the name-based approach: I guess this would also be handy for eventually extending beyond preview features to regular features (e.g. "threading" for the wasm case)

Why would that be handier than just marking the API as [RequiresPreviewFeature(PreviewScope.Library)]?

@akoeplinger
Copy link
Member

Because it wouldn't be a preview feature. We discussed tagging APIs back when we did the platform annotations in the sense of having capability tags rather than simple OS tags (think "threading", "file i/o", "simd", ...). I realize this might be a bit too far fetched for this proposal 😆

Comment on lines +150 to +151
* Shipping `RequiresPreviewFeaturesAttribute` for downlevel
- [dotnet/runtime#79479](https://github.com/dotnet/runtime/issues/79479)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need to ship RequiresPreviewFeaturesAttribute down-level, or would it be the new attribute that gets shipped down-level?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think only the new attribute is needed on down-level platforms.

Copy link
Member Author

@terrajobst terrajobst Jan 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's the intent. I'll update the linked work items once we have consensus on this proposal.

accepted/2023/preview-apis/preview-apis.md Outdated Show resolved Hide resolved
@terrajobst terrajobst marked this pull request as ready for review January 30, 2023 20:31
@terrajobst terrajobst changed the title Library Preview Features Experimental APIs Jan 30, 2023

* **library-level preview feature**. These are library APIs that don't require
any platform-level preview features to be consumed. The only difference to
regular APIs is that these are in preview so their shape isn't stable and thus
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not just not stable, but may be entirely removed, even in a minor version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being explicit that minor versions may remove api's would be really useful for CoreWCF. We have a hard commitment not to remove api's between major version releases, but our major version lifetime is a lot longer than .NET, and the time between our minor releases is relatively short. We've struggled with how to manage introducing new features which might need to have their api changed based on feedback. There might be high confidence in the code quality, but some uncertainty of the new public api's.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also want to offer any guidance about servicing? Should consumers assume that even security patches might break experimental API?
A different way of asking this is -- should consumers assume they are in an unsupported state if their app uses experimental API (because a patch might break them anytime by removing the API) or is it a reasonable configuration to deploy in production, so long as they've tested (because only a minor version update would break the API)
I'm guessing any guidance woudl be -- if you deploy to production with an experimental API, you should make no assumptions about even a security patch breaking you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like our guidance should be as lax as possible, and we should direct libraries to make stronger guarantees if their scenario allows for it. For example, our guidance should be "Any experimental API has no shape or stability guarantees and no support guarantees", but CoreWCF could choose to guarantee "Any experimental API will not change shape in a security patch" for any such-attributed APIs in CoreWCF. This gives the most flexibility for developers to provide experimental features in their libraries without ending up back in a situation where we need to introduce a "SuperExperimental attribute" that signifies something looser than we define here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable.

Copy link

@kirankumarkolli kirankumarkolli Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Also may be all experimental features are not same as well even for a single library.

For example, Azure services might flight many features but may decide to follow-up different path with each feature/API in the same library.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danmoseley

should consumers assume they are in an unsupported state if their app uses experimental API

Yes. I'd argue that's the definition of using a preview/experimental/beta API. They aren't stable and thus unsupported. RCs are an exception to this rule but I think we can safely ignore that nuance.

if you deploy to production with an experimental API, you should make no assumptions about even a security patch breaking you.

Agreed.

@jkoritzinsky

I feel like our guidance should be as lax as possible

I think you mean the opposite: we want the most amount of flexibility, so I think the guidance should be as definitive as possible, along the lines of what Dan was suggesting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever the right term is, I think the guidance should allow the most flexibility possible.

Copy link
Member

@geeknoid geeknoid Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the semantics need to be clear, and keep the guidance to a minimum:

APIs marked as experimental indicate that the author of the API is not committing to supporting this API over time, and the quality of the API's implementation may be lower than the rest of the API surface. If you take a dependency on an experimental API, you may find that the API signature changes or the API is completely removed the next time you upgrade to a new version of the assembly containing the API.

And then the Experimental attribute should have a note property that lets the author provide more detailed guidance if needed. "We're pretty confident about this API, we expect to remove this attribute in the 1.2 release", "Please don't use this right now, we're still in active development". Stuff like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we embed my wording suggestion into the doc? I think it hits all the salient points.

@KrzysztofCwalina
Copy link
Member

I like the idea of having the features named. It will help with the following problem: a user adds "allow preview features" (APF) to be able to call feature A. After a couple of releases feature A ships stable, but nobody ever cleans the project file, the AFP stays in the project system (unnecessary). In the future another developer starts caling experimental feature B without realizing what's happening.

Co-authored-by: Igor Velikorossov <RussKie@users.noreply.github.com>
@timheuer
Copy link
Member

timheuer commented Feb 9, 2023

I think this will be okay, but just throwing this out there -- assuming you would want the initial experimental usage in tools to be shown as 'squiggles' in things like editors.

Also, how would this reflect things where APIs are reflected like objects in UI frameworks? e.g., XAML or Blazor tag helpers?

@terrajobst
Copy link
Member Author

terrajobst commented Feb 9, 2023

@timheuer

I think this will be okay, but just throwing this out there -- assuming you would want the initial experimental usage in tools to be shown as 'squiggles' in things like editors.

Yes, I have linked a work item where this is being discussed with the Roslyn folks as well as the docs team.

Also, how would this reflect things where APIs are reflected like objects in UI frameworks? e.g., XAML or Blazor tag helpers?

That's a good question; I didn't think of that yet. XAML and Tag helpers could work the same way as code, given that there is a text editor rendition of the API use. But there are other places (toolbox and property grid) that could also benefit from this. I updated the doc.

@geeknoid
Copy link
Member

I think we need to say something about the transitive nature of the attribute. Presumably:

  • If the experimental attribute is applied to a type, then all of the type's members and nested types are also considered experiemental.

  • If the experimental attribute is applied to an assembly, then all of the assembly's types and members are considered experimental.

@terrajobst terrajobst merged commit 871890f into dotnet:main Feb 27, 2023
@terrajobst terrajobst deleted the preview-for-libraries branch February 27, 2023 19:27
Copy link

@jskeet jskeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like this very much. I like the "per-feature" opt-in, and I'm fine with the distinction between platform and library in this situation.

One aspect which may be worth thinking about is that this is a per-member opt-in: it behaves quite differently to there being a beta version of a library and a stable version of the same library, where the library owner "knows" the context of whether it's beta or stable. If one piece of code needs to behave in different ways in the different contexts with the experimental attribute, the library author will need to have some sort of "turn on this behavior" method (protected by the attribute). That's entirely feasible of course, and could even be desirable in many cases (e.g. one instance of a type opting in to the new behavior, and another not) but for some contexts it won't be quite what's wanted.

This is in no way a criticism of the feature - it's more that if we publish documentation for library authors, it may be something to highlight.

accepted/2023/preview-apis/preview-apis.md Show resolved Hide resolved
@terrajobst
Copy link
Member Author

terrajobst commented Mar 15, 2023

@jskeet

One aspect which may be worth thinking about is that this is a per-member opt-in: it behaves quite differently to there being a beta version of a library and a stable version of the same library, where the library owner "knows" the context of whether it's beta or stable. If one piece of code needs to behave in different ways in the different contexts with the experimental attribute, the library author will need to have some sort of "turn on this behavior" method (protected by the attribute).

I'm not sure I fully understand. The intent is that suppressing the warning is that mechanism. The consumer has two options: they can decide that the buck stops at them and they intent to shield their consumers from any breaking changes. The other option is that they forward that suppression to their callers by marking themselves with the same attribute.

It's not quite true that we intent to have a member-level opt-in, that's why the diagnostic ID is required. The intent is that string can be shared across many types/members so that they can all be turned on/off as a single step.

Does this make sense?

@jskeet
Copy link

jskeet commented Mar 16, 2023

I think I expressed myself poorly in the previous comment. Let me try again :)
The attribute is API-surface-oriented, rather than behaviorally-oriented. There may well be many features which you'd like users to be able to try out which don't involve any API changes. Examples might be:

These could use this attribute via an API surface addition of "EnableXyzFeature" somewhere - but that would be a bit clunky.

I suspect that AppContext.SetSwitch is a better solution for things like that. And that's fine - I'm just suggesting that if that is indeed the recommendation, it would be worth mentioning in the same documentation that the experiment attribute is described.

Does that make more sense? Happy to hop on a call if that would help more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.