-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Guard the 'CustomSeverityConfigurable' tag with AnalysisLevel 9.0 #72238
Conversation
Fixes dotnet#72094 Recently, as part of implementing support for dotnet#52991, we started respecting `option_name = option_value:severity` in build. As part of this change, if user has conflicting severity configurations from `option_name = option_value:severity` and `dotnet_diagnostic.IDExxxx.severity`, the former was given preference. We knew that this would likely break some customers, hence added functionality to guard this feature by enabling it only when AnalysisLevel was >= 9.0. However, that guard was only implemented for command line build path. This meant the feature was still being enabled by default in the IDE live analysis, which was unintentional. We now ensure that we respect the AnalysisLevel value even for this path and filter out CustomSeverityConfigurable custom tag for AnalysisLevel less than 9. As part of this change, I also identified a bug in the SDK targets: https://github.com/dotnet/sdk/blob/3075c3bc8d8aa4e0ffcf2945459ef2ee383e3ae0/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets#L117-L120 We shouldn't be conditioning the inclusion of CodeStyle targets file on `EnforceCodeStyleInBuild` as we want the target to execute even for live analysis for IDE analyzers shipping inside VS, not only when enabled on build. Fix to remove that condition is required to ensure we thread in AnalysisLevel property value to the analyzers. I will create a PR for the same.
… build Please see the PR description in dotnet/roslyn#72238 (comment), which gives a background on this fix. This change along with that Roslyn PR is needed for fixing the regression in dotnet/roslyn#72094
[InlineData(8.0)] | ||
[InlineData(9.0)] | ||
[WorkItem("https://github.com/dotnet/roslyn/issues/72094")] | ||
public async Task TestWithConflictingSeverityConfigurationEntries(double analysisLevel) |
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.
This is the unit test that mimics the behavior pointed out in #72094. Verified that this test fails for analysisLevel 8.0
prior to the product change
{ | ||
// 'CustomSeverityConfigurable' is only enabled when AnalysisLevel >= 9. |
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.
This is the only change needed for this fix. We now ensure that if AnalysisLevel is lesser than 9.0, we filter out the CustomSeverityConfigurable
tag from the reported code style diagnostics. This tag enables the feature support added for #52991
@@ -55,5 +52,15 @@ public static bool TryGetEditorConfigOption<T>(this AnalyzerConfigOptions analyz | |||
value = default!; | |||
return false; | |||
} | |||
|
|||
public static bool IsAnalysisLevelGreaterThanOrEquals(this AnalyzerConfigOptions analyzerConfigOptions, int minAnalysisLevel) |
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.
This is an existing method that just has been refactored to an internal extension method so it can be invoked from DiagnosticHelper type as well. No functionality changes here.
Which VS version will include this fix? |
@cremor When this is merged into VS a bot will update this PR with that information. |
This PR is assigend to the milestone "17.10 P2". But I can still reproduce the bug in VS 17.10.1 |
@cremor Can you file a new issue for the behavior you are experiencing, including a sample project? |
Fixes #72094
Recently, as part of implementing support for #52991, we started respecting
option_name = option_value:severity
in build. As part of this change, if user has conflicting severity configurations fromoption_name = option_value:severity
anddotnet_diagnostic.IDExxxx.severity
, the former was given preference. We knew that this would likely break some customers, hence added functionality to guard this feature by enabling it only when AnalysisLevel was >= 9.0. However, that guard was only implemented for command line build path. This meant the feature was still being enabled by default in the IDE live analysis, which was unintentional. We now ensure that we respect the AnalysisLevel value even for this path and filter out CustomSeverityConfigurable custom tag for AnalysisLevel less than 9.NOTE: Even though this PR touches almost all the IDE analyzer files, the fix is only limited to one change, rest all are cascaded changes. I'll add a comment on the PR to point out this fix.
SDK bug
As part of this change, I also identified a bug in the SDK targets: https://github.com/dotnet/sdk/blob/3075c3bc8d8aa4e0ffcf2945459ef2ee383e3ae0/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets#L117-L120 We shouldn't be conditioning the inclusion of CodeStyle targets file on
EnforceCodeStyleInBuild
as we want the target to execute even for live analysis for IDE analyzers shipping inside VS, not only when enabled on build. Fix to remove that condition is required to ensure we thread in AnalysisLevel property value to the analyzers for live analysis whenEnforceCodeStyleInBuild = false
. I have created dotnet/sdk#38968 for fixing the SDK issue.