Skip to content
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.

Sign up for GitHub

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

Implement -IncludeSuppressions parameter #1701

Merged
merged 20 commits into from
Aug 4, 2021
Merged

Implement -IncludeSuppressions parameter #1701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
93a0b02
Implement IncludeSuppressed switch to emit suppressed diagnostics as …
rjmholt Jul 24, 2021
e5c48e5
Add tests
rjmholt Jul 24, 2021
a2c64a3
Fix default parameter set
rjmholt Jul 24, 2021
c99cf42
Use mandatory to tie break parameter binding
rjmholt Jul 24, 2021
8fbbaf7
Fix parameter set processing
rjmholt Jul 24, 2021
a450a42
Fix parameter set names
rjmholt Jul 24, 2021
273d0d6
Fix parameter set tests
rjmholt Jul 24, 2021
bcf66f9
Update help for new parameter set
rjmholt Jul 24, 2021
643ff4c
Reinstate bad parameter set for -Recurse
rjmholt Jul 24, 2021
c50eed5
Fix help tests to include cmdlet parameters
rjmholt Jul 24, 2021
9548bcd
Fix PSSA build asset path
rjmholt Jul 24, 2021
4ebe0bb
Wrestle with modules
rjmholt Jul 24, 2021
d91a6f3
Get module info
rjmholt Jul 24, 2021
5465831
Fix
rjmholt Jul 24, 2021
ea28d2b
Show command
rjmholt Jul 24, 2021
fdaca95
Update internal Invoke-ScriptAnalyzer redefinition to conform with cm…
rjmholt Jul 24, 2021
8d09ac0
Fix LibraryUsage tests
rjmholt Jul 25, 2021
50bdfa9
Add IsSuppressed property to diagnostics
rjmholt Jul 25, 2021
6e9e30c
Clean up processing logic in Invoke-ScriptAnalyzer
rjmholt Jul 25, 2021
02ccd84
Update README with new parameter syntax
rjmholt Jul 26, 2021
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
109 changes: 67 additions & 42 deletions Engine/Commands/InvokeScriptAnalyzerCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,17 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands
/// </summary>
[Cmdlet(VerbsLifecycle.Invoke,
"ScriptAnalyzer",
DefaultParameterSetName = "File",
DefaultParameterSetName = ParameterSet_Path_SuppressedOnly,
SupportsShouldProcess = true,
HelpUri = "https://go.microsoft.com/fwlink/?LinkId=525914")]
[OutputType(typeof(DiagnosticRecord))]
[OutputType(typeof(SuppressedRecord))]
[OutputType(typeof(DiagnosticRecord), typeof(SuppressedRecord))]
public class InvokeScriptAnalyzerCommand : PSCmdlet, IOutputWriter
{
private const string ParameterSet_Path_SuppressedOnly = nameof(Path) + "_" + nameof(SuppressedOnly);
private const string ParameterSet_Path_IncludeSuppressed = nameof(Path) + "_" + nameof(IncludeSuppressed);
private const string ParameterSet_ScriptDefinition_SuppressedOnly = nameof(ScriptDefinition) + "_" + nameof(SuppressedOnly);
private const string ParameterSet_ScriptDefinition_IncludeSuppressed = nameof(ScriptDefinition) + "_" + nameof(IncludeSuppressed);

#region Private variables
List<string> processedPaths;
#endregion // Private variables
Expand All @@ -37,7 +41,12 @@ public class InvokeScriptAnalyzerCommand : PSCmdlet, IOutputWriter
/// Path: The path to the file or folder to invoke PSScriptAnalyzer on.
/// </summary>
[Parameter(Position = 0,
ParameterSetName = "File",
ParameterSetName = ParameterSet_Path_IncludeSuppressed,
Mandatory = true,
ValueFromPipeline = true,
ValueFromPipelineByPropertyName = true)]
[Parameter(Position = 0,
ParameterSetName = ParameterSet_Path_SuppressedOnly,
Mandatory = true,
ValueFromPipeline = true,
ValueFromPipelineByPropertyName = true)]
Expand All @@ -54,7 +63,12 @@ public string Path
/// ScriptDefinition: a script definition in the form of a string to run rules on.
/// </summary>
[Parameter(Position = 0,
ParameterSetName = "ScriptDefinition",
ParameterSetName = ParameterSet_ScriptDefinition_IncludeSuppressed,
Mandatory = true,
ValueFromPipeline = true,
ValueFromPipelineByPropertyName = true)]
[Parameter(Position = 0,
ParameterSetName = ParameterSet_ScriptDefinition_SuppressedOnly,
Mandatory = true,
ValueFromPipeline = true,
ValueFromPipelineByPropertyName = true)]
Expand Down Expand Up @@ -84,7 +98,6 @@ public string[] CustomRulePath
/// RecurseCustomRulePath: Find rules within subfolders under the path
/// </summary>
[Parameter(Mandatory = false)]
[SuppressMessage("Microsoft.Performance", "CA1819:PropertiesShouldNotReturnArrays")]
public SwitchParameter RecurseCustomRulePath
{
get { return recurseCustomRulePath; }
Expand All @@ -96,7 +109,6 @@ public SwitchParameter RecurseCustomRulePath
/// IncludeDefaultRules: Invoke default rules along with Custom rules
/// </summary>
[Parameter(Mandatory = false)]
[SuppressMessage("Microsoft.Performance", "CA1819:PropertiesShouldNotReturnArrays")]
public SwitchParameter IncludeDefaultRules
{
get { return includeDefaultRules; }
Expand Down Expand Up @@ -143,11 +155,15 @@ public string[] Severity
}
private string[] severity;

// TODO: This should be only in the Path parameter sets, and is ignored otherwise,
// but we already have a test that depends on it being otherwise
//[Parameter(ParameterSetName = ParameterSet_Path_IncludeSuppressed)]
//[Parameter(ParameterSetName = ParameterSet_Path_SuppressedOnly)]
//
/// <summary>
/// Recurse: Apply to all files within subfolders under the path
/// </summary>
[Parameter(Mandatory = false)]
[SuppressMessage("Microsoft.Performance", "CA1819:PropertiesShouldNotReturnArrays")]
[Parameter]
public SwitchParameter Recurse
{
get { return recurse; }
Expand All @@ -158,19 +174,22 @@ public SwitchParameter Recurse
/// <summary>
/// ShowSuppressed: Show the suppressed message
/// </summary>
[Parameter(Mandatory = false)]
[SuppressMessage("Microsoft.Performance", "CA1819:PropertiesShouldNotReturnArrays")]
public SwitchParameter SuppressedOnly
{
get { return suppressedOnly; }
set { suppressedOnly = value; }
}
private bool suppressedOnly;
[Parameter(ParameterSetName = ParameterSet_Path_SuppressedOnly)]
[Parameter(ParameterSetName = ParameterSet_ScriptDefinition_SuppressedOnly)]
public SwitchParameter SuppressedOnly { get; set; }

/// <summary>
/// Include suppressed diagnostics in the output.
/// </summary>
[Parameter(ParameterSetName = ParameterSet_Path_IncludeSuppressed, Mandatory = true)]
[Parameter(ParameterSetName = ParameterSet_ScriptDefinition_IncludeSuppressed, Mandatory = true)]
public SwitchParameter IncludeSuppressed { get; set; }

/// <summary>
/// Resolves rule violations automatically where possible.
/// </summary>
[Parameter(Mandatory = false, ParameterSetName = "File")]
[Parameter(Mandatory = false, ParameterSetName = ParameterSet_Path_IncludeSuppressed)]
[Parameter(Mandatory = false, ParameterSetName = ParameterSet_Path_SuppressedOnly)]
public SwitchParameter Fix
{
get { return fix; }
Expand Down Expand Up @@ -334,14 +353,20 @@ protected override void BeginProcessing()
this.settings));
}

SuppressionPreference suppressionPreference = SuppressedOnly
? SuppressionPreference.SuppressedOnly
: IncludeSuppressed
? SuppressionPreference.Include
: SuppressionPreference.Omit;

ScriptAnalyzer.Instance.Initialize(
this,
combRulePaths,
this.includeRule,
this.excludeRule,
this.severity,
combRulePaths == null || combIncludeDefaultRules,
this.suppressedOnly);
suppressionPreference);
}

/// <summary>
Expand Down Expand Up @@ -402,29 +427,32 @@ protected override void StopProcessing()

private void ProcessInput()
{
IEnumerable<DiagnosticRecord> diagnosticsList = Enumerable.Empty<DiagnosticRecord>();
if (IsFileParameterSet())
WriteToOutput(RunAnalysis());
}

private IEnumerable<DiagnosticRecord> RunAnalysis()
{
if (!IsFileParameterSet())
{
foreach (var p in processedPaths)
{
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);
}
return ScriptAnalyzer.Instance.AnalyzeScriptDefinition(scriptDefinition, out _, out _);
}
else if (String.Equals(this.ParameterSetName, "ScriptDefinition", StringComparison.OrdinalIgnoreCase))

var diagnostics = new List<DiagnosticRecord>();
foreach (string path in this.processedPaths)
{
diagnosticsList = ScriptAnalyzer.Instance.AnalyzeScriptDefinition(scriptDefinition, out _, out _);
WriteToOutput(diagnosticsList);
if (fix)
{
ShouldProcess(path, $"Analyzing and fixing path with Recurse={this.recurse}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a bug with the original code? If I read this correctly we will take the action no matter what, but ShouldProcess is supposed to notify the user and not take the action in the case of -WhatIf. this code will fix the script even with -WhatIf. I get that it's not as big a deal for just the diagnosis part, but the fix part seems wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I saw this and had the same thought. Definitely something we should review. If @bergmeister agrees, I can open an issue to track it so we can fix it beyond this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This goes back to my very first PR in PSSA, where I added the -Fix switch and since this is an action that changes state, I implemented ShouldProcess for it and declared that on the cmdlet. Since the -Fix parameter is only on the -File parameter set, I did not implement it for the -ScriptDefinition parameter set. Original commit is here: bfa1c54
Therefore I still think it's not really important here, very rarely does one run PSSA where the command would take ages and performing a dry-run would save time. Still happy to have a tracking issue

diagnostics.AddRange(ScriptAnalyzer.Instance.AnalyzeAndFixPath(path, this.ShouldProcess, this.recurse));
}
else
{
ShouldProcess(path, $"Analyzing path with Recurse={this.recurse}");
diagnostics.AddRange(ScriptAnalyzer.Instance.AnalyzePath(path, this.ShouldProcess, this.recurse));
}
}

return diagnostics;
}

private void WriteToOutput(IEnumerable<DiagnosticRecord> diagnosticRecords)
Expand Down Expand Up @@ -497,10 +525,7 @@ private void ProcessPath()
}
}

private bool IsFileParameterSet()
{
return String.Equals(this.ParameterSetName, "File", StringComparison.OrdinalIgnoreCase);
}
private bool IsFileParameterSet() => Path is not null;

private bool OverrideSwitchParam(bool paramValue, string paramName)
{
Expand Down
1 change: 1 addition & 0 deletions Engine/Engine.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
<AssemblyVersion>1.20.0</AssemblyVersion>
<PackageId>Engine</PackageId>
<RootNamespace>Microsoft.Windows.PowerShell.ScriptAnalyzer</RootNamespace> <!-- Namespace needs to match Assembly name for ressource binding -->
<LangVersion>9.0</LangVersion>
</PropertyGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'net452' ">
Expand Down
2 changes: 1 addition & 1 deletion Engine/Formatter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public static string Format<TCmdlet>(

var currentSettings = GetCurrentSettings(settings, rule);
ScriptAnalyzer.Instance.UpdateSettings(currentSettings);
ScriptAnalyzer.Instance.Initialize(cmdlet, null, null, null, null, true, false);
ScriptAnalyzer.Instance.Initialize(cmdlet, null, null, null, null, true, SuppressionPreference.Omit);

text = ScriptAnalyzer.Instance.Fix(text, range, skipParsing, out Range updatedRange, out bool fixesWereApplied, ref scriptAst, ref scriptTokens, skipVariableAnalysis: true);
skipParsing = !fixesWereApplied;
Expand Down
13 changes: 10 additions & 3 deletions Engine/Generic/DiagnosticRecord.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,13 @@ public IEnumerable<CorrectionExtent> SuggestedCorrections
set { suggestedCorrections = value; }
}

public bool IsSuppressed { get; protected set; } = false;

/// <summary>
/// DiagnosticRecord: The constructor for DiagnosticRecord class.
/// </summary>
public DiagnosticRecord()
{

}

/// <summary>
Expand All @@ -109,7 +110,14 @@ public DiagnosticRecord()
/// <param name="severity">The severity of this diagnostic</param>
/// <param name="scriptPath">The full path of the script file being analyzed</param>
/// <param name="suggestedCorrections">The correction suggested by the rule to replace the extent text</param>
public DiagnosticRecord(string message, IScriptExtent extent, string ruleName, DiagnosticSeverity severity, string scriptPath, string ruleId = null, IEnumerable<CorrectionExtent> suggestedCorrections = null)
public DiagnosticRecord(
string message,
IScriptExtent extent,
string ruleName,
DiagnosticSeverity severity,
string scriptPath,
string ruleId = null,
IEnumerable<CorrectionExtent> suggestedCorrections = null)
{
Message = message;
RuleName = ruleName;
Expand All @@ -119,7 +127,6 @@ public DiagnosticRecord(string message, IScriptExtent extent, string ruleName, D
RuleSuppressionID = ruleId;
this.suggestedCorrections = suggestedCorrections;
}

}


Expand Down
1 change: 1 addition & 0 deletions Engine/Generic/SuppressedRecord.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public class SuppressedRecord : DiagnosticRecord
public SuppressedRecord(DiagnosticRecord record, IReadOnlyList<RuleSuppression> suppressions)
{
Suppression = new ReadOnlyCollection<RuleSuppression>(new List<RuleSuppression>(suppressions));
IsSuppressed = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this ever be called with a suppression list count of zero? In that case IsSuppressed really true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good question. I think the mere wrapping in the SuppressedRecord object classes the diagnostic as suppressed, but I agree that this opens up the possibility of an invalid object, which I dislike... I'm not sure I see a way around this without changing a few things around though, and I'm not sure that's worth it.

if (record != null)
{
RuleName = record.RuleName;
Expand Down
37 changes: 27 additions & 10 deletions Engine/ScriptAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer
{
internal enum SuppressionPreference
{
Omit = 0,
Include = 1,
SuppressedOnly = 2,
}

public sealed class ScriptAnalyzer
{
#region Private members
Expand All @@ -41,7 +48,7 @@ public sealed class ScriptAnalyzer
string[] severity;
List<Regex> includeRegexList;
List<Regex> excludeRegexList;
bool suppressedOnly;
private SuppressionPreference _suppressionPreference;
#if !PSV3
ModuleDependencyHandler moduleHandler;
#endif
Expand Down Expand Up @@ -118,7 +125,7 @@ internal void Initialize<TCmdlet>(
string[] excludeRuleNames = null,
string[] severity = null,
bool includeDefaultRules = false,
bool suppressedOnly = false)
SuppressionPreference suppressionPreference = SuppressionPreference.Omit)
where TCmdlet : PSCmdlet, IOutputWriter
{
if (cmdlet == null)
Expand All @@ -135,7 +142,7 @@ internal void Initialize<TCmdlet>(
excludeRuleNames,
severity,
includeDefaultRules,
suppressedOnly);
suppressionPreference);
}

/// <summary>
Expand All @@ -150,6 +157,7 @@ public void Initialize(
string[] severity = null,
bool includeDefaultRules = false,
bool suppressedOnly = false,
bool includeSuppression = false,
string profile = null)
{
if (runspace == null)
Expand All @@ -163,6 +171,11 @@ public void Initialize(
outputWriter);
Helper.Instance.Initialize();

SuppressionPreference suppressionPreference = suppressedOnly
? SuppressionPreference.SuppressedOnly
: includeSuppression
? SuppressionPreference.Include
: SuppressionPreference.Omit;

this.Initialize(
outputWriter,
Expand All @@ -173,7 +186,7 @@ public void Initialize(
excludeRuleNames,
severity,
includeDefaultRules,
suppressedOnly,
suppressionPreference,
profile);
}

Expand All @@ -187,7 +200,7 @@ public void CleanUp()
severity = null;
includeRegexList = null;
excludeRegexList = null;
suppressedOnly = false;
_suppressionPreference = SuppressionPreference.Omit;
}

/// <summary>
Expand Down Expand Up @@ -671,7 +684,7 @@ private void Initialize(
string[] excludeRuleNames,
string[] severity,
bool includeDefaultRules = false,
bool suppressedOnly = false,
SuppressionPreference suppressionPreference = SuppressionPreference.Omit,
string profile = null)
{
if (outputWriter == null)
Expand Down Expand Up @@ -730,7 +743,7 @@ private void Initialize(
}
}

this.suppressedOnly = suppressedOnly;
_suppressionPreference = suppressionPreference;
this.includeRegexList = new List<Regex>();
this.excludeRegexList = new List<Regex>();

Expand Down Expand Up @@ -2324,9 +2337,13 @@ public IEnumerable<DiagnosticRecord> AnalyzeSyntaxTree(
// Need to reverse the concurrentbag to ensure that results are sorted in the increasing order of line numbers
IEnumerable<DiagnosticRecord> diagnosticsList = diagnostics.Reverse();

return this.suppressedOnly ?
suppressed.OfType<DiagnosticRecord>() :
diagnosticsList;
return _suppressionPreference switch
{
SuppressionPreference.SuppressedOnly => suppressed.OfType<DiagnosticRecord>(),
SuppressionPreference.Omit => diagnosticsList,
SuppressionPreference.Include => diagnosticsList.Concat(suppressed.OfType<DiagnosticRecord>()),
_ => throw new ArgumentException($"SuppressionPreference has invalid value '{_suppressionPreference}'"),
};
}
}
}
Loading