From a68b13956ab1136af91066d1ce61113574447945 Mon Sep 17 00:00:00 2001 From: "Christoph Bergmeister [MVP]" Date: Fri, 8 Mar 2019 05:34:37 +0000 Subject: [PATCH] Fix PSSA for Turkish culture and tests when culture is not en-US (#1099) * Fix Parsing of settings that occurs when using turkish culture * use german culture before running tests to exhibit failures * set ui culture as well * fix communityanalyzer for turkish culture * fix ProvideCommentHelp for turkish culture (i problem again) * update comments * fix for import-localizedDatabug in pscore (opened issue 8219 in ps core repo) * change to UI culture * use invariant culture comparison instead of tolowerinvariant and revert change where it is not needed * finish with comments * Use OrdinalIgnoreCase * revert accidental change from 2 commits before * trigger ci * Apply suggestions from code review Co-Authored-By: bergmeister * Use else-less approach to address PR comment --- Engine/Generic/ModuleDependencyHandler.cs | 2 +- Engine/Generic/RuleSuppression.cs | 115 +++++++------- Engine/ScriptAnalyzer.cs | 140 +++++++++--------- Engine/Settings.cs | 4 +- .../Utility/PlatformNaming.cs | 8 +- Rules/ProvideCommentHelp.cs | 2 +- Rules/UseShouldProcessCorrectly.cs | 2 +- ScriptRuleDocumentation.md | 2 +- .../CommunityAnalyzerRules.psm1 | 18 ++- Tests/Engine/InvokeFormatter.tests.ps1 | 11 ++ tools/appveyor.psm1 | 4 + 11 files changed, 156 insertions(+), 152 deletions(-) diff --git a/Engine/Generic/ModuleDependencyHandler.cs b/Engine/Generic/ModuleDependencyHandler.cs index f107c9a56..6023cd82c 100644 --- a/Engine/Generic/ModuleDependencyHandler.cs +++ b/Engine/Generic/ModuleDependencyHandler.cs @@ -271,7 +271,7 @@ public ModuleDependencyHandler( ? "PSScriptAnalyzer" : pssaAppDataPath); - modulesFound = new Dictionary(); + modulesFound = new Dictionary(StringComparer.OrdinalIgnoreCase); // TODO Add PSSA Version in the path symLinkPath = Path.Combine(pssaAppDataPath, symLinkName); diff --git a/Engine/Generic/RuleSuppression.cs b/Engine/Generic/RuleSuppression.cs index f3bb848ff..9082b2e85 100644 --- a/Engine/Generic/RuleSuppression.cs +++ b/Engine/Generic/RuleSuppression.cs @@ -202,61 +202,56 @@ public RuleSuppression(AttributeAst attrAst, int start, int end) break; } - switch (name.ArgumentName.ToLower()) + string argumentName = name.ArgumentName; + if (argumentName.Equals("rulename", StringComparison.OrdinalIgnoreCase)) { - case "rulename": - if (!String.IsNullOrWhiteSpace(RuleName)) - { - Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name); - } - - RuleName = (name.Argument as StringConstantExpressionAst).Value; - goto default; - - case "rulesuppressionid": - if (!String.IsNullOrWhiteSpace(RuleSuppressionID)) - { - Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name); - } - - RuleSuppressionID = (name.Argument as StringConstantExpressionAst).Value; - goto default; - - case "scope": - if (!String.IsNullOrWhiteSpace(Scope)) - { - Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name); - } - - Scope = (name.Argument as StringConstantExpressionAst).Value; - - if (!scopeSet.Contains(Scope)) - { - Error = Strings.WrongScopeArgumentSuppressionAttributeError; - } + if (!String.IsNullOrWhiteSpace(RuleName)) + { + Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name); + } - goto default; + RuleName = (name.Argument as StringConstantExpressionAst).Value; + } + else if (argumentName.Equals("rulesuppressionid", StringComparison.OrdinalIgnoreCase)) + { + if (!String.IsNullOrWhiteSpace(RuleName)) + { + Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name); + } - case "target": - if (!String.IsNullOrWhiteSpace(Target)) - { - Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name); - } + RuleName = (name.Argument as StringConstantExpressionAst).Value; + } + else if (argumentName.Equals("scope", StringComparison.OrdinalIgnoreCase)) + { + if (!String.IsNullOrWhiteSpace(Scope)) + { + Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name); + } - Target = (name.Argument as StringConstantExpressionAst).Value; - goto default; + Scope = (name.Argument as StringConstantExpressionAst).Value; - case "justification": - if (!String.IsNullOrWhiteSpace(Justification)) - { - Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name); - } + if (!scopeSet.Contains(Scope)) + { + Error = Strings.WrongScopeArgumentSuppressionAttributeError; + } + } + else if (argumentName.Equals("target", StringComparison.OrdinalIgnoreCase)) + { + if (!String.IsNullOrWhiteSpace(Target)) + { + Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name); + } - Justification = (name.Argument as StringConstantExpressionAst).Value; - goto default; + Target = (name.Argument as StringConstantExpressionAst).Value; + } + else if (argumentName.Equals("justification", StringComparison.OrdinalIgnoreCase)) + { + if (!String.IsNullOrWhiteSpace(Justification)) + { + Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name); + } - default: - break; + Justification = (name.Argument as StringConstantExpressionAst).Value; } } } @@ -354,23 +349,17 @@ public static List GetSuppressions(IEnumerable at Regex reg = new Regex(String.Format("^{0}$", ruleSupp.Target.Replace(@"*", ".*")), RegexOptions.IgnoreCase); IEnumerable targetAsts = null; - switch (ruleSupp.Scope.ToLower()) + string scope = ruleSupp.Scope; + if (scope.Equals("function", StringComparison.OrdinalIgnoreCase)) { - case "function": - targetAsts = scopeAst.FindAll(item => item is FunctionDefinitionAst && reg.IsMatch((item as FunctionDefinitionAst).Name), true); - goto default; - - #if !(PSV3||PSV4) - - case "class": - targetAsts = scopeAst.FindAll(item => item is TypeDefinitionAst && reg.IsMatch((item as TypeDefinitionAst).Name), true); - goto default; - - #endif - - default: - break; + targetAsts = scopeAst.FindAll(ast => ast is FunctionDefinitionAst && reg.IsMatch((ast as FunctionDefinitionAst).Name), true); + } + #if !(PSV3 || PSV4) + else if (scope.Equals("class", StringComparison.OrdinalIgnoreCase)) + { + targetAsts = scopeAst.FindAll(ast => ast is TypeDefinitionAst && reg.IsMatch((ast as TypeDefinitionAst).Name), true); } + #endif if (targetAsts != null) { diff --git a/Engine/ScriptAnalyzer.cs b/Engine/ScriptAnalyzer.cs index c8e45eff7..18fc06bf8 100644 --- a/Engine/ScriptAnalyzer.cs +++ b/Engine/ScriptAnalyzer.cs @@ -283,21 +283,18 @@ private bool AddProfileItem( Debug.Assert(includeRuleList != null); Debug.Assert(excludeRuleList != null); - switch (key.ToLower()) + if (key.Equals("severity", StringComparison.OrdinalIgnoreCase)) { - case "severity": - severityList.AddRange(values); - break; - case "includerules": - includeRuleList.AddRange(values); - break; - case "excluderules": - excludeRuleList.AddRange(values); - break; - default: - return false; + severityList.AddRange(values); + return true; } - return true; + if (key.Equals("includerules", StringComparison.OrdinalIgnoreCase)) + { + includeRuleList.AddRange(values); + return true; + } + + return false; } private Dictionary GetDictionaryFromHashTableAst( @@ -510,75 +507,70 @@ private bool ParseProfileHashtable(Hashtable profile, PathIntrinsics path, IOutp settings.Keys.CopyTo(settingsKeys, 0); foreach (var settingKey in settingsKeys) { - var key = settingKey.ToLower(); - object value = settings[key]; - switch (key) - { - case "severity": - case "includerules": - case "excluderules": - // value must be either string or collections of string or array - if (value == null - || !(value is string - || value is IEnumerable - || value.GetType().IsArray)) - { - writer.WriteError( - new ErrorRecord( - new InvalidDataException(string.Format(CultureInfo.CurrentCulture, Strings.WrongValueHashTable, value, key)), - Strings.WrongConfigurationKey, - ErrorCategory.InvalidData, - profile)); - hasError = true; - break; - } - List values = new List(); - if (value is string) - { - values.Add(value as string); - } - else if (value is IEnumerable) - { - values.Union(value as IEnumerable); - } - else if (value.GetType().IsArray) - { - // for array case, sometimes we won't be able to cast it directly to IEnumerable - foreach (var val in value as IEnumerable) - { - if (val is string) - { - values.Add(val as string); - } - else - { - writer.WriteError( - new ErrorRecord( - new InvalidDataException(string.Format(CultureInfo.CurrentCulture, Strings.WrongValueHashTable, val, key)), - Strings.WrongConfigurationKey, - ErrorCategory.InvalidData, - profile)); - hasError = true; - break; - } - } - } - AddProfileItem(key, values, severityList, includeRuleList, excludeRuleList); - settings[key] = values; - break; - - case "rules": - break; + object value = settings[settingKey]; - default: + if (settingKey.Equals("severity", StringComparison.OrdinalIgnoreCase) || + settingKey.Equals("includerules", StringComparison.OrdinalIgnoreCase) || + settingKey.Equals("excluderules", StringComparison.OrdinalIgnoreCase)) + { + // value must be either string or collections of string or array + if (value == null + || !(value is string + || value is IEnumerable + || value.GetType().IsArray)) + { writer.WriteError( new ErrorRecord( - new InvalidDataException(string.Format(CultureInfo.CurrentCulture, Strings.WrongKeyHashTable, key)), + new InvalidDataException(string.Format(CultureInfo.CurrentCulture, Strings.WrongValueHashTable, value, settingKey)), Strings.WrongConfigurationKey, ErrorCategory.InvalidData, profile)); hasError = true; break; + } + var values = new List(); + if (value is string) + { + values.Add(value as string); + } + else if (value is IEnumerable) + { + values.Union(value as IEnumerable); + } + else if (value.GetType().IsArray) + { + // for array case, sometimes we won't be able to cast it directly to IEnumerable + foreach (var val in value as IEnumerable) + { + if (val is string) + { + values.Add(val as string); + } + else + { + writer.WriteError( + new ErrorRecord( + new InvalidDataException(string.Format(CultureInfo.CurrentCulture, Strings.WrongValueHashTable, val, settingKey)), + Strings.WrongConfigurationKey, + ErrorCategory.InvalidData, + profile)); + hasError = true; + break; + } + } + } + AddProfileItem(settingKey, values, severityList, includeRuleList, excludeRuleList); + settings[settingKey] = values; + } + else if (settingKey.Equals("excluderules", StringComparison.OrdinalIgnoreCase)) + { + writer.WriteError( + new ErrorRecord( + new InvalidDataException(string.Format(CultureInfo.CurrentCulture, Strings.WrongKeyHashTable, settingKey)), + Strings.WrongConfigurationKey, + ErrorCategory.InvalidData, + profile)); + hasError = true; } } return hasError; diff --git a/Engine/Settings.cs b/Engine/Settings.cs index 565952c8d..506733cc2 100644 --- a/Engine/Settings.cs +++ b/Engine/Settings.cs @@ -395,7 +395,7 @@ private void parseSettingsHashtable(Hashtable settingsHashtable) var settings = GetDictionaryFromHashtable(settingsHashtable); foreach (var settingKey in settings.Keys) { - var key = settingKey.ToLower(); + var key = settingKey.ToLowerInvariant(); // ToLowerInvariant is important to also work with turkish culture, see https://github.com/PowerShell/PSScriptAnalyzer/issues/1095 object val = settings[key]; switch (key) { @@ -514,7 +514,7 @@ private static object GetSafeValueFromExpressionAst(ExpressionAst exprAst) case VariableExpressionAst varExprAst: // $true and $false are VariableExpressionAsts, so look for them here - switch (varExprAst.VariablePath.UserPath.ToLower()) + switch (varExprAst.VariablePath.UserPath.ToLowerInvariant()) { case "true": return true; diff --git a/PSCompatibilityAnalyzer/Microsoft.PowerShell.CrossCompatibility/Utility/PlatformNaming.cs b/PSCompatibilityAnalyzer/Microsoft.PowerShell.CrossCompatibility/Utility/PlatformNaming.cs index 8fcbb4e8a..68bce3dd7 100644 --- a/PSCompatibilityAnalyzer/Microsoft.PowerShell.CrossCompatibility/Utility/PlatformNaming.cs +++ b/PSCompatibilityAnalyzer/Microsoft.PowerShell.CrossCompatibility/Utility/PlatformNaming.cs @@ -35,10 +35,10 @@ public static string GetPlatformName(PlatformData platform) { string psVersion = platform.PowerShell.Version?.ToString(); string osVersion = platform.OperatingSystem.Version; - string osArch = platform.OperatingSystem.Architecture.ToString().ToLower(); - string pArch = platform.PowerShell.ProcessArchitecture.ToString().ToLower(); - string dotnetVersion = platform.Dotnet.ClrVersion.ToString().ToLower(); - string dotnetEdition = platform.Dotnet.Runtime.ToString().ToLower(); + string osArch = platform.OperatingSystem.Architecture.ToString().ToLowerInvariant(); + string pArch = platform.PowerShell.ProcessArchitecture.ToString().ToLowerInvariant(); + string dotnetVersion = platform.Dotnet.ClrVersion.ToString().ToLowerInvariant(); + string dotnetEdition = platform.Dotnet.Runtime.ToString().ToLowerInvariant(); string[] platformNameComponents; switch (platform.OperatingSystem.Family) diff --git a/Rules/ProvideCommentHelp.cs b/Rules/ProvideCommentHelp.cs index 3384debef..63e8d6438 100644 --- a/Rules/ProvideCommentHelp.cs +++ b/Rules/ProvideCommentHelp.cs @@ -419,7 +419,7 @@ public override string ToString() public virtual string ToString(int? tabStop) { var sb = new StringBuilder(); - sb.Append(".").AppendLine(Name.ToUpper()); + sb.Append(".").AppendLine(Name.ToUpperInvariant()); // ToUpperInvariant is important to also work with turkish culture, see https://github.com/PowerShell/PSScriptAnalyzer/issues/1095 if (!String.IsNullOrWhiteSpace(Description)) { sb.Append(Snippetify(tabStop, Description)); diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index 6be9db87e..6cacc2634 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -451,7 +451,7 @@ public override bool Equals(Object other) /// public override int GetHashCode() { - return name.ToLower().GetHashCode(); + return name.ToLowerInvariant().GetHashCode(); } } diff --git a/ScriptRuleDocumentation.md b/ScriptRuleDocumentation.md index f4f9a5844..9c5adcf4e 100644 --- a/ScriptRuleDocumentation.md +++ b/ScriptRuleDocumentation.md @@ -155,7 +155,7 @@ function Measure-RequiresRunAsAdministrator if ($Ast -is [System.Management.Automation.Language.AssignmentStatementAst]) { [System.Management.Automation.Language.AssignmentStatementAst]$asAst = $Ast - if ($asAst.Right.ToString().ToLower() -eq '[system.security.principal.windowsbuiltinrole]::administrator') + if ($asAst.Right.ToString() -eq '[system.security.principal.windowsbuiltinrole]::administrator') { $returnValue = $true } diff --git a/Tests/Engine/CommunityAnalyzerRules/CommunityAnalyzerRules.psm1 b/Tests/Engine/CommunityAnalyzerRules/CommunityAnalyzerRules.psm1 index d5b39baca..f9abf9950 100644 --- a/Tests/Engine/CommunityAnalyzerRules/CommunityAnalyzerRules.psm1 +++ b/Tests/Engine/CommunityAnalyzerRules/CommunityAnalyzerRules.psm1 @@ -1,7 +1,15 @@ #Requires -Version 3.0 # Import Localized Data -Import-LocalizedData -BindingVariable Messages +# Explicit culture needed for culture that do not match when using PowerShell Core: https://github.com/PowerShell/PowerShell/issues/8219 +if ([System.Threading.Thread]::CurrentThread.CurrentUICulture.Name -ne 'en-US') +{ + Import-LocalizedData -BindingVariable Messages -UICulture 'en-US' +} +else +{ + Import-LocalizedData -BindingVariable Messages +} <# .SYNOPSIS @@ -71,7 +79,7 @@ function Measure-RequiresRunAsAdministrator if ($ast -is [System.Management.Automation.Language.AssignmentStatementAst]) { [System.Management.Automation.Language.AssignmentStatementAst]$asAst = $Ast; - if ($asAst.Right.ToString().ToLower() -eq "[system.security.principal.windowsbuiltinrole]::administrator") + if ($asAst.Right.ToString() -eq "[system.security.principal.windowsbuiltinrole]::administrator") { $returnValue = $true } @@ -167,7 +175,7 @@ function Measure-RequiresModules [System.Management.Automation.Language.CommandAst]$cmdAst = $Ast; if ($null -ne $cmdAst.GetCommandName()) { - if ($cmdAst.GetCommandName().ToLower() -eq "import-module") + if ($cmdAst.GetCommandName() -eq "import-module") { $returnValue = $true } @@ -225,8 +233,8 @@ function Measure-RequiresModules } -# The two rules in the following if block use StaticParameterBinder class. -# StaticParameterBinder class was introduced in PSv4. +# The two rules in the following if block use StaticParameterBinder class. +# StaticParameterBinder class was introduced in PSv4. if ($PSVersionTable.PSVersion -ge [Version]'4.0.0') { <# diff --git a/Tests/Engine/InvokeFormatter.tests.ps1 b/Tests/Engine/InvokeFormatter.tests.ps1 index b516a6655..e1c51be1a 100644 --- a/Tests/Engine/InvokeFormatter.tests.ps1 +++ b/Tests/Engine/InvokeFormatter.tests.ps1 @@ -92,6 +92,17 @@ function foo { Invoke-Formatter -ScriptDefinition $def | Should -Be $expected } + + It "Does not throw when using turkish culture - https://github.com/PowerShell/PSScriptAnalyzer/issues/1095" { + $initialCulture = [System.Threading.Thread]::CurrentThread.CurrentCulture + try { + [System.Threading.Thread]::CurrentThread.CurrentCulture = [cultureinfo]::CreateSpecificCulture('tr-TR') + Invoke-Formatter ' foo' | Should -Be 'foo' + } + finally { + [System.Threading.Thread]::CurrentThread.CurrentCulture = $initialCulture + } + } } } diff --git a/tools/appveyor.psm1 b/tools/appveyor.psm1 index 17a69ff3c..da94c8b9e 100644 --- a/tools/appveyor.psm1 +++ b/tools/appveyor.psm1 @@ -60,6 +60,10 @@ function Invoke-AppveyorTest { $testResultsPath = Join-Path ${CheckoutPath} TestResults.xml $testScripts = "${CheckoutPath}\Tests\Engine","${CheckoutPath}\Tests\Rules","${CheckoutPath}\Tests\Documentation","${CheckoutPath}\PSCompatibilityAnalyzer\Tests" + # Change culture to Turkish to test that PSSA works well with different locales + [System.Threading.Thread]::CurrentThread.CurrentCulture = [cultureinfo]::CreateSpecificCulture('tr-TR') + [System.Threading.Thread]::CurrentThread.CurrentUICulture = [cultureinfo]::CreateSpecificCulture('tr-TR') + # Run all tests $testResults = Invoke-Pester -Script $testScripts -OutputFormat NUnitXml -OutputFile $testResultsPath -PassThru