diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index ad25c5e0e..40b8d5cff 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -38,6 +38,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands [Cmdlet(VerbsLifecycle.Invoke, "ScriptAnalyzer", DefaultParameterSetName = "File", + SupportsShouldProcess = true, HelpUri = "http://go.microsoft.com/fwlink/?LinkId=525914")] public class InvokeScriptAnalyzerCommand : PSCmdlet, IOutputWriter { @@ -180,6 +181,17 @@ public SwitchParameter SuppressedOnly } private bool suppressedOnly; + /// + /// Resolves rule violations automatically where possible. + /// + [Parameter(Mandatory = false, ParameterSetName = "File")] + public SwitchParameter Fix + { + get { return fix; } + set { fix = value; } + } + private bool fix; + /// /// Returns path to the file that contains user profile or hash table for ScriptAnalyzer /// @@ -377,7 +389,16 @@ private void ProcessInput() { foreach (var p in processedPaths) { - diagnosticsList = ScriptAnalyzer.Instance.AnalyzePath(p, this.recurse); + if (fix) + { + ShouldProcess(p, $"Analyzing and fixing path with Recurse={this.recurse}"); + diagnosticsList = ScriptAnalyzer.Instance.AnalyzeAndFixPath(p, this.ShouldProcess, this.recurse); + } + else + { + ShouldProcess(p, $"Analyzing path with Recurse={this.recurse}"); + diagnosticsList = ScriptAnalyzer.Instance.AnalyzePath(p, this.ShouldProcess, this.recurse); + } WriteToOutput(diagnosticsList); } } diff --git a/Engine/Formatter.cs b/Engine/Formatter.cs index 098e772a9..bcdbc32b8 100644 --- a/Engine/Formatter.cs +++ b/Engine/Formatter.cs @@ -53,7 +53,8 @@ public static string Format( ScriptAnalyzer.Instance.Initialize(cmdlet, null, null, null, null, true, false); Range updatedRange; - text = ScriptAnalyzer.Instance.Fix(text, range, out updatedRange); + bool fixesWereApplied; + text = ScriptAnalyzer.Instance.Fix(text, range, out updatedRange, out fixesWereApplied); range = updatedRange; } diff --git a/Engine/ScriptAnalyzer.cs b/Engine/ScriptAnalyzer.cs index 5c3a6ec03..cdc3884f5 100644 --- a/Engine/ScriptAnalyzer.cs +++ b/Engine/ScriptAnalyzer.cs @@ -31,6 +31,7 @@ using System.Collections.ObjectModel; using System.Collections; using System.Diagnostics; +using System.Text; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer { @@ -1443,43 +1444,67 @@ public Dictionary> CheckRuleExtension(string[] path, PathIn return results; } -#endregion + #endregion /// /// Analyzes a script file or a directory containing script files. /// /// The path of the file or directory to analyze. + /// Whether the action should be executed. /// /// If true, recursively searches the given file path and analyzes any /// script files that are found. /// /// An enumeration of DiagnosticRecords that were found by rules. - public IEnumerable AnalyzePath(string path, bool searchRecursively = false) + public IEnumerable AnalyzePath(string path, Func shouldProcess, bool searchRecursively = false) { - List scriptFilePaths = new List(); + List scriptFilePaths = ScriptPathList(path, searchRecursively); - if (path == null) + foreach (string scriptFilePath in scriptFilePaths) { - this.outputWriter.ThrowTerminatingError( - new ErrorRecord( - new FileNotFoundException(), - string.Format(CultureInfo.CurrentCulture, Strings.FileNotFound, path), - ErrorCategory.InvalidArgument, - this)); + if (shouldProcess(scriptFilePath, $"Analyzing file {scriptFilePath}")) + { + // Yield each record in the result so that the caller can pull them one at a time + foreach (var diagnosticRecord in this.AnalyzeFile(scriptFilePath)) + { + yield return diagnosticRecord; + } + } } + } + + /// + /// Analyzes a script file or a directory containing script files and fixes warning where possible. + /// + /// The path of the file or directory to analyze. + /// Whether the action should be executed. + /// + /// If true, recursively searches the given file path and analyzes any + /// script files that are found. + /// + /// An enumeration of DiagnosticRecords that were found by rules and could not be fixed automatically. + public IEnumerable AnalyzeAndFixPath(string path, Func shouldProcess, bool searchRecursively = false) + { + List scriptFilePaths = ScriptPathList(path, searchRecursively); - // Create in advance the list of script file paths to analyze. This - // is an optimization over doing the whole operation at once - // and calling .Concat on IEnumerables to join results. - this.BuildScriptPathList(path, searchRecursively, scriptFilePaths); foreach (string scriptFilePath in scriptFilePaths) { - // Yield each record in the result so that the - // caller can pull them one at a time - foreach (var diagnosticRecord in this.AnalyzeFile(scriptFilePath)) + if (shouldProcess(scriptFilePath, $"Analyzing and fixing file {scriptFilePath}")) { - yield return diagnosticRecord; + var fileEncoding = GetFileEncoding(scriptFilePath); + bool fixesWereApplied; + var scriptFileContentWithFixes = Fix(File.ReadAllText(scriptFilePath, fileEncoding), out fixesWereApplied); + if (fixesWereApplied) + { + File.WriteAllText(scriptFilePath, scriptFileContentWithFixes, fileEncoding); + } + + // Yield each record in the result so that the caller can pull them one at a time + foreach (var diagnosticRecord in this.AnalyzeFile(scriptFilePath)) + { + yield return diagnosticRecord; + } } } } @@ -1531,8 +1556,9 @@ public IEnumerable AnalyzeScriptDefinition(string scriptDefini /// Fix the violations in the given script text. /// /// The script text to be fixed. + /// Whether any warnings were fixed. /// The fixed script text. - public string Fix(string scriptDefinition) + public string Fix(string scriptDefinition, out bool fixesWereApplied) { if (scriptDefinition == null) { @@ -1540,7 +1566,7 @@ public string Fix(string scriptDefinition) } Range updatedRange; - return Fix(new EditableText(scriptDefinition), null, out updatedRange).ToString(); + return Fix(new EditableText(scriptDefinition), null, out updatedRange, out fixesWereApplied).ToString(); } /// @@ -1549,8 +1575,9 @@ public string Fix(string scriptDefinition) /// An object of type `EditableText` that encapsulates the script text to be fixed. /// The range in which the fixes are allowed. /// The updated range after the fixes have been applied. + /// Whether any warnings were fixed. /// The same instance of `EditableText` that was passed to the method, but the instance encapsulates the fixed script text. This helps in chaining the Fix method. - public EditableText Fix(EditableText text, Range range, out Range updatedRange) + public EditableText Fix(EditableText text, Range range, out Range updatedRange, out bool fixesWereApplied) { if (text == null) { @@ -1583,7 +1610,9 @@ public EditableText Fix(EditableText text, Range range, out Range updatedRange) this.outputWriter.WriteVerbose($"Found {corrections.Count} violations."); int unusedCorrections; Fix(text, corrections, out unusedCorrections); - this.outputWriter.WriteVerbose($"Fixed {corrections.Count - unusedCorrections} violations."); + var numberOfFixedViolatons = corrections.Count - unusedCorrections; + fixesWereApplied = numberOfFixedViolatons > 0; + this.outputWriter.WriteVerbose($"Fixed {numberOfFixedViolatons} violations."); // This is an indication of an infinite loop. There is a small chance of this. // It is better to abort the fixing operation at this point. @@ -1613,6 +1642,18 @@ public EditableText Fix(EditableText text, Range range, out Range updatedRange) return text; } + private static Encoding GetFileEncoding(string path) + { + using (var stream = new FileStream(path, FileMode.Open)) + { + using (var reader = new StreamReader(stream)) + { + reader.Peek(); // needed in order to populate the CurrentEncoding property + return reader.CurrentEncoding; + } + } + } + private static Range SnapToEdges(EditableText text, Range range) { // todo add TextLines.Validate(range) and TextLines.Validate(position) @@ -1655,6 +1696,28 @@ private static EditableText Fix( return text; } + private List ScriptPathList(string path, bool searchRecursively) + { + List scriptFilePaths = new List(); + + if (path == null) + { + this.outputWriter.ThrowTerminatingError( + new ErrorRecord( + new FileNotFoundException(), + string.Format(CultureInfo.CurrentCulture, Strings.FileNotFound, path), + ErrorCategory.InvalidArgument, + this)); + } + + // Create in advance the list of script file paths to analyze. This + // is an optimization over doing the whole operation at once + // and calling .Concat on IEnumerables to join results. + this.BuildScriptPathList(path, searchRecursively, scriptFilePaths); + + return scriptFilePaths; + } + private void BuildScriptPathList( string path, bool searchRecursively, diff --git a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 index f1989fae4..a915bed85 100644 --- a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 +++ b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 @@ -465,3 +465,35 @@ Describe "Test CustomizedRulePath" { } } } + +Describe "Test -Fix Switch" { + + BeforeEach { + $initialTestScript = Get-Content $directory\TestScriptWithFixableWarnings.ps1 -Raw + } + + AfterEach { + if ($null -ne $initialTestScript) + { + [System.IO.File]::WriteAllText("$($directory)\TestScriptWithFixableWarnings.ps1", $initialTestScript) # Set-Content -NoNewline was only introduced in PS v5 and would therefore not work in CI + } + } + + It "Fixes warnings" { + # we expect the script to contain warnings + $warningsBeforeFix = Invoke-ScriptAnalyzer $directory\TestScriptWithFixableWarnings.ps1 + $warningsBeforeFix.Count | Should Be 5 + + # fix the warnings and expect that it should not return the fixed warnings + $warningsWithFixSwitch = Invoke-ScriptAnalyzer $directory\TestScriptWithFixableWarnings.ps1 -Fix + $warningsWithFixSwitch.Count | Should Be 0 + + # double check that the warnings are really fixed + $warningsAfterFix = Invoke-ScriptAnalyzer $directory\TestScriptWithFixableWarnings.ps1 + $warningsAfterFix.Count | Should Be 0 + + $expectedScriptContentAfterFix = Get-Content $directory\TestScriptWithFixableWarnings_AfterFix.ps1 -Raw + $actualScriptContentAfterFix = Get-Content $directory\TestScriptWithFixableWarnings.ps1 -Raw + $actualScriptContentAfterFix | Should Be $expectedScriptContentAfterFix + } +} diff --git a/Tests/Engine/LibraryUsage.tests.ps1 b/Tests/Engine/LibraryUsage.tests.ps1 index 64570d58f..a945fdd02 100644 --- a/Tests/Engine/LibraryUsage.tests.ps1 +++ b/Tests/Engine/LibraryUsage.tests.ps1 @@ -15,7 +15,7 @@ $directory = Split-Path -Parent $MyInvocation.MyCommand.Path # wraps the usage of ScriptAnalyzer as a .NET library function Invoke-ScriptAnalyzer { param ( - [CmdletBinding(DefaultParameterSetName="File")] + [CmdletBinding(DefaultParameterSetName="File", SupportsShouldProcess = $true)] [parameter(Mandatory = $true, Position = 0, ParameterSetName="File")] [Alias("PSPath")] @@ -48,7 +48,10 @@ function Invoke-ScriptAnalyzer { [switch] $IncludeDefaultRules, [Parameter(Mandatory = $false)] - [switch] $SuppressedOnly + [switch] $SuppressedOnly, + + [Parameter(Mandatory = $false)] + [switch] $Fix ) if ($null -eq $CustomRulePath) @@ -75,7 +78,12 @@ function Invoke-ScriptAnalyzer { ); if ($PSCmdlet.ParameterSetName -eq "File") { - return $scriptAnalyzer.AnalyzePath($Path, $Recurse.IsPresent); + $supportsShouldProcessFunc = [Func[string, string, bool]]{ return $PSCmdlet.Shouldprocess } + if ($Fix.IsPresent) + { + return $scriptAnalyzer.AnalyzeAndFixPath($Path, $supportsShouldProcessFunc, $Recurse.IsPresent); + } + return $scriptAnalyzer.AnalyzePath($Path, $supportsShouldProcessFunc, $Recurse.IsPresent); } else { return $scriptAnalyzer.AnalyzeScriptDefinition($ScriptDefinition); diff --git a/Tests/Engine/ModuleHelp.Tests.ps1 b/Tests/Engine/ModuleHelp.Tests.ps1 index f1b9e5743..b4dd7e94a 100644 --- a/Tests/Engine/ModuleHelp.Tests.ps1 +++ b/Tests/Engine/ModuleHelp.Tests.ps1 @@ -121,7 +121,7 @@ function Get-ParametersDefaultFirst { ) BEGIN { - $Common = 'Debug', 'ErrorAction', 'ErrorVariable', 'InformationAction', 'InformationVariable', 'OutBuffer', 'OutVariable', 'PipelineVariable', 'Verbose', 'WarningAction', 'WarningVariable' + $Common = 'Debug', 'ErrorAction', 'ErrorVariable', 'InformationAction', 'InformationVariable', 'OutBuffer', 'OutVariable', 'PipelineVariable', 'Verbose', 'WarningAction', 'WarningVariable', 'WhatIf', 'Confirm' $parameters = @() } PROCESS { @@ -266,7 +266,7 @@ foreach ($command in $commands) { Context "Test parameter help for $commandName" { $Common = 'Debug', 'ErrorAction', 'ErrorVariable', 'InformationAction', 'InformationVariable', 'OutBuffer', 'OutVariable', - 'PipelineVariable', 'Verbose', 'WarningAction', 'WarningVariable' + 'PipelineVariable', 'Verbose', 'WarningAction', 'WarningVariable', 'WhatIf', 'Confirm' # Get parameters. When >1 parameter with same name, # get parameter from the default parameter set, if any. @@ -315,4 +315,4 @@ foreach ($command in $commands) { } } } -} \ No newline at end of file +} diff --git a/Tests/Engine/TestScriptWithFixableWarnings.ps1 b/Tests/Engine/TestScriptWithFixableWarnings.ps1 new file mode 100644 index 000000000..f191b3c70 --- /dev/null +++ b/Tests/Engine/TestScriptWithFixableWarnings.ps1 @@ -0,0 +1,5 @@ +# Produce PSAvoidUsingCmdletAliases and PSAvoidTrailingWhitespace warnings that should get fixed by replacing it with the actual command +gci . | % { } | ? { } + +# Produces PSAvoidUsingPlainTextForPassword warning that should get fixed by making it a [SecureString] +function Test-bar([string]$PasswordInPlainText){} diff --git a/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 b/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 new file mode 100644 index 000000000..876e8ed8e --- /dev/null +++ b/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 @@ -0,0 +1,5 @@ +# Produce PSAvoidUsingCmdletAliases and PSAvoidTrailingWhitespace warnings that should get fixed by replacing it with the actual command +Get-ChildItem . | ForEach-Object { } | Where-Object { } + +# Produces PSAvoidUsingPlainTextForPassword warning that should get fixed by making it a [SecureString] +function Test-bar([SecureString]$PasswordInPlainText){} diff --git a/docs/markdown/Invoke-ScriptAnalyzer.md b/docs/markdown/Invoke-ScriptAnalyzer.md index 17ce0526c..b421ea076 100644 --- a/docs/markdown/Invoke-ScriptAnalyzer.md +++ b/docs/markdown/Invoke-ScriptAnalyzer.md @@ -12,7 +12,7 @@ Evaluates a script or module based on selected best practice rules ### UNNAMED_PARAMETER_SET_1 ``` Invoke-ScriptAnalyzer [-Path] [-CustomRulePath ] [-RecurseCustomRulePath] - [-ExcludeRule ] [-IncludeRule ] [-Severity ] [-Recurse] [-SuppressedOnly] + [-ExcludeRule ] [-IncludeRule ] [-Severity ] [-Recurse] [-SuppressedOnly] [-Fix] [-Settings ] ``` @@ -399,6 +399,23 @@ Accept pipeline input: False Accept wildcard characters: False ``` +### -Fix +Fixes certain warnings which contain a fix in their DiagnosticRecord. + +When you used Fix, Invoke-ScriptAnalyzer runs as usual but will apply the fixes before running the analysis. Please make sure that you have a backup of your files when using this switch. It tries to preserve the file encoding but there are still some cases where the encoding can change. + +```yaml +Type: SwitchParameter +Parameter Sets: UNNAMED_PARAMETER_SET_1 +Aliases: + +Required: False +Position: Named +Default value: False +Accept pipeline input: False +Accept wildcard characters: False +``` + ### -Settings File path that contains user profile or hash table for ScriptAnalyzer