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

Update configuration parsing logic #10361

Merged
merged 8 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
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
46 changes: 32 additions & 14 deletions documentation/specs/BuildCheck/BuildCheck.md
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,11 @@ For the `.editorconfig` file configuration, following will apply:

```ini
[*.csproj]
build_check.BC0101.Severity=warning
build_check.BC0101.severity=warning

build_check.COND0543.Severity=none
build_check.COND0543.EvaluationAnalysisScope=AnalyzedProjectOnly
build_check.COND0543.CustomSwitch=QWERTY
build_check.COND0543.severity=none
build_check.COND0543.scope=project
build_check.COND0543.custom_switch=QWERTY
```

### User Configurable Options
Expand All @@ -176,15 +176,17 @@ Initial version of BuildCheck plans a limited set of options configurable by use

**NOTE:** The actual naming of the configuration options is yet to be determined.

#### Severity
#### Severity levels

Option `Severity` with following values will be available:

* `Default`
* `None`
* `Suggestion`
* `Warning`
* `Error`
| Severity | EditorConfig option |
| ------------- | ------------- |
| Default | `default` |
| None | `none` |
| Suggestion | `suggestion` |
| Warning | `warning` |
| Error | `error` |

Severity levels are in line with [roslyn analyzers severity levels](https://learn.microsoft.com/en-us/visualstudio/code-quality/use-roslyn-analyzers). `Default` severity in `.editorconfig` will lead to using build-in severity from the analyzer (so this can be used for clearing custom severity setting from higher level `.editorconfig` file). `Default` severity in the build-in code has same effect as if the code doesn't specify severity at all - an infrastruture default of `None` is considered.

Expand All @@ -194,20 +196,36 @@ Each rule has a severity, even if multiple rules are defined in a single analyze

If all the rules from a single analyzer have severity `None` - analyzer won't be given any data for such configured part of the build (specific project or a whole build). If analyzer have some rules enabled and some disabled - it will be still fed with data, but the reports will be post-filtered.

#### Configuring severity level

```ini
[*.csproj]
build_check.BC0101.severity=warning
```

#### Scope of Analysis

Option `EvaluationAnalysisScope` with following possible options will be available:
* `ProjectOnly` - Only the data from currently analyzed project will be sent to the analyzer. Imports will be discarded.
* `ProjectWithImportsFromCurrentWorkTree` - Only the data from currently analyzed project and imports from files under the entry project or solution will be sent to the analyzer. Other imports will be discarded.
* `ProjectWithImportsWithoutSdks` - Imports from SDKs will not be sent to the analyzer. Other imports will be sent.
* `ProjectWithAllImports` - All data will be sent to the analyzer.

| EvaluationAnalysisScope (Solution Explorer) | EditorConfig option | Behavior |
| ------------- | ------------- | ------------- |
| ProjectOnly | `project` | Only the data from currently analyzed project will be sent to the analyzer. Imports will be discarded. |
| ProjectWithImportsFromCurrentWorkTree | `current_imports` | Only the data from currently analyzed project and imports from files under the entry project or solution will be sent to the analyzer. Other imports will be discarded. |
| ProjectWithImportsWithoutSdks | `without_sdks` | Imports from SDKs will not be sent to the analyzer. Other imports will be sent. |
| ProjectWithAllImports | `all` | All data will be sent to the analyzer. |

All rules of a single analyzer must have the `EvaluationAnalysisScope` configured to a same value. If any rule from the analyzer have the value configured differently - a warning will be issued during the build and analyzer will be deregistered.

Same rule can have `EvaluationAnalysisScope` configured to different values for different projects.

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 analyzer code](#BuildAnalyzerConfiguration) or from `.editorconfig` file) for an analyzer that has a subscription to unfilterable data - a warning will be issued during the build and analyzer will be deregistered.

#### Configuring evalution scope

```ini
[*.csproj]
build_check.BC0101.scope=all
```

## Analyzers and Rules Identification

Expand Down
73 changes: 60 additions & 13 deletions src/Build/BuildCheck/API/BuildAnalyzerConfiguration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using Microsoft.Build.Experimental.BuildCheck.Infrastructure;
using Microsoft.Build.Experimental.BuildCheck.Utilities;

namespace Microsoft.Build.Experimental.BuildCheck;

Expand Down Expand Up @@ -66,32 +67,78 @@ public bool? IsEnabled {
/// </summary>
/// <param name="configDictionary">The configuration dictionary containing the settings for the build analyzer. The configuration's keys are expected to be in lower case or the EqualityComparer to ignore case.</param>
/// <returns>A new instance of <see cref="BuildAnalyzerConfiguration"/> with the specified settings.</returns>
internal static BuildAnalyzerConfiguration Create(Dictionary<string, string>? configDictionary)
internal static BuildAnalyzerConfiguration Create(Dictionary<string, string>? configDictionary) => new()
{
return new()
EvaluationAnalysisScope = TryExtractEvaluationAnalysisScope(configDictionary),
Severity = TryExtractSeverity(configDictionary),
};


private static EvaluationAnalysisScope? TryExtractEvaluationAnalysisScope(Dictionary<string, string>? config)
{

if (!TryExtractValue(BuildCheckConstants.scopeConfigurationKey, config, out string? stringValue) || stringValue is null)
{
EvaluationAnalysisScope = TryExtractValue(nameof(EvaluationAnalysisScope), configDictionary, out EvaluationAnalysisScope evaluationAnalysisScope) ? evaluationAnalysisScope : null,
Severity = TryExtractValue(nameof(Severity), configDictionary, out BuildAnalyzerResultSeverity severity) ? severity : null
};
return null;
}

switch (stringValue)
{
case "project":
return BuildCheck.EvaluationAnalysisScope.ProjectOnly;
case "current_imports":
return BuildCheck.EvaluationAnalysisScope.ProjectWithImportsFromCurrentWorkTree;
case "without_sdks":
return BuildCheck.EvaluationAnalysisScope.ProjectWithImportsWithoutSdks;
case "all":
return BuildCheck.EvaluationAnalysisScope.ProjectWithAllImports;
default:
f-alizada marked this conversation as resolved.
Show resolved Hide resolved
ThrowIncorrectValueException(BuildCheckConstants.scopeConfigurationKey, stringValue);
break;
}

return null;
}

private static bool TryExtractValue<T>(string key, Dictionary<string, string>? config, out T value) where T : struct, Enum
private static BuildAnalyzerResultSeverity? TryExtractSeverity(Dictionary<string, string>? config)
{
value = default;
if (!TryExtractValue(BuildCheckConstants.severityConfigurationKey, config, out string? stringValue) || stringValue is null)
{
return null;
}

if (config == null || !config.TryGetValue(key.ToLower(), out var stringValue) || stringValue is null)
switch (stringValue)
{
return false;
case "none":
return BuildAnalyzerResultSeverity.None;
case "default":
return BuildAnalyzerResultSeverity.Default;
case "suggestion":
return BuildAnalyzerResultSeverity.Suggestion;
case "warning":
return BuildAnalyzerResultSeverity.Warning;
case "error":
return BuildAnalyzerResultSeverity.Error;
default:
ThrowIncorrectValueException(BuildCheckConstants.severityConfigurationKey, stringValue);
break;
}

var isParsed = Enum.TryParse(stringValue, true, out value);
f-alizada marked this conversation as resolved.
Show resolved Hide resolved
return null;
}

private static bool TryExtractValue(string key, Dictionary<string, string>? config, out string? stringValue)
{
stringValue = null;

if (!isParsed)
if (config == null || !config.TryGetValue(key.ToLower(), out stringValue) || stringValue is null)
{
ThrowIncorrectValueException(key, stringValue);
return false;
}

return isParsed;
stringValue = stringValue.ToLower();

return true;
}

private static void ThrowIncorrectValueException(string key, string value)
Expand Down
7 changes: 4 additions & 3 deletions src/Build/BuildCheck/Infrastructure/ConfigurationProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Microsoft.Build.Experimental.BuildCheck.Infrastructure.EditorConfig;
using Microsoft.Build.Experimental.BuildCheck;
using System.Collections.Concurrent;
using Microsoft.Build.Experimental.BuildCheck.Utilities;

namespace Microsoft.Build.Experimental.BuildCheck.Infrastructure;

Expand All @@ -31,9 +32,9 @@ internal sealed class ConfigurationProvider
/// </summary>
private readonly ConcurrentDictionary<string, CustomConfigurationData> _customConfigurationData = new ConcurrentDictionary<string, CustomConfigurationData>(StringComparer.InvariantCultureIgnoreCase);

private readonly string[] _infrastructureConfigurationKeys = new string[] {
nameof(BuildAnalyzerConfiguration.EvaluationAnalysisScope).ToLower(),
nameof(BuildAnalyzerConfiguration.Severity).ToLower()
private readonly string[] _infrastructureConfigurationKeys = {
BuildCheckConstants.scopeConfigurationKey,
BuildCheckConstants.severityConfigurationKey,
};

/// <summary>
Expand Down
3 changes: 3 additions & 0 deletions src/Build/BuildCheck/Utilities/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,7 @@ namespace Microsoft.Build.Experimental.BuildCheck.Utilities;
internal static class BuildCheckConstants
{
internal const string infraStatPrefix = "infrastructureStat_";

internal const string severityConfigurationKey = "severity";
internal const string scopeConfigurationKey = "scope";
}
20 changes: 12 additions & 8 deletions src/BuildCheck.UnitTests/BuildAnalyzerConfiguration_Test.cs
Original file line number Diff line number Diff line change
Expand Up @@ -75,16 +75,19 @@ public void CreateBuildAnalyzerConfiguration_SeverityAndEnabledOrder(string para
}

[Theory]
[InlineData("ProjectOnly", EvaluationAnalysisScope.ProjectOnly)]
[InlineData("ProjectWithImportsFromCurrentWorkTree", EvaluationAnalysisScope.ProjectWithImportsFromCurrentWorkTree)]
[InlineData("ProjectWithImportsWithoutSdks", EvaluationAnalysisScope.ProjectWithImportsWithoutSdks)]
[InlineData("ProjectWithAllImports", EvaluationAnalysisScope.ProjectWithAllImports)]
[InlineData("projectwithallimports", EvaluationAnalysisScope.ProjectWithAllImports)]
[InlineData("project", EvaluationAnalysisScope.ProjectOnly)]
[InlineData("PROJECT", EvaluationAnalysisScope.ProjectOnly)]
[InlineData("current_imports", EvaluationAnalysisScope.ProjectWithImportsFromCurrentWorkTree)]
[InlineData("CURRENT_IMPORTS", EvaluationAnalysisScope.ProjectWithImportsFromCurrentWorkTree)]
[InlineData("without_sdks", EvaluationAnalysisScope.ProjectWithImportsWithoutSdks)]
[InlineData("WITHOUT_SDKS", EvaluationAnalysisScope.ProjectWithImportsWithoutSdks)]
[InlineData("all", EvaluationAnalysisScope.ProjectWithAllImports)]
[InlineData("ALL", EvaluationAnalysisScope.ProjectWithAllImports)]
public void CreateBuildAnalyzerConfiguration_EvaluationAnalysisScope(string parameter, EvaluationAnalysisScope? expected)
{
var config = new Dictionary<string, string>()
{
{ "evaluationanalysisscope" , parameter },
{ "scope" , parameter },
};

var buildConfig = BuildAnalyzerConfiguration.Create(config);
Expand All @@ -97,7 +100,7 @@ public void CreateBuildAnalyzerConfiguration_EvaluationAnalysisScope(string para
}

[Theory]
[InlineData("evaluationanalysisscope", "incorrec-value")]
[InlineData("scope", "incorrec-value")]
[InlineData("severity", "incorrec-value")]
public void CreateBuildAnalyzerConfiguration_ExceptionOnInvalidInputValue(string key, string value)
{
Expand All @@ -106,7 +109,8 @@ public void CreateBuildAnalyzerConfiguration_ExceptionOnInvalidInputValue(string
{ key , value },
};

var exception = Should.Throw<BuildCheckConfigurationException>(() => {
var exception = Should.Throw<BuildCheckConfigurationException>(() =>
{
BuildAnalyzerConfiguration.Create(config);
});
exception.Message.ShouldContain($"Incorrect value provided in config for key {key}");
Expand Down
30 changes: 16 additions & 14 deletions src/BuildCheck.UnitTests/ConfigurationProvider_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,9 @@ public void GetRuleIdConfiguration_CustomConfigurationData()
[*.csproj]
build_check.rule_id.property1=value1
build_check.rule_id.property2=value2
build_check.rule_id.isEnabled2=true
build_check.rule_id.is_enabled_2=true
build_check.rule_id.scope=project
build_check.rule_id.severity=default
any_other_key1=any_other_value1
any_other_key2=any_other_value2
any_other_key3=any_other_value3
Expand All @@ -91,7 +93,7 @@ public void GetRuleIdConfiguration_CustomConfigurationData()

configs.ContainsKey("property1").ShouldBeTrue();
configs.ContainsKey("property2").ShouldBeTrue();
configs.ContainsKey("isenabled2").ShouldBeTrue();
configs.ContainsKey("is_enabled_2").ShouldBeTrue();
}

[Fact]
Expand All @@ -105,18 +107,18 @@ public void GetRuleIdConfiguration_ReturnsBuildRuleConfiguration()
root=true

[*.csproj]
build_check.rule_id.Severity=Error
build_check.rule_id.EvaluationAnalysisScope=ProjectOnly
build_check.rule_id.severity=error
build_check.rule_id.scope=project
""");

var configurationProvider = new ConfigurationProvider();
var buildConfig = configurationProvider.GetUserConfiguration(Path.Combine(workFolder1.Path, "test.csproj"), "rule_id");

buildConfig.ShouldNotBeNull();

buildConfig.IsEnabled?.ShouldBeTrue();
buildConfig.Severity?.ShouldBe(BuildAnalyzerResultSeverity.Error);
buildConfig.EvaluationAnalysisScope?.ShouldBe(EvaluationAnalysisScope.ProjectOnly);
buildConfig.IsEnabled.ShouldBe(true);
buildConfig.Severity.ShouldBe(BuildAnalyzerResultSeverity.Error);
buildConfig.EvaluationAnalysisScope.ShouldBe(EvaluationAnalysisScope.ProjectOnly);
}

[Fact]
Expand All @@ -132,12 +134,12 @@ public void GetRuleIdConfiguration_CustomConfigurationValidity_NotValid_Differen
[*.csproj]
build_check.rule_id.property1=value1
build_check.rule_id.property2=value2
build_check.rule_id.isEnabled2=true
build_check.rule_id.is_enabled_2=true

[test123.csproj]
build_check.rule_id.property1=value2
build_check.rule_id.property2=value3
build_check.rule_id.isEnabled2=tru1
build_check.rule_id.is_enabled_2=tru1
""");

var configurationProvider = new ConfigurationProvider();
Expand All @@ -163,13 +165,13 @@ public void GetRuleIdConfiguration_CustomConfigurationValidity_NotValid_Differen
[*.csproj]
build_check.rule_id.property1=value1
build_check.rule_id.property2=value2
build_check.rule_id.isEnabled2=true
build_check.rule_id.is_enabled_2=true

[test123.csproj]
build_check.rule_id.property1=value1
build_check.rule_id.property2=value2
build_check.rule_id.isEnabled2=true
build_check.rule_id.isEnabled3=true
build_check.rule_id.is_enabled_2=true
build_check.rule_id.is_enabled_3=true
""");

var configurationProvider = new ConfigurationProvider();
Expand All @@ -195,12 +197,12 @@ public void GetRuleIdConfiguration_CustomConfigurationValidity_Valid()
[*.csproj]
build_check.rule_id.property1=value1
build_check.rule_id.property2=value2
build_check.rule_id.isEnabled2=true
build_check.rule_id.is_enabled_2=true

[test123.csproj]
build_check.rule_id.property1=value1
build_check.rule_id.property2=value2
build_check.rule_id.isEnabled2=true
build_check.rule_id.is_enabled_2=true
""");

var configurationProvider = new ConfigurationProvider();
Expand Down