Merge pull request #817 from bergmeister/AutoFixWarnings

Add Fix switch parameter for 'File' parameter set to auto-correct warnings (#802)

Author: Kapil Borle

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;
 
+        /// <summary>
+        /// Resolves rule violations automatically where possible.
+        /// </summary>
+        [Parameter(Mandatory = false, ParameterSetName = "File")]
+        public SwitchParameter Fix
+        {
+            get { return fix; }
+            set { fix = value; }
+        }
+        private bool fix;
+
         /// <summary>
         /// Returns path to the file that contains user profile or hash table for ScriptAnalyzer
         /// </summary>
@@ -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);
                 }
             }

Engine/Formatter.cs

@@ -53,7 +53,8 @@ public static string Format<TCmdlet>(
                 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;
             }
 

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<string, List<string>> CheckRuleExtension(string[] path, PathIn
             return results;
         }
 
-#endregion
+        #endregion
 
 
         /// <summary>
         /// Analyzes a script file or a directory containing script files.
         /// </summary>
         /// <param name="path">The path of the file or directory to analyze.</param>
+        /// <param name="shouldProcess">Whether the action should be executed.</param>
         /// <param name="searchRecursively">
         /// If true, recursively searches the given file path and analyzes any
         /// script files that are found.
         /// </param>
         /// <returns>An enumeration of DiagnosticRecords that were found by rules.</returns>
-        public IEnumerable<DiagnosticRecord> AnalyzePath(string path, bool searchRecursively = false)
+        public IEnumerable<DiagnosticRecord> AnalyzePath(string path, Func<string, string, bool> shouldProcess, bool searchRecursively = false)
         {
-            List<string> scriptFilePaths = new List<string>();
+            List<string> 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;
+                    }
+                }
             }
+        }
+
+        /// <summary>
+        /// Analyzes a script file or a directory containing script files and fixes warning where possible.
+        /// </summary>
+        /// <param name="path">The path of the file or directory to analyze.</param>
+        /// <param name="shouldProcess">Whether the action should be executed.</param>
+        /// <param name="searchRecursively">
+        /// If true, recursively searches the given file path and analyzes any
+        /// script files that are found.
+        /// </param>
+        /// <returns>An enumeration of DiagnosticRecords that were found by rules and could not be fixed automatically.</returns>
+        public IEnumerable<DiagnosticRecord> AnalyzeAndFixPath(string path, Func<string, string, bool> shouldProcess, bool searchRecursively = false)
+        {
+            List<string> 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,16 +1556,17 @@ public IEnumerable<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefini
         /// Fix the violations in the given script text.
         /// </summary>
         /// <param name="scriptDefinition">The script text to be fixed.</param>
+        /// <param name="updatedRange">Whether any warnings were fixed.</param>
         /// <returns>The fixed script text.</returns>
-        public string Fix(string scriptDefinition)
+        public string Fix(string scriptDefinition, out bool fixesWereApplied)
         {
             if (scriptDefinition == null)
             {
                 throw new ArgumentNullException(nameof(scriptDefinition));
             }
 
             Range updatedRange;
-            return Fix(new EditableText(scriptDefinition), null, out updatedRange).ToString();
+            return Fix(new EditableText(scriptDefinition), null, out updatedRange, out fixesWereApplied).ToString();
         }
 
         /// <summary>
@@ -1549,8 +1575,9 @@ public string Fix(string scriptDefinition)
         /// <param name="text">An object of type `EditableText` that encapsulates the script text to be fixed.</param>
         /// <param name="range">The range in which the fixes are allowed.</param>
         /// <param name="updatedRange">The updated range after the fixes have been applied.</param>
+        /// <param name="updatedRange">Whether any warnings were fixed.</param>
         /// <returns>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.</returns>
-        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<string> ScriptPathList(string path, bool searchRecursively)
+        {
+            List<string> scriptFilePaths = new List<string>();
+
+            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,

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
+    }
+}

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);

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) {
 			}
 		}
 	}
-}
+}

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){}

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){}

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] <String> [-CustomRulePath <String>] [-RecurseCustomRulePath]
- [-ExcludeRule <String[]>] [-IncludeRule <String[]>] [-Severity <String[]>] [-Recurse] [-SuppressedOnly]
+ [-ExcludeRule <String[]>] [-IncludeRule <String[]>] [-Severity <String[]>] [-Recurse] [-SuppressedOnly] [-Fix]
  [-Settings <String>]
 ```
 
@@ -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