From d8a2a6792ce744241b4a0c3d344b6824c586cd90 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 23 Mar 2016 11:59:10 -0700 Subject: [PATCH 01/25] Add SuggestdCorrection property to DiagnosticRecord --- Engine/Generic/DiagnosticRecord.cs | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/Engine/Generic/DiagnosticRecord.cs b/Engine/Generic/DiagnosticRecord.cs index 5425d9d9a..7d0475361 100644 --- a/Engine/Generic/DiagnosticRecord.cs +++ b/Engine/Generic/DiagnosticRecord.cs @@ -26,6 +26,7 @@ public class DiagnosticRecord private DiagnosticSeverity severity; private string scriptName; private string ruleSuppressionId; + private string suggestedCorrection; /// /// Represents a string from the rule about why this diagnostic was created. @@ -91,6 +92,15 @@ public string RuleSuppressionID set { ruleSuppressionId = value; } } + /// + /// Returns suggested correction + /// return value can be null + /// + public string SuggestedCorrection + { + get { return suggestedCorrection; } + } + /// /// DiagnosticRecord: The constructor for DiagnosticRecord class. /// @@ -98,23 +108,25 @@ public DiagnosticRecord() { } - + /// - /// DiagnosticRecord: The constructor for DiagnosticRecord class. + /// DiagnosticRecord: The constructor for DiagnosticRecord class that takes in suggestedCorrection /// /// A string about why this diagnostic was created /// The place in the script this diagnostic refers to /// The name of the rule that created this diagnostic /// The severity of this diagnostic /// The name of the script file being analyzed - public DiagnosticRecord(string message, IScriptExtent extent, string ruleName, DiagnosticSeverity severity, string scriptName, string ruleId = null) + /// The correction suggested by the rule to replace the extent text + public DiagnosticRecord(string message, IScriptExtent extent, string ruleName, DiagnosticSeverity severity, string scriptName, string ruleId = null, string suggestedCorrection = null) { - Message = string.IsNullOrEmpty(message) ? string.Empty : message; + Message = string.IsNullOrEmpty(message) ? string.Empty : message; RuleName = string.IsNullOrEmpty(ruleName) ? string.Empty : ruleName; - Extent = extent; + Extent = extent; Severity = severity; ScriptName = string.IsNullOrEmpty(scriptName) ? string.Empty : scriptName; ruleSuppressionId = ruleId; + this.suggestedCorrection = suggestedCorrection; } } From c8fe6d6c061020432b88fefbe0c0e56dde453c14 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 23 Mar 2016 12:01:06 -0700 Subject: [PATCH 02/25] Set SuggestedCorrection property in AvoidAlias rule --- Rules/AvoidAlias.cs | 16 +++++++++++++--- Tests/Rules/AvoidUsingAlias.tests.ps1 | 5 +++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/Rules/AvoidAlias.cs b/Rules/AvoidAlias.cs index 1b3e05779..380b86c70 100644 --- a/Rules/AvoidAlias.cs +++ b/Rules/AvoidAlias.cs @@ -40,17 +40,27 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { CommandAst cmdAst = (CommandAst)foundAst; string aliasName = cmdAst.GetCommandName(); + // Handles the exception caused by commands like, {& $PLINK $args 2> $TempErrorFile}. // You can also review the remark section in following document, // MSDN: CommandAst.GetCommandName Method - if (aliasName == null) continue; + if (aliasName == null) + { + continue; + } string cmdletName = Microsoft.Windows.PowerShell.ScriptAnalyzer.Helper.Instance.GetCmdletNameFromAlias(aliasName); if (!String.IsNullOrEmpty(cmdletName)) { - yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesError, aliasName, cmdletName), - cmdAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName, aliasName); + yield return new DiagnosticRecord( + string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesError, aliasName, cmdletName), + cmdAst.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + aliasName, + cmdletName); } } } diff --git a/Tests/Rules/AvoidUsingAlias.tests.ps1 b/Tests/Rules/AvoidUsingAlias.tests.ps1 index 1a57c4f36..77ea90563 100644 --- a/Tests/Rules/AvoidUsingAlias.tests.ps1 +++ b/Tests/Rules/AvoidUsingAlias.tests.ps1 @@ -14,6 +14,11 @@ Describe "AvoidUsingAlias" { It "has the correct description message" { $violations[1].Message | Should Match $violationMessage } + + It "suggests correction" { + $violations[0].SuggestedCorrection | Should Be 'Invoke-Expression' + $violations[1].SuggestedCorrection | Should Be 'Clear-Host' + } } Context "When there are no violations" { From a18446ef6e46a3b4c91034e3c6cda0381b17fe91 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 23 Mar 2016 12:02:07 -0700 Subject: [PATCH 03/25] Set SuggestedCorrection property in MisleadingBacktick rule --- Rules/MisleadingBacktick.cs | 9 ++++++--- Tests/Rules/MisleadingBacktick.tests.ps1 | 6 ++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/Rules/MisleadingBacktick.cs b/Rules/MisleadingBacktick.cs index 1da366faf..829b359d9 100644 --- a/Rules/MisleadingBacktick.cs +++ b/Rules/MisleadingBacktick.cs @@ -56,11 +56,14 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) int lineNumber = ast.Extent.StartLineNumber + i; ScriptPosition start = new ScriptPosition(fileName, lineNumber, match.Index, line); - ScriptPosition end = new ScriptPosition(fileName, lineNumber, match.Index + match.Length, line); - + ScriptPosition end = new ScriptPosition(fileName, lineNumber, match.Index + match.Length, line); yield return new DiagnosticRecord( string.Format(CultureInfo.CurrentCulture, Strings.MisleadingBacktickError), - new ScriptExtent(start, end), GetName(), DiagnosticSeverity.Warning, fileName); + new ScriptExtent(start, end), + GetName(), + DiagnosticSeverity.Warning, + fileName, + suggestedCorrection:string.Empty); } } } diff --git a/Tests/Rules/MisleadingBacktick.tests.ps1 b/Tests/Rules/MisleadingBacktick.tests.ps1 index 659d6fdb3..54c3992ca 100644 --- a/Tests/Rules/MisleadingBacktick.tests.ps1 +++ b/Tests/Rules/MisleadingBacktick.tests.ps1 @@ -8,6 +8,12 @@ Describe "Avoid Misleading Backticks" { Context "When there are violations" { It "has 5 misleading backtick violations" { $violations.Count | Should Be 5 + + foreach ($violation in $violations) + { + $violation.SuggestedCorrection | Should Not Be $null + $violation.SuggestedCorrection | Should BeNullOrEmpty + } } } From 50706a167da87eb20dc42661b2d0ca235a7935d7 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Thu, 24 Mar 2016 19:13:21 -0700 Subject: [PATCH 04/25] Add an extent type to encapsulate correction information --- Engine/Generic/CorrectionExtent.cs | 161 ++++++++++++++++++++++++ Engine/ScriptAnalyzerEngine.csproj | 37 +++--- Tests/Engine/CorrectionExtent.tests.ps1 | 22 ++++ 3 files changed, 202 insertions(+), 18 deletions(-) create mode 100644 Engine/Generic/CorrectionExtent.cs create mode 100644 Tests/Engine/CorrectionExtent.tests.ps1 diff --git a/Engine/Generic/CorrectionExtent.cs b/Engine/Generic/CorrectionExtent.cs new file mode 100644 index 000000000..a298d9978 --- /dev/null +++ b/Engine/Generic/CorrectionExtent.cs @@ -0,0 +1,161 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Management.Automation.Language; +using System.Text; +using System.Threading.Tasks; + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic +{ + public class CorrectionExtent : IScriptExtent + { + public int EndColumnNumber + { + get + { + return endColumnNumber; + } + } + + public int EndLineNumber + { + get + { + return endLineNumber; + } + } + + public int EndOffset + { + get + { + throw new NotImplementedException(); + } + } + + public IScriptPosition EndScriptPosition + { + get + { + throw new NotImplementedException(); + } + } + + public string File + { + get + { + return file; + } + } + + public int StartColumnNumber + { + get + { + return startColumnNumber; + } + } + + public int StartLineNumber + { + get + { + return startLineNumber; + } + } + + public int StartOffset + { + get + { + throw new NotImplementedException(); + } + } + + public IScriptPosition StartScriptPosition + { + get + { + throw new NotImplementedException(); + } + } + + public string Text + { + get + { + return text; + } + } + + private string file; + private int startLineNumber; + private int endLineNumber; + private int startColumnNumber; + private int endColumnNumber; + private string text; + + public CorrectionExtent(string file, int startLineNumber, int endLineNumber, int startColumnNumber, int endColumnNumber, string text) + { + this.startLineNumber = startLineNumber; + this.endLineNumber = endLineNumber; + this.startColumnNumber = startColumnNumber; + this.endColumnNumber = endColumnNumber; + this.file = file; + this.text = text; + ThrowIfInvalidArguments(); + } + + private void ThrowIfInvalidArguments() + { + ThrowIfNull(file, "filename"); + ThrowIfNull(text, "text"); + ThrowIfDecreasing(startLineNumber, endLineNumber, "start line number cannot be less than end line number"); + ThrowIfTextNotConsistent(); + } + + private void ThrowIfTextNotConsistent() + { + using (var stringReader = new StringReader(text)) + { + int numLines = 0; + int expectedNumLines = endLineNumber - startLineNumber + 1; + for (string line = stringReader.ReadLine(); line != null; line = stringReader.ReadLine()) + { + numLines++; + } + if (numLines != expectedNumLines) + { + throw new ArgumentException("number of lines not consistent with text argument"); + } + if (numLines == 1) + { + ThrowIfDecreasing(startColumnNumber, endColumnNumber, "start column number cannot be less then end column number"); + if (endColumnNumber - startColumnNumber != text.Length) + { + throw new ArgumentException("column numbers are inconsistent with the length of the string"); + } + } + } + } + + private void ThrowIfDecreasing(int start, int end, string message) + { + if (start > end) + { + throw new ArgumentException(message); + } + } + + private void ThrowIfNull(T arg, string argName) + { + if (arg == null) + { + throw new ArgumentNullException(argName); + } + } + + } +} diff --git a/Engine/ScriptAnalyzerEngine.csproj b/Engine/ScriptAnalyzerEngine.csproj index 368214b56..84442e131 100644 --- a/Engine/ScriptAnalyzerEngine.csproj +++ b/Engine/ScriptAnalyzerEngine.csproj @@ -34,23 +34,23 @@ Microsoft.Windows.PowerShell.ScriptAnalyzer - true - bin\PSV3 Debug\ - TRACE;DEBUG;PSV3 - full - AnyCPU - prompt - MinimumRecommendedRules.ruleset - - - bin\PSV3 Release\ - TRACE;PSV3 - true - pdbonly - AnyCPU - prompt - MinimumRecommendedRules.ruleset - + true + bin\PSV3 Debug\ + TRACE;DEBUG;PSV3 + full + AnyCPU + prompt + MinimumRecommendedRules.ruleset + + + bin\PSV3 Release\ + TRACE;PSV3 + true + pdbonly + AnyCPU + prompt + MinimumRecommendedRules.ruleset + @@ -70,6 +70,7 @@ + @@ -101,7 +102,7 @@ - + diff --git a/Tests/Engine/CorrectionExtent.tests.ps1 b/Tests/Engine/CorrectionExtent.tests.ps1 new file mode 100644 index 000000000..a152d5acf --- /dev/null +++ b/Tests/Engine/CorrectionExtent.tests.ps1 @@ -0,0 +1,22 @@ +if (!(Get-Module PSScriptAnalyzer)) +{ + Import-Module PSScriptAnalyzer +} + + +Describe "Correction Extent" { + Context "It should throw for invalid arguments" { + It "throw if end line number is less than start line number" { + $filename = "newfile" + $startLineNumber = 2 + $endLineNumber = 1 + $startColumnNumber = 1 + + $text = "Get-ChildItem" + $endColumnNumber = $text.Length + {[Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.CorrectionExtent]::new($filename, $startLineNumber, $startColumnNumber, $endLineNumber, $endColumnNumber, "T")} | Should Throw + } + } +} + + From e9979063063b8b3d5d7136fe8b8bcb3af826b6e5 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Fri, 25 Mar 2016 15:26:18 -0700 Subject: [PATCH 05/25] Modify CorrectionExtent class The class does not implement IScriptExtent interface anymore. The purpose of CorrectionExtent is to emit violation extent and the correction text but this behaviour is not consistent with the IScriptExtent interface. --- Engine/Generic/CorrectionExtent.cs | 64 ++---------------------- Engine/Generic/DiagnosticRecord.cs | 13 ++--- Rules/AvoidAlias.cs | 22 +++++++- Rules/MisleadingBacktick.cs | 27 ++++++++-- Tests/Engine/CorrectionExtent.tests.ps1 | 29 +++++++---- Tests/Rules/AvoidUsingAlias.tests.ps1 | 11 ++-- Tests/Rules/MisleadingBacktick.tests.ps1 | 4 +- 7 files changed, 85 insertions(+), 85 deletions(-) diff --git a/Engine/Generic/CorrectionExtent.cs b/Engine/Generic/CorrectionExtent.cs index a298d9978..144dd859a 100644 --- a/Engine/Generic/CorrectionExtent.cs +++ b/Engine/Generic/CorrectionExtent.cs @@ -8,7 +8,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic { - public class CorrectionExtent : IScriptExtent + public class CorrectionExtent { public int EndColumnNumber { @@ -26,22 +26,6 @@ public int EndLineNumber } } - public int EndOffset - { - get - { - throw new NotImplementedException(); - } - } - - public IScriptPosition EndScriptPosition - { - get - { - throw new NotImplementedException(); - } - } - public string File { get @@ -66,22 +50,6 @@ public int StartLineNumber } } - public int StartOffset - { - get - { - throw new NotImplementedException(); - } - } - - public IScriptPosition StartScriptPosition - { - get - { - throw new NotImplementedException(); - } - } - public string Text { get @@ -97,7 +65,7 @@ public string Text private int endColumnNumber; private string text; - public CorrectionExtent(string file, int startLineNumber, int endLineNumber, int startColumnNumber, int endColumnNumber, string text) + public CorrectionExtent(int startLineNumber, int endLineNumber, int startColumnNumber, int endColumnNumber, string text, string file) { this.startLineNumber = startLineNumber; this.endLineNumber = endLineNumber; @@ -113,34 +81,12 @@ private void ThrowIfInvalidArguments() ThrowIfNull(file, "filename"); ThrowIfNull(text, "text"); ThrowIfDecreasing(startLineNumber, endLineNumber, "start line number cannot be less than end line number"); - ThrowIfTextNotConsistent(); - } - - private void ThrowIfTextNotConsistent() - { - using (var stringReader = new StringReader(text)) + if (startLineNumber == endLineNumber) { - int numLines = 0; - int expectedNumLines = endLineNumber - startLineNumber + 1; - for (string line = stringReader.ReadLine(); line != null; line = stringReader.ReadLine()) - { - numLines++; - } - if (numLines != expectedNumLines) - { - throw new ArgumentException("number of lines not consistent with text argument"); - } - if (numLines == 1) - { - ThrowIfDecreasing(startColumnNumber, endColumnNumber, "start column number cannot be less then end column number"); - if (endColumnNumber - startColumnNumber != text.Length) - { - throw new ArgumentException("column numbers are inconsistent with the length of the string"); - } - } + ThrowIfDecreasing(StartColumnNumber, endColumnNumber, "start column number cannot be less than end column number for a one line extent"); } } - + private void ThrowIfDecreasing(int start, int end, string message) { if (start > end) diff --git a/Engine/Generic/DiagnosticRecord.cs b/Engine/Generic/DiagnosticRecord.cs index 7d0475361..c5d5c0f87 100644 --- a/Engine/Generic/DiagnosticRecord.cs +++ b/Engine/Generic/DiagnosticRecord.cs @@ -10,6 +10,7 @@ // THE SOFTWARE. // +using System.Collections.Generic; using System.Management.Automation.Language; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic @@ -26,7 +27,7 @@ public class DiagnosticRecord private DiagnosticSeverity severity; private string scriptName; private string ruleSuppressionId; - private string suggestedCorrection; + private List suggestedCorrections; /// /// Represents a string from the rule about why this diagnostic was created. @@ -96,9 +97,9 @@ public string RuleSuppressionID /// Returns suggested correction /// return value can be null /// - public string SuggestedCorrection + public List SuggestedCorrections { - get { return suggestedCorrection; } + get { return suggestedCorrections; } } /// @@ -117,8 +118,8 @@ public DiagnosticRecord() /// The name of the rule that created this diagnostic /// The severity of this diagnostic /// The name of the script file being analyzed - /// The correction suggested by the rule to replace the extent text - public DiagnosticRecord(string message, IScriptExtent extent, string ruleName, DiagnosticSeverity severity, string scriptName, string ruleId = null, string suggestedCorrection = null) + /// The correction suggested by the rule to replace the extent text + public DiagnosticRecord(string message, IScriptExtent extent, string ruleName, DiagnosticSeverity severity, string scriptName, string ruleId = null, List suggestedCorrections = null) { Message = string.IsNullOrEmpty(message) ? string.Empty : message; RuleName = string.IsNullOrEmpty(ruleName) ? string.Empty : ruleName; @@ -126,7 +127,7 @@ public DiagnosticRecord(string message, IScriptExtent extent, string ruleName, D Severity = severity; ScriptName = string.IsNullOrEmpty(scriptName) ? string.Empty : scriptName; ruleSuppressionId = ruleId; - this.suggestedCorrection = suggestedCorrection; + this.suggestedCorrections = suggestedCorrections; } } diff --git a/Rules/AvoidAlias.cs b/Rules/AvoidAlias.cs index 380b86c70..f490f417f 100644 --- a/Rules/AvoidAlias.cs +++ b/Rules/AvoidAlias.cs @@ -60,11 +60,31 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) DiagnosticSeverity.Warning, fileName, aliasName, - cmdletName); + suggestedCorrections: GetCorrectionExtent(cmdAst, cmdletName)); } } } + /// + /// Creates a list containing suggested correction + /// + /// + /// + /// Returns a list of suggested corrections + private List GetCorrectionExtent(CommandAst cmdAst, string cmdletName) + { + var corrections = new List(); + var ext = cmdAst.Extent; + corrections.Add(new CorrectionExtent( + ext.StartLineNumber, + ext.EndLineNumber, + ext.StartColumnNumber, + ext.EndColumnNumber, + cmdletName, + ext.File)); + return corrections; + } + /// /// GetName: Retrieves the name of this rule. /// diff --git a/Rules/MisleadingBacktick.cs b/Rules/MisleadingBacktick.cs index 829b359d9..7e8a1a989 100644 --- a/Rules/MisleadingBacktick.cs +++ b/Rules/MisleadingBacktick.cs @@ -55,19 +55,38 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { int lineNumber = ast.Extent.StartLineNumber + i; - ScriptPosition start = new ScriptPosition(fileName, lineNumber, match.Index, line); - ScriptPosition end = new ScriptPosition(fileName, lineNumber, match.Index + match.Length, line); + var start = new ScriptPosition(fileName, lineNumber, match.Index, line); + var end = new ScriptPosition(fileName, lineNumber, match.Index + match.Length, line); + var extent = new ScriptExtent(start, end); yield return new DiagnosticRecord( string.Format(CultureInfo.CurrentCulture, Strings.MisleadingBacktickError), - new ScriptExtent(start, end), + extent, GetName(), DiagnosticSeverity.Warning, fileName, - suggestedCorrection:string.Empty); + suggestedCorrections: GetCorrectionExtent(extent)); } } } + /// + /// Creates a list containing suggested correction + /// + /// + /// Returns a list of suggested corrections + private List GetCorrectionExtent(IScriptExtent violationExtent) + { + var corrections = new List(); + corrections.Add(new CorrectionExtent( + violationExtent.StartLineNumber, + violationExtent.EndLineNumber, + violationExtent.StartColumnNumber, + violationExtent.EndColumnNumber, + String.Empty, + violationExtent.File)); + return corrections; + } + /// /// GetName: Retrieves the name of this rule. /// diff --git a/Tests/Engine/CorrectionExtent.tests.ps1 b/Tests/Engine/CorrectionExtent.tests.ps1 index a152d5acf..9e0a1f678 100644 --- a/Tests/Engine/CorrectionExtent.tests.ps1 +++ b/Tests/Engine/CorrectionExtent.tests.ps1 @@ -3,18 +3,29 @@ if (!(Get-Module PSScriptAnalyzer)) Import-Module PSScriptAnalyzer } - Describe "Correction Extent" { - Context "It should throw for invalid arguments" { - It "throw if end line number is less than start line number" { - $filename = "newfile" - $startLineNumber = 2 - $endLineNumber = 1 - $startColumnNumber = 1 + $type = [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.CorrectionExtent] + + Context "Object construction" { + It "creates the object with correct properties" { + $correctionExtent = $type::new(1, 1, 1, 3, "get-childitem", "newfile") + $correctionExtent.StartLineNumber | Should Be 1 + $correctionExtent.EndLineNumber | Should Be 1 + $correctionExtent.StartColumnNumber | Should Be 1 + $correctionExtent.EndColumnNumber | Should be 3 + $correctionExtent.Text | Should Be "get-childitem" + $correctionExtent.File | Should Be "newfile" + } + + It "throws if end line number is less than start line number" { $text = "Get-ChildItem" - $endColumnNumber = $text.Length - {[Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.CorrectionExtent]::new($filename, $startLineNumber, $startColumnNumber, $endLineNumber, $endColumnNumber, "T")} | Should Throw + {$type::new(2, 1, 1, $text.Length + 1, $text, "newfile")} | Should Throw "start line number" + } + + It "throws if end column number is less than start column number for same line" { + $text = "start-process" + {$type::new(1, 1, 2, 1, $text, "newfile")} | Should Throw "start column number" } } } diff --git a/Tests/Rules/AvoidUsingAlias.tests.ps1 b/Tests/Rules/AvoidUsingAlias.tests.ps1 index 77ea90563..4f796a5eb 100644 --- a/Tests/Rules/AvoidUsingAlias.tests.ps1 +++ b/Tests/Rules/AvoidUsingAlias.tests.ps1 @@ -15,10 +15,13 @@ Describe "AvoidUsingAlias" { $violations[1].Message | Should Match $violationMessage } - It "suggests correction" { - $violations[0].SuggestedCorrection | Should Be 'Invoke-Expression' - $violations[1].SuggestedCorrection | Should Be 'Clear-Host' - } + It "suggests correction" { + $violations[0].SuggestedCorrections.Count | Should Be 1 + $violations[0].SuggestedCorrections.Text | Should Be 'Invoke-Expression' + + $violations[1].SuggestedCorrections.Count | Should Be 1 + $violations[1].SuggestedCorrections.Text | Should Be 'Clear-Host' + } } Context "When there are no violations" { diff --git a/Tests/Rules/MisleadingBacktick.tests.ps1 b/Tests/Rules/MisleadingBacktick.tests.ps1 index 54c3992ca..eb9d79359 100644 --- a/Tests/Rules/MisleadingBacktick.tests.ps1 +++ b/Tests/Rules/MisleadingBacktick.tests.ps1 @@ -11,8 +11,8 @@ Describe "Avoid Misleading Backticks" { foreach ($violation in $violations) { - $violation.SuggestedCorrection | Should Not Be $null - $violation.SuggestedCorrection | Should BeNullOrEmpty + $violation.SuggestedCorrections.Count | Should Be 1 + $violation.SuggestedCorrections[0].Text | Should Be '' } } } From d8115667f014b2476ffe65f6e18f25612a2b606e Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 29 Mar 2016 22:05:03 -0700 Subject: [PATCH 06/25] Modify correction extent of avoid using alias --- Rules/AvoidAlias.cs | 7 +++++-- Tests/Rules/AvoidUsingAlias.tests.ps1 | 6 +++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/Rules/AvoidAlias.cs b/Rules/AvoidAlias.cs index f490f417f..8418693c7 100644 --- a/Rules/AvoidAlias.cs +++ b/Rules/AvoidAlias.cs @@ -75,11 +75,14 @@ private List GetCorrectionExtent(CommandAst cmdAst, string cmd { var corrections = new List(); var ext = cmdAst.Extent; + var alias = cmdAst.GetCommandName(); + var startColumnNumber = ext.StartColumnNumber + ext.Text.IndexOf(alias); + var endColumnNumber = startColumnNumber + alias.Length; corrections.Add(new CorrectionExtent( ext.StartLineNumber, ext.EndLineNumber, - ext.StartColumnNumber, - ext.EndColumnNumber, + startColumnNumber, + endColumnNumber, cmdletName, ext.File)); return corrections; diff --git a/Tests/Rules/AvoidUsingAlias.tests.ps1 b/Tests/Rules/AvoidUsingAlias.tests.ps1 index 4f796a5eb..3557732a9 100644 --- a/Tests/Rules/AvoidUsingAlias.tests.ps1 +++ b/Tests/Rules/AvoidUsingAlias.tests.ps1 @@ -2,7 +2,8 @@ $violationMessage = "'cls' is an alias of 'Clear-Host'. Alias can introduce possible problems and make scripts hard to maintain. Please consider changing alias to its full content." $violationName = "PSAvoidUsingCmdletAliases" $directory = Split-Path -Parent $MyInvocation.MyCommand.Path -$violations = Invoke-ScriptAnalyzer $directory\AvoidUsingAlias.ps1 | Where-Object {$_.RuleName -eq $violationName} +$violationsFilepath = Join-Path $directory 'AvoidUsingAlias.ps1' +$violations = Invoke-ScriptAnalyzer $violationsFilepath | Where-Object {$_.RuleName -eq $violationName} $noViolations = Invoke-ScriptAnalyzer $directory\AvoidUsingAliasNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName} Describe "AvoidUsingAlias" { @@ -16,11 +17,14 @@ Describe "AvoidUsingAlias" { } It "suggests correction" { + Import-Module .\PSScriptAnalyzerTestHelper.psm1 $violations[0].SuggestedCorrections.Count | Should Be 1 $violations[0].SuggestedCorrections.Text | Should Be 'Invoke-Expression' + Get-ExtentText $violations[0].SuggestedCorrections[0] $violationsFilepath | Should Be 'iex' $violations[1].SuggestedCorrections.Count | Should Be 1 $violations[1].SuggestedCorrections.Text | Should Be 'Clear-Host' + Get-ExtentText $violations[1].SuggestedCorrections[0] $violationsFilepath | Should Be 'cls' } } From eb05a8ec210e3766e3a073d03a670d34a6976173 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 29 Mar 2016 22:05:47 -0700 Subject: [PATCH 07/25] Modify correction extent of misleading backticks rule --- Rules/MisleadingBacktick.cs | 10 +++---- Tests/Rules/MisleadingBacktick.tests.ps1 | 34 +++++++++++++++++++----- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/Rules/MisleadingBacktick.cs b/Rules/MisleadingBacktick.cs index 7e8a1a989..ae09361bf 100644 --- a/Rules/MisleadingBacktick.cs +++ b/Rules/MisleadingBacktick.cs @@ -55,8 +55,8 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { int lineNumber = ast.Extent.StartLineNumber + i; - var start = new ScriptPosition(fileName, lineNumber, match.Index, line); - var end = new ScriptPosition(fileName, lineNumber, match.Index + match.Length, line); + var start = new ScriptPosition(fileName, lineNumber, match.Index + 1, line); + var end = new ScriptPosition(fileName, lineNumber, match.Index + match.Length + 1, line); var extent = new ScriptExtent(start, end); yield return new DiagnosticRecord( string.Format(CultureInfo.CurrentCulture, Strings.MisleadingBacktickError), @@ -75,12 +75,12 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) /// /// Returns a list of suggested corrections private List GetCorrectionExtent(IScriptExtent violationExtent) - { + { var corrections = new List(); corrections.Add(new CorrectionExtent( - violationExtent.StartLineNumber, + violationExtent.StartLineNumber , violationExtent.EndLineNumber, - violationExtent.StartColumnNumber, + violationExtent.StartColumnNumber + 1, violationExtent.EndColumnNumber, String.Empty, violationExtent.File)); diff --git a/Tests/Rules/MisleadingBacktick.tests.ps1 b/Tests/Rules/MisleadingBacktick.tests.ps1 index eb9d79359..94ed6600c 100644 --- a/Tests/Rules/MisleadingBacktick.tests.ps1 +++ b/Tests/Rules/MisleadingBacktick.tests.ps1 @@ -1,19 +1,41 @@ Import-Module PSScriptAnalyzer $writeHostName = "PSMisleadingBacktick" $directory = Split-Path -Parent $MyInvocation.MyCommand.Path -$violations = Invoke-ScriptAnalyzer $directory\MisleadingBacktick.ps1 | Where-Object {$_.RuleName -eq $writeHostName} +$violationsFilepath = Join-Path $directory 'MisleadingBacktick.ps1' +$violations = Invoke-ScriptAnalyzer $violationsFilepath | Where-Object {$_.RuleName -eq $writeHostName} $noViolations = Invoke-ScriptAnalyzer $directory\NoMisleadingBacktick.ps1 | Where-Object {$_.RuleName -eq $clearHostName} Describe "Avoid Misleading Backticks" { Context "When there are violations" { It "has 5 misleading backtick violations" { + Import-Module .\PSScriptAnalyzerTestHelper.psm1 $violations.Count | Should Be 5 - foreach ($violation in $violations) - { - $violation.SuggestedCorrections.Count | Should Be 1 - $violation.SuggestedCorrections[0].Text | Should Be '' - } + $idx = 0 + $violations[$idx].SuggestedCorrections.Count | Should Be 1 + $violations[$idx].SuggestedCorrections[0].Text | Should Be '' + Get-ExtentText $violations[$idx].SuggestedCorrections[0] $violationsFilepath | Should BeExactly ' ' + + $idx = 1 + $violations[$idx].SuggestedCorrections.Count | Should Be 1 + $violations[$idx].SuggestedCorrections[0].Text | Should Be '' + Get-ExtentText $violations[$idx].SuggestedCorrections[0] $violationsFilepath | Should BeExactly ' ' + + $idx = 2 + $violations[$idx].SuggestedCorrections.Count | Should Be 1 + $violations[$idx].SuggestedCorrections[0].Text | Should Be '' + Get-ExtentText $violations[$idx].SuggestedCorrections[0] $violationsFilepath | Should BeExactly ' ' + + $idx = 3 + $violations[$idx].SuggestedCorrections.Count | Should Be 1 + $violations[$idx].SuggestedCorrections[0].Text | Should Be '' + Get-ExtentText $violations[$idx].SuggestedCorrections[0] $violationsFilepath | Should BeExactly ' ' + + $idx = 4 + $violations[$idx].SuggestedCorrections.Count | Should Be 1 + $violations[$idx].SuggestedCorrections[0].Text | Should Be '' + Get-ExtentText $violations[$idx].SuggestedCorrections[0] $violationsFilepath | Should BeExactly ' ' + } } From b0a7c578d5dff41ce0a2900b35903657e787d755 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 29 Mar 2016 22:09:57 -0700 Subject: [PATCH 08/25] Add a file that contains helper functions for tests --- Tests/Rules/PSScriptAnalyzerTestHelper.psm1 | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 Tests/Rules/PSScriptAnalyzerTestHelper.psm1 diff --git a/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 b/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 new file mode 100644 index 000000000..9bf1fee6e --- /dev/null +++ b/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 @@ -0,0 +1,14 @@ +Function Get-ExtentText +{ + Param( + [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.CorrectionExtent] $violation, + [string] $scriptPath + ) + $scriptContent = Get-Content -Path $scriptPath + $start = [System.Management.Automation.Language.ScriptPosition]::new($scriptPath, $violation.StartLineNumber, $violation.StartColumnNumber, $scriptContent[$violation.StartLineNumber - 1]) + $end = [System.Management.Automation.Language.ScriptPosition]::new($scriptPath, $violation.EndLineNumber, $violation.EndColumnNumber, $scriptContent[$violation.EndLineNumber - 1]) + $extent = [System.Management.Automation.Language.ScriptExtent]::new($start, $end) + return($extent.Text) +} + +Export-ModuleMember -Function Get-ExtentText \ No newline at end of file From 8a02fa728860b426edc546e0fe9ec71f10f460a3 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 30 Mar 2016 14:23:24 -0700 Subject: [PATCH 09/25] Add correction extent to AvoidUsingPlainTextForPassword rule --- Rules/AvoidUsingPlainTextForPassword.cs | 21 ++++++++++++++++++- .../AvoidUsingPlainTextForPassword.tests.ps1 | 19 ++++++++++++++++- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/Rules/AvoidUsingPlainTextForPassword.cs b/Rules/AvoidUsingPlainTextForPassword.cs index 766aa78ee..dcc181686 100644 --- a/Rules/AvoidUsingPlainTextForPassword.cs +++ b/Rules/AvoidUsingPlainTextForPassword.cs @@ -60,11 +60,30 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { yield return new DiagnosticRecord( String.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPlainTextForPasswordError, paramAst.Name), - paramAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName); + paramAst.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + suggestedCorrections: GetCorrectionExtent(paramAst)); } } } + private List GetCorrectionExtent(ParameterAst paramAst) + { + IScriptExtent ext = paramAst.Extent; + var corrections = new List(); + string correctionText = string.Format("{0} {1}", "[SecureString]", paramAst.Name.Extent.Text); + corrections.Add(new CorrectionExtent( + ext.StartLineNumber, + ext.EndLineNumber, + ext.StartColumnNumber, + ext.EndColumnNumber, + correctionText, + ext.File)); + return corrections; + } + /// /// GetName: Retrieves the name of this rule. /// diff --git a/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 b/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 index 945c1bf9b..fca2c5f66 100644 --- a/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 +++ b/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 @@ -3,7 +3,8 @@ $violationMessage = [regex]::Escape("Parameter '`$password' should use SecureString, otherwise this will expose sensitive information. See ConvertTo-SecureString for more information.") $violationName = "PSAvoidUsingPlainTextForPassword" $directory = Split-Path -Parent $MyInvocation.MyCommand.Path -$violations = Invoke-ScriptAnalyzer $directory\AvoidUsingPlainTextForPassword.ps1 | Where-Object {$_.RuleName -eq $violationName} +$violationsFilepath = Join-Path $directory 'AvoidUsingPlainTextForPassword.ps1' +$violations = Invoke-ScriptAnalyzer $violationsFilepath | Where-Object {$_.RuleName -eq $violationName} $noViolations = Invoke-ScriptAnalyzer $directory\AvoidUsingPlainTextForPasswordNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName} Describe "AvoidUsingPlainTextForPassword" { @@ -12,6 +13,22 @@ Describe "AvoidUsingPlainTextForPassword" { $violations.Count | Should Be 4 } + It "suggests corrections" { + Import-Module .\PSScriptAnalyzerTestHelper.psm1 + Function Test-Extent($idx, $violationText, $correctionText) + { + $violation = $violations[$idx] + $violation.SuggestedCorrections.Count | Should Be 1 + Get-ExtentText $violation.SuggestedCorrections[0] $violationsFilepath | Should Be $violationText + $violation.SuggestedCorrections[0].Text | Should Be $correctionText + } + + Test-Extent 0 '$passphrases' '[SecureString] $passphrases' + Test-Extent 1 '$passwordparam' '[SecureString] $passwordparam' + Test-Extent 2 '$credential' '[SecureString] $credential' + Test-Extent 3 '$password' '[SecureString] $password' + } + It "has the correct violation message" { $violations[3].Message | Should Match $violationMessage } From e65b9a9a236fb751c881a36f163c7cde45f613cf Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 30 Mar 2016 14:49:50 -0700 Subject: [PATCH 10/25] Refactor some tests that check correction extent --- Tests/Rules/AvoidUsingAlias.tests.ps1 | 13 ++---- .../AvoidUsingPlainTextForPassword.tests.ps1 | 22 ++++------ Tests/Rules/MisleadingBacktick.tests.ps1 | 40 +++++-------------- Tests/Rules/PSScriptAnalyzerTestHelper.psm1 | 20 +++++++++- 4 files changed, 41 insertions(+), 54 deletions(-) diff --git a/Tests/Rules/AvoidUsingAlias.tests.ps1 b/Tests/Rules/AvoidUsingAlias.tests.ps1 index 3557732a9..69393b730 100644 --- a/Tests/Rules/AvoidUsingAlias.tests.ps1 +++ b/Tests/Rules/AvoidUsingAlias.tests.ps1 @@ -2,8 +2,8 @@ $violationMessage = "'cls' is an alias of 'Clear-Host'. Alias can introduce possible problems and make scripts hard to maintain. Please consider changing alias to its full content." $violationName = "PSAvoidUsingCmdletAliases" $directory = Split-Path -Parent $MyInvocation.MyCommand.Path -$violationsFilepath = Join-Path $directory 'AvoidUsingAlias.ps1' -$violations = Invoke-ScriptAnalyzer $violationsFilepath | Where-Object {$_.RuleName -eq $violationName} +$violationFilepath = Join-Path $directory 'AvoidUsingAlias.ps1' +$violations = Invoke-ScriptAnalyzer $violationFilepath | Where-Object {$_.RuleName -eq $violationName} $noViolations = Invoke-ScriptAnalyzer $directory\AvoidUsingAliasNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName} Describe "AvoidUsingAlias" { @@ -18,13 +18,8 @@ Describe "AvoidUsingAlias" { It "suggests correction" { Import-Module .\PSScriptAnalyzerTestHelper.psm1 - $violations[0].SuggestedCorrections.Count | Should Be 1 - $violations[0].SuggestedCorrections.Text | Should Be 'Invoke-Expression' - Get-ExtentText $violations[0].SuggestedCorrections[0] $violationsFilepath | Should Be 'iex' - - $violations[1].SuggestedCorrections.Count | Should Be 1 - $violations[1].SuggestedCorrections.Text | Should Be 'Clear-Host' - Get-ExtentText $violations[1].SuggestedCorrections[0] $violationsFilepath | Should Be 'cls' + Test-CorrectionExtent $violationFilepath $violations[0] 1 'iex' 'Invoke-Expression' + Test-CorrectionExtent $violationFilepath $violations[1] 1 'cls' 'Clear-Host' } } diff --git a/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 b/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 index fca2c5f66..d946d5f8e 100644 --- a/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 +++ b/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 @@ -3,8 +3,8 @@ $violationMessage = [regex]::Escape("Parameter '`$password' should use SecureString, otherwise this will expose sensitive information. See ConvertTo-SecureString for more information.") $violationName = "PSAvoidUsingPlainTextForPassword" $directory = Split-Path -Parent $MyInvocation.MyCommand.Path -$violationsFilepath = Join-Path $directory 'AvoidUsingPlainTextForPassword.ps1' -$violations = Invoke-ScriptAnalyzer $violationsFilepath | Where-Object {$_.RuleName -eq $violationName} +$violationFilepath = Join-Path $directory 'AvoidUsingPlainTextForPassword.ps1' +$violations = Invoke-ScriptAnalyzer $violationFilepath | Where-Object {$_.RuleName -eq $violationName} $noViolations = Invoke-ScriptAnalyzer $directory\AvoidUsingPlainTextForPasswordNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName} Describe "AvoidUsingPlainTextForPassword" { @@ -14,19 +14,11 @@ Describe "AvoidUsingPlainTextForPassword" { } It "suggests corrections" { - Import-Module .\PSScriptAnalyzerTestHelper.psm1 - Function Test-Extent($idx, $violationText, $correctionText) - { - $violation = $violations[$idx] - $violation.SuggestedCorrections.Count | Should Be 1 - Get-ExtentText $violation.SuggestedCorrections[0] $violationsFilepath | Should Be $violationText - $violation.SuggestedCorrections[0].Text | Should Be $correctionText - } - - Test-Extent 0 '$passphrases' '[SecureString] $passphrases' - Test-Extent 1 '$passwordparam' '[SecureString] $passwordparam' - Test-Extent 2 '$credential' '[SecureString] $credential' - Test-Extent 3 '$password' '[SecureString] $password' + Import-Module .\PSScriptAnalyzerTestHelper.psm1 + Test-CorrectionExtent $violationFilepath $violations[0] 1 '$passphrases' '[SecureString] $passphrases' + Test-CorrectionExtent $violationFilepath $violations[1] 1 '$passwordparam' '[SecureString] $passwordparam' + Test-CorrectionExtent $violationFilepath $violations[2] 1 '$credential' '[SecureString] $credential' + Test-CorrectionExtent $violationFilepath $violations[3] 1 '$password' '[SecureString] $password' } It "has the correct violation message" { diff --git a/Tests/Rules/MisleadingBacktick.tests.ps1 b/Tests/Rules/MisleadingBacktick.tests.ps1 index 94ed6600c..90daa2007 100644 --- a/Tests/Rules/MisleadingBacktick.tests.ps1 +++ b/Tests/Rules/MisleadingBacktick.tests.ps1 @@ -1,42 +1,24 @@ Import-Module PSScriptAnalyzer $writeHostName = "PSMisleadingBacktick" $directory = Split-Path -Parent $MyInvocation.MyCommand.Path -$violationsFilepath = Join-Path $directory 'MisleadingBacktick.ps1' -$violations = Invoke-ScriptAnalyzer $violationsFilepath | Where-Object {$_.RuleName -eq $writeHostName} +$violationFilepath = Join-Path $directory 'MisleadingBacktick.ps1' +$violations = Invoke-ScriptAnalyzer $violationFilepath | Where-Object {$_.RuleName -eq $writeHostName} $noViolations = Invoke-ScriptAnalyzer $directory\NoMisleadingBacktick.ps1 | Where-Object {$_.RuleName -eq $clearHostName} Describe "Avoid Misleading Backticks" { Context "When there are violations" { It "has 5 misleading backtick violations" { - Import-Module .\PSScriptAnalyzerTestHelper.psm1 $violations.Count | Should Be 5 - - $idx = 0 - $violations[$idx].SuggestedCorrections.Count | Should Be 1 - $violations[$idx].SuggestedCorrections[0].Text | Should Be '' - Get-ExtentText $violations[$idx].SuggestedCorrections[0] $violationsFilepath | Should BeExactly ' ' - - $idx = 1 - $violations[$idx].SuggestedCorrections.Count | Should Be 1 - $violations[$idx].SuggestedCorrections[0].Text | Should Be '' - Get-ExtentText $violations[$idx].SuggestedCorrections[0] $violationsFilepath | Should BeExactly ' ' - - $idx = 2 - $violations[$idx].SuggestedCorrections.Count | Should Be 1 - $violations[$idx].SuggestedCorrections[0].Text | Should Be '' - Get-ExtentText $violations[$idx].SuggestedCorrections[0] $violationsFilepath | Should BeExactly ' ' - - $idx = 3 - $violations[$idx].SuggestedCorrections.Count | Should Be 1 - $violations[$idx].SuggestedCorrections[0].Text | Should Be '' - Get-ExtentText $violations[$idx].SuggestedCorrections[0] $violationsFilepath | Should BeExactly ' ' - - $idx = 4 - $violations[$idx].SuggestedCorrections.Count | Should Be 1 - $violations[$idx].SuggestedCorrections[0].Text | Should Be '' - Get-ExtentText $violations[$idx].SuggestedCorrections[0] $violationsFilepath | Should BeExactly ' ' - } + + It "suggests correction" { + Import-Module .\PSScriptAnalyzerTestHelper.psm1 + Test-CorrectionExtent $violationFilepath $violations[0] 1 ' ' '' + Test-CorrectionExtent $violationFilepath $violations[1] 1 ' ' '' + Test-CorrectionExtent $violationFilepath $violations[2] 1 ' ' '' + Test-CorrectionExtent $violationFilepath $violations[3] 1 ' ' '' + Test-CorrectionExtent $violationFilepath $violations[4] 1 ' ' '' + } } Context "When there are no violations" { diff --git a/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 b/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 index 9bf1fee6e..11c1d691e 100644 --- a/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 +++ b/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 @@ -11,4 +11,22 @@ Function Get-ExtentText return($extent.Text) } -Export-ModuleMember -Function Get-ExtentText \ No newline at end of file +Function Test-CorrectionExtent +{ + Param( + [string] $violationFilepath, + [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord] $diagnosticRecord, + [int] $correctionsCount, + [string] $violationText, + [string] $correctionText + ) + $corrections = $diagnosticRecord.SuggestedCorrections + $corrections.Count | Should Be 1 + $corrections[0].Text | Should Be $correctionText + Get-ExtentText $corrections[0] $violationFilepath | ` + Should Be $violationText +} + + +Export-ModuleMember -Function Get-ExtentText +Export-ModuleMember -Function Test-CorrectionExtent \ No newline at end of file From 567b1aedadc6d79de7a6f7a5250b93aff08b52a7 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 30 Mar 2016 15:25:41 -0700 Subject: [PATCH 11/25] Fix a bug in a test helper function --- Tests/Rules/PSScriptAnalyzerTestHelper.psm1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 b/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 index 11c1d691e..e9f116408 100644 --- a/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 +++ b/Tests/Rules/PSScriptAnalyzerTestHelper.psm1 @@ -21,7 +21,7 @@ Function Test-CorrectionExtent [string] $correctionText ) $corrections = $diagnosticRecord.SuggestedCorrections - $corrections.Count | Should Be 1 + $corrections.Count | Should Be $correctionsCount $corrections[0].Text | Should Be $correctionText Get-ExtentText $corrections[0] $violationFilepath | ` Should Be $violationText From cc68a2e065ec702dffc223fb48965916182a2ab5 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 4 Apr 2016 15:18:00 -0700 Subject: [PATCH 12/25] Refactor MissingModuleManifest Rule Moves the module manifest testing logic to a separate method in the Helper class. This will enable reuse of the functionality by other rules. --- Engine/Helper.cs | 52 +++++++++++++++++++++++++++++ Rules/MissingModuleManifestField.cs | 28 ++++------------ 2 files changed, 58 insertions(+), 22 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 6ba8556b1..731371bb7 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -228,6 +228,58 @@ public bool IsDscResourceModule(string filePath) return false; } + + /// + /// Gets the module manifest + /// + /// + /// + /// Returns a object of type PSModuleInfo + public PSModuleInfo GetModuleManifest(string filePath, out IEnumerable errorRecord) + { + errorRecord = null; + PSModuleInfo psModuleInfo = null; + Collection psObj = null; + var ps = System.Management.Automation.PowerShell.Create(); + if (ps == null) + { + return null; + } + try + { + ps.AddCommand("Test-ModuleManifest"); + ps.AddParameter("Path", filePath); + + // Suppress warnings emitted during the execution of Test-ModuleManifest + ps.AddParameter("WarningAction", ActionPreference.SilentlyContinue); + psObj = ps.Invoke(); + } + catch { } + if (ps.HadErrors && ps.Streams != null && ps.Streams.Error != null) + { + var errorRecordArr = new ErrorRecord[ps.Streams.Error.Count]; + ps.Streams.Error.CopyTo(errorRecordArr, 0); + errorRecord = errorRecordArr; + } + if (psObj != null && psObj.Any() && psObj[0] != null) + { + psModuleInfo = psObj[0].ImmediateBaseObject as PSModuleInfo; + } + ps.Dispose(); + return psModuleInfo; + } + + /// + /// Checks if the error record is MissingMemberException + /// + /// + /// Returns a boolean value indicating the presence of MissingMemberException + public bool IsMissingMemberException(ErrorRecord errorRecord) + { + return errorRecord.CategoryInfo != null + && errorRecord.CategoryInfo.Category == ErrorCategory.ResourceUnavailable + && string.Equals("MissingMemberException", errorRecord.CategoryInfo.Reason, StringComparison.OrdinalIgnoreCase); + } /// /// Get the list of exported function by analyzing the ast diff --git a/Rules/MissingModuleManifestField.cs b/Rules/MissingModuleManifestField.cs index 6df37980c..4034d4661 100644 --- a/Rules/MissingModuleManifestField.cs +++ b/Rules/MissingModuleManifestField.cs @@ -38,41 +38,25 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (String.Equals(System.IO.Path.GetExtension(fileName), ".psd1", StringComparison.OrdinalIgnoreCase)) { - var ps = System.Management.Automation.PowerShell.Create(); - - try - { - ps.AddCommand("Test-ModuleManifest"); - ps.AddParameter("Path", fileName); - - // Suppress warnings emitted during the execution of Test-ModuleManifest - // ModuleManifest rule must catch any violations (warnings/errors) and generate DiagnosticRecord(s) - ps.AddParameter("WarningAction", ActionPreference.SilentlyContinue); - ps.Invoke(); - - } catch { } - - if (ps != null && ps.HadErrors && ps.Streams != null && ps.Streams.Error != null) + IEnumerable errorRecords; + var psModuleInfo = Helper.Instance.GetModuleManifest(fileName, out errorRecords); + if (errorRecords != null) { - foreach (var errorRecord in ps.Streams.Error) + foreach (var errorRecord in errorRecords) { - if (errorRecord.CategoryInfo != null && errorRecord.CategoryInfo.Category == System.Management.Automation.ErrorCategory.ResourceUnavailable - && String.Equals("MissingMemberException", errorRecord.CategoryInfo.Reason, StringComparison.OrdinalIgnoreCase)) + if (Helper.Instance.IsMissingMemberException(errorRecord)) { System.Diagnostics.Debug.Assert(errorRecord.Exception != null && !String.IsNullOrWhiteSpace(errorRecord.Exception.Message), Strings.NullErrorMessage); - yield return new DiagnosticRecord(errorRecord.Exception.Message, ast.Extent, GetName(), DiagnosticSeverity.Warning, fileName); } } } - - ps.Dispose(); } } - + /// /// GetName: Retrieves the name of this rule. /// From afccf35543fb4c694ca15205e10e1c95b188bed2 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 4 Apr 2016 19:26:42 -0700 Subject: [PATCH 13/25] Add correction extent to UseToExportFieldsInManifest rule Suggest corrections to FunctionsToExport and AliasesToExport when their values are set to '*' or '$null', e.g., FunctionsToExport = '*'. This does not support wildcard in an array, eg @('Set-Foo', 'Get-*'). --- Rules/UseToExportFieldsInManifest.cs | 99 +++++++++++++++--- .../ManifestBadAliasesWildcard.psd1 | Bin 6598 -> 6656 bytes .../ManifestBadAliasesWildcard.psm1 | 12 +++ .../ManifestBadFunctionsNull.psd1 | Bin 6602 -> 6656 bytes .../ManifestBadFunctionsNull.psm1 | 12 +++ .../ManifestBadFunctionsWildcard.psd1 | Bin 6598 -> 6660 bytes .../ManifestBadFunctionsWildcard.psm1 | 12 +++ .../ManifestBadVariablesWildcard.psd1 | Bin 6598 -> 6664 bytes .../UseToExportFieldsInManifest.tests.ps1 | 21 ++++ 9 files changed, 139 insertions(+), 17 deletions(-) create mode 100644 Tests/Rules/TestManifest/ManifestBadAliasesWildcard.psm1 create mode 100644 Tests/Rules/TestManifest/ManifestBadFunctionsNull.psm1 create mode 100644 Tests/Rules/TestManifest/ManifestBadFunctionsWildcard.psm1 diff --git a/Rules/UseToExportFieldsInManifest.cs b/Rules/UseToExportFieldsInManifest.cs index 324c60487..4901bab5e 100644 --- a/Rules/UseToExportFieldsInManifest.cs +++ b/Rules/UseToExportFieldsInManifest.cs @@ -19,6 +19,7 @@ using System.ComponentModel.Composition; using System.Globalization; using System.Text.RegularExpressions; +using System.Diagnostics; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -29,6 +30,12 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules [Export(typeof(IScriptRule))] public class UseToExportFieldsInManifest : IScriptRule { + + private const string functionsToExport = "FunctionsToExport"; + private const string cmdletsToExport = "CmdletsToExport"; + private const string variablesToExport = "VariablesToExport"; + private const string aliasesToExport = "AliasesToExport"; + /// /// AnalyzeScript: Analyzes the AST to check if AliasToExport, CmdletsToExport, FunctionsToExport /// and VariablesToExport fields do not use wildcards and $null in their entries. @@ -48,12 +55,14 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) yield break; } - if (!IsValidManifest(ast, fileName)) - { + // check if valid module manifest + IEnumerable errorRecord = null; + PSModuleInfo psModuleInfo = Helper.Instance.GetModuleManifest(fileName, out errorRecord); + if ((errorRecord != null && errorRecord.Count() > 0) || psModuleInfo == null) + { yield break; } - - String[] manifestFields = {"FunctionsToExport", "CmdletsToExport", "VariablesToExport", "AliasesToExport"}; + var hashtableAst = ast.Find(x => x is HashtableAst, false) as HashtableAst; if (hashtableAst == null) @@ -61,30 +70,86 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) yield break; } - foreach(String field in manifestFields) + string[] manifestFields = { functionsToExport, cmdletsToExport, variablesToExport, aliasesToExport }; + + foreach(string field in manifestFields) { IScriptExtent extent; if (!HasAcceptableExportField(field, hashtableAst, ast.Extent.Text, out extent) && extent != null) { - yield return new DiagnosticRecord(GetError(field), extent, GetName(), DiagnosticSeverity.Warning, fileName); + yield return new DiagnosticRecord( + GetError(field), + extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + suggestedCorrections: GetCorrectionExtent(field, extent, psModuleInfo)); } } } - - /// - /// Checks if the manifest file is valid. - /// - /// - /// - /// A boolean value indicating the validity of the manifest file. - private bool IsValidManifest(Ast ast, string fileName) + + private string GetListLiteral(Dictionary exportedItems) { - var missingManifestRule = new MissingModuleManifestField(); - return !missingManifestRule.AnalyzeScript(ast, fileName).GetEnumerator().MoveNext(); - + if (exportedItems == null || exportedItems.Keys == null) + { + return null; + } + var sbuilder = new System.Text.StringBuilder(); + sbuilder.Append("@("); + sbuilder.Append(string.Join(", ", exportedItems.Keys.Select(key => string.Format("'{0}'", key)))); + sbuilder.Append(")"); + return sbuilder.ToString(); } + + private List GetCorrectionExtent(string field, IScriptExtent extent, PSModuleInfo psModuleInfo) + { + Debug.Assert(field != null); + Debug.Assert(psModuleInfo != null); + Debug.Assert(extent != null); + var corrections = new List(); + string correctionText = null; + switch (field) + { + case functionsToExport: + correctionText = GetListLiteral(psModuleInfo.ExportedFunctions); + break; + case cmdletsToExport: + correctionText = GetListLiteral(psModuleInfo.ExportedCmdlets); + break; + case variablesToExport: + correctionText = GetListLiteral(psModuleInfo.ExportedVariables); + break; + case aliasesToExport: + correctionText = GetListLiteral(psModuleInfo.ExportedAliases); + break; + default: + throw new NotImplementedException(string.Format("{0} not implemented", field)); + } + corrections.Add(new CorrectionExtent( + extent.StartLineNumber, + extent.EndLineNumber, + extent.StartColumnNumber, + extent.EndColumnNumber, + correctionText, + extent.File)); + return corrections; + } + + ///// + ///// Checks if the manifest file is valid. + ///// + ///// + ///// + ///// A boolean value indicating the validity of the manifest file. + //private bool IsValidManifest(Ast ast, string fileName) + //{ + // var missingManifestRule = new MissingModuleManifestField(); + // return !missingManifestRule.AnalyzeScript(ast, fileName).GetEnumerator().MoveNext(); + + //} + /// /// Checks if the ast contains wildcard character. /// diff --git a/Tests/Rules/TestManifest/ManifestBadAliasesWildcard.psd1 b/Tests/Rules/TestManifest/ManifestBadAliasesWildcard.psd1 index bc7994035c8ff78b864da7472221f4f2173f901d..ed9f0f640b58c14f78a6a10e0643daaac0467e58 100644 GIT binary patch delta 63 zcmX?R++ea{8slUpVUfw}7&#`oh^ad=!K9R}m7&#`oXbAc-lrrQ1p&mm4Loq`xgW=|_jG__%5Wov& delta 40 tcmZoLIc2rnNLoq`xgW=|_jDivXL7EIQ delta 16 XcmeA$IcB_J8{_0xY Date: Mon, 4 Apr 2016 19:49:16 -0700 Subject: [PATCH 14/25] Add Description property to CorrectionExtent class --- Engine/Generic/CorrectionExtent.cs | 39 ++++++++++++++++++++++++- Tests/Engine/CorrectionExtent.tests.ps1 | 3 +- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/Engine/Generic/CorrectionExtent.cs b/Engine/Generic/CorrectionExtent.cs index 144dd859a..0974bea74 100644 --- a/Engine/Generic/CorrectionExtent.cs +++ b/Engine/Generic/CorrectionExtent.cs @@ -58,14 +58,48 @@ public string Text } } + public string Description + { + get + { + return description; + } + } + private string file; private int startLineNumber; private int endLineNumber; private int startColumnNumber; private int endColumnNumber; private string text; + private string description; - public CorrectionExtent(int startLineNumber, int endLineNumber, int startColumnNumber, int endColumnNumber, string text, string file) + public CorrectionExtent( + int startLineNumber, + int endLineNumber, + int startColumnNumber, + int endColumnNumber, + string text, + string file) + : this( + startLineNumber, + endLineNumber, + startColumnNumber, + endColumnNumber, + text, + file, + null) + { + } + + public CorrectionExtent( + int startLineNumber, + int endLineNumber, + int startColumnNumber, + int endColumnNumber, + string text, + string file, + string description) { this.startLineNumber = startLineNumber; this.endLineNumber = endLineNumber; @@ -73,9 +107,12 @@ public CorrectionExtent(int startLineNumber, int endLineNumber, int startColumnN this.endColumnNumber = endColumnNumber; this.file = file; this.text = text; + this.description = description; ThrowIfInvalidArguments(); } + + private void ThrowIfInvalidArguments() { ThrowIfNull(file, "filename"); diff --git a/Tests/Engine/CorrectionExtent.tests.ps1 b/Tests/Engine/CorrectionExtent.tests.ps1 index 9e0a1f678..e4f1c36ef 100644 --- a/Tests/Engine/CorrectionExtent.tests.ps1 +++ b/Tests/Engine/CorrectionExtent.tests.ps1 @@ -8,7 +8,7 @@ Describe "Correction Extent" { Context "Object construction" { It "creates the object with correct properties" { - $correctionExtent = $type::new(1, 1, 1, 3, "get-childitem", "newfile") + $correctionExtent = $type::new(1, 1, 1, 3, "get-childitem", "newfile", "cool description") $correctionExtent.StartLineNumber | Should Be 1 $correctionExtent.EndLineNumber | Should Be 1 @@ -16,6 +16,7 @@ Describe "Correction Extent" { $correctionExtent.EndColumnNumber | Should be 3 $correctionExtent.Text | Should Be "get-childitem" $correctionExtent.File | Should Be "newfile" + $correctionExtent.Description | Should Be "cool description" } It "throws if end line number is less than start line number" { From 807287371e4a636797a490a8ed233358d72b805a Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 4 Apr 2016 20:06:21 -0700 Subject: [PATCH 15/25] Add CorrectionExtent description to some rules --- Rules/AvoidAlias.cs | 4 +++- Rules/AvoidUsingPlainTextForPassword.cs | 6 ++++-- Rules/MisleadingBacktick.cs | 6 ++++-- Rules/UseToExportFieldsInManifest.cs | 6 ++++-- Tests/Rules/AvoidUsingAlias.tests.ps1 | 3 +++ Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 | 2 ++ Tests/Rules/UseToExportFieldsInManifest.tests.ps1 | 1 + 7 files changed, 21 insertions(+), 7 deletions(-) diff --git a/Rules/AvoidAlias.cs b/Rules/AvoidAlias.cs index 8418693c7..f949884ef 100644 --- a/Rules/AvoidAlias.cs +++ b/Rules/AvoidAlias.cs @@ -78,13 +78,15 @@ private List GetCorrectionExtent(CommandAst cmdAst, string cmd var alias = cmdAst.GetCommandName(); var startColumnNumber = ext.StartColumnNumber + ext.Text.IndexOf(alias); var endColumnNumber = startColumnNumber + alias.Length; + string description = string.Format("Replace {0} with {1}", alias, cmdletName); corrections.Add(new CorrectionExtent( ext.StartLineNumber, ext.EndLineNumber, startColumnNumber, endColumnNumber, cmdletName, - ext.File)); + ext.File, + description)); return corrections; } diff --git a/Rules/AvoidUsingPlainTextForPassword.cs b/Rules/AvoidUsingPlainTextForPassword.cs index dcc181686..bab5a4055 100644 --- a/Rules/AvoidUsingPlainTextForPassword.cs +++ b/Rules/AvoidUsingPlainTextForPassword.cs @@ -73,14 +73,16 @@ private List GetCorrectionExtent(ParameterAst paramAst) { IScriptExtent ext = paramAst.Extent; var corrections = new List(); - string correctionText = string.Format("{0} {1}", "[SecureString]", paramAst.Name.Extent.Text); + string correctionText = string.Format("[SecureString] {0}", paramAst.Name.Extent.Text); + string description = string.Format("Set {0} type to SecureString", paramAst.Name.Extent.Text); corrections.Add(new CorrectionExtent( ext.StartLineNumber, ext.EndLineNumber, ext.StartColumnNumber, ext.EndColumnNumber, correctionText, - ext.File)); + ext.File, + description)); return corrections; } diff --git a/Rules/MisleadingBacktick.cs b/Rules/MisleadingBacktick.cs index ae09361bf..abada65ac 100644 --- a/Rules/MisleadingBacktick.cs +++ b/Rules/MisleadingBacktick.cs @@ -76,14 +76,16 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) /// Returns a list of suggested corrections private List GetCorrectionExtent(IScriptExtent violationExtent) { - var corrections = new List(); + var corrections = new List(); + string description = "Remove trailing whilespace"; corrections.Add(new CorrectionExtent( violationExtent.StartLineNumber , violationExtent.EndLineNumber, violationExtent.StartColumnNumber + 1, violationExtent.EndColumnNumber, String.Empty, - violationExtent.File)); + violationExtent.File, + description)); return corrections; } diff --git a/Rules/UseToExportFieldsInManifest.cs b/Rules/UseToExportFieldsInManifest.cs index 4901bab5e..b550fae8a 100644 --- a/Rules/UseToExportFieldsInManifest.cs +++ b/Rules/UseToExportFieldsInManifest.cs @@ -126,14 +126,16 @@ private List GetCorrectionExtent(string field, IScriptExtent e break; default: throw new NotImplementedException(string.Format("{0} not implemented", field)); - } + } + string description = string.Format("Replace {0} with {1}", extent.Text, correctionText); corrections.Add(new CorrectionExtent( extent.StartLineNumber, extent.EndLineNumber, extent.StartColumnNumber, extent.EndColumnNumber, correctionText, - extent.File)); + extent.File, + description)); return corrections; } diff --git a/Tests/Rules/AvoidUsingAlias.tests.ps1 b/Tests/Rules/AvoidUsingAlias.tests.ps1 index 69393b730..3ccc91bcf 100644 --- a/Tests/Rules/AvoidUsingAlias.tests.ps1 +++ b/Tests/Rules/AvoidUsingAlias.tests.ps1 @@ -19,7 +19,10 @@ Describe "AvoidUsingAlias" { It "suggests correction" { Import-Module .\PSScriptAnalyzerTestHelper.psm1 Test-CorrectionExtent $violationFilepath $violations[0] 1 'iex' 'Invoke-Expression' + $violations[0].SuggestedCorrections[0].Description | Should Be 'Replace iex with Invoke-Expression' + Test-CorrectionExtent $violationFilepath $violations[1] 1 'cls' 'Clear-Host' + $violations[1].SuggestedCorrections[0].Description | Should Be 'Replace cls with Clear-Host' } } diff --git a/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 b/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 index d946d5f8e..3cee868bc 100644 --- a/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 +++ b/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 @@ -16,6 +16,8 @@ Describe "AvoidUsingPlainTextForPassword" { It "suggests corrections" { Import-Module .\PSScriptAnalyzerTestHelper.psm1 Test-CorrectionExtent $violationFilepath $violations[0] 1 '$passphrases' '[SecureString] $passphrases' + $violations[0].SuggestedCorrections[0].Description | Should Be 'Set $passphrases type to SecureString' + Test-CorrectionExtent $violationFilepath $violations[1] 1 '$passwordparam' '[SecureString] $passwordparam' Test-CorrectionExtent $violationFilepath $violations[2] 1 '$credential' '[SecureString] $credential' Test-CorrectionExtent $violationFilepath $violations[3] 1 '$password' '[SecureString] $password' diff --git a/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 b/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 index 091887db8..a64f0276a 100644 --- a/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 +++ b/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 @@ -44,6 +44,7 @@ Describe "UseManifestExportFields" { $violations = Run-PSScriptAnalyzerRule $testManifestBadFunctionsWildcardPath $violationFilepath = Join-path $testManifestPath $testManifestBadFunctionsWildcardPath Test-CorrectionExtent $violationFilepath $violations[0] 1 "'*'" "@('Get-Foo', 'Get-Bar')" + $violations[0].SuggestedCorrections[0].Description | Should Be "Replace '*' with @('Get-Foo', 'Get-Bar')" } It "detects FunctionsToExport with null" { From 7de186a3d0fcc098a9cc950fb7256068d683c5c9 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 18 Apr 2016 17:54:57 -0700 Subject: [PATCH 16/25] Add violation correction to readme --- README.md | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/README.md b/README.md index b272eeee5..aa0417d0e 100644 --- a/README.md +++ b/README.md @@ -194,6 +194,50 @@ public System.Collections.Generic.IEnumerable GetRule(string[] moduleName string[] ruleNames) ``` +Violation Correction +==================== + +Most violations can be fixed by replacing the violation causing content with the correct alternative. In an attempt to provide the user with the ability to correct the violation we provide a property - `SuggestedCorrections`, in each DiagnosticRecord instance. This property contains the information needed to rectify the violation. For example, consider a script `C:\tmp\test.ps1` with the following content. + +```powershell +PS> Get-Content C:\tmp\test.ps1 +gci C:\ +``` + +Invoking PSScriptAnalyzer on the file gives the following output. + +```powershell +PS>$diagnosticRecord = Invoke-ScriptAnalyzer -Path C:\tmp\test.p1 +PS>$diagnosticRecord | select SuggestedCorrections | Format-Custom + +class DiagnosticRecord +{ + SuggestedCorrections = + [ + class CorrectionExtent + { + EndColumnNumber = 4 + EndLineNumber = 1 + File = C:\Users\kabawany\tmp\test3.ps1 + StartColumnNumber = 1 + StartLineNumber = 1 + Text = Get-ChildItem + Description = Replace gci with Get-ChildItem + } + ] + +} +``` + +The `*LineNumber` and `*ColumnNumber` properties give the region of the script that can be replaced by the contents of `Text` property, i.e., replace gci with Get-ChildItem. + +The main motivation behind having `SuggestedCorrections` is to enable quick-fix like scenarios in editors like VSCode, Sublime, etc. At present, we provide valid `SuggestedCorrection` only for the following rules, while gradually adding this feature to more rules. + + * AvoidAlias.cs + * AvoidUsingPlainTextForPassword.cs + * MisleadingBacktick.cs + * MissingModuleManifestField.cs + * UseToExportFieldsInManifest.cs Building the Code ================= From 804ead7c9bf20633bb77486186262e3e8e89b128 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 18 Apr 2016 18:54:48 -0700 Subject: [PATCH 17/25] Fix failing tests Sets the path of the test helper module correctly. --- Tests/Rules/AvoidUsingAlias.tests.ps1 | 2 +- Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 | 3 +-- Tests/Rules/MisleadingBacktick.tests.ps1 | 2 +- Tests/Rules/UseToExportFieldsInManifest.tests.ps1 | 4 +--- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/Tests/Rules/AvoidUsingAlias.tests.ps1 b/Tests/Rules/AvoidUsingAlias.tests.ps1 index 3ccc91bcf..f304b0318 100644 --- a/Tests/Rules/AvoidUsingAlias.tests.ps1 +++ b/Tests/Rules/AvoidUsingAlias.tests.ps1 @@ -5,6 +5,7 @@ $directory = Split-Path -Parent $MyInvocation.MyCommand.Path $violationFilepath = Join-Path $directory 'AvoidUsingAlias.ps1' $violations = Invoke-ScriptAnalyzer $violationFilepath | Where-Object {$_.RuleName -eq $violationName} $noViolations = Invoke-ScriptAnalyzer $directory\AvoidUsingAliasNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName} +Import-Module (Join-Path $directory "PSScriptAnalyzerTestHelper.psm1") Describe "AvoidUsingAlias" { Context "When there are violations" { @@ -17,7 +18,6 @@ Describe "AvoidUsingAlias" { } It "suggests correction" { - Import-Module .\PSScriptAnalyzerTestHelper.psm1 Test-CorrectionExtent $violationFilepath $violations[0] 1 'iex' 'Invoke-Expression' $violations[0].SuggestedCorrections[0].Description | Should Be 'Replace iex with Invoke-Expression' diff --git a/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 b/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 index 3cee868bc..e18a66877 100644 --- a/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 +++ b/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 @@ -1,11 +1,11 @@ Import-Module PSScriptAnalyzer - $violationMessage = [regex]::Escape("Parameter '`$password' should use SecureString, otherwise this will expose sensitive information. See ConvertTo-SecureString for more information.") $violationName = "PSAvoidUsingPlainTextForPassword" $directory = Split-Path -Parent $MyInvocation.MyCommand.Path $violationFilepath = Join-Path $directory 'AvoidUsingPlainTextForPassword.ps1' $violations = Invoke-ScriptAnalyzer $violationFilepath | Where-Object {$_.RuleName -eq $violationName} $noViolations = Invoke-ScriptAnalyzer $directory\AvoidUsingPlainTextForPasswordNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName} +Import-Module (Join-Path $directory "PSScriptAnalyzerTestHelper.psm1") Describe "AvoidUsingPlainTextForPassword" { Context "When there are violations" { @@ -14,7 +14,6 @@ Describe "AvoidUsingPlainTextForPassword" { } It "suggests corrections" { - Import-Module .\PSScriptAnalyzerTestHelper.psm1 Test-CorrectionExtent $violationFilepath $violations[0] 1 '$passphrases' '[SecureString] $passphrases' $violations[0].SuggestedCorrections[0].Description | Should Be 'Set $passphrases type to SecureString' diff --git a/Tests/Rules/MisleadingBacktick.tests.ps1 b/Tests/Rules/MisleadingBacktick.tests.ps1 index 90daa2007..05f674b39 100644 --- a/Tests/Rules/MisleadingBacktick.tests.ps1 +++ b/Tests/Rules/MisleadingBacktick.tests.ps1 @@ -4,6 +4,7 @@ $directory = Split-Path -Parent $MyInvocation.MyCommand.Path $violationFilepath = Join-Path $directory 'MisleadingBacktick.ps1' $violations = Invoke-ScriptAnalyzer $violationFilepath | Where-Object {$_.RuleName -eq $writeHostName} $noViolations = Invoke-ScriptAnalyzer $directory\NoMisleadingBacktick.ps1 | Where-Object {$_.RuleName -eq $clearHostName} +Import-Module (Join-Path $directory "PSScriptAnalyzerTestHelper.psm1") Describe "Avoid Misleading Backticks" { Context "When there are violations" { @@ -12,7 +13,6 @@ Describe "Avoid Misleading Backticks" { } It "suggests correction" { - Import-Module .\PSScriptAnalyzerTestHelper.psm1 Test-CorrectionExtent $violationFilepath $violations[0] 1 ' ' '' Test-CorrectionExtent $violationFilepath $violations[1] 1 ' ' '' Test-CorrectionExtent $violationFilepath $violations[2] 1 ' ' '' diff --git a/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 b/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 index a64f0276a..e31c9d4ec 100644 --- a/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 +++ b/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 @@ -10,6 +10,7 @@ $testManifestBadVariablesWildcardPath = "ManifestBadVariablesWildcard.psd1" $testManifestBadAllPath = "ManifestBadAll.psd1" $testManifestGoodPath = "ManifestGood.psd1" $testManifestInvalidPath = "ManifestInvalid.psd1" +Import-Module (Join-Path $directory "PSScriptAnalyzerTestHelper.psm1") Function Run-PSScriptAnalyzerRule { @@ -40,7 +41,6 @@ Describe "UseManifestExportFields" { } It "suggests corrections for FunctionsToExport with wildcard" { - Import-Module .\PSScriptAnalyzerTestHelper.psm1 $violations = Run-PSScriptAnalyzerRule $testManifestBadFunctionsWildcardPath $violationFilepath = Join-path $testManifestPath $testManifestBadFunctionsWildcardPath Test-CorrectionExtent $violationFilepath $violations[0] 1 "'*'" "@('Get-Foo', 'Get-Bar')" @@ -54,7 +54,6 @@ Describe "UseManifestExportFields" { } It "suggests corrections for FunctionsToExport with null" { - Import-Module .\PSScriptAnalyzerTestHelper.psm1 $violations = Run-PSScriptAnalyzerRule $testManifestBadFunctionsNullPath $violationFilepath = Join-path $testManifestPath $testManifestBadFunctionsNullPath Test-CorrectionExtent $violationFilepath $violations[0] 1 '$null' "@('Get-Foo', 'Get-Bar')" @@ -84,7 +83,6 @@ Describe "UseManifestExportFields" { } It "suggests corrections for AliasesToExport with wildcard" { - Import-Module .\PSScriptAnalyzerTestHelper.psm1 $violations = Run-PSScriptAnalyzerRule $testManifestBadAliasesWildcardPath $violationFilepath = Join-path $testManifestPath $testManifestBadAliasesWildcardPath Test-CorrectionExtent $violationFilepath $violations[0] 1 "'*'" "@('gfoo', 'gbar')" From ba7463b32e1ebab88a7ea80a16b2e02975c1564e Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 20 Apr 2016 09:52:19 -0700 Subject: [PATCH 18/25] Add violation correction section to about_PSScriptScriptAnalyzer --- Engine/about_PSScriptAnalyzer.help.txt | 41 ++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/Engine/about_PSScriptAnalyzer.help.txt b/Engine/about_PSScriptAnalyzer.help.txt index 43bf6e88c..63a57a1a8 100644 --- a/Engine/about_PSScriptAnalyzer.help.txt +++ b/Engine/about_PSScriptAnalyzer.help.txt @@ -183,6 +183,47 @@ RULE SUPPRESSSION Target="*")] param() } + +VIOLATION CORRECTION + +Most violations can be fixed by replacing the violation causing content with the correct alternative. In an attempt to provide the user with the ability to correct the violation we provide a property - `SuggestedCorrections`, in each DiagnosticRecord instance. This property contains the information needed to rectify the violation. For example, consider a script `C:\tmp\test.ps1` with the following content. + +PS> Get-Content C:\tmp\test.ps1 +gci C:\ + +Invoking PSScriptAnalyzer on the file gives the following output. + +PS>$diagnosticRecord = Invoke-ScriptAnalyzer -Path C:\tmp\test.p1 +PS>$diagnosticRecord | select SuggestedCorrections | Format-Custom + +class DiagnosticRecord +{ + SuggestedCorrections = + [ + class CorrectionExtent + { + EndColumnNumber = 4 + EndLineNumber = 1 + File = C:\Users\kabawany\tmp\test3.ps1 + StartColumnNumber = 1 + StartLineNumber = 1 + Text = Get-ChildItem + Description = Replace gci with Get-ChildItem + } + ] + +} + +The *LineNumber and *ColumnNumber properties give the region of the script that can be replaced by the contents of Text property, i.e., replace gci with Get-ChildItem. + +The main motivation behind having SuggestedCorrections is to enable quick-fix like scenarios in editors like VSCode, Sublime, etc. At present, we provide valid SuggestedCorrection only for the following rules, while gradually adding this feature to more rules. + + * AvoidAlias.cs + * AvoidUsingPlainTextForPassword.cs + * MisleadingBacktick.cs + * MissingModuleManifestField.cs + * UseToExportFieldsInManifest.cs + EXTENSIBILITY From 40d07082d9dade7b7d9d3da9829993c9d4f21d76 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Fri, 22 Apr 2016 21:34:53 -0700 Subject: [PATCH 19/25] Add localized strings for correction description --- Rules/Strings.Designer.cs | 27 +++++++++++++++++++++++++++ Rules/Strings.resx | 9 +++++++++ 2 files changed, 36 insertions(+) diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index c8bc1fe79..ee3aaebe2 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -528,6 +528,15 @@ internal static string AvoidUsingCmdletAliasesCommonName { } } + /// + /// Looks up a localized string similar to Replace {0} with {1}. + /// + internal static string AvoidUsingCmdletAliasesCorrectionDescription { + get { + return ResourceManager.GetString("AvoidUsingCmdletAliasesCorrectionDescription", resourceCulture); + } + } + /// /// Looks up a localized string similar to An alias is an alternate name or nickname for a cmdlet or for a command element, such as a function, script, file, or executable file. But when writing scripts that will potentially need to be maintained over time, either by the original author or another Windows PowerShell scripter, please consider using full cmdlet name instead of alias. Aliases can introduce these problems, readability, understandability and availability.. /// @@ -780,6 +789,15 @@ internal static string AvoidUsingPlainTextForPasswordCommonName { } } + /// + /// Looks up a localized string similar to Set {0} type to SecureString. + /// + internal static string AvoidUsingPlainTextForPasswordCorrectionDescription { + get { + return ResourceManager.GetString("AvoidUsingPlainTextForPasswordCorrectionDescription", resourceCulture); + } + } + /// /// Looks up a localized string similar to Password parameters that take in plaintext will expose passwords and compromise the security of your system.. /// @@ -2004,6 +2022,15 @@ internal static string UseToExportFieldsInManifestCommonName { } } + /// + /// Looks up a localized string similar to Replace {0} with {1}. + /// + internal static string UseToExportFieldsInManifestCorrectionDescription { + get { + return ResourceManager.GetString("UseToExportFieldsInManifestCorrectionDescription", resourceCulture); + } + } + /// /// Looks up a localized string similar to In a module manifest, AliasesToExport, CmdletsToExport, FunctionsToExport and VariablesToExport fields should not use wildcards or $null in their entries. During module auto-discovery, if any of these entries are missing or $null or wildcard, PowerShell does some potentially expensive work to analyze the rest of the module.. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index a7c969482..a07448d27 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -810,4 +810,13 @@ UseToExportFieldsInManifest + + Replace {0} with {1} + + + Set {0} type to SecureString + + + Replace {0} with {1} + \ No newline at end of file From c62cdca3c697a160b605c9fb54f1a997ce85f42a Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Fri, 22 Apr 2016 21:37:24 -0700 Subject: [PATCH 20/25] Change SuggestedCorrections type to IEnumerable --- Engine/Generic/DiagnosticRecord.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Engine/Generic/DiagnosticRecord.cs b/Engine/Generic/DiagnosticRecord.cs index c5d5c0f87..a8487a94a 100644 --- a/Engine/Generic/DiagnosticRecord.cs +++ b/Engine/Generic/DiagnosticRecord.cs @@ -97,7 +97,7 @@ public string RuleSuppressionID /// Returns suggested correction /// return value can be null /// - public List SuggestedCorrections + public IEnumerable SuggestedCorrections { get { return suggestedCorrections; } } From d85e978fdf07c6e4151742f68ab8c82b74635865 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Fri, 22 Apr 2016 21:41:29 -0700 Subject: [PATCH 21/25] Fix AvoidAlias correction extent This implementation correctly handles PowerShell escape sequences that IndexOf cannot. --- Rules/AvoidAlias.cs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/Rules/AvoidAlias.cs b/Rules/AvoidAlias.cs index f949884ef..dee522973 100644 --- a/Rules/AvoidAlias.cs +++ b/Rules/AvoidAlias.cs @@ -48,9 +48,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { continue; } - - string cmdletName = Microsoft.Windows.PowerShell.ScriptAnalyzer.Helper.Instance.GetCmdletNameFromAlias(aliasName); - + string cmdletName = Helper.Instance.GetCmdletNameFromAlias(aliasName); if (!String.IsNullOrEmpty(cmdletName)) { yield return new DiagnosticRecord( @@ -68,24 +66,26 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) /// /// Creates a list containing suggested correction /// - /// - /// - /// Returns a list of suggested corrections + /// Command AST of an alias + /// Full name of the alias + /// Retruns a list of suggested corrections private List GetCorrectionExtent(CommandAst cmdAst, string cmdletName) { var corrections = new List(); var ext = cmdAst.Extent; - var alias = cmdAst.GetCommandName(); - var startColumnNumber = ext.StartColumnNumber + ext.Text.IndexOf(alias); - var endColumnNumber = startColumnNumber + alias.Length; - string description = string.Format("Replace {0} with {1}", alias, cmdletName); + var alias = cmdAst.GetCommandName(); + string description = string.Format( + CultureInfo.CurrentCulture, + Strings.AvoidUsingCmdletAliasesCorrectionDescription, + alias, + cmdletName); corrections.Add(new CorrectionExtent( ext.StartLineNumber, ext.EndLineNumber, - startColumnNumber, - endColumnNumber, + cmdAst.CommandElements[0].Extent.StartColumnNumber, + cmdAst.CommandElements[0].Extent.EndColumnNumber, cmdletName, - ext.File, + ext.File, description)); return corrections; } From 13c5c3fbe95dc76c3f5f04ef66f7b12e0b6d8b23 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Fri, 22 Apr 2016 21:47:42 -0700 Subject: [PATCH 22/25] Fix AvoidUsingPlainTextForPassword correction extent Handles the following cases * [string] [Parameter(ValueFromPipeline)] $Password * [Parameter(ValueFromPipeline)] $Password * [Parameter(ValueFromPipeline)] [string[]] $Password * [string] <#whatever#> $Password --- Rules/AvoidUsingPlainTextForPassword.cs | 53 +++++++++++++++---- .../Rules/AvoidUsingPlainTextForPassword.ps1 | 23 +++++++- .../AvoidUsingPlainTextForPassword.tests.ps1 | 8 ++- 3 files changed, 71 insertions(+), 13 deletions(-) diff --git a/Rules/AvoidUsingPlainTextForPassword.cs b/Rules/AvoidUsingPlainTextForPassword.cs index bab5a4055..0da7230bb 100644 --- a/Rules/AvoidUsingPlainTextForPassword.cs +++ b/Rules/AvoidUsingPlainTextForPassword.cs @@ -17,6 +17,7 @@ using System.ComponentModel.Composition; using System.Globalization; using System.Reflection; +using System.Text; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -71,21 +72,53 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) private List GetCorrectionExtent(ParameterAst paramAst) { - IScriptExtent ext = paramAst.Extent; - var corrections = new List(); - string correctionText = string.Format("[SecureString] {0}", paramAst.Name.Extent.Text); - string description = string.Format("Set {0} type to SecureString", paramAst.Name.Extent.Text); + //Find the parameter type extent and replace that with secure string + IScriptExtent extent; + var typeAttributeAst = GetTypeAttributeAst(paramAst); + var corrections = new List(); + string correctionText; + if (typeAttributeAst == null) + { + // cannot find any type attribute + extent = paramAst.Name.Extent; + correctionText = string.Format("[SecureString] {0}", paramAst.Name.Extent.Text); + } + else + { + // replace only the existing type with [SecureString] + extent = typeAttributeAst.Extent; + correctionText = typeAttributeAst.TypeName.IsArray ? "[SecureString[]]" : "[SecureString]"; + } + string description = string.Format( + CultureInfo.CurrentCulture, + Strings.AvoidUsingPlainTextForPasswordCorrectionDescription, + paramAst.Name.Extent.Text); corrections.Add(new CorrectionExtent( - ext.StartLineNumber, - ext.EndLineNumber, - ext.StartColumnNumber, - ext.EndColumnNumber, - correctionText, - ext.File, + extent.StartLineNumber, + extent.EndLineNumber, + extent.StartColumnNumber, + extent.EndColumnNumber, + correctionText.ToString(), + paramAst.Extent.File, description)); return corrections; } + private TypeConstraintAst GetTypeAttributeAst(ParameterAst paramAst) + { + if (paramAst.Attributes != null) + { + foreach(var attr in paramAst.Attributes) + { + if (attr.GetType() == typeof(TypeConstraintAst)) + { + return attr as TypeConstraintAst; + } + } + } + return null; + } + /// /// GetName: Retrieves the name of this rule. /// diff --git a/Tests/Rules/AvoidUsingPlainTextForPassword.ps1 b/Tests/Rules/AvoidUsingPlainTextForPassword.ps1 index 872324967..147e3b52d 100644 --- a/Tests/Rules/AvoidUsingPlainTextForPassword.ps1 +++ b/Tests/Rules/AvoidUsingPlainTextForPassword.ps1 @@ -38,4 +38,25 @@ } function TestFunction($password, [System.Security.SecureString[]]$passphrases, [string]$passThru){ -} \ No newline at end of file +} + +function TestFunction2 +{ + Param( + [string] <#some dumb comment#> $password + ) +} + +function TestFunction3 +{ + Param( + [string[]] $password + ) +} + +function TestFunction4 +{ + Param( + [string] [Parameter(ValueFromPipeline)] $Password + ) +} diff --git a/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 b/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 index e18a66877..45c90167c 100644 --- a/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 +++ b/Tests/Rules/AvoidUsingPlainTextForPassword.tests.ps1 @@ -9,8 +9,9 @@ Import-Module (Join-Path $directory "PSScriptAnalyzerTestHelper.psm1") Describe "AvoidUsingPlainTextForPassword" { Context "When there are violations" { - It "has 3 avoid using plain text for password violations" { - $violations.Count | Should Be 4 + $expectedNumViolations = 7 + It "has $expectedNumViolations violations" { + $violations.Count | Should Be $expectedNumViolations } It "suggests corrections" { @@ -20,6 +21,9 @@ Describe "AvoidUsingPlainTextForPassword" { Test-CorrectionExtent $violationFilepath $violations[1] 1 '$passwordparam' '[SecureString] $passwordparam' Test-CorrectionExtent $violationFilepath $violations[2] 1 '$credential' '[SecureString] $credential' Test-CorrectionExtent $violationFilepath $violations[3] 1 '$password' '[SecureString] $password' + Test-CorrectionExtent $violationFilepath $violations[4] 1 '[string]' '[SecureString]' + Test-CorrectionExtent $violationFilepath $violations[5] 1 '[string[]]' '[SecureString[]]' + Test-CorrectionExtent $violationFilepath $violations[6] 1 '[string]' '[SecureString]' } It "has the correct violation message" { From c4065e7d3aa877984920291f5b5a2f610d925921 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Fri, 22 Apr 2016 21:52:32 -0700 Subject: [PATCH 23/25] Rename and make static a method in Helper --- Engine/Helper.cs | 2 +- Rules/MissingModuleManifestField.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 731371bb7..f75b015e4 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -274,7 +274,7 @@ public PSModuleInfo GetModuleManifest(string filePath, out IEnumerable /// /// Returns a boolean value indicating the presence of MissingMemberException - public bool IsMissingMemberException(ErrorRecord errorRecord) + public static bool IsMissingManifestMemberException(ErrorRecord errorRecord) { return errorRecord.CategoryInfo != null && errorRecord.CategoryInfo.Category == ErrorCategory.ResourceUnavailable diff --git a/Rules/MissingModuleManifestField.cs b/Rules/MissingModuleManifestField.cs index 4034d4661..0acd85278 100644 --- a/Rules/MissingModuleManifestField.cs +++ b/Rules/MissingModuleManifestField.cs @@ -44,7 +44,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { foreach (var errorRecord in errorRecords) { - if (Helper.Instance.IsMissingMemberException(errorRecord)) + if (Helper.IsMissingManifestMemberException(errorRecord)) { System.Diagnostics.Debug.Assert(errorRecord.Exception != null && !String.IsNullOrWhiteSpace(errorRecord.Exception.Message), Strings.NullErrorMessage); yield return From 6fcf0598444f177a36e6e8652c53dc20f23dc8ec Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Fri, 22 Apr 2016 21:57:00 -0700 Subject: [PATCH 24/25] Modify UseToExportFieldInManifest rule * Doesn't check for VariablesToExport * Adds ability to wrap around correction string --- Rules/UseToExportFieldsInManifest.cs | 40 ++++++++++++---- .../ManifestBadFunctionsNull.psm1 | 46 +++++++++++++++++-- .../UseToExportFieldsInManifest.tests.ps1 | 18 ++------ 3 files changed, 79 insertions(+), 25 deletions(-) diff --git a/Rules/UseToExportFieldsInManifest.cs b/Rules/UseToExportFieldsInManifest.cs index b550fae8a..5521642bc 100644 --- a/Rules/UseToExportFieldsInManifest.cs +++ b/Rules/UseToExportFieldsInManifest.cs @@ -20,6 +20,7 @@ using System.Globalization; using System.Text.RegularExpressions; using System.Diagnostics; +using System.Text; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -33,7 +34,6 @@ public class UseToExportFieldsInManifest : IScriptRule private const string functionsToExport = "FunctionsToExport"; private const string cmdletsToExport = "CmdletsToExport"; - private const string variablesToExport = "VariablesToExport"; private const string aliasesToExport = "AliasesToExport"; /// @@ -70,7 +70,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) yield break; } - string[] manifestFields = { functionsToExport, cmdletsToExport, variablesToExport, aliasesToExport }; + string[] manifestFields = { functionsToExport, cmdletsToExport, aliasesToExport }; foreach(string field in manifestFields) { @@ -85,19 +85,43 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) fileName, suggestedCorrections: GetCorrectionExtent(field, extent, psModuleInfo)); } + else + { + + } } } private string GetListLiteral(Dictionary exportedItems) { + const int lineWidth = 64; if (exportedItems == null || exportedItems.Keys == null) { return null; } - var sbuilder = new System.Text.StringBuilder(); + var sbuilder = new StringBuilder(); sbuilder.Append("@("); - sbuilder.Append(string.Join(", ", exportedItems.Keys.Select(key => string.Format("'{0}'", key)))); + var sbuilderInner = new StringBuilder(); + int charLadder = lineWidth; + int keyCount = exportedItems.Keys.Count; + foreach (var key in exportedItems.Keys) + { + sbuilderInner.Append("'"); + sbuilderInner.Append(key); + sbuilderInner.Append("'"); + if (--keyCount > 0) + { + sbuilderInner.Append(", "); + if (sbuilderInner.Length > charLadder) + { + charLadder += lineWidth; + sbuilderInner.AppendLine(); + sbuilderInner.Append('\t', 2); + } + } + } + sbuilder.Append(sbuilderInner); sbuilder.Append(")"); return sbuilder.ToString(); } @@ -118,16 +142,16 @@ private List GetCorrectionExtent(string field, IScriptExtent e case cmdletsToExport: correctionText = GetListLiteral(psModuleInfo.ExportedCmdlets); break; - case variablesToExport: - correctionText = GetListLiteral(psModuleInfo.ExportedVariables); - break; case aliasesToExport: correctionText = GetListLiteral(psModuleInfo.ExportedAliases); break; default: throw new NotImplementedException(string.Format("{0} not implemented", field)); } - string description = string.Format("Replace {0} with {1}", extent.Text, correctionText); + string description = string.Format( + Strings.UseToExportFieldsInManifestCorrectionDescription, + extent.Text, + correctionText); corrections.Add(new CorrectionExtent( extent.StartLineNumber, extent.EndLineNumber, diff --git a/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psm1 b/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psm1 index 34618fd93..3908d6537 100644 --- a/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psm1 +++ b/Tests/Rules/TestManifest/ManifestBadFunctionsNull.psm1 @@ -1,12 +1,50 @@ -Function Get-Foo +Function Get-Foo1 { } -Function Get-Bar +Function Get-Foo2 +{ + +} +Function Get-Foo3 +{ + +} +Function Get-Foo4 +{ + +} +Function Get-Foo5 +{ + +} +Function Get-Foo6 +{ + +} +Function Get-Foo7 +{ + +} +Function Get-Foo8 +{ + +} +Function Get-Foo9 +{ + +} +Function Get-Foo10 +{ + +} +Function Get-Foo11 +{ + +} +Function Get-Foo12 { } -Export-ModuleMember -Function Get-Foo -Export-ModuleMember -Function Get-Bar \ No newline at end of file diff --git a/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 b/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 index e31c9d4ec..2829de4bc 100644 --- a/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 +++ b/Tests/Rules/UseToExportFieldsInManifest.tests.ps1 @@ -53,20 +53,18 @@ Describe "UseManifestExportFields" { $results[0].Extent.Text | Should be '$null' } - It "suggests corrections for FunctionsToExport with null" { + It "suggests corrections for FunctionsToExport with null and line wrapping" { $violations = Run-PSScriptAnalyzerRule $testManifestBadFunctionsNullPath $violationFilepath = Join-path $testManifestPath $testManifestBadFunctionsNullPath - Test-CorrectionExtent $violationFilepath $violations[0] 1 '$null' "@('Get-Foo', 'Get-Bar')" + Test-CorrectionExtent $violationFilepath $violations[0] 1 '$null' "@('Get-Foo1', 'Get-Foo2', 'Get-Foo3', 'Get-Foo4', 'Get-Foo5', 'Get-Foo6', `r`n`t`t'Get-Foo7', 'Get-Foo8', 'Get-Foo9', 'Get-Foo10', 'Get-Foo11', `r`n`t`t'Get-Foo12')" } It "detects array element containing wildcard" { + # if more than two elements contain wildcard we can show only the first one as of now. $results = Run-PSScriptAnalyzerRule $testManifestBadFunctionsWildcardInArrayPath - $results.Count | Should be 3 + $results.Count | Should be 2 $results.Where({$_.Message -match "FunctionsToExport"}).Extent.Text | Should be "'Get-*'" $results.Where({$_.Message -match "CmdletsToExport"}).Extent.Text | Should be "'Update-*'" - - # if more than two elements contain wildcard we can show only the first one as of now. - $results.Where({$_.Message -match "VariablesToExport"}).Extent.Text | Should be "'foo*'" } @@ -88,15 +86,9 @@ Describe "UseManifestExportFields" { Test-CorrectionExtent $violationFilepath $violations[0] 1 "'*'" "@('gfoo', 'gbar')" } - It "detects VariablesToExport with wildcard" { - $results = Run-PSScriptAnalyzerRule $testManifestBadVariablesWildcardPath - $results.Count | Should be 1 - $results[0].Extent.Text | Should be "'*'" - } - It "detects all the *ToExport violations" { $results = Run-PSScriptAnalyzerRule $testManifestBadAllPath - $results.Count | Should be 4 + $results.Count | Should be 3 } } From 5e29444f8f3aabd27695426069f568421a141ec5 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Fri, 22 Apr 2016 22:04:36 -0700 Subject: [PATCH 25/25] Modify a helper method Adds exception handling for a particular case in GetModuleManifest method --- Engine/Helper.cs | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index f75b015e4..3837ef477 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -19,6 +19,7 @@ using System.Management.Automation.Language; using System.Globalization; using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; +using System.Management.Automation.Runspaces; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer { @@ -103,7 +104,6 @@ internal set private string[] functionScopes = new string[] { "global:", "local:", "script:", "private:" }; private string[] variableScopes = new string[] { "global:", "local:", "script:", "private:", "variable:", ":" }; - #endregion /// @@ -111,6 +111,7 @@ internal set /// private Helper() { + } /// @@ -236,25 +237,28 @@ public bool IsDscResourceModule(string filePath) /// /// Returns a object of type PSModuleInfo public PSModuleInfo GetModuleManifest(string filePath, out IEnumerable errorRecord) - { + { errorRecord = null; PSModuleInfo psModuleInfo = null; Collection psObj = null; var ps = System.Management.Automation.PowerShell.Create(); - if (ps == null) - { - return null; - } try - { + { ps.AddCommand("Test-ModuleManifest"); ps.AddParameter("Path", filePath); - - // Suppress warnings emitted during the execution of Test-ModuleManifest ps.AddParameter("WarningAction", ActionPreference.SilentlyContinue); psObj = ps.Invoke(); } - catch { } + catch (CmdletInvocationException e) + { + // Invoking Test-ModuleManifest on a module manifest that doesn't have all the valid keys + // throws a NullReferenceException. This is probably a bug in Test-ModuleManifest and hence + // we consume it to allow execution of the of this method. + if (e.InnerException == null || e.InnerException.GetType() != typeof(System.NullReferenceException)) + { + throw; + } + } if (ps.HadErrors && ps.Streams != null && ps.Streams.Error != null) { var errorRecordArr = new ErrorRecord[ps.Streams.Error.Count]; @@ -265,7 +269,7 @@ public PSModuleInfo GetModuleManifest(string filePath, out IEnumerable