-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[BuildCheck]: Guide of rules/analyzers id name #10088
base: main
Are you sure you want to change the base?
Changes from 1 commit
1c94207
51e0068
754c21c
3ab0327
17b0d9c
2c73ed0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,113 @@ | ||||||||||
# MSBuild Rules/Analyzers Identification | ||||||||||
|
||||||||||
## Background and Context | ||||||||||
|
||||||||||
The MSBuild team is currently working on delivering the MSBuild analyzers (aka BuildCheck). The BuildCheck infrastructure has built-in analyzers and functionality to support custom ones. Hence, we need to make sure it will be possible to configure and differentiate built-in and custom analyzers. | ||||||||||
|
||||||||||
Note: Single analyzer can have multiple rules. | ||||||||||
|
||||||||||
### Problems to address: | ||||||||||
- The report should unambiguously point to the rule. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd strongly recommend using the RFC 2119 language:
Suggested change
|
||||||||||
- Errors and execution time reporting for analyzers. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering whether the term 'error' might confuse anyone here (as we use term error as well for the type of finding). How about "Execution and configuration issues and execution time reporting for analyzers"? |
||||||||||
- Preventing clashes of identification within a single build: Clashes with well-known rules/analyzers. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We basically want to prevent 2 things:
To prevent first case - we reserve prefix for ourselves. Thus it cannot be used in any analyzer in any build There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for pointing! |
||||||||||
- Possibility to configure the rule. | ||||||||||
- Categorization of the rules/analyzers. | ||||||||||
|
||||||||||
## Proposal | ||||||||||
|
||||||||||
### Built-in analyzers | ||||||||||
Every built-in analyzer will have the friendly name: `BuildCheck.{FriendlyName}`. | ||||||||||
- Regular expression for the name: `^BuildCheck.[A-Z]{1,}[a-zA-Z0-9_]{0,}$` | ||||||||||
|
||||||||||
Each Rule that is shipped inbox will contain the RuleId as an identifier of the rule for this analyzer. | ||||||||||
- The rule id format is as follows: `^BC[A-Za-z_.]{0,}[0-9]{1,}$`. | ||||||||||
Comment on lines
+18
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd appreciate more reasoning and description of the concepts "friendly name" and "RuleId". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw. do we need a more strict and opinionated guidance on form and especially length of the ids? @YuliiaKovalova - can you point us to contacts for VS Error Window? - as that team might have some pre-existing guidances |
||||||||||
|
||||||||||
#### Example of a built-in analyzer: | ||||||||||
- Name: `BuildCheck.SharedOutputPath` | ||||||||||
- RuleId: `BC0101` or `BC.AdditionalInfo0101` or `BC.Prefix.Test0123` | ||||||||||
|
||||||||||
### Custom analyzers | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wild idea - do we want to allow prefixes for the custom analyzer rules as well? And then being able to refer to that prefix in order to configure the rules by group - e.g.: FancyBuildChecks.SharedOutputPath.enabled = false But I haven't thought about that too deeply :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this idea, but my worry here is when two third party analyzers use the same prefix. The same problem that we would have with the rule IDs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea is interesting, however the document presents the idea of having less flexibility at early stages of the development. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FancyBuildChecks.*.enabled = true This is dangerous, because listing by wildcard means new rules can get enabled/fail your build by nonobvious upgrades. Less dangerous for package-delivered ones since those should require an explicit upgrade gesture. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is the naming difference between built-in and custom checks coming from? As far as I know Roslyn doesn't have a similar concept here. |
||||||||||
Custom analyzer will have the friendly name: `{NameOfTheAnalyzer}` with defined format: | ||||||||||
- `^[A-Z]{1,}[a-zA-Z_]{1,}$` | ||||||||||
- should not start with `BuildCheck.` this is built-in prefix for built-in. | ||||||||||
|
||||||||||
Each Custom Analyzer Rule will have the rule id format as follows: | ||||||||||
- `^[A-Z]{1}[A-Za-z]{0,}[0-9]{1,}$`. | ||||||||||
- should not start from `BC.` this is reserved prefix for built-in rules. | ||||||||||
|
||||||||||
#### Example of a custom analyzer: | ||||||||||
- Name: `SharedOutputPath`, `SharedOutputPath` | ||||||||||
- RuleId: `SOMEPREFIX123` | ||||||||||
|
||||||||||
Any registered analyzers that don't follow the pattern (built-in and custom) will raise an exception and fail the build. | ||||||||||
|
||||||||||
The identification of the rule consists of two components: the Friendlyname and the RuleId. | ||||||||||
|
||||||||||
#### Examples | ||||||||||
- Built-in | ||||||||||
- `BuildCheck.SharedOutputPath.BC0001` | ||||||||||
- `BuildCheck.SharedOutputPath.BC0002` | ||||||||||
- `BuildCheck.PropertyAssignedIncorrectly.BC0002` | ||||||||||
- Custom | ||||||||||
- `SharedOutputPath.ID0001` | ||||||||||
- `SharedOutputPath.ID0002` | ||||||||||
- `PropertyAssignedIncorrectly.ID0002` | ||||||||||
|
||||||||||
#### Example of the output: | ||||||||||
``` | ||||||||||
... | ||||||||||
Determining projects to restore... | ||||||||||
MSBUILD : error : BuildCheck.SharedOutputPath.BC0002: Projects FooBar-Copy.csproj and FooBar.csproj have onflicting output paths: C:\projects\msbuild\playground\buildcheck\bin\Debug\net8.0\. | ||||||||||
MSBUILD : error : BuildCheck.SharedOutputPath.BC0002: Projects FooBar-Copy.csproj and FooBar.csproj have onflicting output paths: C:\projects\msbuild\playground\buildcheck\obj\Debug\net8.0\. | ||||||||||
Comment on lines
+59
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
Restore: | ||||||||||
... | ||||||||||
``` | ||||||||||
|
||||||||||
### Categorization | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is something where I'd be curious about wider feedback. I'm not sure if personally I'd prefer groupping by categories (accross all rules) or rather by producers (package names) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The categorization of the analyzers (all possible categories) will be presented by the MSBuild team. This means that it will be represented as enum values, and the identification of the categorization will be maintained by the MSBuild team. Could you please point out the scenario that you are thinking about which is not very conveniently configurable from the document? (Possibly I'm missing something.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was trying to come up with reasonable set of Categories for the intial list of analyzers (except There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like categories in general, because of the "I turned on 'all style checks' and then changed to a new compiler and am now mad because there are new style warnings" case. I support dropping them from v1. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The categories section feels a little bit out of place right now. I had a bit of trouble understanding how it fit with the analyzer identification topic. I think expanding it a bit would be nice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Left comment regarding the same topic: #10088 (comment) |
||||||||||
The proposal is to implement the configuration of the class/category of analyzers by single line in .editorconfig | ||||||||||
For example, categories can include: | ||||||||||
- Output | ||||||||||
- MSBuildProperty | ||||||||||
|
||||||||||
Category configuration's priority is higher than that of a singular rule or analyzer. | ||||||||||
f-alizada marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
Example configuration for a category rule: | ||||||||||
- build_check.Category.Output.enabled = true|false | ||||||||||
- build_check.Category.MSBuildProperty.severity = Error | ||||||||||
|
||||||||||
#### Priority of configuration | ||||||||||
|
||||||||||
- Category | ||||||||||
- Analyzer | ||||||||||
- Rule | ||||||||||
|
||||||||||
|
||||||||||
### EditorConfig configurations | ||||||||||
|
||||||||||
Any build check related configuration should start with the `build_check.` prefix. Without the prefix, the BuildCheck infrastructure will not recognize the input config as a valid key-value, and the config will not be respected. | ||||||||||
|
||||||||||
- Built-in BuildCheck rule configuration | ||||||||||
- `build_check.BuildCheck.SharedOutputPath.BC0001.enabled = true|false` | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "build_check.BuildCheck..." - feels like duplication. "build_check.Microsoft...." might be more expressive here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree it feels like it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Additional option: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think build_check.MSBuild.SharedOutputPath.enabled = true makes sense to me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with Rainer here - having the |
||||||||||
- `build_check.BuildCheck.SharedOutputPath.BC0001.severity = Error` | ||||||||||
|
||||||||||
- Custom BuildCheck rules configuration | ||||||||||
- `build_check.SharedOutputPath.ID0001.enabled = true|false` | ||||||||||
- `build_check.SharedOutputPath.ID0001.severity = Error` | ||||||||||
Comment on lines
+86
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find using |
||||||||||
- `build_check.SharedOutputPathSecond.AnotherRuleId0001.enabled = true|false` | ||||||||||
- `build_check.SharedOutputPathSecond.AnotherRuleId0001.severity = Error` | ||||||||||
|
||||||||||
- To configure the analyzer (Priority of this is higher than configuring the single rule) | ||||||||||
- `build_check.SharedOutputPath.enabled = true|false` | ||||||||||
|
||||||||||
.editorconfig example: | ||||||||||
|
||||||||||
``` | ||||||||||
root=true | ||||||||||
|
||||||||||
[FooBar.csproj] | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did we discuss this in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It shouldn't matter - due to the hierarchical nature and the fact that even in a co-located .editorconfig the user would still need the |
||||||||||
build_check.BuildCheck.SharedOutputPath.BC0002.IsEnabled=true | ||||||||||
build_check.BuildCheck.SharedOutputPath.BC0002.Severity=error | ||||||||||
|
||||||||||
[FooBar-Copy.csproj] | ||||||||||
build_check.BuildCheck.SharedOutputPath.BC0002.IsEnabled=true | ||||||||||
build_check.BuildCheck.SharedOutputPath.BC0002.Severity=error | ||||||||||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe say "method" or "assembly" or "class" or something?