From 09d7bd9386511f459cf7606870a09fcff9d21de1 Mon Sep 17 00:00:00 2001 From: Quoc Truong Date: Tue, 23 Jun 2015 14:31:34 -0700 Subject: [PATCH 1/3] Only raise NullComparisonRule if the RHS is an array or has unknown type --- Engine/Helper.cs | 2 +- Rules/PossibleIncorrectComparisonWithNull.cs | 65 +++++++++++++++++-- .../PossibleIncorrectComparisonWithNull.ps1 | 19 ++++++ ...sibleIncorrectComparisonWithNull.tests.ps1 | 4 +- ...ncorrectComparisonWithNullNoViolations.ps1 | 11 ++++ 5 files changed, 91 insertions(+), 10 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index da5bbb890..66105b1b3 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -679,7 +679,7 @@ internal string GetTypeFromMemberExpressionAstHelper(MemberExpressionAst memberA /// /// /// - internal Type GetTypeFromAnalysis(VariableExpressionAst varAst, Ast ast) + public Type GetTypeFromAnalysis(VariableExpressionAst varAst, Ast ast) { try { diff --git a/Rules/PossibleIncorrectComparisonWithNull.cs b/Rules/PossibleIncorrectComparisonWithNull.cs index da97c13f6..cc589f28a 100644 --- a/Rules/PossibleIncorrectComparisonWithNull.cs +++ b/Rules/PossibleIncorrectComparisonWithNull.cs @@ -11,6 +11,7 @@ // using System; +using System.Linq; using System.Collections.Generic; using System.Management.Automation.Language; using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; @@ -33,17 +34,67 @@ public class PossibleIncorrectComparisonWithNull : IScriptRule { public IEnumerable AnalyzeScript(Ast ast, string fileName) { if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); - IEnumerable binExpressionAsts = ast.FindAll(testAst => testAst is BinaryExpressionAst, true); + IEnumerable binExpressionAsts = ast.FindAll(testAst => testAst is BinaryExpressionAst, false); - if (binExpressionAsts != null) { - foreach (BinaryExpressionAst binExpressionAst in binExpressionAsts) { - if ((binExpressionAst.Operator.Equals(TokenKind.Equals) || binExpressionAst.Operator.Equals(TokenKind.Ceq) - || binExpressionAst.Operator.Equals(TokenKind.Cne) || binExpressionAst.Operator.Equals(TokenKind.Ine) || binExpressionAst.Operator.Equals(TokenKind.Ieq)) - && binExpressionAst.Right.Extent.Text.Equals("$null", StringComparison.OrdinalIgnoreCase)) { - yield return new DiagnosticRecord(Strings.PossibleIncorrectComparisonWithNullError, binExpressionAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName); + foreach (BinaryExpressionAst binExpressionAst in binExpressionAsts) { + if ((binExpressionAst.Operator.Equals(TokenKind.Equals) || binExpressionAst.Operator.Equals(TokenKind.Ceq) + || binExpressionAst.Operator.Equals(TokenKind.Cne) || binExpressionAst.Operator.Equals(TokenKind.Ine) || binExpressionAst.Operator.Equals(TokenKind.Ieq)) + && binExpressionAst.Right.Extent.Text.Equals("$null", StringComparison.OrdinalIgnoreCase)) + { + if (IncorrectComparisonWithNull(binExpressionAst, ast)) + { + yield return new DiagnosticRecord(Strings.PossibleIncorrectComparisonWithNullError, binExpressionAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName); } } } + + IEnumerable funcAsts = ast.FindAll(item => item is FunctionDefinitionAst, true).Union(ast.FindAll(item => item is FunctionMemberAst, true)); + foreach (Ast funcAst in funcAsts) + { + IEnumerable binAsts = funcAst.FindAll(item => item is BinaryExpressionAst, true); + foreach (BinaryExpressionAst binAst in binAsts) + { + if (IncorrectComparisonWithNull(binAst, funcAst)) + { + yield return new DiagnosticRecord(Strings.PossibleIncorrectComparisonWithNullError, binAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName); + } + } + } + } + + private bool IncorrectComparisonWithNull(BinaryExpressionAst binExpressionAst, Ast ast) + { + if ((binExpressionAst.Operator.Equals(TokenKind.Equals) || binExpressionAst.Operator.Equals(TokenKind.Ceq) + || binExpressionAst.Operator.Equals(TokenKind.Cne) || binExpressionAst.Operator.Equals(TokenKind.Ine) || binExpressionAst.Operator.Equals(TokenKind.Ieq)) + && binExpressionAst.Right.Extent.Text.Equals("$null", StringComparison.OrdinalIgnoreCase)) + { + if (binExpressionAst.Left.StaticType.IsArray) + { + return true; + } + else if (binExpressionAst.Left is VariableExpressionAst) + { + // ignores if the variable is a special variable + if (!Helper.Instance.HasSpecialVars((binExpressionAst.Left as VariableExpressionAst).VariablePath.UserPath)) + { + Type lhsType = Helper.Instance.GetTypeFromAnalysis(binExpressionAst.Left as VariableExpressionAst, ast); + if (lhsType == null) + { + return true; + } + else if (lhsType.IsArray || lhsType == typeof(object) || lhsType == typeof(Undetermined) || lhsType == typeof(Unreached)) + { + return true; + } + } + } + else if (binExpressionAst.Left.StaticType == typeof(object)) + { + return true; + } + } + + return false; } /// diff --git a/Tests/Rules/PossibleIncorrectComparisonWithNull.ps1 b/Tests/Rules/PossibleIncorrectComparisonWithNull.ps1 index a2b520739..fbfffd93d 100644 --- a/Tests/Rules/PossibleIncorrectComparisonWithNull.ps1 +++ b/Tests/Rules/PossibleIncorrectComparisonWithNull.ps1 @@ -1,4 +1,23 @@ function CompareWithNull { if ($DebugPreference -eq $null) { } +} + +if (@("dfd", "eee") -eq $null) +{ +} + +if ($randomUninitializedVariable -eq $null) +{ +} + +function Test +{ + $b = "dd", "ddfd"; + if ($b -ceq $null) + { + if ("dd","ee" -eq $null) + { + } + } } \ No newline at end of file diff --git a/Tests/Rules/PossibleIncorrectComparisonWithNull.tests.ps1 b/Tests/Rules/PossibleIncorrectComparisonWithNull.tests.ps1 index 4ef850c55..bdcb40902 100644 --- a/Tests/Rules/PossibleIncorrectComparisonWithNull.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectComparisonWithNull.tests.ps1 @@ -7,8 +7,8 @@ $noViolations = Invoke-ScriptAnalyzer $directory\PossibleIncorrectComparisonWith Describe "PossibleIncorrectComparisonWithNull" { Context "When there are violations" { - It "has 1 possible incorrect comparison with null violation" { - $violations.Count | Should Be 1 + It "has 4 possible incorrect comparison with null violation" { + $violations.Count | Should Be 4 } It "has the correct description message" { diff --git a/Tests/Rules/PossibleIncorrectComparisonWithNullNoViolations.ps1 b/Tests/Rules/PossibleIncorrectComparisonWithNullNoViolations.ps1 index 5ac741e0a..6a34d3c80 100644 --- a/Tests/Rules/PossibleIncorrectComparisonWithNullNoViolations.ps1 +++ b/Tests/Rules/PossibleIncorrectComparisonWithNullNoViolations.ps1 @@ -1,4 +1,15 @@ function CompareWithNull { if ($null -eq $DebugPreference) { } + if ($DebugPreference -eq $null) { + } +} + +$a = 3 + +if ($a -eq $null) +{ + if (3 -eq $null) + { + } } \ No newline at end of file From 21167ca75eb2ad9458b027f67d4028d8cf78b7ee Mon Sep 17 00:00:00 2001 From: "Raghu Shantha [MSFT]" Date: Wed, 24 Jun 2015 14:38:40 -0700 Subject: [PATCH 2/3] Update CHANGELOG.MD --- CHANGELOG.MD | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.MD b/CHANGELOG.MD index 4bec47113..c818a08fd 100644 --- a/CHANGELOG.MD +++ b/CHANGELOG.MD @@ -1,3 +1,30 @@ +## Released v1.0.2 (June.25, 2015) +###Features: +- Perf improvements in the Engine to execute rules concurrently. + + +###Rules: +- New rule to validate the presence of deprecated module manifest fields. +- Removed PSAvoidTrapStatement rule from the builtin set – since this is not deprecated and using trap is a better choice in certain scenarios. + + +###Fixes: +- Verbose Message rule applies to only DSC cmdlet based resources. +- Multiple fixes to AvoidUninitializedVariable to work with non-mandatory parameters, fix in the flow graphs for throw statements; support for missing preference variables; support for automatic variables. +- PSAvoidUsingInternalsURLs to work with xPath expressions. +- UseSingularNouns rule to raise warnings for plural phrases. +- Added .gitignore to exclude certain files from being reported as untracked. +- Revisited severity for DSC rules. +- PSUseOutputTypeCorrectly rule not to get triggered for functions returning system.void type. +- PSAvoidDefaultTrueValueSwitchParameter to work with switch attribute when supplied as fully qualified. +- Ignore PSObject and PSCustomObject for UseOutputTypeCorrectly rule. +- Only raise NullComparisonRule if the RHS is an array or has unknown type. +- PSUseDeclaredVarsMoreThanAssignments rule to be raised for script variables and for setting the property of a variable. +- Support for using PSUseCmdletCorrectly rule when mandatory parameters are supplied in a splatted hashtable. +- AvoidUsingPlainTextForPassword rule to be raised only strings or object types. +- Fix for PositionalParameterUsed method (Helper.cs) uses unsafe method to exclude ForEach-Object and Where-Object. + + ## Released v1.0.1 (May.8, 2015) ###Features: - Integrated with waffle.io for Project Management. @@ -18,7 +45,6 @@ ##Released v1.0.0 on Apr.24, 2015 - ###Features: - Finalized three levels of Severity - Error/Warning/Information. - Improved PSScriptAnalyzer engine behavior: emits non-terminating errors (Ex: for failed ast parse) and continues rule application when running on multiple scripts. From 610b12c3ac6e241ded00cec2e4643a2bb926e4ae Mon Sep 17 00:00:00 2001 From: "Raghu Shantha [MSFT]" Date: Wed, 24 Jun 2015 14:39:04 -0700 Subject: [PATCH 3/3] Update PSScriptAnalyzer.psd1 --- Engine/PSScriptAnalyzer.psd1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Engine/PSScriptAnalyzer.psd1 b/Engine/PSScriptAnalyzer.psd1 index 2057d6ca3..8d26cea21 100644 --- a/Engine/PSScriptAnalyzer.psd1 +++ b/Engine/PSScriptAnalyzer.psd1 @@ -11,7 +11,7 @@ Author = 'Microsoft Corporation' RootModule = 'Microsoft.Windows.PowerShell.ScriptAnalyzer.dll' # Version number of this module. -ModuleVersion = '1.0.1' +ModuleVersion = '1.0.2' # ID used to uniquely identify this module GUID = '324fc715-36bf-4aee-8e58-72e9b4a08ad9'