Update the condition used to include the CodeStyle .globalconfig - #80540
Conversation
The logic of the original check seems to be a bit confused. It seems reasonable to include the globalconfig if the build has specifically called for it by setting either AnalysisLevelStyle or AnalysisLevelMode prior to our target running. In addition we keep the AnalysisLevel gate that will automatically add the configuration when the level is high enough.
|
|
||
| <!-- From .NET 11, the global config is systematically added if the file exists. Please check https://github.com/dotnet/roslyn/pull/71173 for more info. --> | ||
| <ItemGroup Condition="Exists('$(_GlobalAnalyzerConfigFile_MicrosoftCodeAnalysis{language}CodeStyle)') and | ||
| ('$(AnalysisLevelStyle)' != '$(AnalysisLevel)' or '$(AnalysisModeStyle)' != '$(AnalysisMode)' or ('$(EffectiveAnalysisLevelStyle)' != '' and $([MSBuild]::VersionGreaterThanOrEquals('$(EffectiveAnalysisLevelStyle)', '11.0'))))"> |
There was a problem hiding this comment.
As brought up in this comment #63036 (comment), the condition would exclude users who had the following configuration despite configuring AnalysisLevelStyle directly.
<AnalysisLevel>latest-all</AnalysisLevel>
<AnalysisLevelStyle>latest-all</AnalysisLevelStyle>
There was a problem hiding this comment.
Does this mean that we will start warning in these scenarios? Should we document this as a breaking change?
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
src/CodeStyle/Tools/Program.cs — The _IncludeStyleConfiguration property assignment has malformed XML/Condition syntax (missing… |
What changed in this PR
Updates the MSBuild logic emitted by src/CodeStyle/Tools/Program.cs to decide when the CodeStyle .globalconfig should be included, aiming to make inclusion depend on explicit Style-level/mode configuration and an analysis-level threshold.
Changes:
- Introduces a computed
_IncludeStyleConfigurationproperty to centralize the “should we include the globalconfig?” decision. - Replaces the prior inline
ItemGroupcondition with a simpler condition based on_IncludeStyleConfiguration.
| File | Description |
|---|---|
| src/CodeStyle/Tools/Program.cs | Adjusts generated MSBuild target logic for including the CodeStyle .globalconfig. |
Suppressed comments (2)
src/CodeStyle/Tools/Program.cs:250
- This line closes
_IncludeStyleConfigurationwith<_IncludeStyleConfiguration>instead of</_IncludeStyleConfiguration>, producing malformed XML in the generated .targets file.
<!-- Check whether the analysis level is high enough that we should include the analyzer configuration.
From .NET 12, the global config is systematically added if the file exists. Please check https://github.com/dotnet/roslyn/pull/71173 for more info. -->
<_IncludeStyleConfiguration Condition="'$(EffectiveAnalysisLevelStyle)' != '' and $([MSBuild]::VersionGreaterThanOrEquals('$(EffectiveAnalysisLevelStyle)', '12.0'))">true<_IncludeStyleConfiguration>
src/CodeStyle/Tools/Program.cs:268
- The ItemGroup
Conditionhas an extra trailing)which makes the Condition expression invalid and will cause MSBuild to fail when importing the generated targets.
<!-- Add the analyzer configuration. -->
<ItemGroup Condition="Exists('$(_GlobalAnalyzerConfigFile_MicrosoftCodeAnalysis{language}CodeStyle)') and '$(_IncludeStyleConfiguration)' == 'true')">
<EditorConfigFiles Include="$(_GlobalAnalyzerConfigFile_MicrosoftCodeAnalysis{language}CodeStyle)" />
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
src/CodeStyle/Tools/Program.cs — The ItemGroup Condition has an extra closing ) at the end, which makes the MSBuild condition… |
|
src/CodeStyle/Tools/Program.cs — This comment says the Style level/mode were "configured separate" from… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
src/CodeStyle/Tools/Program.cs — The _IncludeStyleConfiguration property assignment has malformed XML/Condition syntax (missing… View resolved comment |
Suppressed comments (1)
src/CodeStyle/Tools/Program.cs:251
- The MSBuild property element on this line is malformed (closing tag is
<_IncludeStyleConfiguration>instead of</_IncludeStyleConfiguration>), which will break MSBuild XML parsing for the generated targets file.
<!-- Check whether the analysis level is high enough that we should include the analyzer configuration.
From .NET 12, the global config is systematically added if the file exists. Please check https://github.com/dotnet/roslyn/pull/71173 for more info. -->
<_IncludeStyleConfiguration Condition="'$(EffectiveAnalysisLevelStyle)' != '' and $([MSBuild]::VersionGreaterThanOrEquals('$(EffectiveAnalysisLevelStyle)', '12.0'))">true<_IncludeStyleConfiguration>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Review tier: Lite
Findings: None
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
src/CodeStyle/Tools/Program.cs — This comment says the Style level/mode were "configured separate" from… View resolved comment |
|
src/CodeStyle/Tools/Program.cs — The ItemGroup Condition has an extra closing ) at the end, which makes the MSBuild condition… View resolved comment |
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/CodeStyle/Tools/Program.cs:250
- This change raises the automatic-inclusion threshold from 11.0 (previous condition) to 12.0. If older SDKs still rely on this target to include the CodeStyle .globalconfig at AnalysisLevelStyle >= 11, this would be a behavior regression. Please confirm the intended SDK/version boundary and align the comment + VersionGreaterThanOrEquals check accordingly.
<!-- Check whether the analysis level is high enough that we should include the analyzer configuration.
From .NET 12, the global config is systematically added if the file exists. Please check https://github.com/dotnet/roslyn/pull/71173 for more info. -->
<_IncludeStyleConfiguration Condition="'$(EffectiveAnalysisLevelStyle)' != '' and $([MSBuild]::VersionGreaterThanOrEquals('$(EffectiveAnalysisLevelStyle)', '12.0'))">true<_IncludeStyleConfiguration>
src/CodeStyle/Tools/Program.cs:250
- MSBuild XML is malformed here: the _IncludeStyleConfiguration element isn't closed properly (missing "/" in the end tag). This will break evaluation/parsing of the generated .targets content.
<_IncludeStyleConfiguration Condition="'$(EffectiveAnalysisLevelStyle)' != '' and $([MSBuild]::VersionGreaterThanOrEquals('$(EffectiveAnalysisLevelStyle)', '12.0'))">true<_IncludeStyleConfiguration>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
src/CodeStyle/Tools/Program.cs — The comment here references “From .NET 12…”, but the gating logic is based on… |
| <!-- Check whether the analysis level is high enough that we should include the analyzer configuration. | ||
| From .NET 12, the global config is systematically added if the file exists. Please check https://github.com/dotnet/roslyn/pull/71173 for more info. --> | ||
| <_IncludeStyleConfiguration Condition="'$(EffectiveAnalysisLevelStyle)' != '' and $([MSBuild]::VersionGreaterThanOrEquals('$(EffectiveAnalysisLevelStyle)', '12.0'))">true</_IncludeStyleConfiguration> |


The logic of the original check seems to be a bit confused. It seems reasonable to include the globalconfig if the build has specifically called for it by setting either AnalysisLevelStyle or AnalysisModeStyle prior to our target running. In addition we keep the AnalysisLevel gate that will automatically add the configuration when the level is high enough.