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

Guard the 'CustomSeverityConfigurable' tag with AnalysisLevel 9.0 #72238

Merged
merged 1 commit into from
Feb 24, 2024

Conversation

mavasani
Copy link
Contributor

@mavasani mavasani commented Feb 23, 2024

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

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 when EnforceCodeStyleInBuild = false. I have created dotnet/sdk#38968 for fixing the SDK issue.

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.
@mavasani mavasani added Area-IDE IDE-CodeStyle Built-in analyzers, fixes, and refactorings labels Feb 23, 2024
@mavasani mavasani requested a review from a team as a code owner February 23, 2024 11:32
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Feb 23, 2024
mavasani added a commit to dotnet/sdk that referenced this pull request Feb 23, 2024
… 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)
Copy link
Contributor Author

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.
Copy link
Contributor Author

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)
Copy link
Contributor Author

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.

@mavasani mavasani merged commit ae73e6a into dotnet:main Feb 24, 2024
27 checks passed
@mavasani mavasani deleted the FixEditorConfigRegression branch February 24, 2024 05:45
@cremor
Copy link

cremor commented Feb 26, 2024

Which VS version will include this fix?

@CyrusNajmabadi
Copy link
Member

@cremor When this is merged into VS a bot will update this PR with that information.

@cremor
Copy link

cremor commented Jun 4, 2024

This PR is assigend to the milestone "17.10 P2". But I can still reproduce the bug in VS 17.10.1
Did the fix not get merged to the correct VS branch or is there still something else broken?

@sharwell
Copy link
Member

sharwell commented Jun 4, 2024

@cremor Can you file a new issue for the behavior you are experiencing, including a sample project?

@cremor
Copy link

cremor commented Jun 5, 2024

@sharwell Done: #73858

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE IDE-CodeStyle Built-in analyzers, fixes, and refactorings untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
6 participants