Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 32 commits into from
Nov 30, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
77ff614
Add AutoFix switch parameter for 'File' parameter set, which uses the…
bergmeister Aug 30, 2017
ef75719
PR 817: Name switch 'Fix' and preserver Encoding of the file. However…
bergmeister Sep 12, 2017
24732ee
PR 817 Minor syntax fix for markdown file
bergmeister Sep 12, 2017
4dc4aeb
Improve encoding handling by not using the detectEncodingFromByteOrde…
bergmeister Sep 30, 2017
792fe30
improve wording of help as suggested in PR
bergmeister Nov 6, 2017
5a2f47b
Add simple test for -Fix switch
bergmeister Nov 6, 2017
023f9e3
improve test by restoring original file and also asserting against th…
bergmeister Nov 6, 2017
5d0b52d
Merge branch 'master' of https://github.com/PowerShell/PSScriptAnalyz…
bergmeister Nov 6, 2017
78d9663
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
bergmeister Nov 6, 2017
6edeef4
Clean up test
bergmeister Nov 6, 2017
f129846
Refactor 'fix' switch out of AnalyzePath method as requested in PR
bergmeister Nov 8, 2017
bfa1c54
Implement SupportShouldProcess for InvokeScriptAnalyzerCommand down t…
bergmeister Nov 8, 2017
50af5d2
Adapt the full .net wrapper in LibraryUsage.tests.ps1 with the -fix s…
bergmeister Nov 8, 2017
771ffb6
Fix Func wrapper in LibraryUsage.tests.ps1
bergmeister Nov 8, 2017
701c264
Another fix to LibraryUsage.tests.ps1 to accomodate -Fix switch and a…
bergmeister Nov 8, 2017
a16c232
Always return true for shouldprocess
bergmeister Nov 9, 2017
e64335f
Add supportshouldprocess to full .net wrapper library
bergmeister Nov 9, 2017
4297ff4
Add 'WhatIf' to common words for help tests
bergmeister Nov 9, 2017
9785993
Add 'Confirm' to common words for help tests
bergmeister Nov 9, 2017
14197ce
Add debug line to debug failing test
bergmeister Nov 9, 2017
fffcd76
Add more debug info for test failure
bergmeister Nov 9, 2017
c707a1e
Add comment about deliberate whitespace warning
bergmeister Nov 9, 2017
9d4e8ef
Number of expected warnings now 5 in -fix test due to PSAvoidTrailin…
bergmeister Nov 9, 2017
c0f881d
Update expected test file
bergmeister Nov 9, 2017
c59b6b0
Remove trailing whitespace in expected file since this gets fixed now
bergmeister Nov 9, 2017
6f6896a
Remove debugging line since test passes now
bergmeister Nov 9, 2017
f5ac9eb
Set-Content -NoNewline was only introduced in PS v5 and would therefo…
bergmeister Nov 9, 2017
8e46750
Syntax fix of last commit
bergmeister Nov 9, 2017
e184e3b
Another small syntax fix (sorry, I am commiting from my mobile)
bergmeister Nov 9, 2017
0df445b
Return $pscmdlet.shouldprocess in .net test wrapper
bergmeister Nov 9, 2017
8525e03
Optimize -Fix switch to only write the fixed file to disk if any fixe…
bergmeister Nov 13, 2017
e12c4a6
Merge branch 'AutoFixWarnings' of https://github.com/bergmeister/PSSc…
bergmeister Nov 13, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion Engine/Commands/InvokeScriptAnalyzerCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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);
}
}
Expand Down
3 changes: 2 additions & 1 deletion Engine/Formatter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
107 changes: 85 additions & 22 deletions Engine/ScriptAnalyzer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
using System.Collections.ObjectModel;
using System.Collections;
using System.Diagnostics;
using System.Text;

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer
{
Expand Down Expand Up @@ -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;
}
}
}
}
Expand Down Expand Up @@ -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>
Expand All @@ -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)
{
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down
32 changes: 32 additions & 0 deletions Tests/Engine/InvokeScriptAnalyzer.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
14 changes: 11 additions & 3 deletions Tests/Engine/LibraryUsage.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down Expand Up @@ -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)
Expand All @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions Tests/Engine/ModuleHelp.Tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -315,4 +315,4 @@ foreach ($command in $commands) {
}
}
}
}
}
5 changes: 5 additions & 0 deletions Tests/Engine/TestScriptWithFixableWarnings.ps1
Original file line number Diff line number Diff line change
@@ -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){}
5 changes: 5 additions & 0 deletions Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1
Original file line number Diff line number Diff line change
@@ -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){}
19 changes: 18 additions & 1 deletion docs/markdown/Invoke-ScriptAnalyzer.md
Original file line number Diff line number Diff line change
Expand Up @@ -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>]
```

Expand Down Expand Up @@ -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

Expand Down