Support Url and Message parameters in RequiresPreviewFeaturesAttribute

[pgovind]

Customer Impact

Early usage of the RequiresPreviewFeaturesAttribute and the Preview Feature analyzer (CA2252) surfaced the desire for custom messages and Urls to be supported by the attribute and analyzer. This will allow preview features (such as Kestrel's Http3 support) to surface custom diagnostic messages with feature-specific Urls that guide users to learn more about those preview features.

dotnet/runtime#56938 added the Message and URL properties to RequiresPreviewFeaturesAttribute. This PR updates the RequiresPreviewFeaturesAnalyzer to support the new properties in the message diagnostic. Including this change in 6.0 GA will improve the initial customer experience of using preview features in .NET 6.0 by providing tailored messages and improved discoverability of documentation for our preview features.

For illustration, here's the current message for using HTTP/3 in Kestrel (without this change):

Using 'UseQuic' requires opting into preview features. See https://aka.ms/dotnet-warnings/preview-features for more information.

With this change, the emitted diagnostic message will be:

Kestrel HTTP/3 support for .NET 6 is in preview. Using 'UseQuic' requires opting into preview features. See https://aka.ms/aspnet/kestrel/http3reqs for more information.

This was originally requested in dotnet/runtime#56498 by @JamesNK. If approved for 6.0 GA, we will also update all Generic Math APIs to utilize the Message and URL such that those messages are shown as:

Generic Math is in preview. Using 'IAdditionOperators' requires opting into preview features. See https://aka.ms/dotnet-warnings/generic-math-preview for more information.

Testing

Unit tests were updated to validate that the diagnostics are reported using the custom properties when they are present. Manual testing was also conducted with a local build of the analyzer, validating against our existing preview attributes.

Risk

Moderate, but mitigated. This functionality is landing very late in the release, but that was anticipated when we received the suggestion for adding the custom properties to the attribute. Because of this timing, the PR takes a low-churn approach to the implementation. In theory, if we wanted to be more performant, we could fold the new ConcurrentDictionary into the existing SymbolIsAnnotatedAsPreview call. That would however mean changes to each caller and it would quickly mean touching every routine in the analyzer. Instead, we decided to just look for the Url and Message property once per symbol just before we report a diagnostic.

[pgovind]

The new Message and Url properties on RequiresPreviewFeatureAttribute shipped in RC1

[buyaa-n]

This method is only called for OperatingSystem.IsXYZ() methods, the guard attributes added into OperatingSystem.IsXYZ() methods will not have a version, instead its version should be set the same as the version of the OperatingSystem.IsXYZVersionAtLeast(int,int,int,int) parameter. Therefore added an overload for HasAnyGuardAttribute(...) method which parses the attributes, the functionality is same as original method, just version is passed from OperatingSystem.IsXYZVersionAtLeast(int,int,int,int) parameter

[buyaa-n]

if we wanted to be uber performant, I could fold the new ConcurrentDictionary into the existing SymbolIsAnnotatedAsPreview call. That would however mean changes to each caller and it would quickly mean touching every routine in the analyzer. Instead, I decided to just look for the Url and Message property once per symbol just before we report a diagnostic.

It seems to me you can use only one existing ConcurrentDictionary and keep all info (bool, url, message) together, by setting them once in SymbolIsAnnotatedAsPreview, doesn't seem need much change, but this is up to you.

I have more stronger feeling for manual message concatenation, better to add placeholder for message and url for each resource string, this is not optional comment 😉

And not sure if you are aware, somehow the build is still failing

[buyaa-n]

Left a few NIT comments, overall LGTM

[pgovind]

Thanks for the stamp Buyaa. I'm just waiting for a review from one of the analyzer folks to address all the feedback in 1 go and merge.

[jmarolf]

Why are we mixing diagnostic styles here?

either we don't need the {||} parse holes in csCurrentAssemblyCode or we don't need to manually specify the diagnostics here right?

[pgovind]

Why are we mising diagnostic styles here?

What do diagnostic styles mean? I don't know that term :/

Hmm, not sure I completely understand, but the analyzer reports multiple diagnostics, so I need to use both the {| |} syntax and add the diagnostics explicitly in the ExpectedDiagnostics.Add method right? Is there another way?

[jmarolf]

but the analyzer reports multiple diagnostics

But the additional diagnostics have locations right? So you can do {|{||}|} to nest diagnostics whose locations overlap. I would prefer we use the brackets syntax as its easier to visually understand where the diagnostics are appearing.

[pgovind]

But the additional diagnostics have locations right?

Wait maybe I wasn't being clear. I meant that DetectPreviewFeaturesAnalyzer can report ~17 different diagnostics, not that it's reporting the same diagnostic in multiple places. So the only option in the unit tests is to use both the {| |} syntax and the ExpectedDiagnostics.Add method IIRC

[buyaa-n]

So you can do {|{||}|} to nest diagnostics whose locations overlap.

@jmarolf I might be missing something, but i don't see such overlaps in the diagnostics

[jmarolf]

Have a few things that need fixing and then we are good to go. I also don't remember why we specify the diagnostic locations in the test twice using two separate approaches

[jeffhandley]

I pushed commits to remove the quotes around the Urls and to improve a little formatting/indentation.

[jeffhandley]

This was approved in tactics for 6.0 GA.

[buyaa-n]

This was approved in tactics for 6.0 GA.

The PR got a conflict after @tannergooding's PR merged , resolved the conflict and gonna enable automerge