From 750cc9995a477626bccd3c57e3d95d601ad7b5b2 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Mon, 7 Dec 2020 13:30:47 -0800 Subject: [PATCH 1/9] Support global analyzer config levels: - 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 --- .../Test/CommandLine/CommandLineTests.cs | 4 + .../Analyzers/AnalyzerConfigTests.cs | 228 ++++++++++++++++++ .../Portable/CommandLine/AnalyzerConfig.cs | 50 +++- .../Portable/CommandLine/AnalyzerConfigSet.cs | 49 ++-- 4 files changed, 310 insertions(+), 21 deletions(-) diff --git a/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs b/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs index ec9d6162d20eb..701b877900094 100644 --- a/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs +++ b/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs @@ -12968,11 +12968,13 @@ class C var analyzerConfigFile = dir.CreateFile(".globalconfig"); var analyzerConfig = analyzerConfigFile.WriteAllText(@" is_global = true +global_level = 100 option1 = abc"); var analyzerConfigFile2 = dir.CreateFile(".globalconfig2"); var analyzerConfig2 = analyzerConfigFile2.WriteAllText(@" is_global = true +global_level = 100 option1 = def"); var output = VerifyOutput(dir, src, additionalFlags: new[] { "/analyzerconfig:" + analyzerConfig.Path + "," + analyzerConfig2.Path }, expectedWarningCount: 1, includeCurrentAssemblyAsAnalyzerReference: false); @@ -12985,11 +12987,13 @@ class C analyzerConfig = analyzerConfigFile.WriteAllText(@" is_global = true +global_level = 100 [/file.cs] option1 = abc"); analyzerConfig2 = analyzerConfigFile2.WriteAllText(@" is_global = true +global_level = 100 [/file.cs] option1 = def"); diff --git a/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs b/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs index d3e47d7a43aac..91b4059094ce8 100644 --- a/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs +++ b/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs @@ -2175,6 +2175,234 @@ 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)] + [InlineData("/.GLOBALCONFIG", true)] + [InlineData("/.glObalConfiG", true)] + [InlineData("/path/to/.globalconfig", true)] + [InlineData("/my.globalconfig", false)] + [InlineData("/globalconfig", false)] + [InlineData("/path/to/globalconfig", false)] + [InlineData("/path/to/my.globalconfig", false)] + [InlineData("/.editorconfig", false)] + public void FileNameCausesConfigToBeReportedAsGlobal(string fileName, bool shouldBeTreatedAsGlobal) + { + var config = Parse("", fileName); + Assert.Equal(shouldBeTreatedAsGlobal, config.IsGlobal); + } + + [Fact] + public void GlobalLevelCanBeReadFromConfig() + { + var config = Parse("global_level = 5", "/.globalconfig"); + + Assert.True(config.IsGlobal); + Assert.Equal(5, config.GlobalLevel); + } + + [Fact] + public void GlobalLevelDefaultsTo100ForUserGlobalConfigs() + { + var config = Parse("", "/"+AnalyzerConfig.UserGlobalConfigName); + + Assert.True(config.IsGlobal); + Assert.Equal(100, config.GlobalLevel); + } + + [Fact] + public void GlobalLevelDefaultsToZeroForNonUserGlobalConfigs() + { + var config = Parse("is_global = true", "/.nugetconfig"); + + Assert.True(config.IsGlobal); + Assert.Equal(0, config.GlobalLevel); + } + + [Fact] + public void GlobalLevelIsNotPresentInConfigSet() + { + var config = Parse("global_level = 123", "/.globalconfig"); + + var set = AnalyzerConfigSet.Create(ImmutableArray.Create(config)); + var globalOptions = set.GlobalConfigOptions; + + Assert.Empty(globalOptions.AnalyzerOptions); + Assert.Empty(globalOptions.TreeOptions); + Assert.Empty(globalOptions.Diagnostics); + } + + [Fact] + public void GlobalLevelInSectionIsPresentInConfigSet() + { + var config = Parse(@" +[/path] +global_level = 123", "/.globalconfig"); + + var set = AnalyzerConfigSet.Create(ImmutableArray.Create(config)); + var globalOptions = set.GlobalConfigOptions; + + Assert.Empty(globalOptions.AnalyzerOptions); + Assert.Empty(globalOptions.TreeOptions); + Assert.Empty(globalOptions.Diagnostics); + + var sectionOptions = set.GetOptionsForSourcePath("/path"); + + Assert.Single(sectionOptions.AnalyzerOptions); + Assert.Equal("123", sectionOptions.AnalyzerOptions["global_level"]); + Assert.Empty(sectionOptions.TreeOptions); + Assert.Empty(sectionOptions.Diagnostics); + } + + [Theory] + [InlineData(1, 2)] + [InlineData(2, 1)] + [InlineData(-2, -1)] + [InlineData(2, -1)] + public void GlobalLevelAllowsOverrideOfGlobalKeys(int level1, int level2) + { + var configs = ArrayBuilder.GetInstance(); + configs.Add(Parse($@" +is_global = true +global_level = {level1} +option1 = value1 +", "/.globalconfig1")); + + configs.Add(Parse($@" +is_global = true +global_level = {level2} +option1 = value2", +"/.globalconfig2")); + + var globalConfig = AnalyzerConfigSet.MergeGlobalConfigs(configs, out var diagnostics); + diagnostics.Verify(); + + Assert.Single(globalConfig.GlobalSection.Properties.Keys, "option1"); + + string expectedValue = level1 > level2 ? "value1" : "value2"; + Assert.Single(globalConfig.GlobalSection.Properties.Values, expectedValue); + + configs.Free(); + } + + [Fact] + public void GlobalLevelAllowsOverrideOfSectionKeys() + { + var configs = ArrayBuilder.GetInstance(); + configs.Add(Parse(@" +is_global = true +global_level = 1 + +[/path] +option1 = value1 +", "/.globalconfig1")); + + configs.Add(Parse(@" +is_global = true +global_level = 2 + +[/path] +option1 = value2", +"/.globalconfig2")); + + var globalConfig = AnalyzerConfigSet.MergeGlobalConfigs(configs, out var diagnostics); + diagnostics.Verify(); + + Assert.Single(globalConfig.NamedSections); + Assert.Equal("/path", globalConfig.NamedSections[0].Name); + Assert.Single(globalConfig.NamedSections[0].Properties.Keys, "option1"); + Assert.Single(globalConfig.NamedSections[0].Properties.Values, "value2"); + + configs.Free(); + } + + [Theory] + [InlineData(1, 2, 3, "value3")] + [InlineData(2, 1, 3, "value3")] + [InlineData(3, 2, 1, "value1")] + [InlineData(1, 2, 1, "value2")] + [InlineData(1, 1, 2, "value3")] + [InlineData(2, 1, 1, "value1")] + public void GlobalLevelAllowsOverrideOfDuplicateGlobalKeys(int level1, int level2, int level3, string expectedValue) + { + var configs = ArrayBuilder.GetInstance(); + configs.Add(Parse($@" +is_global = true +global_level = {level1} +option1 = value1 +", "/.globalconfig1")); + + configs.Add(Parse($@" +is_global = true +global_level = {level2} +option1 = value2", +"/.globalconfig2")); + + configs.Add(Parse($@" +is_global = true +global_level = {level3} +option1 = value3", +"/.globalconfig3")); + + var globalConfig = AnalyzerConfigSet.MergeGlobalConfigs(configs, out var diagnostics); + diagnostics.Verify(); + + Assert.Single(globalConfig.GlobalSection.Properties.Keys, "option1"); + Assert.Single(globalConfig.GlobalSection.Properties.Values, expectedValue); + + configs.Free(); + } + + [Fact] + public void GlobalLevelReportsConflictsOnlyAtTheHighestLevel() + { + var configs = ArrayBuilder.GetInstance(); + configs.Add(Parse($@" +is_global = true +global_level = 1 +option1 = value1 +", "/.globalconfig1")); + + configs.Add(Parse($@" +is_global = true +global_level = 1 +option1 = value2", +"/.globalconfig2")); + + configs.Add(Parse($@" +is_global = true +global_level = 3 +option1 = value3", +"/.globalconfig3")); + + configs.Add(Parse($@" +is_global = true +global_level = 3 +option1 = value4", +"/.globalconfig4")); + + configs.Add(Parse($@" +is_global = true +global_level = 2 +option1 = value5", +"/.globalconfig5")); + + configs.Add(Parse($@" +is_global = true +global_level = 2 +option1 = value6", +"/.globalconfig6")); + + var globalConfig = AnalyzerConfigSet.MergeGlobalConfigs(configs, out var diagnostics); + + // we don't report config1, 2, 5, or 6, because they didn't conflict: 3 + 4 overrode them, but then themselves were conflicting + diagnostics.Verify( + Diagnostic("MultipleGlobalAnalyzerKeys").WithArguments("option1", "Global Section", "/.globalconfig3, /.globalconfig4").WithLocation(1, 1) + ); + + configs.Free(); + } + + #endregion } } diff --git a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs index 895e515ea2a49..c7347c0e15b2b 100644 --- a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs +++ b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs @@ -28,6 +28,16 @@ public sealed partial class AnalyzerConfig /// internal const string GlobalKey = "is_global"; + /// + /// Key that indicates the precedence of this config when is true + /// + internal const string GlobalLevelKey = "global_level"; + + /// + /// Filename that indicates this file is a user provided global config + /// + internal const string UserGlobalConfigName = ".globalconfig"; + /// /// A set of keys that are reserved for special interpretation for the editorconfig specification. /// All values corresponding to reserved keys in a (key,value) property pair are always lowercased @@ -86,7 +96,43 @@ public sealed partial class AnalyzerConfig /// /// Gets whether this editorconfig is a global editorconfig. /// - internal bool IsGlobal => GlobalSection.Properties.ContainsKey(GlobalKey); + internal bool IsGlobal => IsGlobalFileName(this.PathToFile) || GlobalSection.Properties.ContainsKey(GlobalKey); + + /// + /// Get the global level of this config, used to resolve conflicting keys + /// + /// + /// A user can explicitly set the global level via the . + /// When no global level is explicitly set, we use a heuristic: + /// + /// + /// Any file matching the is determined to be a user supplied global config and gets a level of 100 + /// + /// + /// Any other file gets a default level of 0 + /// + /// + /// + /// This value is unsued when is false; + /// + internal int GlobalLevel + { + get + { + if (GlobalSection.Properties.TryGetValue(GlobalLevelKey, out string? val) && int.TryParse(val, out int level)) + { + return level; + } + else if (IsGlobalFileName(this.PathToFile)) + { + return 100; + } + else + { + return 0; + } + } + } private AnalyzerConfig( Section globalSection, @@ -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); + /// /// Represents a named section of the editorconfig file, which consists of a name followed by a set /// of key-value pairs. diff --git a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs index e8b4ab4940166..f6905faf12d6a 100644 --- a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs +++ b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs @@ -481,8 +481,8 @@ internal static GlobalAnalyzerConfig MergeGlobalConfigs(ArrayBuilder internal struct GlobalAnalyzerConfigBuilder { - private ImmutableDictionary.Builder>.Builder? _values; - private ImmutableDictionary>.Builder>.Builder? _duplicates; + private ImmutableDictionary.Builder>.Builder? _values; + private ImmutableDictionary configPaths)>.Builder>.Builder? _duplicates; internal const string GlobalConfigPath = ""; internal const string GlobalSectionName = "Global Section"; @@ -491,16 +491,16 @@ internal void MergeIntoGlobalConfig(AnalyzerConfig config, DiagnosticBag diagnos { if (_values is null) { - _values = ImmutableDictionary.CreateBuilder.Builder>(Section.NameEqualityComparer); - _duplicates = ImmutableDictionary.CreateBuilder>.Builder>(Section.NameEqualityComparer); + _values = ImmutableDictionary.CreateBuilder.Builder>(Section.NameEqualityComparer); + _duplicates = ImmutableDictionary.CreateBuilder)>.Builder>(Section.NameEqualityComparer); } - MergeSection(config.PathToFile, config.GlobalSection, isGlobalSection: true); + MergeSection(config.PathToFile, config.GlobalSection, config.GlobalLevel, isGlobalSection: true); foreach (var section in config.NamedSections) { if (IsAbsoluteEditorConfigPath(section.Name)) { - MergeSection(config.PathToFile, section, isGlobalSection: false); + MergeSection(config.PathToFile, section, config.GlobalLevel, isGlobalSection: false); } else { @@ -525,7 +525,7 @@ internal GlobalAnalyzerConfig Build(DiagnosticBag diagnostics) { bool isGlobalSection = string.IsNullOrWhiteSpace(section); string sectionName = isGlobalSection ? GlobalSectionName : section; - foreach ((var keyName, var configPaths) in keys) + foreach ((var keyName, (_, var configPaths)) in keys) { diagnostics.Add(Diagnostic.Create( MultipleGlobalAnalyzerKeysDescriptor, @@ -562,21 +562,21 @@ private Section GetSection(string sectionName) return new Section(sectionName, result); } - private void MergeSection(string configPath, Section section, bool isGlobalSection) + private void MergeSection(string configPath, Section section, int globalLevel, bool isGlobalSection) { Debug.Assert(_values is object); Debug.Assert(_duplicates is object); if (!_values.TryGetValue(section.Name, out var sectionDict)) { - sectionDict = ImmutableDictionary.CreateBuilder(Section.PropertiesKeyComparer); + sectionDict = ImmutableDictionary.CreateBuilder(Section.PropertiesKeyComparer); _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; } @@ -584,30 +584,39 @@ private void MergeSection(string configPath, Section section, bool isGlobalSecti bool keyInSection = sectionDict.ContainsKey(key); bool keyDuplicated = duplicateDict?.ContainsKey(key) ?? false; - // if this key is neither already present, or already duplicate, we can add it - if (!keyInSection && !keyDuplicated) - { - sectionDict.Add(key, (value, configPath)); + if ((!keyInSection && !keyDuplicated) // this key is neither present nor a duplicate, we can add it + || (keyInSection && sectionDict[key].globalLevel < globalLevel) // this key should override the existing section one + || (keyDuplicated && duplicateDict![key].globalLevel < globalLevel)) // this key should override previous duplicates + { + sectionDict[key] = (value, configPath, globalLevel); + + if (keyDuplicated) + { + duplicateDict!.Remove(key); + } } - else + else if((keyDuplicated && duplicateDict![key].globalLevel == globalLevel) // this key is another duplicate at the same level + || (keyInSection && sectionDict[key].globalLevel == globalLevel)) // this key is a duplicate of a current section key { if (duplicateDict is null) { - duplicateDict = ImmutableDictionary.CreateBuilder>(Section.PropertiesKeyComparer); + duplicateDict = ImmutableDictionary.CreateBuilder)>(Section.PropertiesKeyComparer); _duplicates.Add(section.Name, duplicateDict); } // record that this key is now a duplicate - ArrayBuilder configList = keyDuplicated ? duplicateDict[key] : ArrayBuilder.GetInstance(); + ArrayBuilder configList = keyDuplicated ? duplicateDict[key].configPaths : ArrayBuilder.GetInstance(); configList.Add(configPath); - duplicateDict[key] = configList; + duplicateDict[key] = (globalLevel, configList); // if we'd previously added this key, remove it and remember the extra duplicate location if (keyInSection) { - var originalConfigPath = sectionDict[key].configPath; + var originalEntry = sectionDict[key]; + Debug.Assert(originalEntry.globalLevel == globalLevel); + sectionDict.Remove(key); - duplicateDict[key].Insert(0, originalConfigPath); + duplicateDict[key].configPaths.Insert(0, originalEntry.configPath); } } } From 2a64b0b6dcb0ca34db247bd59bddc0bcaa0c8356 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Mon, 7 Dec 2020 14:26:40 -0800 Subject: [PATCH 2/9] Add extra test for non-well formed configs --- .../Analyzers/AnalyzerConfigTests.cs | 31 ++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs b/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs index 91b4059094ce8..45a20b52fc0eb 100644 --- a/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs +++ b/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs @@ -2192,11 +2192,9 @@ public void FileNameCausesConfigToBeReportedAsGlobal(string fileName, bool shoul } [Fact] - public void GlobalLevelCanBeReadFromConfig() + public void GlobalLevelCanBeReadFromAnyConfig() { - var config = Parse("global_level = 5", "/.globalconfig"); - - Assert.True(config.IsGlobal); + var config = Parse("global_level = 5", "/.editorconfig"); Assert.Equal(5, config.GlobalLevel); } @@ -2209,6 +2207,15 @@ public void GlobalLevelDefaultsTo100ForUserGlobalConfigs() Assert.Equal(100, config.GlobalLevel); } + [Fact] + public void GlobalLevelCanBeOverriddenForUserGlobalConfigs() + { + var config = Parse("global_level = 5", "/" + AnalyzerConfig.UserGlobalConfigName); + + Assert.True(config.IsGlobal); + Assert.Equal(5, config.GlobalLevel); + } + [Fact] public void GlobalLevelDefaultsToZeroForNonUserGlobalConfigs() { @@ -2402,6 +2409,22 @@ public void GlobalLevelReportsConflictsOnlyAtTheHighestLevel() configs.Free(); } + [Fact] + public void InvalidGlobalLevelIsIgnored() + { + var userGlobalConfig = Parse($@" +is_global = true +global_level = abc +", "/.globalconfig"); + + var nonUserGlobalConfig = Parse($@" +is_global = true +global_level = abc +", "/.editorconfig"); + + Assert.Equal(100, userGlobalConfig.GlobalLevel); + Assert.Equal(0, nonUserGlobalConfig.GlobalLevel); + } #endregion } From 1dce77f1131f47d48d7f13a0ad2e2df277a5ca26 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Mon, 7 Dec 2020 15:00:44 -0800 Subject: [PATCH 3/9] Fix formatting --- .../Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs | 2 +- src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs b/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs index 45a20b52fc0eb..b2469fdf362bb 100644 --- a/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs +++ b/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs @@ -2201,7 +2201,7 @@ public void GlobalLevelCanBeReadFromAnyConfig() [Fact] public void GlobalLevelDefaultsTo100ForUserGlobalConfigs() { - var config = Parse("", "/"+AnalyzerConfig.UserGlobalConfigName); + var config = Parse("", "/" + AnalyzerConfig.UserGlobalConfigName); Assert.True(config.IsGlobal); Assert.Equal(100, config.GlobalLevel); diff --git a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs index f6905faf12d6a..01c4672cc3dbe 100644 --- a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs +++ b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs @@ -587,7 +587,7 @@ private void MergeSection(string configPath, Section section, int globalLevel, b if ((!keyInSection && !keyDuplicated) // this key is neither present nor a duplicate, we can add it || (keyInSection && sectionDict[key].globalLevel < globalLevel) // this key should override the existing section one || (keyDuplicated && duplicateDict![key].globalLevel < globalLevel)) // this key should override previous duplicates - { + { sectionDict[key] = (value, configPath, globalLevel); if (keyDuplicated) @@ -595,7 +595,7 @@ private void MergeSection(string configPath, Section section, int globalLevel, b duplicateDict!.Remove(key); } } - else if((keyDuplicated && duplicateDict![key].globalLevel == globalLevel) // this key is another duplicate at the same level + else if ((keyDuplicated && duplicateDict![key].globalLevel == globalLevel) // this key is another duplicate at the same level || (keyInSection && sectionDict[key].globalLevel == globalLevel)) // this key is a duplicate of a current section key { if (duplicateDict is null) From f6ef278100b3e0590efa84858238611a8f0aa886 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Wed, 16 Dec 2020 13:27:40 -0800 Subject: [PATCH 4/9] Pr feedback: fix typos --- src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs index c7347c0e15b2b..be341180bbdf3 100644 --- a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs +++ b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs @@ -113,7 +113,7 @@ public sealed partial class AnalyzerConfig /// /// /// - /// This value is unsued when is false; + /// This value is unused when is false. /// internal int GlobalLevel { From 8e6639dd91ea3a8b77c395ba2bcf1c453e21300f Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Wed, 16 Dec 2020 17:40:51 -0800 Subject: [PATCH 5/9] PR Feedback: hold onto the key objects to simplify later lookups --- .../Portable/CommandLine/AnalyzerConfigSet.cs | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs index 01c4672cc3dbe..2294db28cc43b 100644 --- a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs +++ b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs @@ -581,12 +581,14 @@ private void MergeSection(string configPath, Section section, int globalLevel, b continue; } - bool keyInSection = sectionDict.ContainsKey(key); - bool keyDuplicated = duplicateDict?.ContainsKey(key) ?? false; + bool keyInSection = sectionDict.TryGetValue(key, out var sectionValue) == true; + + (int globalLevel, ArrayBuilder configPaths) duplicateValue = default; + bool keyDuplicated = duplicateDict?.TryGetValue(key, out duplicateValue) == true; if ((!keyInSection && !keyDuplicated) // this key is neither present nor a duplicate, we can add it - || (keyInSection && sectionDict[key].globalLevel < globalLevel) // this key should override the existing section one - || (keyDuplicated && duplicateDict![key].globalLevel < globalLevel)) // this key should override previous duplicates + || (keyInSection && sectionValue.globalLevel < globalLevel) // this key should override the existing section one + || (keyDuplicated && duplicateValue.globalLevel < globalLevel)) // this key should override previous duplicates { sectionDict[key] = (value, configPath, globalLevel); @@ -595,8 +597,8 @@ private void MergeSection(string configPath, Section section, int globalLevel, b duplicateDict!.Remove(key); } } - else if ((keyDuplicated && duplicateDict![key].globalLevel == globalLevel) // this key is another duplicate at the same level - || (keyInSection && sectionDict[key].globalLevel == globalLevel)) // this key is a duplicate of a current section key + else if ((keyDuplicated && duplicateValue.globalLevel == globalLevel) // this key is another duplicate at the same level + || (keyInSection && sectionValue.globalLevel == globalLevel)) // this key is a duplicate of a current section key { if (duplicateDict is null) { @@ -605,18 +607,18 @@ private void MergeSection(string configPath, Section section, int globalLevel, b } // record that this key is now a duplicate - ArrayBuilder configList = keyDuplicated ? duplicateDict[key].configPaths : ArrayBuilder.GetInstance(); + ArrayBuilder configList = duplicateValue.configPaths ?? ArrayBuilder.GetInstance(); configList.Add(configPath); duplicateDict[key] = (globalLevel, configList); // if we'd previously added this key, remove it and remember the extra duplicate location if (keyInSection) { - var originalEntry = sectionDict[key]; + var originalEntry = sectionValue; Debug.Assert(originalEntry.globalLevel == globalLevel); sectionDict.Remove(key); - duplicateDict[key].configPaths.Insert(0, originalEntry.configPath); + configList.Insert(0, originalEntry.configPath); } } } From 7f764fadb59056eeec0398516f542c0850976740 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Thu, 17 Dec 2020 10:49:40 -0800 Subject: [PATCH 6/9] PR Feedback: remove unneeded code. --- src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs index 2294db28cc43b..27446115db27a 100644 --- a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs +++ b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs @@ -581,7 +581,7 @@ private void MergeSection(string configPath, Section section, int globalLevel, b continue; } - bool keyInSection = sectionDict.TryGetValue(key, out var sectionValue) == true; + bool keyInSection = sectionDict.TryGetValue(key, out var sectionValue); (int globalLevel, ArrayBuilder configPaths) duplicateValue = default; bool keyDuplicated = duplicateDict?.TryGetValue(key, out duplicateValue) == true; From f0315f9dce2e7ea1da736ad9e8cff0afb0accb0c Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Fri, 18 Dec 2020 14:52:24 -0800 Subject: [PATCH 7/9] PR feedback --- .../Analyzers/AnalyzerConfigTests.cs | 1 + .../Portable/CommandLine/AnalyzerConfig.cs | 9 +-- .../Portable/CommandLine/AnalyzerConfigSet.cs | 60 +++++++++++-------- 3 files changed, 40 insertions(+), 30 deletions(-) diff --git a/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs b/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs index b2469fdf362bb..1624752b3330e 100644 --- a/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs +++ b/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs @@ -2185,6 +2185,7 @@ public void GlobalConfigIssuesWarningWithInvalidSectionNames_PlatformSpecific(st [InlineData("/path/to/globalconfig", false)] [InlineData("/path/to/my.globalconfig", false)] [InlineData("/.editorconfig", false)] + [InlineData("/.globalconfİg", false)] public void FileNameCausesConfigToBeReportedAsGlobal(string fileName, bool shouldBeTreatedAsGlobal) { var config = Parse("", fileName); diff --git a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs index be341180bbdf3..38ea614e0b2ef 100644 --- a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs +++ b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs @@ -96,7 +96,7 @@ public sealed partial class AnalyzerConfig /// /// Gets whether this editorconfig is a global editorconfig. /// - internal bool IsGlobal => IsGlobalFileName(this.PathToFile) || GlobalSection.Properties.ContainsKey(GlobalKey); + internal bool IsGlobal => _hasGlobalFileName || GlobalSection.Properties.ContainsKey(GlobalKey); /// /// Get the global level of this config, used to resolve conflicting keys @@ -123,7 +123,7 @@ internal int GlobalLevel { return level; } - else if (IsGlobalFileName(this.PathToFile)) + else if (_hasGlobalFileName) { return 100; } @@ -134,6 +134,8 @@ internal int GlobalLevel } } + internal readonly bool _hasGlobalFileName; + private AnalyzerConfig( Section globalSection, ImmutableArray
namedSections, @@ -142,6 +144,7 @@ private AnalyzerConfig( GlobalSection = globalSection; NamedSections = namedSections; PathToFile = pathToFile; + _hasGlobalFileName = Path.GetFileName(pathToFile).Equals(UserGlobalConfigName, StringComparison.OrdinalIgnoreCase); // Find the containing directory and normalize the path separators string directory = Path.GetDirectoryName(pathToFile) ?? pathToFile; @@ -264,8 +267,6 @@ private static bool IsComment(string line) return false; } - private static bool IsGlobalFileName(string fileName) => Path.GetFileName(fileName).Equals(UserGlobalConfigName, StringComparison.OrdinalIgnoreCase); - /// /// Represents a named section of the editorconfig file, which consists of a name followed by a set /// of key-value pairs. diff --git a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs index 27446115db27a..44afe2036ed1d 100644 --- a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs +++ b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs @@ -584,41 +584,49 @@ private void MergeSection(string configPath, Section section, int globalLevel, b bool keyInSection = sectionDict.TryGetValue(key, out var sectionValue); (int globalLevel, ArrayBuilder configPaths) duplicateValue = default; - bool keyDuplicated = duplicateDict?.TryGetValue(key, out duplicateValue) == true; + bool keyDuplicated = !keyInSection && duplicateDict?.TryGetValue(key, out duplicateValue) == true; - if ((!keyInSection && !keyDuplicated) // this key is neither present nor a duplicate, we can add it - || (keyInSection && sectionValue.globalLevel < globalLevel) // this key should override the existing section one - || (keyDuplicated && duplicateValue.globalLevel < globalLevel)) // this key should override previous duplicates + // if this key is neither already present, or already duplicate, we can add it + if (!keyInSection && !keyDuplicated) { - sectionDict[key] = (value, configPath, globalLevel); - - if (keyDuplicated) - { - duplicateDict!.Remove(key); - } + sectionDict.Add(key, (value, configPath, globalLevel)); } - else if ((keyDuplicated && duplicateValue.globalLevel == globalLevel) // this key is another duplicate at the same level - || (keyInSection && sectionValue.globalLevel == globalLevel)) // this key is a duplicate of a current section key + else { - if (duplicateDict is null) + int currentGlobalLevel = keyInSection ? sectionValue.globalLevel : duplicateValue.globalLevel; + + // if this key overrides one we knew about previously, replace it + if (currentGlobalLevel < globalLevel) { - duplicateDict = ImmutableDictionary.CreateBuilder)>(Section.PropertiesKeyComparer); - _duplicates.Add(section.Name, duplicateDict); + sectionDict[key] = (value, configPath, globalLevel); + if (keyDuplicated) + { + duplicateDict!.Remove(key); + } } + // this key conflicts with a previous one + else if(currentGlobalLevel == globalLevel) + { + if (duplicateDict is null) + { + duplicateDict = ImmutableDictionary.CreateBuilder)>(Section.PropertiesKeyComparer); + _duplicates.Add(section.Name, duplicateDict); + } - // record that this key is now a duplicate - ArrayBuilder configList = duplicateValue.configPaths ?? ArrayBuilder.GetInstance(); - configList.Add(configPath); - duplicateDict[key] = (globalLevel, configList); + // record that this key is now a duplicate + ArrayBuilder configList = duplicateValue.configPaths ?? ArrayBuilder.GetInstance(); + configList.Add(configPath); + duplicateDict[key] = (globalLevel, configList); - // if we'd previously added this key, remove it and remember the extra duplicate location - if (keyInSection) - { - var originalEntry = sectionValue; - Debug.Assert(originalEntry.globalLevel == globalLevel); + // if we'd previously added this key, remove it and remember the extra duplicate location + if (keyInSection) + { + var originalEntry = sectionValue; + Debug.Assert(originalEntry.globalLevel == globalLevel); - sectionDict.Remove(key); - configList.Insert(0, originalEntry.configPath); + sectionDict.Remove(key); + configList.Insert(0, originalEntry.configPath); + } } } } From c4e4cb9df3d1932505c6b960994e32640ef160a5 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Fri, 18 Dec 2020 15:05:10 -0800 Subject: [PATCH 8/9] Private member --- src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs index 38ea614e0b2ef..a1c4f920e136e 100644 --- a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs +++ b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs @@ -134,7 +134,7 @@ internal int GlobalLevel } } - internal readonly bool _hasGlobalFileName; + private readonly bool _hasGlobalFileName; private AnalyzerConfig( Section globalSection, From b2e7bbfd69e2dc7daea536e43c4001bcb7a55234 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Fri, 18 Dec 2020 16:18:58 -0800 Subject: [PATCH 9/9] Formatting --- src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs index 44afe2036ed1d..b6ac8fe1ab9e2 100644 --- a/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs +++ b/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs @@ -605,7 +605,7 @@ private void MergeSection(string configPath, Section section, int globalLevel, b } } // this key conflicts with a previous one - else if(currentGlobalLevel == globalLevel) + else if (currentGlobalLevel == globalLevel) { if (duplicateDict is null) {