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.

Sign up for GitHub

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

Convert .ruleset to .globalconfig #49181

Merged
merged 4 commits into from
Nov 5, 2020
Merged

Convert .ruleset to .globalconfig #49181

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1af3260
Convert .ruleset to .globalconfig
sharwell Nov 4, 2020
0ea2492
Extract Common.globalconfig to avoid MultipleGlobalAnalyzerKeys warning
sharwell Nov 4, 2020
6fa00b2
Remove Empty.ruleset
sharwell Nov 5, 2020
9ad67f2
Rename eng/config/rulesets to eng/config/globalconfigs
sharwell Nov 5, 2020
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
99 changes: 99 additions & 0 deletions eng/config/rulesets/Common.globalconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
is_global = true

dotnet_diagnostic.CA1067.severity = warning
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I did not validate that each of the entries in the global configs map to the entries in current rulesets.

dotnet_diagnostic.CA1068.severity = warning
dotnet_diagnostic.CA1200.severity = warning
dotnet_diagnostic.CA1821.severity = warning

dotnet_diagnostic.IDE0055.severity = warning

dotnet_diagnostic.RS1001.severity = none
dotnet_diagnostic.RS1002.severity = none
dotnet_diagnostic.RS1003.severity = none
dotnet_diagnostic.RS1004.severity = none
dotnet_diagnostic.RS1005.severity = none
dotnet_diagnostic.RS1006.severity = none
dotnet_diagnostic.RS1008.severity = none
dotnet_diagnostic.RS1009.severity = none
dotnet_diagnostic.RS1010.severity = none
dotnet_diagnostic.RS1011.severity = none
dotnet_diagnostic.RS1012.severity = none
dotnet_diagnostic.RS1013.severity = none
dotnet_diagnostic.RS1014.severity = warning
# https://github.com/dotnet/roslyn/issues/26420: Enable rule RS1022 for Roslyn.sln
dotnet_diagnostic.RS1022.severity = none
# RS1024: Compare symbols correctly
dotnet_diagnostic.RS1024.severity = refactoring

dotnet_diagnostic.AD0001.severity = error

dotnet_diagnostic.RS0001.severity = warning
dotnet_diagnostic.RS0002.severity = warning
dotnet_diagnostic.RS0006.severity = error
dotnet_diagnostic.RS0012.severity = warning
dotnet_diagnostic.RS0014.severity = warning
dotnet_diagnostic.RS0015.severity = warning
dotnet_diagnostic.RS0016.severity = error
dotnet_diagnostic.RS0017.severity = error
dotnet_diagnostic.RS0018.severity = warning
dotnet_diagnostic.RS0022.severity = error
dotnet_diagnostic.RS0023.severity = error
dotnet_diagnostic.RS0024.severity = error
dotnet_diagnostic.RS0025.severity = error
dotnet_diagnostic.RS0026.severity = error
dotnet_diagnostic.RS0027.severity = error
# RS0040: Defaultable types should have defaultable fields
dotnet_diagnostic.RS0040.severity = none
# RS0046: Avoid the 'Opt' suffix in nullable-enabled code (too noisy right now)
dotnet_diagnostic.RS0046.severity = suggestion
# RS0047: (Suppressor) The 'Type.FullName' property is never null when using the syntax 'typeof(T).FullName'
dotnet_diagnostic.RS0047.severity = none

# allow skipped tests, with a code fix to unskip them
dotnet_diagnostic.xUnit1004.severity = refactoring
# xUnit2006: "do not use generic Assert.Equal to test string equality" is a valid assert, but very noisy right now
dotnet_diagnostic.xUnit2006.severity = none
# xUnit2009: "do not use Assert.True to check for substrings" is a valid assert, but very noisy right now
dotnet_diagnostic.xUnit2009.severity = none
# xUnit2012: "do not use Enumerable.Any() to check if a value exists in a collection" is a valid assert, but very noisy right now
dotnet_diagnostic.xUnit2012.severity = none
# xUnit2013: "do not use Assert.Equal() to check for collection size" is a valid assert, but very noisy right now
dotnet_diagnostic.xUnit2013.severity = none
# xUnit2017: "do not use Contains() to check if a value exists in a collection" is a valid assert, but very noisy right now
dotnet_diagnostic.xUnit2017.severity = none

# https://github.com/Microsoft/VSSDK-Analyzers/blob/master/doc/index.md
# VSSDK001: Derive from AsyncPackage
dotnet_diagnostic.VSSDK001.severity = warning
# VSSDK003: Support async tool windows
dotnet_diagnostic.VSSDK003.severity = warning
# VSSDK006: Remove suppression. https://github.com/dotnet/roslyn/issues/38471
dotnet_diagnostic.VSSDK006.severity = none

# VSTHRD002: Avoid problematic synchronous waits
dotnet_diagnostic.VSTHRD002.severity = none
# VSTHRD003: Avoid awaiting foreign Tasks https://github.com/dotnet/roslyn/issues/34331
dotnet_diagnostic.VSTHRD003.severity = none
# VSTHRD103: Call async methods when in an async method
dotnet_diagnostic.VSTHRD103.severity = none
# VSTHRD010: Invoke single-threaded types on Main thread https://github.com/dotnet/roslyn/issues/29275
dotnet_diagnostic.VSTHRD010.severity = none
# VSTHRD110: Observe result of async calls
dotnet_diagnostic.VSTHRD110.severity = none

dotnet_diagnostic.HAA0101.severity = none
dotnet_diagnostic.HAA0102.severity = none
dotnet_diagnostic.HAA0201.severity = none
dotnet_diagnostic.HAA0202.severity = none
dotnet_diagnostic.HAA0301.severity = none
dotnet_diagnostic.HAA0302.severity = none
dotnet_diagnostic.HAA0303.severity = none
dotnet_diagnostic.HAA0401.severity = none
dotnet_diagnostic.HAA0501.severity = none
dotnet_diagnostic.HAA0502.severity = none
dotnet_diagnostic.HAA0503.severity = none
dotnet_diagnostic.HAA0506.severity = none
dotnet_diagnostic.HAA0601.severity = none
dotnet_diagnostic.HAA0602.severity = none
dotnet_diagnostic.HAA0603.severity = none
dotnet_diagnostic.HeapAnalyzerReadonlyMethodGroupAllocationRule.severity = none
25 changes: 25 additions & 0 deletions eng/config/rulesets/NonShipping.globalconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
is_global = true

# For tests, the ConfigureAwait(true) is good enough. Either they are already running on a thread pool
# thread where ConfigureAwait(false) does nothing, or we're running the workload from an STA thread
# where we want to marshal the continuations back to it.
dotnet_diagnostic.CA2007.severity = none

# Mark constant field as 'const' instead of static readonly - not useful for tests
dotnet_diagnostic.CA1802.severity = none

# CodeFix providers should override GetFixAllProvider - suppress for non-shipping/test projects
dotnet_diagnostic.RS1016.severity = none
# DiagnosticId must be unique across analyzers - suppress for non-shipping/test projects
dotnet_diagnostic.RS1019.severity = none
# Configure generated code analysis - suppress for non-shipping/test projects
dotnet_diagnostic.RS1025.severity = none
# Enable concurrent execution - suppress for non-shipping/test projects
dotnet_diagnostic.RS1026.severity = none
# Do not use generic CodeAction.Create to create CodeAction - not useful for tests
dotnet_diagnostic.RS0005.severity = none
# Do not call 'GetTestAccessor()' from production code: does not apply to tests
dotnet_diagnostic.RS0043.severity = none

# VSTHRD200: Use "Async" suffix for async methods
dotnet_diagnostic.VSTHRD200.severity = none
40 changes: 0 additions & 40 deletions eng/config/rulesets/NonShipping.ruleset
View file
Open in desktop

This file was deleted.

6 changes: 6 additions & 0 deletions eng/config/rulesets/Shipping.globalconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
is_global = true

dotnet_diagnostic.CA1802.severity = warning
dotnet_diagnostic.CA2007.severity = warning

dotnet_diagnostic.RS0005.severity = warning
95 changes: 0 additions & 95 deletions eng/config/rulesets/Shipping.ruleset
View file
Open in desktop

This file was deleted.

14 changes: 9 additions & 5 deletions eng/targets/Imports.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,15 @@
<ProduceReferenceAssembly>true</ProduceReferenceAssembly>
</PropertyGroup>

<!-- Rulesets -->
<PropertyGroup Condition="'$(CodeAnalysisRuleSet)' == ''">
<CodeAnalysisRuleSet Condition="'$(IsShipping)' == 'true'">$(RepositoryEngineeringDir)config\rulesets\Shipping.ruleset</CodeAnalysisRuleSet>
<CodeAnalysisRuleSet Condition="'$(IsShipping)' != 'true'">$(RepositoryEngineeringDir)config\rulesets\NonShipping.ruleset</CodeAnalysisRuleSet>
</PropertyGroup>
<!-- Global Analyzer Config -->
<ItemGroup>
<!-- Always include Common.globalconfig -->
<EditorConfigFiles Include="$(RepositoryEngineeringDir)config\rulesets\Common.globalconfig" />
<!-- Include Shipping.globalconfig for shipping projects -->
<EditorConfigFiles Condition="'$(IsShipping)' == 'true'" Include="$(RepositoryEngineeringDir)config\rulesets\Shipping.globalconfig" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tagging @chsienki - I believe including these as GlobalAnalyzerConfigFiles should work and make it more clear that these are global analyzer configs, not editorconfigs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two problems with this approach:

  1. This repository currently uses a compiler build that does not support this property

  2. The conversion of GlobalAnalyzerConfigFiles to EditorConfigFiles is conditioned on $(DiscoverGlobalAnalyzerConfigFiles):

    roslyn/src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets

    Line 89 in 8637000

    <EditorConfigFiles Include="@(GlobalAnalyzerConfigFiles->Exists())" Condition="'$(DiscoverGlobalAnalyzerConfigFiles)' != 'false'" />

    We would need to remove this condition (and wait for it to apply to the SDK used for this repository) before GlobalAnalyzerConfigFiles becomes a viable alternative to EditorConfigFiles.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the property GlobalAnalyzerConfigFiles works as expected: ClassLibrary58.zip

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the property GlobalAnalyzerConfigFiles works as expected

It might work in newer SDKs, but it doesn't work in the version of the SDK and/or compiler currently specified for Roslyn command line builds.

It also doesn't address the fact that setting unrelated property DiscoverGlobalAnalyzerConfigFiles currently disables the manual inclusion of global analyzer config files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DiscoverGlobalAnalyzerConfigFiles is not defaulted anywhere as per my knowledge, so the condition should be true by default and current logic should work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also doesn't address the fact that setting unrelated property DiscoverGlobalAnalyzerConfigFiles currently disables the manual inclusion of global analyzer config files.

Yes, that seems reasonable - @chsienki I can file a tracking bug for you if you agree.

<!-- Include NonShipping.globalconfig for non-shipping projects -->
<EditorConfigFiles Condition="'$(IsShipping)' != 'true'" Include="$(RepositoryEngineeringDir)config\rulesets\NonShipping.globalconfig" />
</ItemGroup>

<!--
Some dependencies bring in references to UIAutomationClient and UIAutomationTypes. These conflict with the faster
Expand Down