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

Setup default experimental URL #4130

Merged
merged 1 commit into from
Jun 30, 2023
Merged

Setup default experimental URL #4130

merged 1 commit into from
Jun 30, 2023

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Jun 28, 2023

null

Microsoft Reviewers: Open in CodeFlow

@ghost ghost assigned RussKie Jun 28, 2023
@RussKie RussKie requested review from joperezr and geeknoid June 28, 2023 07:04
@geeknoid
Copy link
Member

geeknoid commented Jun 28, 2023

Should we be using the same ID for everything? Or would it make sense to pick an ID per package perhaps? I wonder if some sort of a centralized inventory is needed in fact, like in eng/ExperimentalIds.props or something, so that the IDs can be stable over time and non-overlapping across packages? Or maybe that's overkill.

I don't believe in the need for IDs like this, but since they are there, I suppose we should use that as intended.

Maybe @terrajobst has some opinion on this?

@terrajobst
Copy link
Member

I'd say you want one ID per experimental feature. For example, generic math would use one ID that is applied to many APIs. The idea being that users can suppress usages on a per-feature level.

If all experimental APIs use the same diagnostic ID then it's just like obsolete used to be. Folks can only suppress on a per-call site basis (which is noisy) or at the global level which is akin to turning obsoletion warnings off. Neither is desirable.

@joperezr
Copy link
Member

I think the most important question here is what do we expect customers depending on these packages will want, and then optimize for that scenario. For instance, do we think it will be likely to have the case where consumers of 3 or more packages produced from this repo would be okay with using experimental APIs from package A, but not from B and C? IMO, the real value of this feature is that not every library author will have the same meaning of what is considered to be "experimental", but in the context of a single repo with the same maintainers, it is likely that the meaning of "experimental" would be the same. That means that I believe that consumers who are okay with suppressing the errors for experimental APIs in Microsoft.Extensions.Resilience, will very likely be okay with also suppressing the errors from package Microsoft.Extensions.Telemetry, but may have a different opinion about suppressing errors from package MyTotallyExperimentalPackage.nupkg. Based on this, I think it would be a bad experience to design a system where customers using 20 of our packages, require to list 20 diagnosticIDs on their project, and for this reason, we should probably come up with a different design.

My proposal would be to have n different diagnostic ids, where n represents the number of different definitions of "experimental" that we have within the repo. So we may have APIs that are Experimental, but are really something that we have thought about extensively and we are mainly just looking for customer validation for a year or so, but we are pretty confident they will end up shipping as they are. And on the other hand, we may have Experimental APIs that really are very early on their design phase, and will very likely change in the future. I propose that if we only have those two categories, then we should just have two diagnostic ids that are well documented, so customers could decide if they are okay using our APIs that are more "stable" than the others.

@RussKie RussKie force-pushed the experimental branch 2 times, most recently from a292c0c to 5db4057 Compare June 29, 2023 00:14
Comment on lines 78 to 84
<ItemGroup Condition="'$(Stage)' == 'dev' AND '$(OutputType)' != 'Exe' AND '$(Api)' != 'false'">
<AssemblyAttribute Include="System.Diagnostics.CodeAnalysis.ExperimentalAttribute">
<_Parameter1>TBD</_Parameter1>
<_Parameter2>UrlFormat = "https://aka.ms/dotnet-extensions-warnings/{0}"</_Parameter2>
<_Parameter2_IsLiteral>true</_Parameter2_IsLiteral>
</AssemblyAttribute>
</ItemGroup>
Copy link
Member Author

Choose a reason for hiding this comment

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

@geeknoid FYI this essentially will be a no-op as the compiler won't provide support the assembly level - as noted in dotnet/roslyn#68702.

Copy link
Member

Choose a reason for hiding this comment

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

@terrajobst Do you have a recommendation on how to deal with this?

Right now, in this repo you can indicate a package is in dev stage. This means the whole package is in development, every API it contains is inherently experimental and the package may in fact be removed. With the Experimental attribute not being enforced at the assembly level, it would require engineers to mark every single public type in the assembly as experimental in order to make sure customers don't take an unintended dependency on the API.

Is this right? Shouldn't experimental support assembly scope?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@terrajobst terrajobst Jul 17, 2023

Choose a reason for hiding this comment

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

This was discussed here.

Basically, this doesn't work for obsolete today and we concluded we'd rather want to align with that for now and revisit if this is identified as a limitation. When I spec'ed it, I did include module and assembly level because that's what we did for the preview feature and platform compat attributes before and it seemed convenient, pretty much for the reason you're spelling out.

I think I'd prefer if the feature allowed for assembly level, but we should probably enable this for obsoletion as well then for consistency.

@jaredpar @jcouv what do you guys think? Should I file a separate issue so we can discuss this? No idea what the cost of such a change would be.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please file an issue. We (compiler) generally prefer module-level over assembly-level, but I understand that multi-module scenario is not a scenario with Core... So that might need some discussion.

Copy link
Member

Choose a reason for hiding this comment

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

From discussion with Aleksey, supporting both assembly- and module-level would work and make sense for this attribute, without causing complications.

@RussKie RussKie changed the title Setup default experimental diagnostic ID along with URL for more info Setup default experimental URL Jun 29, 2023
@RussKie
Copy link
Member Author

RussKie commented Jun 29, 2023

I dropped the diagnostic ID, it remains at "TBD". This pull-request just sets the default URL.
Each feature team will need to define scenarios and define diagnostic IDs as necessary.

@RussKie
Copy link
Member Author

RussKie commented Jun 29, 2023

Also, we can't really test this until dotnet/roslyn#68702 is complete and flows through.

@RussKie RussKie enabled auto-merge (squash) June 30, 2023 00:44
@RussKie RussKie merged commit 12be3be into dotnet:main Jun 30, 2023
@ghost ghost added this to the 8.0 Preview7 milestone Jun 30, 2023
@RussKie RussKie deleted the experimental branch June 30, 2023 01:28
@terrajobst
Copy link
Member

terrajobst commented Jul 17, 2023

@joperezr

That means that I believe that consumers who are okay with suppressing the errors for experimental APIs in Microsoft.Extensions.Resilience, will very likely be okay with also suppressing the errors from package Microsoft.Extensions.Telemetry, but may have a different opinion about suppressing errors from package MyTotallyExperimentalPackage.nupkg

It's less about them having the same bar and more about how much debt the consumer is buying into.

Let's say I build an app that consumes resilience. There is a new cool integration with Polly v8 and it happens to be marked as preview. Let's say I've written a ton of integrations with previous versions of Polly and I've been keeping up with Polly v8's design so I'm comfortable taking a dependency on it now because I think I'll be able to absorb any changes easily and quickly. That doesn't mean I'm willing to take in breaking changes in telemetry because maybe that's a space I know less about and I also don't have a need for. Me turning on preview features for resilience shouldn't necessarily turn on preview features in, say, telemetry.

As a principle, as I user I don't want features where turning on an experimental or an obsoleted feature somewhere now results in me no longer having guard rails for unrelated experimental or obsoleted features, regardless of which party built them.

Does this make sense?

@ghost ghost locked as resolved and limited conversation to collaborators Aug 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants