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

Thread in a new MSBuild property EffectiveAnalysisLevelStyle to IDE code style analyzers #70794

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

mavasani
Copy link
Contributor

@mavasani mavasani commented Nov 14, 2023

Recommend to review commit-by-commit (first commit 591d47e just converts existing string to a raw string literal)

Addresses the concern raised in #52991 (comment)

The core idea is to define and thread in a new numeric AnalysisLevel value specific to IDE Code style, that gets passed to the analyzers to conditionally enable any new breaking change features, such as the one proposed in #52991.

Currently, we support AnalysisLevel and per-category AnalysisLevel{Category} properties to determine the set of first party CA and IDE rules to enable by default: https://learn.microsoft.com/dotnet/core/project-sdk/msbuild-props#analysislevelcategory. This also includes AnalysisLevelStyle property for IDE CodeStyle analyzers, which already defaults to AnalysisLevel. However, this property can hold well-known non-numeric values such as none, preview, latest or compound values in the form <version>-<mode> (documented here). This PR adds the below additional logic:

  1. Add a new MSBuild 'EffectiveAnalysisLevelStyle' property, that contains a numeric value derived from the existing AnalysisLevelStyle property, similar to the way we have EffectiveAnalysisLevel derived from AnalysisLevel property (see https://github.com/dotnet/sdk/blob/fcc367b2164e4b41b8fb6622110aeb9f3247dd92/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets#L35-L56). Note that the category specific logic added here (for 'Style' category) is identical to the existing logic in Microsoft.CodeAnalysis.NetAnalyzers.targets file for different CA analyzer categories that is generated by tooling and inserted into the .NET SDK: https://github.com/dotnet/roslyn-analyzers/blob/b924542a1b526322929725a1aaa9586c21b1b231/src/Tools/GenerateDocumentationAndConfigFiles/Program.cs#L1528C62-L1553.

  2. Mark this new 'EffectiveAnalysisLevelStyle' property as a CompilerVisibileProperty to pass it to the analyzers via analyzer config options.

Note that this Target gets added to C# and VB CodeStyle targets file that is imported here: https://github.com/dotnet/sdk/blob/f52240f11ad291e6ee3cff86e83c0f7a21b60370/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets#L117-L120. I verified locally that <%ProjectName%>.GeneratedMSBuildEditorConfig.editorconfig generated by our tooling contains an entry with key build_property.EffectiveAnalysisLevelStyle.

The core idea is to define and thread in a new numeric `AnalysisLevel` value specific to IDE Code style, that gets passed to the analyzers to conditionally enable any new breaking change features, such as the one proposed in dotnet#52991.

Currently, we support `AnalysisLevel` and per-category `AnalysisLevel{Category}` properties to determine the set of first party CA and IDE rules to enable by default: https://learn.microsoft.com/dotnet/core/project-sdk/msbuild-props#analysislevelcategory. This also includes `AnalysisLevelStyle` property for IDE CodeStyle analyzers, which already defaults to `AnalysisLevel`. However, this property can hold non-numeric well-known values such as `none`, `preview`, `latest` or compound values in the form `<version>-<mode>` (documented [here](https://learn.microsoft.com/dotnet/core/project-sdk/msbuild-props#analysislevel). This PR adds the below additional logic:

1. Add a new MSBuild 'EffectiveAnalysisLevelStyle' property, that contains a numeric value derived from `AnalysisLevelStyle` property, similar to the way we have `EffectiveAnalysisLevel` derived from `AnalysisLevel` property (see https://github.com/dotnet/sdk/blob/fcc367b2164e4b41b8fb6622110aeb9f3247dd92/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets#L35-L56). Note that the category specific logic added here (for 'Style' category) is identical to the existing logic in Microsoft.CodeAnalysis.NetAnalyzers.targets file for different CA analyzer categories that is generated by tooling and inserted into the .NET SDK: https://github.com/dotnet/roslyn-analyzers/blob/b924542a1b526322929725a1aaa9586c21b1b231/src/Tools/GenerateDocumentationAndConfigFiles/Program.cs#L1528C62-L1553.

2. Mark this new 'EffectiveAnalysisLevelStyle' property as a `CompilerVisibileProperty` to pass it to the analyzers via analyzer config options.

Note that this Target gets added to C# and VB CodeStyle targets file that is imported here: https://github.com/dotnet/sdk/blob/f52240f11ad291e6ee3cff86e83c0f7a21b60370/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets#L117-L120. I verified locally that `<%ProjectName%>.GeneratedMSBuildEditorConfig.editorconfig` generated by our tooling contains an entry with key `build_property.EffectiveAnalysisLevelStyle`.
@mavasani mavasani added the IDE-CodeStyle Built-in analyzers, fixes, and refactorings label Nov 14, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 14, 2023
Comment on lines 216 to 217
<!-- Default 'AnalysisLevelStyle' to the core 'AnalysisLevel' -->
<AnalysisLevelStyle Condition="'$(AnalysisLevelStyle)' == ''">$(AnalysisLevel)</AnalysisLevelStyle>
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 where we default AnalysisLevelStyle to AnalysisLevel when not set

@mavasani mavasani marked this pull request as ready for review November 14, 2023 09:50
@mavasani mavasani requested a review from a team as a code owner November 14, 2023 09:50
@mavasani
Copy link
Contributor Author

Thanks @arkalyanms

@mavasani mavasani merged commit 0735137 into dotnet:main Nov 15, 2023
24 checks passed
@ghost ghost added this to the Next milestone Nov 15, 2023
@mavasani mavasani deleted the AnalysisLevelStyle branch November 15, 2023 03:38
@RikkiGibson RikkiGibson modified the milestones: Next, 17.9 P2 Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers IDE-CodeStyle Built-in analyzers, fixes, and refactorings untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants