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

Make it possible to specify default enablement and severity of Rules (based on SdkAnalysisLevel) #9822

Closed
JanKrivanek opened this issue Mar 5, 2024 · 1 comment · Fixed by #10033

Comments

@JanKrivanek
Copy link
Member

JanKrivanek commented Mar 5, 2024

Background

Currently the rules have fixed defaults - overridable by customer in editorconfig.
The idea of 'WarningWaves' is to slowly enable and increase severity of specific rules - to allow users to onboard with lower noise and over time get more strictness.
We should not increase number of rules and their severity simply with new versions of MSBuild - that would break VS upgrading scenarios. Customers with unchanged projects/solutions should get identical reports regardless of MSBuild version they are using,
however they should get more rules and more strictness based on some explicit gesture of theirs. Increasing the version in TFM feels like a good candidate. SdkAnalysisLevel was designed for this specifically.

Goal

  • Study the design doc - Add a design for SdkAnalysisLevel designs#308 and propose the solution.
  • Built-in analyzers are priority in this, as acquired analyzers [nuget/project/binary] are imported already based on an explicit gesture.
  • But if we design/code this so that type of analyzer (built-in vs acquired) doesn't matter - we can expose the functionality to both types.

Gotchas

  • For Visual Studio (so FullFW MSBuild) we very likely do not want to introduce breaking behavior without code change on user side
  • Different behavior (and even build succeeding vs failing) on commandline vs in VS might not be perceived good
  • The TFM is not know untill certain point in evaluation - so we might need buffer some potential input into analyzers or vice versa send all input unrestricted, but buffer output untill we are sure what the right configuration should be.
  • Similarly with $SdkAnalysisLevel as it is an MSBuild property. This is problematic - as until evaluation starts we would not know whether the enalyzer will be enabled or not - and some analyzers will need to request some extra data

Related

@ladipro
Copy link
Member

ladipro commented Apr 15, 2024

Edit: We discussed offline and this is not relevant anymore.

@JanKrivanek and @baronfel, for the disconnect between .NET SDK and VS, how would you feel about honoring the SdkAnalysisLevel property globally, so even in builds that don't use the .NET SDK?

- Option 1: We never enable any analyzers for projects that don't use the .NET SDK or are not SDK-style at all. Users have to explicitly opt in with .editorconfig.
- Option 2: We define the enabled-by-default set of analyzers for some of the most widely used non-SDK projects such as classic .csproj and .vbproj. We evolve this independently from SdkAnalysisLevel and use a different prop for it.
- Option 3: Like 2 but we will call it SdkAnalysisLevel everywhere.

Simple to dismiss with Option 1 but it seems unfortunate to not at least try to innovate outside of the .NET SDK and bring the benefits to many more users.

ladipro added a commit that referenced this issue May 9, 2024
Fixes #9822
Fixes #9723

### Context

Some of the built-in BuildCheck analyzers will eventually be enabled by default. We need to figure out the mechanics of how it's going to happen.

### Changes Made

Updated parts of the BuildCheck spec documents.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants