Add [RequiresPreviewFeaturesAttribute] to Http3

[wtgodbe]

Fixes #34738. Confirmed in ILSpy that the attribute is present on the 2 enum values.

[Tratcher]

How does this show up in the sample app, as a warning?

aspnetcore/src/Servers/Kestrel/samples/Http3SampleApp/Program.cs

Line 48 in eb0f414

listenOptions.Protocols = HttpProtocols.Http3;

Do we need a suppression there?

[wtgodbe]

How does this show up in the sample app, as a warning?

Unhandled exception. System.NotSupportedException: QUIC is not supported or enabled on this platform. See https://aka.ms/aspnet/kestrel/http3reqs for details.

So I guess we either need to suppress it, or have the sample require preview features as well?

[Tratcher]

That's a runtime error about your machine not supporting quic. The preview attribute should have given you a compile time warning/error?

[wtgodbe]

Nope, no warnings/errors on building the app

[Tratcher]

Hmm, then it's not working and we need to figure out why before merging it.

[wtgodbe]

@pgovind @jeffhandley @jkotas this PR adds the RequiresPreviewFeaturesAttribute to the enum values for the Http3 protocol, yet our sample using that protocol gives no warning:

aspnetcore/src/Servers/Kestrel/samples/Http3SampleApp/Program.cs

Line 48 in eb0f414

listenOptions.Protocols = HttpProtocols.Http3;

This is surprising, are we doing something wrong?

[jeffhandley]

The analyzer is currently set to produce Info level diagnostics. We are still refining the implementation ahead of increasing it to Warning/Error. Can you check to see if there was an Info message produced? If you are getting those, then you could proceed in putting your suppression in ahead of us increasing the level.

https://github.com/dotnet/roslyn-analyzers/blob/release/6.0.1xx/src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md#:~:text=ca2252

[pgovind]

Can you check to see if there was an Info message produced? If you are getting those, then you could proceed in putting your suppression in ahead of us increasing the level.

Either this, or you can change the severity of the analyzer from Info to Error and see if VS reports errors. I'm almost certain it will. We have unit tests testing this exact scenario in the analyzer.

[wtgodbe]

Yeah, I do get Using 'Http3' requires opting into preview features. See https://aka.ms/dotnet-warnings/preview-features for more information. Adding a NoWarn suppresses it.

[JamesNK]

That looks like what we want.

https://aka.ms/dotnet-warnings/preview-features links to Bing? I'm guessing this will be fixed before .NET 6 RTMs.

@pgovind @jeffhandley @jkotas Is there a reason why there is no way to add a custom message with this attribute?

[jeffhandley]

We typically don't set up the aka.ms links until documentation is in place and published, which won't happen until RC1 ships. (aka.ms defaults to bing for undefined links).

I don't recall if the API Design Review discussion included potentially having a custom message. @pgovind / @terrajobst, was there a good reason not to?

[pgovind]

Is there a reason there is no way to add a custom message with this attribute?

Interesting. We hadn't considered that because we didn't have a use case for it. @terrajobst should decide if we want to enable it. I'll just point out that adding a message might mean changes in S.P.CoreLib, so we should decide soon if we want it for .NET6 (or we just add it as a new features for .NET 7)

[JamesNK]

For the context of HTTP/3, it would be nice to link to a doc about why the feature is preview, and how to manage using it, but it's not critical.

Its like the ObsoleteAttribute where you want to describe why something is obsolete and how to update your app.

In .NET 7 a constructor overload could be added to the attribute that takes a message. Give a chance to provide more detail to the user.

[JamesNK]

Created issue: dotnet/runtime#56498

[terrajobst]

I believe this was briefly discussed a few days ago. Yes, I believe the usage of [RequiresPreviewFeatures] is appropriate here and would communicate our intent (the capability to use Http3 is there, but it's not supported and only provided as a preview).

[terrajobst]

Clicked the wrong button because I'm a PM

[terrajobst]

If we do this, is there a reason why we wouldn't mark this API?

namespace System.Net
{
    public static class HttpVersion
    {
        [RequiresPreviewFeatures]
        public static readonly Version Version30;
    }
}

@dotnet/ncl

[JamesNK]

A client can upgrade to use HTTP/3 without it being configured using that setting.

For example, Version = 2.0 and RequestedVersionOrHigher will upgrade to HTTP/3 without someone seeing the preview warning. That's why a runtime appcontext switch is better for their situation.

[adityamandaleeka]

Nit: might be good to add something like "because HTTP/3 is currently in preview state" or something here, so the intent is clear to someone in the future.

[adityamandaleeka]

LGTM aside from the minor nit.