From 3701e651bf0faceb304ec01cd57684decbcd99bd Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 2 Jun 2018 21:51:22 +0100 Subject: [PATCH 1/6] Fix NullReferenceException in AvoidAssignmentToAutomaticVariable rule when assigning a net property to a .Net property --- Rules/AvoidAssignmentToAutomaticVariable.cs | 25 +++++++++++-------- ...oidAssignmentToAutomaticVariable.tests.ps1 | 15 +++++++++++ 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/Rules/AvoidAssignmentToAutomaticVariable.cs b/Rules/AvoidAssignmentToAutomaticVariable.cs index 6011300ef..87c8b66a9 100644 --- a/Rules/AvoidAssignmentToAutomaticVariable.cs +++ b/Rules/AvoidAssignmentToAutomaticVariable.cs @@ -47,18 +47,21 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) foreach (var assignmentStatementAst in assignmentStatementAsts) { var variableExpressionAst = assignmentStatementAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst; - var variableName = variableExpressionAst.VariablePath.UserPath; - if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase)) + if (variableExpressionAst != null) { - yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName), - variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName); - } - - if (_readOnlyAutomaticVariablesIntroducedInVersion6_0.Contains(variableName, StringComparer.OrdinalIgnoreCase)) - { - var severity = IsPowerShellVersion6OrGreater() ? DiagnosticSeverity.Error : DiagnosticSeverity.Warning; - yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableIntroducedInPowerShell6_0Error, variableName), - variableExpressionAst.Extent, GetName(), severity, fileName); + var variableName = variableExpressionAst.VariablePath.UserPath; + if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase)) + { + yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName), + variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName); + } + + if (_readOnlyAutomaticVariablesIntroducedInVersion6_0.Contains(variableName, StringComparer.OrdinalIgnoreCase)) + { + var severity = IsPowerShellVersion6OrGreater() ? DiagnosticSeverity.Error : DiagnosticSeverity.Warning; + yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableIntroducedInPowerShell6_0Error, variableName), + variableExpressionAst.Extent, GetName(), severity, fileName); + } } } diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index 5709244ab..0e7792e01 100644 --- a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -63,6 +63,21 @@ Describe "AvoidAssignmentToAutomaticVariables" { $warnings.Count | Should -Be 0 } + It "Does not throw a NullReferenceException when using assigning a .Net property to a .Net property (Bug in 1.17.0 - issue 1007)" { + $exceptionThrown = $false + try + { + Invoke-ScriptAnalyzer -ScriptDefinition '[foo]::bar = [baz]::qux' -ErrorAction Stop + } + catch + { + $exceptionThrown = $true + } + + $exceptionThrown | Should -Be $false + } + + It "Setting Variable throws exception in applicable PowerShell version to verify the variables is read-only" -TestCases $testCases_ReadOnlyVariables { param ($VariableName, $ExpectedSeverity, $OnlyPresentInCoreClr) From 9910fdae9a19c20dbb6bec3b51555204de1519b0 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 4 Jun 2018 21:11:46 +0100 Subject: [PATCH 2/6] when variableExpressionAst is null, then continue loop to reduce nesting --- Rules/AvoidAssignmentToAutomaticVariable.cs | 26 ++++++++++----------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/Rules/AvoidAssignmentToAutomaticVariable.cs b/Rules/AvoidAssignmentToAutomaticVariable.cs index 87c8b66a9..1be9ba3a3 100644 --- a/Rules/AvoidAssignmentToAutomaticVariable.cs +++ b/Rules/AvoidAssignmentToAutomaticVariable.cs @@ -47,21 +47,19 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) foreach (var assignmentStatementAst in assignmentStatementAsts) { var variableExpressionAst = assignmentStatementAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst; - if (variableExpressionAst != null) + if (variableExpressionAst == null) { continue; } + var variableName = variableExpressionAst.VariablePath.UserPath; + if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase)) { - var variableName = variableExpressionAst.VariablePath.UserPath; - if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase)) - { - yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName), - variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName); - } - - if (_readOnlyAutomaticVariablesIntroducedInVersion6_0.Contains(variableName, StringComparer.OrdinalIgnoreCase)) - { - var severity = IsPowerShellVersion6OrGreater() ? DiagnosticSeverity.Error : DiagnosticSeverity.Warning; - yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableIntroducedInPowerShell6_0Error, variableName), - variableExpressionAst.Extent, GetName(), severity, fileName); - } + yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName), + variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName); + } + + if (_readOnlyAutomaticVariablesIntroducedInVersion6_0.Contains(variableName, StringComparer.OrdinalIgnoreCase)) + { + var severity = IsPowerShellVersion6OrGreater() ? DiagnosticSeverity.Error : DiagnosticSeverity.Warning; + yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableIntroducedInPowerShell6_0Error, variableName), + variableExpressionAst.Extent, GetName(), severity, fileName); } } From e4d1c72d4fb2162a62f382594fb67035ce5925e1 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 4 Jun 2018 21:13:09 +0100 Subject: [PATCH 3/6] fix indentation for cleaner diff --- Rules/AvoidAssignmentToAutomaticVariable.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Rules/AvoidAssignmentToAutomaticVariable.cs b/Rules/AvoidAssignmentToAutomaticVariable.cs index 1be9ba3a3..1eba2e878 100644 --- a/Rules/AvoidAssignmentToAutomaticVariable.cs +++ b/Rules/AvoidAssignmentToAutomaticVariable.cs @@ -52,14 +52,14 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase)) { yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName), - variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName); + variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName); } if (_readOnlyAutomaticVariablesIntroducedInVersion6_0.Contains(variableName, StringComparer.OrdinalIgnoreCase)) { var severity = IsPowerShellVersion6OrGreater() ? DiagnosticSeverity.Error : DiagnosticSeverity.Warning; yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableIntroducedInPowerShell6_0Error, variableName), - variableExpressionAst.Extent, GetName(), severity, fileName); + variableExpressionAst.Extent, GetName(), severity, fileName); } } From 81366384257c9da57f0d245cd9ac3576573896ed Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 4 Jun 2018 22:46:21 +0100 Subject: [PATCH 4/6] Fix issue #1013 as well by making sure the rule looks only the LHS --- Rules/AvoidAssignmentToAutomaticVariable.cs | 4 ++-- Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/Rules/AvoidAssignmentToAutomaticVariable.cs b/Rules/AvoidAssignmentToAutomaticVariable.cs index 1eba2e878..b92ad527f 100644 --- a/Rules/AvoidAssignmentToAutomaticVariable.cs +++ b/Rules/AvoidAssignmentToAutomaticVariable.cs @@ -44,9 +44,9 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); IEnumerable assignmentStatementAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: true); - foreach (var assignmentStatementAst in assignmentStatementAsts) + foreach (AssignmentStatementAst assignmentStatementAst in assignmentStatementAsts) { - var variableExpressionAst = assignmentStatementAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst; + var variableExpressionAst = assignmentStatementAst.Left.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst; if (variableExpressionAst == null) { continue; } var variableName = variableExpressionAst.VariablePath.UserPath; if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase)) diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index 0e7792e01..0e0f73523 100644 --- a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -77,6 +77,10 @@ Describe "AvoidAssignmentToAutomaticVariables" { $exceptionThrown | Should -Be $false } + It "Does not flag RHS of variable assignment (Bug in 1.17.0, issue 1013)" { + [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition '[foo]::bar = $true' + $warnings.Count | Should -Be 0 + } It "Setting Variable throws exception in applicable PowerShell version to verify the variables is read-only" -TestCases $testCases_ReadOnlyVariables { param ($VariableName, $ExpectedSeverity, $OnlyPresentInCoreClr) From f818df7740c3042dbe092c64ca18f70709afe61e Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 4 Jun 2018 23:27:09 +0100 Subject: [PATCH 5/6] Fix issue #1012 as well --- Rules/AvoidAssignmentToAutomaticVariable.cs | 2 +- Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Rules/AvoidAssignmentToAutomaticVariable.cs b/Rules/AvoidAssignmentToAutomaticVariable.cs index b92ad527f..e3b69ff57 100644 --- a/Rules/AvoidAssignmentToAutomaticVariable.cs +++ b/Rules/AvoidAssignmentToAutomaticVariable.cs @@ -46,7 +46,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) IEnumerable assignmentStatementAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: true); foreach (AssignmentStatementAst assignmentStatementAst in assignmentStatementAsts) { - var variableExpressionAst = assignmentStatementAst.Left.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst; + var variableExpressionAst = assignmentStatementAst.Left.Find(testAst => testAst is VariableExpressionAst && testAst.Parent == assignmentStatementAst, searchNestedScriptBlocks: false) as VariableExpressionAst; if (variableExpressionAst == null) { continue; } var variableName = variableExpressionAst.VariablePath.UserPath; if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase)) diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index 0e0f73523..f9e08cab6 100644 --- a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -77,6 +77,11 @@ Describe "AvoidAssignmentToAutomaticVariables" { $exceptionThrown | Should -Be $false } + It "Does not flag properties of a readonly variable (issue 1012)" { + [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition '$Host.PrivateData["ErrorBackgroundColor"] = "Black"' + $warnings.Count | Should -Be 0 + } + It "Does not flag RHS of variable assignment (Bug in 1.17.0, issue 1013)" { [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition '[foo]::bar = $true' $warnings.Count | Should -Be 0 From 9fbe4e1b68f7357adc6f7bf104c905afdaaa48ba Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Tue, 5 Jun 2018 07:26:15 +0100 Subject: [PATCH 6/6] Simplify test that checks that PSSA does not throw to address PR review --- .../AvoidAssignmentToAutomaticVariable.tests.ps1 | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index f9e08cab6..e2a363da5 100644 --- a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -64,17 +64,7 @@ Describe "AvoidAssignmentToAutomaticVariables" { } It "Does not throw a NullReferenceException when using assigning a .Net property to a .Net property (Bug in 1.17.0 - issue 1007)" { - $exceptionThrown = $false - try - { - Invoke-ScriptAnalyzer -ScriptDefinition '[foo]::bar = [baz]::qux' -ErrorAction Stop - } - catch - { - $exceptionThrown = $true - } - - $exceptionThrown | Should -Be $false + Invoke-ScriptAnalyzer -ScriptDefinition '[foo]::bar = [baz]::qux' -ErrorAction Stop } It "Does not flag properties of a readonly variable (issue 1012)" {