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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 123 additions & 0 deletions documentation/specs/proposed/BuildCheck-Rules-Identification.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
# 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.
Copy link
Member

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?


### Problems to address:
- The report should unambiguously point to the rule.
Copy link
Member

Choose a reason for hiding this comment

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

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.

- Execution and configuration issues and execution time reporting for analyzers.
Copy link
Member

Choose a reason for hiding this comment

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

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.

- Preventing clashes of identification within a single build and clashes of custom rules/analyzers with well-known rules/analyzers.
- Possibility to configure the rule.

## 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
Copy link
Member

Choose a reason for hiding this comment

The 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".

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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 will consist 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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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\.
MSBUILD : error : BuildCheck.SharedOutputPath.BC0002: Projects FooBar-Copy.csproj and FooBar.csproj have conflicting output paths: C:\projects\msbuild\playground\buildcheck\bin\Debug\net8.0\.
MSBUILD : error : BuildCheck.SharedOutputPath.BC0002: Projects FooBar-Copy.csproj and FooBar.csproj have conflicting output paths: C:\projects\msbuild\playground\buildcheck\obj\Debug\net8.0\.

Restore:
...
```

### Rules Identification clash prevention

#### Custom VS Built-In
The prevention of having the same analyzer/rule's name/id's between built-in and custom is guaranteed by preserved prefixes
- Name Prefix: (BuildCheck|MSBuild|Microsoft)
Copy link
Member

Choose a reason for hiding this comment

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

Microsoft is interesting here as there will surely be many other parts of Microsoft besides the MSBuild team that are interested in authoring rules.

Copy link
Member

Choose a reason for hiding this comment

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

Even our team might want to possibly ship rules out of the band (orthogonal to official releases - to test-drive new checks).

I'm wondering what might be a good way out of a 3rd party pretending to be MSFT authored check, while allowing Microsoft teams (including us) to ship the checks via nugets as well... I suppose one thing we can rely on is the package name (as trustworthy feeds should already employ reserving the prefixes) - so we might allow the reserved Check prefix, only if it comes from package that has same prefix (we'd need to make sure to reserve the prefixes on nuget.org)

Copy link
Member

Choose a reason for hiding this comment

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

Similar can apply to RuleId prefixes as well - basically "Only if you are build-in Check, or a Check comming from a package with some hardcoded recognized prefix (can be the 'Microsoft', 'BuildCheck', 'MSBuild' group) you can use any Check FriendlyName and RuleId prefixes you want, otherwise exclusions apply".

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Microsoft* is reserved on NuGet but I don't like the idea of coupling something in the rule engine (name validity) to NuGet in that way (I can't see a way of doing it other than "look in the path for a nuget package id pattern).

Copy link
Member

Choose a reason for hiding this comment

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

Let's try to brainstorm options:

  • Do not allow reserved prefixes in any custom analyzers (incl internaly produced)
  • No checking of restricted prefixes
  • Pigyback on nuget naming
  • ?? (need ideas here :-))

- Id Prefix: (BC|MSB|MS)
Copy link
Member

Choose a reason for hiding this comment

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

I'd add some other known-to-the-ecosystem prefixes

Suggested change
- Id Prefix: (BC|MSB|MS)
- Id Prefix: (BC|MSB|MS|CS|CA|VB)

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.
Copy link
Member

Choose a reason for hiding this comment

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

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)



### 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`
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I think

build_check.MSBuild.SharedOutputPath.enabled = true

makes sense to me

Copy link
Member

Choose a reason for hiding this comment

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

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

- `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
Copy link
Member

Choose a reason for hiding this comment

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

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

- `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 examples:

```
root=true

[FooBar.csproj]
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

The 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 [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.

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

```
root=true

[FooBar.csproj]
build_check.BuildCheck.SharedOutputPath.IsEnabled=true
build_check.BuildCheck.SharedOutputPath.Severity=error

[FooBar-Copy.csproj]
build_check.BuildCheck.SharedOutputPath.IsEnabled=true
build_check.BuildCheck.SharedOutputPath.Severity=error
```

#### Priority of configuration

- Rule
- Analyzer
Comment on lines +122 to +123
Copy link
Member

Choose a reason for hiding this comment

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

Elaborate on this please?