From e925335ea9d4d23408bd2e996412eca1a08747c0 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 7 Sep 2016 20:57:05 -0700 Subject: [PATCH 1/8] Add rule to check hashtable initialization --- Rules/ScriptAnalyzerBuiltinRules.csproj | 1 + Rules/Strings.Designer.cs | 36 +++++ Rules/Strings.resx | 68 +++++---- Rules/UseLiteralInitializerForHashtable.cs | 168 +++++++++++++++++++++ 4 files changed, 245 insertions(+), 28 deletions(-) create mode 100644 Rules/UseLiteralInitializerForHashtable.cs diff --git a/Rules/ScriptAnalyzerBuiltinRules.csproj b/Rules/ScriptAnalyzerBuiltinRules.csproj index dba1427af..504f683e5 100644 --- a/Rules/ScriptAnalyzerBuiltinRules.csproj +++ b/Rules/ScriptAnalyzerBuiltinRules.csproj @@ -95,6 +95,7 @@ Strings.resx + diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index ffab47fe3..f6788175b 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1824,6 +1824,42 @@ internal static string UseIdenticalParametersDSCName { } } + /// + /// Looks up a localized string similar to Create hashtables with literal initializers. + /// + internal static string UseLiteralInitilializerForHashtableCommonName { + get { + return ResourceManager.GetString("UseLiteralInitilializerForHashtableCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Create a hashtable using a constructor without specifying IEqualityComparer for string type keys result in case-sensitive lookup of keys.. + /// + internal static string UseLiteralInitilializerForHashtableDescription { + get { + return ResourceManager.GetString("UseLiteralInitilializerForHashtableDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Create hashtables with literal initliazers. + /// + internal static string UseLiteralInitilializerForHashtableError { + get { + return ResourceManager.GetString("UseLiteralInitilializerForHashtableError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to UseLiteralInitializerForHashtable. + /// + internal static string UseLiteralInitilializerForHashtableName { + get { + return ResourceManager.GetString("UseLiteralInitilializerForHashtableName", resourceCulture); + } + } + /// /// Looks up a localized string similar to Use OutputType Correctly. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index c4ed5c240..ba314ee9f 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1,17 +1,17 @@  - @@ -822,4 +822,16 @@ Replace {0} with {1} - + + Create hashtables with literal initializers + + + Create a hashtable using a constructor without specifying IEqualityComparer for string type keys result in case-sensitive lookup of keys. + + + Create hashtables with literal initliazers + + + UseLiteralInitializerForHashtable + + \ No newline at end of file diff --git a/Rules/UseLiteralInitializerForHashtable.cs b/Rules/UseLiteralInitializerForHashtable.cs new file mode 100644 index 000000000..f32ff33a4 --- /dev/null +++ b/Rules/UseLiteralInitializerForHashtable.cs @@ -0,0 +1,168 @@ +// +// Copyright (c) Microsoft Corporation. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +// + +using System; +using System.Collections.Generic; +using System.Management.Automation.Language; +#if !CORECLR +using System.ComponentModel.Composition; +#endif +using System.Globalization; +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules +{ +#if !CORECLR + [Export(typeof(IScriptRule))] +#endif + class UseLiteralInitializerForHashtable : AstVisitor, IScriptRule + { + private List diagnosticRecords; + private HashSet presetTypeNameSet; + private string fileName; + + public UseLiteralInitializerForHashtable() + { + var presetTypeNames = new string[] + { + "system.collection.hashtable", + "collection.hashtable", + "hashtable" + }; + presetTypeNameSet = new HashSet(presetTypeNames, StringComparer.OrdinalIgnoreCase); + diagnosticRecords = new List(); + } + + public IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) + { + throw new ArgumentNullException("ast"); + } + this.fileName = fileName; + diagnosticRecords.Clear(); + ast.Visit(this); + return diagnosticRecords; + } + + public override AstVisitAction VisitCommand(CommandAst commandAst) + { + if (commandAst == null + || commandAst.CommandElements.Count < 2) + { + return AstVisitAction.SkipChildren; + } + + if (!commandAst.GetCommandName().Equals("new-object", StringComparison.OrdinalIgnoreCase)) + { + return AstVisitAction.Continue; + } + AnalyzeNewObjectCommand(commandAst); + return AstVisitAction.Continue; + } + + private void AnalyzeNewObjectCommand(CommandAst commandAst) + { + //new-object hashtable + var typeNameAst = commandAst.CommandElements[1] as StringConstantExpressionAst; + if (typeNameAst != null) + { + if (presetTypeNameSet.Contains(typeNameAst.Value)) + { + var dr = new DiagnosticRecord( + Strings.UseLiteralInitilializerForHashtableDescription, + commandAst.Extent, + GetName(), + GetDiagnosticSeverity(), + fileName, + ruleId: null, + suggestedCorrections: null); + diagnosticRecords.Add(dr); + } + } + } + + public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressionAst methodCallAst) + { + if (methodCallAst == null) + { + return AstVisitAction.SkipChildren; + } + + var typeExprAst = methodCallAst.Expression as TypeExpressionAst; + if (typeExprAst == null + || !presetTypeNameSet.Contains(typeExprAst.TypeName.FullName)) + { + return AstVisitAction.Continue; + } + + var memberStringConstantExprAst = methodCallAst.Member as StringConstantExpressionAst; + if (memberStringConstantExprAst == null + || !memberStringConstantExprAst.Value.Equals("new", StringComparison.OrdinalIgnoreCase)) + { + return AstVisitAction.Continue; + } + + // no arguments provided to new + if (methodCallAst.Arguments == null) + { + + var dr = new DiagnosticRecord( + Strings.UseLiteralInitilializerForHashtableDescription, + methodCallAst.Extent, + GetName(), + GetDiagnosticSeverity(), + fileName, + ruleId: null, + suggestedCorrections: null); + diagnosticRecords.Add(dr); + } + + return AstVisitAction.Continue; + } + + public string GetCommonName() + { + return Strings.UseLiteralInitilializerForHashtableCommonName; + } + + public string GetDescription() + { + return Strings.UseLiteralInitilializerForHashtableDescription; + } + + public string GetName() + { + return Strings.UseLiteralInitilializerForHashtableName; + } + + public RuleSeverity GetSeverity() + { + return RuleSeverity.Warning; + } + + private DiagnosticSeverity GetDiagnosticSeverity() + { + return DiagnosticSeverity.Warning; + } + + public string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + + public SourceType GetSourceType() + { + return SourceType.Builtin; + } + } +} From bdff3321a83f2b9e67894f3f169da91778b0a6a7 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Thu, 8 Sep 2016 21:44:38 -0700 Subject: [PATCH 2/8] Add documentation for UseLiteralInitializerForHashtable rule --- RuleDocumentation/README.md | 1 + .../UseLiteralInitializerForHashtable.md | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 RuleDocumentation/UseLiteralInitializerForHashtable.md diff --git a/RuleDocumentation/README.md b/RuleDocumentation/README.md index 493f35eb7..f219f2a93 100644 --- a/RuleDocumentation/README.md +++ b/RuleDocumentation/README.md @@ -38,6 +38,7 @@ |[UseDeclaredVarsMoreThanAssignments](./UseDeclaredVarsMoreThanAssignments.md) | Warning| |[UseIdenticalMandatoryParametersDSC](./UseIdenticalMandatoryParametersDSC.md) | Error | |[UseIdenticalParametersDSC](./UseIdenticalParametersDSC.md) | Error | +|[UseLiteralInitializerForHashtable](./UseLiteralInitializerForHashtable.md) | Warning | |[UseOutputTypeCorrectly](./UseOutputTypeCorrectly.md) | Information| |[UsePSCredentialType](./UsePSCredentialType.md) | Warning| |[UseShouldProcessCorrectly](./UseShouldProcessCorrectly.md) | Warning| diff --git a/RuleDocumentation/UseLiteralInitializerForHashtable.md b/RuleDocumentation/UseLiteralInitializerForHashtable.md new file mode 100644 index 000000000..571dd1f71 --- /dev/null +++ b/RuleDocumentation/UseLiteralInitializerForHashtable.md @@ -0,0 +1,24 @@ +# UseLiteralInitializerForHashtable +**Severity Level: Warning** + +## Description +Creating a hashtable by either `[hashtable]::new()` or `New-Object -TypeName hashtable` will create a hashtable wherein the keys are looked-up in a case-sensitive manner, unless an `IEqualityComparer` object is passed as an argument. However, PowerShell is case-insensitive in nature and it is best to create hashtables with case-insensitive key look-up. This rule is intended to warn the author of the case-sensitive nature of the hashtable if he/she creates a hashtable using the `new` member or the `New-Object` cmdlet. + +## How to Fix +Use the full cmdlet name and not an alias. + +## Example +### Wrong： +``` PowerShell +$hashtable = [hashtable]::new() +``` + +### Wrong： +``` PowerShell +$hashtable = New-Object -TypeName hashtable +``` + +### Correct: +``` PowerShell +$hashtable = @{} +``` From b8aedb2bf9c1398da067bb42d37cee699546419f Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Thu, 8 Sep 2016 21:45:14 -0700 Subject: [PATCH 3/8] Allow CorrectionExtent to take null filename --- Engine/Generic/CorrectionExtent.cs | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/Engine/Generic/CorrectionExtent.cs b/Engine/Generic/CorrectionExtent.cs index 0974bea74..8455f1f41 100644 --- a/Engine/Generic/CorrectionExtent.cs +++ b/Engine/Generic/CorrectionExtent.cs @@ -73,16 +73,16 @@ public string Description private int endColumnNumber; private string text; private string description; - + public CorrectionExtent( - int startLineNumber, - int endLineNumber, - int startColumnNumber, - int endColumnNumber, - string text, - string file) + int startLineNumber, + int endLineNumber, + int startColumnNumber, + int endColumnNumber, + string text, + string file) : this( - startLineNumber, + startLineNumber, endLineNumber, startColumnNumber, endColumnNumber, @@ -93,11 +93,11 @@ public CorrectionExtent( } public CorrectionExtent( - int startLineNumber, - int endLineNumber, - int startColumnNumber, - int endColumnNumber, - string text, + int startLineNumber, + int endLineNumber, + int startColumnNumber, + int endColumnNumber, + string text, string file, string description) { @@ -115,9 +115,8 @@ public CorrectionExtent( private void ThrowIfInvalidArguments() { - ThrowIfNull(file, "filename"); ThrowIfNull(text, "text"); - ThrowIfDecreasing(startLineNumber, endLineNumber, "start line number cannot be less than end line number"); + ThrowIfDecreasing(startLineNumber, endLineNumber, "start line number cannot be less than end line number"); if (startLineNumber == endLineNumber) { ThrowIfDecreasing(StartColumnNumber, endColumnNumber, "start column number cannot be less than end column number for a one line extent"); From 37adfe9b745a6dfb9af3351489c86b51a105768e Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Thu, 8 Sep 2016 21:46:27 -0700 Subject: [PATCH 4/8] Add localization for UseLiteralInitializerForHashtable rule --- Rules/Strings.Designer.cs | 2 +- Rules/Strings.resx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index f6788175b..9987fb8b8 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1834,7 +1834,7 @@ internal static string UseLiteralInitilializerForHashtableCommonName { } /// - /// Looks up a localized string similar to Create a hashtable using a constructor without specifying IEqualityComparer for string type keys result in case-sensitive lookup of keys.. + /// Looks up a localized string similar to Use literal initializer, \@\{\}, for creating a hashtable as they are case-insensitive by default. /// internal static string UseLiteralInitilializerForHashtableDescription { get { diff --git a/Rules/Strings.resx b/Rules/Strings.resx index ba314ee9f..345f5367e 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -826,7 +826,7 @@ Create hashtables with literal initializers - Create a hashtable using a constructor without specifying IEqualityComparer for string type keys result in case-sensitive lookup of keys. + Use literal initializer, @{{}}, for creating a hashtable as they are case-insensitive by default Create hashtables with literal initliazers From a585a552b878c7075e4f943a797ae6afe61fd6ae Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Thu, 8 Sep 2016 21:47:58 -0700 Subject: [PATCH 5/8] Add tests for UseLiteralInitializerForHashtable rule --- Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 10 +-- ...seLiteralInitializerForHashtable.tests.ps1 | 65 +++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) create mode 100644 Tests/Rules/UseLiteralInitializerForHashtable.tests.ps1 diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index d1108ce4b..d06ad73f0 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -16,7 +16,7 @@ Describe "Test available parameters" { It "has a RuleName parameter" { $params.ContainsKey("Name") | Should Be $true } - + It "accepts string" { $params["Name"].ParameterType.FullName | Should Be "System.String[]" } @@ -61,10 +61,10 @@ Describe "Test Name parameters" { It "get Rules with no parameters supplied" { $defaultRules = Get-ScriptAnalyzerRule - $expectedNumRules = 41 + $expectedNumRules = 42 if ((Test-PSEditionCoreClr)) { - $expectedNumRules = 40 + $expectedNumRules = 41 } $defaultRules.Count | Should be $expectedNumRules } @@ -117,7 +117,7 @@ Describe "Test RuleExtension" { It "with Name of a built-in rules" { $ruleExtension = Get-ScriptAnalyzerRule -CustomizedRulePath $directory\CommunityAnalyzerRules\CommunityAnalyzerRules.psm1 -Name $singularNouns - $ruleExtension.Count | Should Be 0 + $ruleExtension.Count | Should Be 0 } It "with Names of built-in, DSC and non-built-in rules" { @@ -137,7 +137,7 @@ Describe "Test RuleExtension" { } catch { - $Error[0].FullyQualifiedErrorId | should match "PathNotFound,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.GetScriptAnalyzerRuleCommand" + $Error[0].FullyQualifiedErrorId | should match "PathNotFound,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.GetScriptAnalyzerRuleCommand" } } diff --git a/Tests/Rules/UseLiteralInitializerForHashtable.tests.ps1 b/Tests/Rules/UseLiteralInitializerForHashtable.tests.ps1 new file mode 100644 index 000000000..8465d01c5 --- /dev/null +++ b/Tests/Rules/UseLiteralInitializerForHashtable.tests.ps1 @@ -0,0 +1,65 @@ +Import-Module PSScriptAnalyzer +$ruleName = "PSUseLiteralInitializerForHashtable" + +Describe "UseLiteralInitlializerForHashtable" { + Context "When new-object hashtable is used to create a hashtable" { + It "has violation" { + $violationScriptDef = @' + $htable1 = new-object hashtable + $htable2 = new-object system.collection.hashtable + $htable3 = new-object -Typename hashtable -ArgumentList 10 +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $violationScriptDef -IncludeRule $ruleName + $violations.Count | Should Be 3 + } + + It "does not detect violation if arguments given to new-object contain ignore case string comparer" { + $violationScriptDef = @' + $htable1 = new-object hashtable [system.stringcomparer]::ordinalignorecase + $htable2 = new-object -Typename hashtable -ArgumentList [system.stringcomparer]::ordinalignorecase + $htable3 = new-object hashtable -ArgumentList [system.stringcomparer]::ordinalignorecase + $htable4 = new-object -Typename hashtable [system.stringcomparer]::ordinalignorecase +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $violationScriptDef -IncludeRule $ruleName + $violations.Count | Should Be 0 + } + + It "suggests correction" { + $violationScriptDef = @' + $htable1 = new-object hashtable +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $violationScriptDef -IncludeRule $ruleName + $violations[0].SuggestedCorrections[0].Text | Should Be '@{}' + } + } + + Context "When [hashtable]::new is used to create a hashtable" { + It "has violation" { + $violationScriptDef = @' + $htable1 = [hashtable]::new() + $htable2 = [system.collection.hashtable]::new() + $htable3 = [hashtable]::new(10) +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $violationScriptDef -IncludeRule $ruleName + $violations.Count | Should Be 3 + } + + It "does not detect violation if arguments given to [hashtable]::new contain ignore case string comparer" { + $violationScriptDef = @' + $htable1 = [hashtable]::new(10, [system.stringcomparer]::ordinalignorecase) + $htable2 = [hashtable]::new(10, [system.stringcomparer]::ordinalignorecase) +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $violationScriptDef -IncludeRule $ruleName + $violations.Count | Should Be 0 + } + + It "suggests correction" { + $violationScriptDef = @' + $htable1 = [hashtable]::new() +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $violationScriptDef -IncludeRule $ruleName + $violations[0].SuggestedCorrections[0].Text | Should Be '@{}' + } + + } +} \ No newline at end of file From 95d08943b074c4d2255b1d53ebccbd4d71e998de Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Thu, 8 Sep 2016 21:49:14 -0700 Subject: [PATCH 6/8] Add more checks for UseLiteralInitializerForHashtable rule --- Engine/Helper.cs | 82 +++++---- Rules/UseLiteralInitializerForHashtable.cs | 198 ++++++++++++++++++--- 2 files changed, 224 insertions(+), 56 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index c8b26c274..ae21638e6 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -342,6 +342,52 @@ public static bool IsMissingManifestMemberException(ErrorRecord errorRecord) && string.Equals("MissingMemberException", errorRecord.CategoryInfo.Reason, StringComparison.OrdinalIgnoreCase); } + public IEnumerable GetStringsFromExpressionAst(ExpressionAst exprAst) + { + if (exprAst == null) + { + throw new ArgumentNullException("exprAst"); + } + + var result = new List(); + if (exprAst is StringConstantExpressionAst) + { + result.Add((exprAst as StringConstantExpressionAst).Value); + } + // Array of the form "v-n", "v-n1" + else if (exprAst is ArrayLiteralAst) + { + result.AddRange(Helper.Instance.GetStringsFromArrayLiteral(exprAst as ArrayLiteralAst)); + } + // Array of the form @("v-n", "v-n1") + else if (exprAst is ArrayExpressionAst) + { + ArrayExpressionAst arrExAst = exprAst as ArrayExpressionAst; + if (arrExAst.SubExpression != null && arrExAst.SubExpression.Statements != null) + { + foreach (StatementAst stAst in arrExAst.SubExpression.Statements) + { + if (stAst is PipelineAst) + { + PipelineAst pipeAst = stAst as PipelineAst; + if (pipeAst.PipelineElements != null) + { + foreach (CommandBaseAst cmdBaseAst in pipeAst.PipelineElements) + { + if (cmdBaseAst is CommandExpressionAst) + { + result.AddRange(Helper.Instance.GetStringsFromArrayLiteral((cmdBaseAst as CommandExpressionAst).Expression as ArrayLiteralAst)); + } + } + } + } + } + } + } + + return result; + } + /// /// Get the list of exported function by analyzing the ast /// @@ -433,41 +479,7 @@ public HashSet GetExportedFunction(Ast ast) if (exprAst != null) { - // One string so just add this to the list - if (exprAst is StringConstantExpressionAst) - { - exportedFunctions.Add((exprAst as StringConstantExpressionAst).Value); - } - // Array of the form "v-n", "v-n1" - else if (exprAst is ArrayLiteralAst) - { - exportedFunctions.UnionWith(Helper.Instance.GetStringsFromArrayLiteral(exprAst as ArrayLiteralAst)); - } - // Array of the form @("v-n", "v-n1") - else if (exprAst is ArrayExpressionAst) - { - ArrayExpressionAst arrExAst = exprAst as ArrayExpressionAst; - if (arrExAst.SubExpression != null && arrExAst.SubExpression.Statements != null) - { - foreach (StatementAst stAst in arrExAst.SubExpression.Statements) - { - if (stAst is PipelineAst) - { - PipelineAst pipeAst = stAst as PipelineAst; - if (pipeAst.PipelineElements != null) - { - foreach (CommandBaseAst cmdBaseAst in pipeAst.PipelineElements) - { - if (cmdBaseAst is CommandExpressionAst) - { - exportedFunctions.UnionWith(Helper.Instance.GetStringsFromArrayLiteral((cmdBaseAst as CommandExpressionAst).Expression as ArrayLiteralAst)); - } - } - } - } - } - } - } + exportedFunctions.UnionWith(Helper.Instance.GetStringsFromExpressionAst(exprAst)); } i += 1; diff --git a/Rules/UseLiteralInitializerForHashtable.cs b/Rules/UseLiteralInitializerForHashtable.cs index f32ff33a4..d69ea86f9 100644 --- a/Rules/UseLiteralInitializerForHashtable.cs +++ b/Rules/UseLiteralInitializerForHashtable.cs @@ -12,12 +12,15 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Management.Automation.Language; #if !CORECLR using System.ComponentModel.Composition; #endif using System.Globalization; using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; +using System.Collections.Specialized; +using System.Collections.ObjectModel; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -35,7 +38,6 @@ public UseLiteralInitializerForHashtable() var presetTypeNames = new string[] { "system.collection.hashtable", - "collection.hashtable", "hashtable" }; presetTypeNameSet = new HashSet(presetTypeNames, StringComparer.OrdinalIgnoreCase); @@ -62,7 +64,9 @@ public override AstVisitAction VisitCommand(CommandAst commandAst) return AstVisitAction.SkipChildren; } - if (!commandAst.GetCommandName().Equals("new-object", StringComparison.OrdinalIgnoreCase)) + var commandName = commandAst.GetCommandName(); + if (commandName == null + || !commandName.Equals("new-object", StringComparison.OrdinalIgnoreCase)) { return AstVisitAction.Continue; } @@ -72,23 +76,133 @@ public override AstVisitAction VisitCommand(CommandAst commandAst) private void AnalyzeNewObjectCommand(CommandAst commandAst) { - //new-object hashtable - var typeNameAst = commandAst.CommandElements[1] as StringConstantExpressionAst; - if (typeNameAst != null) + string typeName; + List argumentList; + GetParametersFromCommandAst(commandAst, out typeName, out argumentList); + if (typeName == null + || !presetTypeNameSet.Contains(typeName)) { - if (presetTypeNameSet.Contains(typeNameAst.Value)) + return; + } + + if (argumentList != null) + { + if (argumentList.Any(arg => arg != null && arg.EndsWith("ignorecase", StringComparison.OrdinalIgnoreCase))) + { + return; + } + } + var dr = new DiagnosticRecord( + Strings.UseLiteralInitilializerForHashtableDescription, + commandAst.Extent, + GetName(), + GetDiagnosticSeverity(), + fileName, + ruleId: null, + suggestedCorrections: GetSuggestedCorrections(commandAst, this.fileName)); + diagnosticRecords.Add(dr); + } + + private void GetParametersFromCommandAst(CommandAst commandAst, out string typeName, out List argumentList) + { + // This should read the command in all the following form + // new-object hashtable + // new-object -Typename hashtable + // new-object hashtable -ArgumentList comparer + // new-object -Typename hashtable -ArgumentList blah1,blah2 + // new-object -ArgumentList blah1,blah2 -typename hashtable + + argumentList = null; + typeName = null; + var namedArguments = new OrderedDictionary(StringComparer.OrdinalIgnoreCase); + namedArguments.Add("typename", null); + namedArguments.Add("argumentlist", null); + var positinalElems = GetNamedArguments(commandAst.CommandElements, ref namedArguments); + GetPositionalArguments(new ReadOnlyCollection (positinalElems), ref namedArguments); + if (namedArguments["typename"] == null) + { + return; + } + + var typenameAst = namedArguments["typename"] as StringConstantExpressionAst; + if (typenameAst == null) + { + return; + } + + typeName = typenameAst.Value; + var argumentListAst = namedArguments["argumentlist"] as ExpressionAst; + if (argumentListAst == null) + { + return; + } + + argumentList = new List(Helper.Instance.GetStringsFromExpressionAst(argumentListAst)); + } + + private int GetFirstEmptyIndex(OrderedDictionary namedArguments) + { + for (int k = 0; k < namedArguments.Count; k++) + { + if (namedArguments[k] == null) { - var dr = new DiagnosticRecord( - Strings.UseLiteralInitilializerForHashtableDescription, - commandAst.Extent, - GetName(), - GetDiagnosticSeverity(), - fileName, - ruleId: null, - suggestedCorrections: null); - diagnosticRecords.Add(dr); + return k; } } + return -1; + } + + private void GetPositionalArguments(ReadOnlyCollection positinalArguments, ref OrderedDictionary namedArguments) + { + for (int k = 0; k < positinalArguments.Count; k++) + { + int firstEmptyIndex = GetFirstEmptyIndex(namedArguments); + if (firstEmptyIndex == -1) + { + return; + } + var elem = positinalArguments[k]; + namedArguments[firstEmptyIndex] = elem as ExpressionAst; + } + } + + private List GetNamedArguments(ReadOnlyCollection commandElements, ref OrderedDictionary namedArguments) + { + bool paramFound = false; + string paramName = null; + var remainingCommandElements = new List(); + for (int k = 1; k < commandElements.Count; k++) + { + if (paramFound) + { + paramFound = false; + var argAst = commandElements[k] as ExpressionAst; + if (argAst != null) + { + namedArguments[paramName] = argAst; + continue; + } + } + var paramAst = commandElements[k] as CommandParameterAst; + if (paramAst != null) + { + foreach (var key in namedArguments.Keys) + { + var keyStr = key as string; + if (keyStr.Equals(paramAst.ParameterName, StringComparison.OrdinalIgnoreCase)) + { + paramFound = true; + paramName = paramAst.ParameterName; + break; + } + } + } + else + { + remainingCommandElements.Add(commandElements[k]); + } + } + return remainingCommandElements; } public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressionAst methodCallAst) @@ -112,8 +226,10 @@ public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressio return AstVisitAction.Continue; } - // no arguments provided to new - if (methodCallAst.Arguments == null) + // no arguments provided to new OR one of the argument ends with ignorecase + // (heuristics find to something like [system.stringcomparer]::ordinalignorecase) + if (methodCallAst.Arguments == null + || !HasIgnoreCaseComparerArg(methodCallAst.Arguments)) { var dr = new DiagnosticRecord( @@ -123,26 +239,66 @@ public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressio GetDiagnosticSeverity(), fileName, ruleId: null, - suggestedCorrections: null); + suggestedCorrections: GetSuggestedCorrections(methodCallAst, this.fileName)); diagnosticRecords.Add(dr); } return AstVisitAction.Continue; } + private bool HasIgnoreCaseComparerArg(ReadOnlyCollection arguments) + { + foreach (var arg in arguments) + { + var memberExprAst = arg as MemberExpressionAst; + if (memberExprAst == null) + { + continue; + } + var strConstExprAst = memberExprAst.Member as StringConstantExpressionAst; + if (strConstExprAst == null) + { + continue; + } + if (strConstExprAst.Value.EndsWith("ignorecase")) + { + return true; + } + } + return false; + } + + private List GetSuggestedCorrections(Ast violation, string filename) + { + var correctionExtents = new List(); + correctionExtents.Add(new CorrectionExtent( + violation.Extent.StartLineNumber, + violation.Extent.EndLineNumber, + violation.Extent.StartColumnNumber, + violation.Extent.EndColumnNumber, + "@{}", + filename, + GetDescription())); + return correctionExtents; + } + public string GetCommonName() { - return Strings.UseLiteralInitilializerForHashtableCommonName; + return string.Format(CultureInfo.CurrentCulture, Strings.UseLiteralInitilializerForHashtableCommonName); } public string GetDescription() { - return Strings.UseLiteralInitilializerForHashtableDescription; + return string.Format(CultureInfo.CurrentCulture, Strings.UseLiteralInitilializerForHashtableDescription); } public string GetName() { - return Strings.UseLiteralInitilializerForHashtableName; + return string.Format( + CultureInfo.CurrentCulture, + Strings.NameSpaceFormat, + GetSourceName(), + Strings.UseLiteralInitilializerForHashtableName); } public RuleSeverity GetSeverity() From 3168d36737470021688fd133fe426161eb5ca2e1 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Fri, 9 Sep 2016 15:37:58 -0700 Subject: [PATCH 7/8] Add inline documentation --- Rules/UseLiteralInitializerForHashtable.cs | 275 +++++++++++++-------- 1 file changed, 176 insertions(+), 99 deletions(-) diff --git a/Rules/UseLiteralInitializerForHashtable.cs b/Rules/UseLiteralInitializerForHashtable.cs index d69ea86f9..ddc2fada9 100644 --- a/Rules/UseLiteralInitializerForHashtable.cs +++ b/Rules/UseLiteralInitializerForHashtable.cs @@ -1,5 +1,4 @@ -// -// Copyright (c) Microsoft Corporation. +// Copyright (c) Microsoft Corporation. // // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR // IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, @@ -8,23 +7,25 @@ // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN // THE SOFTWARE. -// using System; using System.Collections.Generic; -using System.Linq; -using System.Management.Automation.Language; +using System.Collections.ObjectModel; +using System.Collections.Specialized; #if !CORECLR using System.ComponentModel.Composition; #endif using System.Globalization; +using System.Linq; +using System.Management.Automation.Language; using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; -using System.Collections.Specialized; -using System.Collections.ObjectModel; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { -#if !CORECLR + /// + /// A class to walk an AST to check if hashtable is not initialized using [hashtable]::new or new-object hashtable + /// + #if !CORECLR [Export(typeof(IScriptRule))] #endif class UseLiteralInitializerForHashtable : AstVisitor, IScriptRule @@ -44,18 +45,89 @@ public UseLiteralInitializerForHashtable() diagnosticRecords = new List(); } + /// + /// Analyzes the given ast to find if a hashtable is initialized using [hashtable]::new or New-Object Hashtable + /// + /// AST to be analyzed. This should be non-null + /// Name of file that corresponds to the input AST. + /// A an enumerable type containing the violations public IEnumerable AnalyzeScript(Ast ast, string fileName) { if (ast == null) { throw new ArgumentNullException("ast"); } + this.fileName = fileName; diagnosticRecords.Clear(); ast.Visit(this); return diagnosticRecords; } + /// + /// Retrieves the common name of this rule. + /// + public string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.UseLiteralInitilializerForHashtableCommonName); + } + + /// + /// Retrieves the description of this rule. + /// + public string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.UseLiteralInitilializerForHashtableDescription); + } + + /// + /// Retrieves the name of this rule. + /// + public string GetName() + { + return string.Format( + CultureInfo.CurrentCulture, + Strings.NameSpaceFormat, + GetSourceName(), + Strings.UseLiteralInitilializerForHashtableName); + } + + /// + /// Retrieves the severity of the rule: error, warning or information. + /// + public RuleSeverity GetSeverity() + { + return RuleSeverity.Warning; + } + + /// + /// Gets the severity of the returned diagnostic record: error, warning, or information. + /// + /// + public DiagnosticSeverity GetDiagnosticSeverity() + { + return DiagnosticSeverity.Warning; + } + + /// + /// Retrieves the name of the module/assembly the rule is from. + /// + public string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + + /// + /// Retrieves the type of the rule, Builtin, Managed or Module. + /// + public SourceType GetSourceType() + { + return SourceType.Builtin; + } + + /// + /// Visits command ast to check for "new-object" command + /// public override AstVisitAction VisitCommand(CommandAst commandAst) { if (commandAst == null @@ -70,10 +142,57 @@ public override AstVisitAction VisitCommand(CommandAst commandAst) { return AstVisitAction.Continue; } + AnalyzeNewObjectCommand(commandAst); return AstVisitAction.Continue; } + /// + /// Checks if a hashtable is created using [hashtable]::new() + /// + public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressionAst methodCallAst) + { + if (methodCallAst == null) + { + return AstVisitAction.SkipChildren; + } + + var typeExprAst = methodCallAst.Expression as TypeExpressionAst; + if (typeExprAst == null + || !presetTypeNameSet.Contains(typeExprAst.TypeName.FullName)) + { + return AstVisitAction.Continue; + } + + var memberStringConstantExprAst = methodCallAst.Member as StringConstantExpressionAst; + if (memberStringConstantExprAst == null + || !memberStringConstantExprAst.Value.Equals("new", StringComparison.OrdinalIgnoreCase)) + { + return AstVisitAction.Continue; + } + + // no arguments provided to new OR one of the argument ends with ignorecase + // (heuristics find to something like [system.stringcomparer]::ordinalignorecase) + if (methodCallAst.Arguments == null + || !HasIgnoreCaseComparerArg(methodCallAst.Arguments)) + { + var dr = new DiagnosticRecord( + Strings.UseLiteralInitilializerForHashtableDescription, + methodCallAst.Extent, + GetName(), + GetDiagnosticSeverity(), + fileName, + ruleId: null, + suggestedCorrections: GetSuggestedCorrections(methodCallAst, this.fileName)); + diagnosticRecords.Add(dr); + } + + return AstVisitAction.Continue; + } + + /// + /// Analyzes command ast to check for new-object command and parse its arguments + /// private void AnalyzeNewObjectCommand(CommandAst commandAst) { string typeName; @@ -92,6 +211,7 @@ private void AnalyzeNewObjectCommand(CommandAst commandAst) return; } } + var dr = new DiagnosticRecord( Strings.UseLiteralInitilializerForHashtableDescription, commandAst.Extent, @@ -103,22 +223,27 @@ private void AnalyzeNewObjectCommand(CommandAst commandAst) diagnosticRecords.Add(dr); } + /// + /// Interpret the named and unnamed arguments and assign them their corresponding parameters + /// + /// An non-null instance of CommandAst. Expects it be commandast of "new-object" command + /// Returns the TypeName argument + /// Returns the ArgumentList argument + /// This should read the command in all the following form + /// new-object hashtable + /// new-object -Typename hashtable + /// new-object hashtable -ArgumentList comparer + /// new-object -Typename hashtable -ArgumentList blah1,blah2 + /// new-object -ArgumentList blah1,blah2 -typename hashtable private void GetParametersFromCommandAst(CommandAst commandAst, out string typeName, out List argumentList) { - // This should read the command in all the following form - // new-object hashtable - // new-object -Typename hashtable - // new-object hashtable -ArgumentList comparer - // new-object -Typename hashtable -ArgumentList blah1,blah2 - // new-object -ArgumentList blah1,blah2 -typename hashtable - argumentList = null; typeName = null; var namedArguments = new OrderedDictionary(StringComparer.OrdinalIgnoreCase); namedArguments.Add("typename", null); namedArguments.Add("argumentlist", null); var positinalElems = GetNamedArguments(commandAst.CommandElements, ref namedArguments); - GetPositionalArguments(new ReadOnlyCollection (positinalElems), ref namedArguments); + GetPositionalArguments(new ReadOnlyCollection(positinalElems), ref namedArguments); if (namedArguments["typename"] == null) { return; @@ -140,6 +265,11 @@ private void GetParametersFromCommandAst(CommandAst commandAst, out string typeN argumentList = new List(Helper.Instance.GetStringsFromExpressionAst(argumentListAst)); } + /// + /// Returns the first index whose value is null + /// + /// An ordered dictionary. It must not be null + /// Returns the first index whose value is null else returns -1 private int GetFirstEmptyIndex(OrderedDictionary namedArguments) { for (int k = 0; k < namedArguments.Count; k++) @@ -149,23 +279,36 @@ private int GetFirstEmptyIndex(OrderedDictionary namedArguments) return k; } } + return -1; } - private void GetPositionalArguments(ReadOnlyCollection positinalArguments, ref OrderedDictionary namedArguments) + /// + /// Assigns the unnamed arguments to their corresponding parameters + /// + /// A collection of arguments that need to be assigned + /// An ordered dictionary of parameters in their corresponding positions + private void GetPositionalArguments(ReadOnlyCollection unnamedArguments, ref OrderedDictionary namedArguments) { - for (int k = 0; k < positinalArguments.Count; k++) + for (int k = 0; k < unnamedArguments.Count; k++) { int firstEmptyIndex = GetFirstEmptyIndex(namedArguments); if (firstEmptyIndex == -1) { return; } - var elem = positinalArguments[k]; + + var elem = unnamedArguments[k]; namedArguments[firstEmptyIndex] = elem as ExpressionAst; } } + /// + /// Gets the named arguments from a list of command elements + /// + /// A list of command elements, typically a property of CommandAst instance + /// An ordered dictionary of parameters in their corresponding positions + /// Returns a list of unnamed arguments that remain after taking into account named parameters private List GetNamedArguments(ReadOnlyCollection commandElements, ref OrderedDictionary namedArguments) { bool paramFound = false; @@ -183,6 +326,7 @@ private List GetNamedArguments(ReadOnlyCollection GetNamedArguments(ReadOnlyCollection + /// Checks if any argument in the given collection ends with "ignorecase" + /// + /// A collection of argument asts. Neither this nor any elements within it should be null private bool HasIgnoreCaseComparerArg(ReadOnlyCollection arguments) { foreach (var arg in arguments) @@ -255,19 +363,27 @@ private bool HasIgnoreCaseComparerArg(ReadOnlyCollection argument { continue; } + var strConstExprAst = memberExprAst.Member as StringConstantExpressionAst; if (strConstExprAst == null) { continue; } + if (strConstExprAst.Value.EndsWith("ignorecase")) { return true; } } + return false; } + /// + /// Suggested corrections to replace the violations + /// + /// Ast representing the violation. This should not be null. + /// Name of file containing the violation private List GetSuggestedCorrections(Ast violation, string filename) { var correctionExtents = new List(); @@ -281,44 +397,5 @@ private List GetSuggestedCorrections(Ast violation, string fil GetDescription())); return correctionExtents; } - - public string GetCommonName() - { - return string.Format(CultureInfo.CurrentCulture, Strings.UseLiteralInitilializerForHashtableCommonName); - } - - public string GetDescription() - { - return string.Format(CultureInfo.CurrentCulture, Strings.UseLiteralInitilializerForHashtableDescription); - } - - public string GetName() - { - return string.Format( - CultureInfo.CurrentCulture, - Strings.NameSpaceFormat, - GetSourceName(), - Strings.UseLiteralInitilializerForHashtableName); - } - - public RuleSeverity GetSeverity() - { - return RuleSeverity.Warning; - } - - private DiagnosticSeverity GetDiagnosticSeverity() - { - return DiagnosticSeverity.Warning; - } - - public string GetSourceName() - { - return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); - } - - public SourceType GetSourceType() - { - return SourceType.Builtin; - } } } From 974b1d146f8c0cb66caa66511da7a3c947c11dad Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 12 Sep 2016 13:39:24 -0700 Subject: [PATCH 8/8] Add more type check to UseLiteralInitializerForHashtable rule --- Rules/UseLiteralInitializerForHashtable.cs | 38 +++++++++++++++---- ...seLiteralInitializerForHashtable.tests.ps1 | 10 +++-- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/Rules/UseLiteralInitializerForHashtable.cs b/Rules/UseLiteralInitializerForHashtable.cs index ddc2fada9..b978f0b92 100644 --- a/Rules/UseLiteralInitializerForHashtable.cs +++ b/Rules/UseLiteralInitializerForHashtable.cs @@ -38,7 +38,8 @@ public UseLiteralInitializerForHashtable() { var presetTypeNames = new string[] { - "system.collection.hashtable", + "system.collections.hashtable", + "collections.hashtable", "hashtable" }; presetTypeNameSet = new HashSet(presetTypeNames, StringComparer.OrdinalIgnoreCase); @@ -204,12 +205,11 @@ private void AnalyzeNewObjectCommand(CommandAst commandAst) return; } - if (argumentList != null) + + if (argumentList != null + && HasIgnoreCaseComparerArg(argumentList)) { - if (argumentList.Any(arg => arg != null && arg.EndsWith("ignorecase", StringComparison.OrdinalIgnoreCase))) - { - return; - } + return; } var dr = new DiagnosticRecord( @@ -225,6 +225,9 @@ private void AnalyzeNewObjectCommand(CommandAst commandAst) /// /// Interpret the named and unnamed arguments and assign them their corresponding parameters + /// + /// PSv4 onwards there exists System.Management.Automation.Language.StaticParameterBinder.BindCommand to + /// achieve identical objective. But since we support PSSA on PSv3 too we need this implementation. /// /// An non-null instance of CommandAst. Expects it be commandast of "new-object" command /// Returns the TypeName argument @@ -356,9 +359,11 @@ private List GetNamedArguments(ReadOnlyCollectionA collection of argument asts. Neither this nor any elements within it should be null private bool HasIgnoreCaseComparerArg(ReadOnlyCollection arguments) { + var argumentsAsStrings = new List(); foreach (var arg in arguments) { var memberExprAst = arg as MemberExpressionAst; + argumentsAsStrings.Add(null); if (memberExprAst == null) { continue; @@ -369,13 +374,30 @@ private bool HasIgnoreCaseComparerArg(ReadOnlyCollection argument { continue; } + argumentsAsStrings[argumentsAsStrings.Count - 1] = strConstExprAst.Value; + } + return HasIgnoreCaseComparerArg(argumentsAsStrings); + } - if (strConstExprAst.Value.EndsWith("ignorecase")) + /// + /// Checks if any argument in the given collection ends with "ignorecase" + /// + /// An enumerable of type string. Elements can be null but the collection must be non-null . + /// + private bool HasIgnoreCaseComparerArg(IEnumerable arguments) + { + + foreach (var arg in arguments) + { + if (arg == null) + { + continue; + } + if (arg.EndsWith("ignorecase", StringComparison.OrdinalIgnoreCase)) { return true; } } - return false; } diff --git a/Tests/Rules/UseLiteralInitializerForHashtable.tests.ps1 b/Tests/Rules/UseLiteralInitializerForHashtable.tests.ps1 index 8465d01c5..97227068e 100644 --- a/Tests/Rules/UseLiteralInitializerForHashtable.tests.ps1 +++ b/Tests/Rules/UseLiteralInitializerForHashtable.tests.ps1 @@ -6,11 +6,12 @@ Describe "UseLiteralInitlializerForHashtable" { It "has violation" { $violationScriptDef = @' $htable1 = new-object hashtable - $htable2 = new-object system.collection.hashtable + $htable2 = new-object system.collections.hashtable $htable3 = new-object -Typename hashtable -ArgumentList 10 + $htable4 = new-object collections.hashtable '@ $violations = Invoke-ScriptAnalyzer -ScriptDefinition $violationScriptDef -IncludeRule $ruleName - $violations.Count | Should Be 3 + $violations.Count | Should Be 4 } It "does not detect violation if arguments given to new-object contain ignore case string comparer" { @@ -37,11 +38,12 @@ Describe "UseLiteralInitlializerForHashtable" { It "has violation" { $violationScriptDef = @' $htable1 = [hashtable]::new() - $htable2 = [system.collection.hashtable]::new() + $htable2 = [system.collections.hashtable]::new() $htable3 = [hashtable]::new(10) + $htable4 = [collections.hashtable]::new() '@ $violations = Invoke-ScriptAnalyzer -ScriptDefinition $violationScriptDef -IncludeRule $ruleName - $violations.Count | Should Be 3 + $violations.Count | Should Be 4 } It "does not detect violation if arguments given to [hashtable]::new contain ignore case string comparer" {