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

Mark CA2252 as Error severity #5637

Merged
merged 1 commit into from
Oct 15, 2021
Merged

Conversation

jeffhandley
Copy link
Member

@jeffhandley jeffhandley commented Oct 14, 2021

Fixes #5636 by setting CA2252 to Error severity.

Customer Impact

We intended for the CA2252 Preview Feature analyzer to produce build errors by default, forcing customers to opt into using preview features. We had validated the experience across our own repos and other existing projects, and that error behavior was experienced. In new projects however, the analyzer was being treated as info-level by default.

The net result is that using APIs marked with [RequiresPreviewFeatures] was not yielding build errors by default, even if the customer had not opted in. This means customers are able to unknowingly use preview features. This is a blocker for 6.0 GA that needs to be fixed.

Testing

This issue had been missed because we had not conducted end-to-end testing of this analyzer through the .NET CLI or in Visual Studio while working in a brand new project. Instead, all prior testing had been conducted while working in projects that already had the projects configured such that this analyzer's diagnostics would be reported as errors. Therefore the behavior of the analyzer itself had been validated, but its default analysis level of Error had not been validated properly.

Using a private build of the analyzers package, we've now validated that the analyzer has the Error severity as the effective default. This has been validated using both command-line and Visual Studio.

Mitigation of Similar Oversights

Risks

This will surface as a build breaking change for someone who consumed preview APIs in Preview 7, RC1, and RC2 in a new project where they had not opted into using preview features. This is very unfortunate, but it's a necessary effect to ensure we have the expected experience in 6.0 GA.

The following APIs are marked as preview:

  • All APIs related to Generic Math exposed through the System.Runtime.Experimental package
  • System.Runtime.Intrinsics.X86.AvxVnni
  • Microsoft.AspNetCore.Server.Kestrel.Core.Http3
  • Microsoft.AspNetCore.Server.Kestrel.Core.Http1AndHttp2AndHttp3
  • Microsoft.AspNetCore.Hosting.WebHostBuilderQuicExtensions.UseQuic

@jeffhandley jeffhandley requested a review from a team as a code owner October 14, 2021 17:50
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

I manually built a NuGet package locally with this change and can confirm that installing it directly to the project produces CA2252 error and the generated AnalysisLevel_default_6.editorconfig also no longer has the suggestion severity entry.

@jeffhandley
Copy link
Member Author

I will update the PR description to fill out the template for tactics approval, but I won't be able to do so for a couple of hours.

@codecov
Copy link

codecov bot commented Oct 14, 2021

Codecov Report

Merging #5637 (7eb4200) into release/6.0.1xx (49576a0) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #5637      +/-   ##
===================================================
- Coverage            95.55%   95.54%   -0.01%     
===================================================
  Files                 1275     1275              
  Lines               292003   292003              
  Branches             17622    17622              
===================================================
- Hits                279009   279001       -8     
- Misses               10608    10617       +9     
+ Partials              2386     2385       -1     

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.

4 participants