Skip to content

Commit

Permalink
Fix the scope setting doc and guard (#10517)
Browse files Browse the repository at this point in the history
  • Loading branch information
JanKrivanek authored Aug 15, 2024
1 parent befaec7 commit f422d8d
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 25 deletions.
11 changes: 6 additions & 5 deletions documentation/specs/BuildCheck/BuildCheck.md
Original file line number Diff line number Diff line change
Expand Up @@ -205,19 +205,20 @@ build_check.BC0101.severity=warning

#### Scope of Check

Option `EvaluationCheckScope` with following possible options will be available:
Option `EvaluationCheckScope` (just `scope` in `.editorconfig`) with following possible options will be available:

| EvaluationCheckScope (Solution Explorer) | EditorConfig option | Behavior |
| EvaluationCheckScope (scope) | EditorConfig option | Behavior |
| ------------- | ------------- | ------------- |
| ProjectFileOnly | `project_file` | Only the data from currently checked project will be sent to the check. Imports will be discarded. |
| WorkTreeImports | `work_tree_imports` | Only the data from currently checked project and imports from files not recognized to be in nuget cache or SDK install folder will be sent to the check. Other imports will be discarded. |
| ProjectWithAllImports | `all` | All data will be sent to the check. |

All rules of a single check must have the `EvaluationCheckScope` configured to a same value. If any rule from the check have the value configured differently - a warning will be issued during the build and check will be deregistered.
Same rule can have `EvaluationCheckScope` configured to different values for different projects. If check has multiple rules (this is e.g. case of PropertyUsageCheck rules - [BC0201](Codes.md#bc0201---usage-of-undefined-property), [BC0202](Codes.md#bc0202---property-first-declared-after-it-was-used) and [BC0203](Codes.md#bc0203----property-declared-but-never-used)) - those can have the `EvaluationCheckScope` set to distinct values.

Same rule can have `EvaluationCheckScope` configured to different values for different projects.
Currently the proper filtering of data is at the discretion of the Check - as the infrastructure might not be able to decide what can be considered in scope (e.g. in case of [BC0203](Codes.md#bc0203----property-declared-but-never-used) - "_Property declared, but never used_" - the property writes (definitions) are scoped, but reads (usages) are not, while [BC0201](Codes.md#bc0201---usage-of-undefined-property) "_Usage of undefined property_" needs to scope reads, but not writes (definitions). Identical input data need to be scoped differently based on the meaning of the Check).

Some checks might completely ignore the `EvaluationCheckScope` setting - as they can operate on data, that are sourced from build execution (as opposed from build evaluation) and hence the scoping is not possible. This is e.g. case of [BC0102](Codes.md#bc0102---double-writes) "_Double Writes_" check.

BuildCheck might not be able to guarantee to properly filter the data with this distinction for all [registration types](#RegisterActions) - in case an explicit value is attempted to be configured (either [from the check code](#BuildExecutionCheckConfiguration) or from `.editorconfig` file) for an check that has a subscription to unfilterable data - a warning will be issued during the build and check will be deregistered.

#### Configuring evalution scope

Expand Down
30 changes: 18 additions & 12 deletions documentation/specs/BuildCheck/Codes.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ Report codes are chosen to conform to suggested guidelines. Those guidelines are

| Diagnostic Code | Default Severity | Reason |
|:-----|-------|----------|
| [BC0101](#BC0101) | Warning | Shared output path. |
| [BC0102](#BC0102) | Warning | Double writes. |
| [BC0103](#BC0103) | Suggestion | Used environment variable. |
| [BC0201](#BC0201) | Warning | Usage of undefined property. |
| [BC0202](#BC0202) | Warning | Property first declared after it was used. |
| [BC0203](#BC0203) | None | Property declared but never used. |
| [BC0101](#bc0101---shared-output-path) | Warning | Shared output path. |
| [BC0102](#bc0102---double-writes) | Warning | Double writes. |
| [BC0103](#bc0103---used-environment-variable) | Suggestion | Used environment variable. |
| [BC0201](#bc0201---usage-of-undefined-property) | Warning | Usage of undefined property. |
| [BC0202](#bc0202---property-first-declared-after-it-was-used) | Warning | Property first declared after it was used. |
| [BC0203](#bc0203----property-declared-but-never-used) | None | Property declared but never used. |


To enable verbose logging in order to troubleshoot issue(s), enable [binary logging](https://github.com/dotnet/msbuild/blob/main/documentation/wiki/Binary-Log.md#msbuild-binary-log-overview)
Expand All @@ -19,7 +19,8 @@ _Cmd:_
dotnet build -bl -check
```

## <a name="BC0101"></a>BC0101 - Shared output path.
<a name="BC0101"></a>
## BC0101 - Shared output path.

"Two projects should not share their OutputPath nor IntermediateOutputPath locations"

Expand All @@ -28,15 +29,17 @@ It is not recommended to share output path nor intermediate output path between
If you want to produce outputs in a consolidated output folder - consider using the [Artifacts output layout](https://learn.microsoft.com/en-us/dotnet/core/sdk/artifacts-output) and/or [Microsoft.Build.Artifacts SDK](https://github.com/microsoft/MSBuildSdks/tree/main/src/Artifacts).


## <a name="BC0102"></a>BC0102 - Double writes.
<a name="BC0102"></a>
## BC0102 - Double writes.

"Two tasks should not write the same file"

This is a similar problem as ['BC0101 - Shared output path'](#BC0101) - however with higher granularity. It is not recomended that multiple tasks attempt to write to a single file - as such behavior might lead to nondeterminism of a build (as result can be dependent on the order of the tasks execution if those belong to independent projects) or/and to a lost updates.

If you want multiple tasks to update file in a one-by-one pipeline fashion, it is recommended to give each intermediate output a distinct name - preventing silent mixups if any of the tasks in the chain are skipped or removed.

## <a name="BC0103"></a>BC0103 - Used environment variable.
<a name="BC0103"></a>
## BC0103 - Used environment variable.

"Environment variables should not be used as a value source for the properties"

Expand All @@ -45,7 +48,8 @@ Relying on environment variables introduces variability and unpredictability, as

This practice can result in inconsistent build outcomes and makes debugging difficult, since environment variables are external to project files and build scripts. To ensure consistent and reproducible builds, avoid using environment variables. Instead, explicitly pass properties using the /p option, which offers better control and traceability.

## <a name="BC0201"></a>BC0201 - Usage of undefined property.
<a name="BC0201"></a>
## BC0201 - Usage of undefined property.

"A property that is accessed should be declared first."

Expand All @@ -69,7 +73,8 @@ There are couple cases which are allowed by the check:

BC0201 and BC0202 must have same value for the optional switch - as both operate on top of same data and same filtering.

## <a name="BC0202"></a>BC0202 - Property first declared after it was used.
<a name="BC0202"></a>
## BC0202 - Property first declared after it was used.

"A property should be declared before it is first used."

Expand All @@ -79,7 +84,8 @@ This check indicates that a property was accessed before it was declared. The de

If `BC0202` and [BC0201](#BC0201) are both enabled - then `BC0201` reports only the undefined reads that are not reported by this rule (so those that do not have late definitions).

## <a name="BC0203"></a>BC0203 - Property declared but never used.
<a name="BC0203"></a>
## BC0203 - Property declared but never used.

"A property that is not used should not be declared."

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -275,14 +275,6 @@ private void SetupSingleCheck(CheckFactoryContext checkFactoryContext, string pr
// Update the wrapper
wrapper.StartNewProject(projectFullPath, configurations);
}

if (configurations.GroupBy(c => c.EvaluationCheckScope).Count() > 1)
{
throw new BuildCheckConfigurationException(
string.Format("All rules for a single check should have the same EvaluationCheckScope for a single project (violating rules: [{0}], project: {1})",
checkFactoryContext.RuleIds.ToCsvString(),
projectFullPath));
}
}

private void SetupChecksForNewProject(string projectFullPath, ICheckContext checkContext)
Expand Down

0 comments on commit f422d8d

Please sign in to comment.