[BuildCheck]: Guide of rules/analyzers id name #10088

f-alizada · 2024-04-30T09:35:52Z

Context

Provide the document describing the guidence on the rule id and analyzers name built in and custom ones.

Formatted view: https://github.com/f-alizada/msbuild/blob/f-alizada/dev/design-for-rules-id/documentation/specs/proposed/BuildCheck-Rules-Identification.md

The PR does not introduce the actual implementation. The implementation and changes will be made once this PR is merged (meaning all comments are addressed)

JanKrivanek

I love the writing - thanks for putting this together!

Not yet approving - as there will be probably some followup discussions... but I feel pretty good about the proposal!

JanKrivanek · 2024-04-30T12:02:00Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+
+### Problems to address:
+- The report should unambiguously point to the rule.
+- Errors and execution time reporting for analyzers.


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"?

JanKrivanek · 2024-04-30T12:06:50Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+### Problems to address:
+- The report should unambiguously point to the rule.
+- Errors and execution time reporting for analyzers.
+- Preventing clashes of identification within a single build: Clashes with well-known rules/analyzers.


Suggested change

- Preventing clashes of identification within a single build: Clashes with well-known rules/analyzers.

- Preventing clashes of identification within a single build and clashes of custom rules/analyzers with well-known rules/analyzers.

We basically want to prevent 2 things:

a community analyzer/rule producing reports that can be confused with the reports from official MSFT analyzers

some community analyzers having same id's and those being loaded into the build produces report that uses identification, while that identification cannot be unambiguously linked to the analyzer package.

To prevent first case - we reserve prefix for ourselves. Thus it cannot be used in any analyzer in any build
To prevent second case - we need to control clashes on the level of the individual builds - as we do not have mechanism for producers to reserve their own prefixes

Thank you for pointing!
The section "Rules Identification clash prevention" is added to the document.

JanKrivanek · 2024-04-30T12:11:11Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+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`


"build_check.BuildCheck..." - feels like duplication. "build_check.Microsoft...." might be more expressive here?

Agree it feels like it.
What do you think about the "msbuild_analyzer.BuildCheck.AnalyzerName ... " where msbuild_analyzer is a required prefix to be respected by infra?

Additional option:
To leave the build_check as a infra-key, but reserve the MSBuild as a reserved prefix for built-in analyzers.

I think

build_check.MSBuild.SharedOutputPath.enabled = true

makes sense to me

Agree with Rainer here - having the build_check 'namespace' is a good thing (see how language or ecosystem prefixes are used to scope rules for other systems), I just worry a bit about verbosity here. I'd want ideally no more than say 4 segments. <namespace>.<analyzer_id>.<rule_id>.<setting> = <value>.

JanKrivanek · 2024-04-30T12:16:02Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+- Name: `BuildCheck.SharedOutputPath`
+- RuleId: `BC0101` or `BC.AdditionalInfo0101` or `BC.Prefix.Test0123`
+
+### Custom analyzers


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
FancyBuildChecks.*.enabled = true

But I haven't thought about that too deeply :)

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.

The idea is interesting, however the document presents the idea of having less flexibility at early stages of the development.
On later stages we could work on providing more flexibility into configuraition, naming.
Having the configuration based on the prefix is something that I would leave for the future, if agreed across team.
Open to the discussion

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.

JanKrivanek · 2024-04-30T12:18:15Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+...
+```
+
+### Categorization


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)
But probably - if authors can choose their categorization identifiaction freely - both effects can be eachieved?

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.)

I was trying to come up with reasonable set of Categories for the intial list of analyzers (except usage, style), however I believe having only one Category is not sufficient to actually show what the analyzer is about.
Attaching multiple catogories (labeling) possibly could help here, but was not thinking of adding it in the v1.
What do you think about not introducing the Categorization in the v1 of BuildCheck?

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.

documentation/specs/proposed/BuildCheck-Rules-Identification.md

maridematte · 2024-05-02T12:09:30Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+...
+```
+
+### Categorization


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.

Left comment regarding the same topic: #10088 (comment)

maridematte · 2024-05-02T12:11:54Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+- Name: `BuildCheck.SharedOutputPath`
+- RuleId: `BC0101` or `BC.AdditionalInfo0101` or `BC.Prefix.Test0123`
+
+### Custom analyzers


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.

rainersigwald · 2024-05-06T14:24:38Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+
+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.


Maybe say "method" or "assembly" or "class" or something?

rainersigwald · 2024-05-06T14:24:49Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+Note: Single analyzer can have multiple rules.
+
+### Problems to address:
+- The report should unambiguously point to the rule.


I'd strongly recommend using the RFC 2119 language:

Suggested change

- The report should unambiguously point to the rule.

- The report must unambiguously identify the rule.

rainersigwald · 2024-05-06T14:27:14Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+
+### Problems to address:
+- The report should unambiguously point to the rule.
+- Execution and configuration issues and execution time reporting for analyzers.


Does this mean something like this, or something else?

Suggested change

- Execution and configuration issues and execution time reporting for analyzers.

- Execution and configuration issues and execution time reporting must be attributable to analyzers.

rainersigwald · 2024-05-06T14:41:32Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+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,}$`.


I'd appreciate more reasoning and description of the concepts "friendly name" and "RuleId".

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

rainersigwald · 2024-05-06T14:49:19Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+- Name: `BuildCheck.SharedOutputPath`
+- RuleId: `BC0101` or `BC.AdditionalInfo0101` or `BC.Prefix.Test0123`
+
+### Custom analyzers


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.

rainersigwald · 2024-05-06T16:04:47Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+If custom analyzer will not meet predefined pattern the registration of the custom analyzer will fail.
+
+#### Custom VS Custom
+The prevention of having the same analyzer/rule's name/id's between custom analyzers is not guaranteed, and during the registration of the custom analyzer, an additional check will happen to ensure that the analyzer name is not already registered.


Is the only recourse here to drop one or the other? No remap capability in the engine (I don't think there should be! but we should be clear)

rainersigwald · 2024-05-06T16:05:47Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+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`


I think

build_check.MSBuild.SharedOutputPath.enabled = true

makes sense to me

rainersigwald · 2024-05-06T16:06:46Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+ - `build_check.SharedOutputPath.ID0001.enabled = true|false`
+ - `build_check.SharedOutputPath.ID0001.severity = Error`


I find using SharedOutputPath as your example custom rule a bit confusing, can you make that more distinct? CustomThirdParty or something?

rainersigwald · 2024-05-06T16:07:45Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+- Rule
+- Analyzer


Elaborate on this please?

rainersigwald · 2024-05-06T16:08:19Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+```
+root=true
+
+[FooBar.csproj]


Did we discuss this in the .editorconfig meeting? Do we generally expect people to configure per-project by adding a project-folder .editorconfig or by adding a section like this?

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 [FooBar.csproj] section header. So you can organize in a single file with N project sections, or N files with 1 project section, or any combination thereof.

baronfel · 2024-05-10T18:34:00Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+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`


Agree with Rainer here - having the build_check 'namespace' is a good thing (see how language or ecosystem prefixes are used to scope rules for other systems), I just worry a bit about verbosity here. I'd want ideally no more than say 4 segments. <namespace>.<analyzer_id>.<rule_id>.<setting> = <value>.

baronfel · 2024-05-10T18:35:49Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+```
+root=true
+
+[FooBar.csproj]


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 [FooBar.csproj] section header. So you can organize in a single file with N project sections, or N files with 1 project section, or any combination thereof.

baronfel · 2024-05-10T18:36:56Z

documentation/specs/proposed/BuildCheck-Rules-Identification.md

+- Name: `BuildCheck.SharedOutputPath`
+- RuleId: `BC0101` or `BC.AdditionalInfo0101` or `BC.Prefix.Test0123`
+
+### Custom analyzers


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.

The document to provide guide of rules/analyzers id name

1c94207

JanKrivanek reviewed Apr 30, 2024

View reviewed changes

maridematte reviewed May 2, 2024

View reviewed changes

Farhad Alizada added 5 commits May 6, 2024 08:25

Merge branch 'main' into f-alizada/dev/design-for-rules-id

51e0068

Apply suggestions from PR reviews

754c21c

Add section of same analyzer name prevention

3ab0327

Remove cagory section for now

17b0d9c

rephrase

2c73ed0

f-alizada requested review from JanKrivanek and maridematte May 6, 2024 12:24

rainersigwald reviewed May 6, 2024

View reviewed changes

JanKrivanek requested a review from baronfel May 9, 2024 14:07

baronfel reviewed May 10, 2024

View reviewed changes

f-alizada changed the title ~~The document to provide guide of rules/analyzers id name~~ [BuildCheck]: Guide of rules/analyzers id name May 17, 2024

	- Preventing clashes of identification within a single build: Clashes with well-known rules/analyzers.
	- Preventing clashes of identification within a single build and clashes of custom rules/analyzers with well-known rules/analyzers.


		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.

	- The report should unambiguously point to the rule.
	- The report must unambiguously identify the rule.

	- Execution and configuration issues and execution time reporting for analyzers.
	- Execution and configuration issues and execution time reporting must be attributable to analyzers.

		- `build_check.SharedOutputPath.ID0001.enabled = true\|false`
		- `build_check.SharedOutputPath.ID0001.severity = Error`

[BuildCheck]: Guide of rules/analyzers id name #10088

Are you sure you want to change the base?

[BuildCheck]: Guide of rules/analyzers id name #10088

Conversation

f-alizada commented Apr 30, 2024

Context

JanKrivanek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment