-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Support global analyzer config levels: #49834
Changes from all commits
750cc99
2a64b0b
1dce77f
f6ef278
8e6639d
7f764fa
f0315f9
c4e4cb9
b2e7bbf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2175,6 +2175,258 @@ 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)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a case where we use a Turkish i ( |
||
[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)] | ||
[InlineData("/.globalconfİg", false)] | ||
public void FileNameCausesConfigToBeReportedAsGlobal(string fileName, bool shouldBeTreatedAsGlobal) | ||
{ | ||
var config = Parse("", fileName); | ||
Assert.Equal(shouldBeTreatedAsGlobal, config.IsGlobal); | ||
} | ||
|
||
[Fact] | ||
public void GlobalLevelCanBeReadFromAnyConfig() | ||
{ | ||
var config = Parse("global_level = 5", "/.editorconfig"); | ||
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 GlobalLevelCanBeOverriddenForUserGlobalConfigs() | ||
{ | ||
var config = Parse("global_level = 5", "/" + AnalyzerConfig.UserGlobalConfigName); | ||
|
||
Assert.True(config.IsGlobal); | ||
Assert.Equal(5, 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<AnalyzerConfig>.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<AnalyzerConfig>.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<AnalyzerConfig>.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<AnalyzerConfig>.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(); | ||
} | ||
|
||
[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 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,16 @@ public sealed partial class AnalyzerConfig | |
/// </summary> | ||
internal const string GlobalKey = "is_global"; | ||
|
||
/// <summary> | ||
/// Key that indicates the precedence of this config when <see cref="IsGlobal"/> is true | ||
/// </summary> | ||
internal const string GlobalLevelKey = "global_level"; | ||
|
||
/// <summary> | ||
/// Filename that indicates this file is a user provided global config | ||
/// </summary> | ||
internal const string UserGlobalConfigName = ".globalconfig"; | ||
|
||
/// <summary> | ||
/// 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,45 @@ public sealed partial class AnalyzerConfig | |
/// <summary> | ||
/// Gets whether this editorconfig is a global editorconfig. | ||
/// </summary> | ||
internal bool IsGlobal => GlobalSection.Properties.ContainsKey(GlobalKey); | ||
internal bool IsGlobal => _hasGlobalFileName || GlobalSection.Properties.ContainsKey(GlobalKey); | ||
|
||
/// <summary> | ||
/// Get the global level of this config, used to resolve conflicting keys | ||
/// </summary> | ||
/// <remarks> | ||
/// A user can explicitly set the global level via the <see cref="GlobalLevelKey"/>. | ||
/// When no global level is explicitly set, we use a heuristic: | ||
/// <list type="bullet"> | ||
/// <item><description> | ||
/// Any file matching the <see cref="UserGlobalConfigName"/> is determined to be a user supplied global config and gets a level of 100 | ||
/// </description></item> | ||
/// <item><description> | ||
/// Any other file gets a default level of 0 | ||
/// </description></item> | ||
/// </list> | ||
/// | ||
/// This value is unused when <see cref="IsGlobal"/> is <c>false</c>. | ||
/// </remarks> | ||
internal int GlobalLevel | ||
{ | ||
get | ||
{ | ||
if (GlobalSection.Properties.TryGetValue(GlobalLevelKey, out string? val) && int.TryParse(val, out int level)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If this fails, shouldn't we return 100 rather than potentially 0? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
{ | ||
return level; | ||
} | ||
else if (_hasGlobalFileName) | ||
{ | ||
return 100; | ||
} | ||
else | ||
{ | ||
return 0; | ||
} | ||
} | ||
} | ||
|
||
private readonly bool _hasGlobalFileName; | ||
|
||
private AnalyzerConfig( | ||
Section globalSection, | ||
|
@@ -96,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; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, makes sense.