Support global analyzer config levels: #49834

chsienki · 2020-12-07T22:08:40Z

Read the 'global_level' special key
Allow conflicting keys in global configs to be resolved by level
Only warn about conflicts that have the same level
Automatically treat as global and assign a level of 100 for configs called '.globalconfig'
Add tests

- Read the 'global_level' special key - Allow conflicting keys in global configs to be resolved by level - Only warn about conflicts that have the same level - Automatically treat as global and assign a level of 100 for configs called '.globalconfig' - Add tests

chsienki · 2020-12-07T22:27:50Z

@dotnet/roslyn-compiler for review please.

//cc @mavasani

mavasani · 2020-12-07T22:43:33Z

Thanks @chsienki. I am not familiar with the code being changed here, so will let the compiler team review the PR. I can vouch that this is a very valuable feature add. We ship some globalconfigs with the .NET SDK along with SDK analyzers, and we have seen lot of users complain that they are seeing conflicts in user provided globalconfigs with the ones shipping in the SDK. This PR should address all these user requests.

Implements category specific AnalysisLevel properties as per dotnet/roslyn#49250 (comment). Users would now be able to specify the core AnalysisLevel and override category specific AnalysisLevel for each category. ```xml <AnalysisLevel>5-minimum</AnalysisLevel> <AnalysisLevelSecurity>latest-all</ AnalysisLevelSecurity > <AnalysisLevelPerformance>preview-recommended</AnalysisLevelPerformance> <AnalysisLevelGlobalization>5-none</AnalysisLevelGlobalization> ``` 1. The post-build tool now generates category-specific global configs for each supported analysis mode 2. The tool also generates category-specific `AddGlobalAnalyzerConfigForPackage` targets so that if user overrides a category-specific AnalysisLevel property NOTE: This PR depends on feature dotnet/roslyn#48634, which is currently in code review: dotnet/roslyn#49834

mavasani · 2020-12-11T01:42:31Z

Note for @chsienki - once this PR is merged, kindly open an issue or PR on dotnet/docs repo to update the documentation for global config precedence: https://docs.microsoft.com/dotnet/fundamentals/code-analysis/configuration-files#precedence

src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs

cston · 2020-12-16T20:57:41Z

src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs

+        {
+            get
+            {
+                if (GlobalSection.Properties.TryGetValue(GlobalLevelKey, out string? val) && int.TryParse(val, out int level))


int.TryParse(val, out int level) [](start = 93, length = 32)

If this fails, shouldn't we return 100 rather than potentially 0?

If we can't read an explicit value, we fall back to the else case below and make a decision based on filename. So a user config with an error will get 100, but a nuget one with an error gets 0. See AnalyzerConfigTests.InvalidGlobalLevelIsIgnored

src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs

cston · 2020-12-16T21:02:25Z

src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs

+        ///     </description></item>
+        ///  </list>
+        ///  
+        /// This value is unsued when <see cref="IsGlobal"/> is <c>false</c>;


This value is unsued when is false [](start = 12, length = 64)

Is this correct that the value is unused when IsGlobal is false? It looks like MergeSection() uses this value whenever there is a conflict. #Closed

Yeah, MergeSection() is part of the global config builder, so we'll only get there when IsGlobal is true

cston · 2020-12-16T21:53:14Z

src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs

                    _values.Add(section.Name, sectionDict);
                }

                _duplicates.TryGetValue(section.Name, out var duplicateDict);
                foreach ((var key, var value) in section.Properties)
                {
-                    if (isGlobalSection && Section.PropertiesKeyComparer.Equals(key, GlobalKey))
+                    if (isGlobalSection && (Section.PropertiesKeyComparer.Equals(key, GlobalKey) || Section.PropertiesKeyComparer.Equals(key, GlobalLevelKey)))
                    {
                        continue;
                    }

                    bool keyInSection = sectionDict.ContainsKey(key);
                    bool keyDuplicated = duplicateDict?.ContainsKey(key) ?? false;


Consider getting the dictionary values, to simplify the code below:

bool keyInSection = sectionDict.TryGetValue(key, out var sectionValue); ImmutableDictionary<string, (int, ArrayBuilder<string>)>.Builder duplicateValue = null; bool keyDuplicated = duplicateDict?.TryGetValue(key, out duplicateValue) == true; if ((!keyInSection && !keyDuplicated) || (keyInSection && sectionValue.globalLevel < globalLevel) || (keyDuplicated && duplicateValue.globalLevel < globalLevel)) { ... ``` #Closed

cston · 2020-12-16T21:55:19Z

src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs

                    {
-                        sectionDict.Add(key, (value, configPath));
+                        sectionDict[key] = (value, configPath, globalLevel);


sectionDict[key] = (value, configPath, globalLevel); [](start = 24, length = 52)

Is this correct if sectionValue.globalLevel >= globalLevel and dictionaryValue.globalLevel < globalLevel?

We can't get into that situation: If we have one in the section, then it must be the highest precedence, and thus there isn't anything in the duplicate dictionary. If we have duplicates then they must be of the same precedence, and we would have removed the item from the section previously (which we assert at line 616) #Closed

I don't quite follow. But does that mean if we find the key in the sectionDict, then we won't find it in duplicateDict? If so, would it help to make the assumption clear by only checking for duplicate if not found in the section?

bool keyInSection = sectionDict.TryGetValue(key, out var sectionValue); bool keyDuplicated = !keyInSection && duplicateDict?.TryGetValue(key, out duplicateValue) == true; if (!keyInSection || !keyDuplicated) { sectionDict.Add(key, (value, configPath, globalLevel)); } else { int existingGlobalLevel = keyInSection ? sectionValue.globalLevel : duplicateValue.GlobalLevel; if (existingGlobalLevel < globalLevel) { ... } else if (existingGlobalLevel == globalLevel) { ... } }

In reply to: 544743061 [](ancestors = 544743061)

Hmm, that is a far nicer way of writing the same logic, thanks!

cston · 2020-12-17T04:19:14Z

src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs

                    {
                        continue;
                    }

-                    bool keyInSection = sectionDict.ContainsKey(key);
-                    bool keyDuplicated = duplicateDict?.ContainsKey(key) ?? false;
+                    bool keyInSection = sectionDict.TryGetValue(key, out var sectionValue) == true;


== true [](start = 90, length = 8)

Minor: == true is not necessary. #Closed

cston · 2020-12-17T04:21:03Z

src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs

                            _duplicates.Add(section.Name, duplicateDict);
                        }

                        // record that this key is now a duplicate
-                        ArrayBuilder<string> configList = keyDuplicated ? duplicateDict[key] : ArrayBuilder<string>.GetInstance();
+                        ArrayBuilder<string> configList = duplicateValue.configPaths ?? ArrayBuilder<string>.GetInstance();


. [](start = 72, length = 1)

?. #Closed

duplicateValue is a tuple, so it's never null, just the configPaths it contains when we didn't find it in the dictionary.

jaredpar · 2020-12-18T17:09:20Z

src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs

@@ -2175,6 +2175,257 @@ public void GlobalConfigIssuesWarningWithInvalidSectionNames(string sectionName,
        public void GlobalConfigIssuesWarningWithInvalidSectionNames_PlatformSpecific(string sectionName, bool isValidWindows, bool isValidOther)
            => GlobalConfigIssuesWarningWithInvalidSectionNames(sectionName, ExecutionConditionUtil.IsWindows ? isValidWindows : isValidOther);

+        [Theory]
+        [InlineData("/.globalconfig", true)]


Consider adding a case where we use a Turkish i (İ) to make sure we use an invariant comparer here.

jaredpar · 2020-12-18T18:03:37Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

 [/file.cs]
 option1 = abc");

            analyzerConfig2 = analyzerConfigFile2.WriteAllText(@"
 is_global = true
+global_level = 100


Why did we need to add these entries here? Didn't see any change to the tests verifying the text here so not sure why we needed to add this.

These tests are checking the error messages that occur when keys collide, so I made it explicit that they would continue to collide. I could rename the files and let the heuristic pick up the 0 value, but I figured it was better to be explicit in the test.

Ah okay, makes sense.

jaredpar · 2020-12-18T18:05:52Z

src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs

@@ -218,6 +264,8 @@ private static bool IsComment(string line)
            return false;
        }

+        private static bool IsGlobalFileName(string fileName) => Path.GetFileName(fileName).Equals(UserGlobalConfigName, StringComparison.OrdinalIgnoreCase);


This method is called a number of times and every time is an allocation. There also doesn't appear to be a case where we call it zero times. Consider just calculating this once and storing it in a field.

cston · 2020-12-18T23:00:31Z

src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs

+            }
+        }
+
+        internal readonly bool _hasGlobalFileName;


internal [](start = 8, length = 8)

private?

See: dotnet/roslyn#49834

The note regarding the necessity of `is_global = true` in files named `.globalconfig` is incorrect since dotnet/roslyn#49834.

chsienki added Area-Compilers Feature - Editor Config labels Dec 7, 2020

chsienki requested a review from a team as a code owner December 7, 2020 22:08

Add extra test for non-well formed configs

2a64b0b

Fix formatting

1dce77f

chsienki force-pushed the global_config_levels branch from 0283414 to 1dce77f Compare December 8, 2020 01:50

runfoapp bot mentioned this pull request Dec 8, 2020

Analyzer diagnostics cause CI to fail #49021

Closed

mavasani mentioned this pull request Dec 11, 2020

Add support for category-specific AnalysisLevel properties dotnet/roslyn-analyzers#4556

Merged

cston reviewed Dec 16, 2020

View reviewed changes

src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs Outdated Show resolved Hide resolved

cston reviewed Dec 16, 2020

View reviewed changes

src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs Outdated Show resolved Hide resolved

cston reviewed Dec 16, 2020

View reviewed changes

Pr feedback: fix typos

f6ef278

cston reviewed Dec 16, 2020

View reviewed changes

PR Feedback: hold onto the key objects to simplify later lookups

8e6639d

cston reviewed Dec 17, 2020

View reviewed changes

PR Feedback: remove unneeded code.

7f764fa

jaredpar approved these changes Dec 18, 2020

View reviewed changes

PR feedback

f0315f9

cston reviewed Dec 18, 2020

View reviewed changes

Private member

c4e4cb9

cston approved these changes Dec 18, 2020

View reviewed changes

Formatting

b2e7bbf

chsienki changed the base branch from master to release/dev16.9 December 19, 2020 00:21

chsienki merged commit cf42e6b into dotnet:release/dev16.9 Dec 19, 2020

mavasani mentioned this pull request Jan 14, 2021

TreatWarningsAsErrors ignored when severity set in EditorConfig #43051

Closed

reduckted mentioned this pull request Feb 15, 2021

Global analyzer config precedence #48634

Closed

mavasani mentioned this pull request May 20, 2021

Define spec for NuGet packages providing .editorconfig defaults #19028

Open

3 tasks

chsienki mentioned this pull request Mar 11, 2022

Treat files copied from a NuGet package content as generated #55992

Closed

alexrp added a commit to vezel-dev/zig-sdk that referenced this pull request Mar 29, 2022

Remove unnecessary directive from .globalconfig.

4026d09

See: dotnet/roslyn#49834

alexrp added a commit to vezel-dev/cathode that referenced this pull request Mar 29, 2022

Remove unnecessary directive from .globalconfig.

c77f27b

See: dotnet/roslyn#49834

kriwar mentioned this pull request May 12, 2022

docs: fix info regarding is_global entry in .globalconfig dotnet/docs#29430

Merged

gewarren pushed a commit to dotnet/docs that referenced this pull request May 16, 2022

docs: fix info regarding is_global entry in .globalconfig (#29430)

70fd2ca

The note regarding the necessity of `is_global = true` in files named `.globalconfig` is incorrect since dotnet/roslyn#49834.

belav mentioned this pull request May 10, 2023

Feature - distribute analyzer config that disables conflicting rules belav/csharpier#896

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support global analyzer config levels: #49834

Support global analyzer config levels: #49834

chsienki commented Dec 7, 2020

chsienki commented Dec 7, 2020

mavasani commented Dec 7, 2020

mavasani commented Dec 11, 2020

cston Dec 16, 2020

chsienki Dec 16, 2020

cston Dec 16, 2020 •

edited

Loading

chsienki Dec 16, 2020

cston Dec 16, 2020 •

edited

Loading

cston Dec 16, 2020

chsienki Dec 17, 2020 •

edited by cston

Loading

cston Dec 17, 2020

chsienki Dec 18, 2020

cston Dec 17, 2020 •

edited

Loading

cston Dec 17, 2020 •

edited

Loading

chsienki Dec 17, 2020

jaredpar Dec 18, 2020

jaredpar Dec 18, 2020

chsienki Dec 18, 2020

jaredpar Dec 18, 2020

jaredpar Dec 18, 2020

cston Dec 18, 2020

Support global analyzer config levels: #49834

Support global analyzer config levels: #49834

Conversation

chsienki commented Dec 7, 2020

chsienki commented Dec 7, 2020

mavasani commented Dec 7, 2020

mavasani commented Dec 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chsienki Dec 17, 2020 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

cston Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Dec 16, 2020 •

edited

Loading

cston Dec 16, 2020 •

edited

Loading

chsienki Dec 17, 2020 •

edited by cston

Loading

cston Dec 17, 2020 •

edited

Loading

cston Dec 17, 2020 •

edited

Loading