Skip to content

Add some necessary contents for diagnostic suppressions. #1694

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

Closed
wants to merge 2 commits into from
Closed

Add some necessary contents for diagnostic suppressions. #1694

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3139f05
add-suppressions
t-lipingma Jul 16, 2021
791bd0c
parametersets
t-lipingma Jul 23, 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
15 changes: 14 additions & 1 deletion Engine/Commands/InvokeScriptAnalyzerCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,18 @@ public SwitchParameter SuppressedOnly
}
private bool suppressedOnly;

/// <summary>
/// ShowAll: Show the suppressed and non-suppressed message
/// </summary>
[Parameter(Mandatory = false)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't mutually exclusive with SuppressedOnly and probably should be. What should a user expect when both are provided?

Given that -SuppressedOnly already exists and we don't want to remove or otherwise break it, we probably can't move to an enum parameter like -SuppressionPreference <Exclude | Include | SuppressedOnly>.

An alternative might be to put each switch into its own parameter set, but that starts to get complicated. We should discuss this further.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[Parameter(Mandatory = false)]
[Parameter]

Mandatory defaults to false

Copy link
Contributor

Choose a reason for hiding this comment

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

correct, if it's not mandatory, that should just be omitted. if SuppressedOnly should not be used with this, then parameter sets should be used (and it does get complicated). Although it doesn't look to complicated at the moment it would mean an additional 2 parameter sets.

Copy link
Author

Choose a reason for hiding this comment

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

yeah. I totally agree with you guys on that point.
If you decided to change the parameter set, let's discuss more details on the design, I'd like to help.

Copy link
Contributor

Choose a reason for hiding this comment

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

From our offline conversation:

If the parameter set change is too complicated, can we check in the current solution firstly?

Yeah I think it's a good idea for us to separate out the suppression combining parts from the new parameter/feature. We can then discuss offline or over a call what the best way forward is.

Copy link
Author

Choose a reason for hiding this comment

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

To maintain PSSA's existing design, I think we can make '-SuppressedOnly' / '-IncludeSuppressions' exclusive in the short term. What do you think about it? Look forward to your reply.

Copy link
Author

Choose a reason for hiding this comment

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

Btw, ExcludeRule and IncludeRule should also be mutually exclusive.

Copy link
Contributor

Choose a reason for hiding this comment

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

make '-SuppressedOnly' / '-IncludeSuppressions' exclusive

Yeah, they should be in different parameter sets.

Btw, ExcludeRule and IncludeRule should also be mutually exclusive.

No, those can be used together to include a non-default rule while excluding a default one.

Copy link
Author

Choose a reason for hiding this comment

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

To make -SuppressedOnly / -IncludeSuppressions exclusive.
I used the logic below:

 Function TestParamSet {
    [CmdletBinding(DefaultParameterSetName="File1")]
    Param
    (
	[Parameter(Position = 0,ParameterSetName = 'File1',Mandatory = true)]
        [Parameter(Position = 0,ParameterSetName = 'File2',Mandatory = true]
        [switch] $Path,
        
	[Parameter(Position = 0,ParameterSetName = 'ScriptDefinition1',Mandatory = true)]
        [Parameter(Position = 0,ParameterSetName = 'ScriptDefinition2',Mandatory = true)]
	[switch] $ScriptDefinition,
        
        
        [Parameter(ParameterSetName = 'File1', Mandatory = false)]
        [Parameter(ParameterSetName = 'ScriptDefinition1', Mandatory = false)]
        [switch] $SuppressedOnly,

        [Parameter(ParameterSetName = 'File2', Mandatory = true)]
        [Parameter(ParameterSetName = 'ScriptDefinition2', Mandatory = true)]
        [switch] $IncludeSuppressions

        [Parameter(Position = 0,ParameterSetName = 'File1',Mandatory = false)]
        [Parameter(Position = 0,ParameterSetName = 'File2',Mandatory = false]
        [switch] $Fix,
    )

    Process {
        #Do Nothing
    }
}

so it's to say:

1、Invoke-ScriptAnalyzer C:\build.ps1 --> ParameterSetName==DefaultParameterSetName.

2、Invoke-ScriptAnalyzer -Path C:\build.ps1 --> ParameterSetName==DefaultParameterSetName.

3、Invoke-ScriptAnalyzer -Path C:\build.ps1 -SuppressedOnly --> ParameterSetName=="File1".

4、Invoke-ScriptAnalyzer -Path C:\build.ps1 -IncludeSuppressions --> ParameterSetName=="File2".

5、Invoke-ScriptAnalyzer -ScriptDefinition $tmp --> ParameterSetName=="ScriptDefinition1".

6、Invoke-ScriptAnalyzer -ScriptDefinition $tmp -SuppressedOnly --> ParameterSetName=="ScriptDefinition1"

7、Invoke-ScriptAnalyzer -ScriptDefinition $tmp -IncludeSuppressions -->ParameterSetName=="ScriptDefinition2"

LMK if you have any suggestions, Thanks!

[SuppressMessage("Microsoft.Performance", "CA1819:PropertiesShouldNotReturnArrays")]
public SwitchParameter IncludeSuppressions
{
get { return includeSuppressions; }
set { includeSuppressions = value; }
}
private bool includeSuppressions;

/// <summary>
/// Resolves rule violations automatically where possible.
/// </summary>
Expand Down Expand Up @@ -341,7 +353,8 @@ protected override void BeginProcessing()
this.excludeRule,
this.severity,
combRulePaths == null || combIncludeDefaultRules,
this.suppressedOnly);
this.suppressedOnly,
this.includeSuppressions);
}

/// <summary>
Expand Down
19 changes: 19 additions & 0 deletions Engine/Generic/RuleSuppression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,15 @@ public string Justification
set;
}

/// <summary>
/// Returns the kind of the suppression
/// </summary>
public SuppressionKind Kind
{
get;
set;
}

private static HashSet<string> scopeSet;

/// <summary>
Expand Down Expand Up @@ -376,5 +385,15 @@ public static List<RuleSuppression> GetSuppressions(IEnumerable<AttributeAst> at

return result;
}

/// <summary>
/// A string that indicates where the suppression is persisted.
/// </summary>
public enum SuppressionKind
{
None,
InSource,
External
}
}
}
21 changes: 16 additions & 5 deletions Engine/Generic/SuppressedRecord.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.Collections.Generic;

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic
{
/// <summary>
Expand All @@ -11,20 +13,29 @@ public class SuppressedRecord : DiagnosticRecord
/// <summary>
/// The rule suppression of this record
/// </summary>
public RuleSuppression Suppression
public IList<RuleSuppression> Suppressions
{
get;
set;
get
{
if (suppressions == null) suppressions = new List<RuleSuppression>();

return suppressions;
}
set
{
suppressions = value;
}
}
private IList<RuleSuppression> suppressions;

/// <summary>
/// Creates a suppressed record based on a diagnostic record and the rule suppression
/// </summary>
/// <param name="record"></param>
/// <param name="Suppression"></param>
public SuppressedRecord(DiagnosticRecord record, RuleSuppression suppression)
public SuppressedRecord(DiagnosticRecord record, IList<RuleSuppression> suppressions)
{
Suppression = suppression;
Suppressions = suppressions;
if (record != null)
{
RuleName = record.RuleName;
Expand Down
151 changes: 67 additions & 84 deletions Engine/Helper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1348,50 +1348,47 @@ public Tuple<List<SuppressedRecord>, List<DiagnosticRecord>> SuppressRule(
}

List<RuleSuppression> ruleSuppressions = ruleSuppressionsDict[ruleName];
var offsetArr = GetOffsetArray(diagnostics);
int recordIndex = 0;
int startRecord = 0;
bool[] suppressed = new bool[diagnostics.Count];
foreach (RuleSuppression ruleSuppression in ruleSuppressions)
{
int suppressionCount = 0;
while (startRecord < diagnostics.Count
// && diagnostics[startRecord].Extent.StartOffset < ruleSuppression.StartOffset)
// && diagnostics[startRecord].Extent.StartLineNumber < ruleSuppression.st)
&& offsetArr[startRecord] != null && offsetArr[startRecord].Item1 < ruleSuppression.StartOffset)
{
startRecord += 1;
}
bool[] applied = new bool[ruleSuppressions.Count];

// at this point, start offset of startRecord is greater or equals to rulesuppression.startoffset
recordIndex = startRecord;

while (recordIndex < diagnostics.Count)
foreach(DiagnosticRecord diagnostic in diagnostics)
{
var curOffset = GetOffsetArray(diagnostic);
List<RuleSuppression> suppressions = new List<RuleSuppression>();
for (int ruleSuppressionIndex = 0; ruleSuppressionIndex < ruleSuppressions.Count; ruleSuppressionIndex++)
{
DiagnosticRecord record = diagnostics[recordIndex];
var curOffset = offsetArr[recordIndex];

//if (record.Extent.EndOffset > ruleSuppression.EndOffset)
if (curOffset != null && curOffset.Item2 > ruleSuppression.EndOffset)
RuleSuppression ruleSuppression = ruleSuppressions[ruleSuppressionIndex];
//if (diagnostic.Extent.StartOffset < ruleSuppression.StartOffset||diagnostic.Extent.EndOffset > ruleSuppression.EndOffset)
if (curOffset != null && (curOffset.Item1 < ruleSuppression.StartOffset || curOffset.Item2 > ruleSuppression.EndOffset))
{
break;
continue;
}

// we suppress if there is no suppression id or if there is suppression id and it matches
if (string.IsNullOrWhiteSpace(ruleSuppression.RuleSuppressionID)
|| (!String.IsNullOrWhiteSpace(record.RuleSuppressionID) &&
string.Equals(ruleSuppression.RuleSuppressionID, record.RuleSuppressionID, StringComparison.OrdinalIgnoreCase)))
if (string.IsNullOrWhiteSpace(ruleSuppression.RuleSuppressionID) ||
(!String.IsNullOrWhiteSpace(diagnostic.RuleSuppressionID) &&
string.Equals(ruleSuppression.RuleSuppressionID, diagnostic.RuleSuppressionID, StringComparison.OrdinalIgnoreCase)))
{
suppressed[recordIndex] = true;
suppressedRecords.Add(new SuppressedRecord(record, ruleSuppression));
suppressionCount += 1;
ruleSuppression.Kind = RuleSuppression.SuppressionKind.InSource;
suppressions.Add(ruleSuppression);
applied[ruleSuppressionIndex] = true;
}
}

recordIndex += 1;
if (suppressions.Count() != 0)
{
suppressedRecords.Add(new SuppressedRecord(diagnostic, suppressions));
}
else
{
unSuppressedRecords.Add(diagnostic);
}
}

// If we cannot found any error but the rulesuppression has a rulesuppressionid then it must be used wrongly
if (!String.IsNullOrWhiteSpace(ruleSuppression.RuleSuppressionID) && suppressionCount == 0)
// If we cannot found any error but the rulesuppression has a rulesuppressionid then it must be used wrongly
for (int ruleSuppressionIndex = 0; ruleSuppressionIndex < ruleSuppressions.Count; ruleSuppressionIndex++)
{
RuleSuppression ruleSuppression = ruleSuppressions[ruleSuppressionIndex];
if ((!String.IsNullOrWhiteSpace(ruleSuppression.RuleSuppressionID)) && !applied[ruleSuppressionIndex])
{
// checks whether are given a string or a file path
if (String.IsNullOrWhiteSpace(diagnostics.First().Extent.File))
Expand All @@ -1409,70 +1406,56 @@ public Tuple<List<SuppressedRecord>, List<DiagnosticRecord>> SuppressRule(
}
}

for (int i = 0; i < suppressed.Length; i += 1)
{
if (!suppressed[i])
{
unSuppressedRecords.Add(diagnostics[i]);
}
}

return result;
}

private Tuple<int,int>[] GetOffsetArray(List<DiagnosticRecord> diagnostics)
private Tuple<int,int> GetOffsetArray(DiagnosticRecord diagnostic)
{
Func<int,int,Tuple<int,int>> GetTuple = (x, y) => new Tuple<int, int>(x, y);
Func<Tuple<int, int>> GetDefaultTuple = () => GetTuple(0, 0);
var offsets = new Tuple<int, int>[diagnostics.Count];
for (int k = 0; k < diagnostics.Count; k++)
var offset = new Tuple<int, int>(0, 0);
var ext = diagnostic.Extent;
if (ext == null)
{
var ext = diagnostics[k].Extent;
if (ext == null)
return null;
}
if (ext.StartOffset == 0 && ext.EndOffset == 0)
{
// check if line and column number correspond to 0 offsets
if (ext.StartLineNumber == 1
&& ext.StartColumnNumber == 1
&& ext.EndLineNumber == 1
&& ext.EndColumnNumber == 1)
{
continue;
return offset;
}
if (ext.StartOffset == 0 && ext.EndOffset == 0)
// created using the ScriptExtent constructor, which sets
// StartOffset and EndOffset to 0
// find the token the corresponding start line and column number
var startToken = Tokens.Where(x
=> x.Extent.StartLineNumber == ext.StartLineNumber
&& x.Extent.StartColumnNumber == ext.StartColumnNumber)
.FirstOrDefault();
if (startToken == null)
{
// check if line and column number correspond to 0 offsets
if (ext.StartLineNumber == 1
&& ext.StartColumnNumber == 1
&& ext.EndLineNumber == 1
&& ext.EndColumnNumber == 1)
{
offsets[k] = GetDefaultTuple();
continue;
}
// created using the ScriptExtent constructor, which sets
// StartOffset and EndOffset to 0
// find the token the corresponding start line and column number
var startToken = Tokens.Where(x
=> x.Extent.StartLineNumber == ext.StartLineNumber
&& x.Extent.StartColumnNumber == ext.StartColumnNumber)
.FirstOrDefault();
if (startToken == null)
{
offsets[k] = GetDefaultTuple();
continue;
}
var endToken = Tokens.Where(x
=> x.Extent.EndLineNumber == ext.EndLineNumber
&& x.Extent.EndColumnNumber == ext.EndColumnNumber)
.FirstOrDefault();
if (endToken == null)
{
offsets[k] = GetDefaultTuple();
continue;
}
offsets[k] = GetTuple(startToken.Extent.StartOffset, endToken.Extent.EndOffset);
return offset;
}
else
var endToken = Tokens.Where(x
=> x.Extent.EndLineNumber == ext.EndLineNumber
&& x.Extent.EndColumnNumber == ext.EndColumnNumber)
.FirstOrDefault();
if (endToken == null)
{
// Extent has valid offsets
offsets[k] = GetTuple(ext.StartOffset, ext.EndOffset);
return offset;
}
offset = GetTuple(startToken.Extent.StartOffset, endToken.Extent.EndOffset);
}
else
{
// Extent has valid offsets
offset = GetTuple(ext.StartOffset, ext.EndOffset);
}
return offsets;

return offset;
}

public static string[] ProcessCustomRulePaths(string[] rulePaths, SessionState sessionState, bool recurse = false)
Expand Down
17 changes: 14 additions & 3 deletions Engine/ScriptAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ public sealed class ScriptAnalyzer
List<Regex> includeRegexList;
List<Regex> excludeRegexList;
bool suppressedOnly;
bool includeSuppressions;
#if !PSV3
ModuleDependencyHandler moduleHandler;
#endif
Expand Down Expand Up @@ -118,7 +119,8 @@ internal void Initialize<TCmdlet>(
string[] excludeRuleNames = null,
string[] severity = null,
bool includeDefaultRules = false,
bool suppressedOnly = false)
bool suppressedOnly = false,
bool includeSuppressions = false)
where TCmdlet : PSCmdlet, IOutputWriter
{
if (cmdlet == null)
Expand All @@ -135,7 +137,8 @@ internal void Initialize<TCmdlet>(
excludeRuleNames,
severity,
includeDefaultRules,
suppressedOnly);
suppressedOnly,
includeSuppressions);
}

/// <summary>
Expand All @@ -150,6 +153,7 @@ public void Initialize(
string[] severity = null,
bool includeDefaultRules = false,
bool suppressedOnly = false,
bool includeSuppressions = false,
string profile = null)
{
if (runspace == null)
Expand All @@ -174,6 +178,7 @@ public void Initialize(
severity,
includeDefaultRules,
suppressedOnly,
includeSuppressions,
profile);
}

Expand All @@ -188,6 +193,7 @@ public void CleanUp()
includeRegexList = null;
excludeRegexList = null;
suppressedOnly = false;
includeSuppressions = false;
}

/// <summary>
Expand Down Expand Up @@ -672,6 +678,7 @@ private void Initialize(
string[] severity,
bool includeDefaultRules = false,
bool suppressedOnly = false,
bool includeSuppressions = false,
string profile = null)
{
if (outputWriter == null)
Expand Down Expand Up @@ -731,6 +738,7 @@ private void Initialize(
}

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

Expand Down Expand Up @@ -2323,9 +2331,12 @@ 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();
IEnumerable<DiagnosticRecord> suppressedList = suppressed.OfType<DiagnosticRecord>();
IEnumerable<DiagnosticRecord> allRecordsList = diagnosticsList.Concat(suppressedList);

return this.suppressedOnly ?
suppressed.OfType<DiagnosticRecord>() :
suppressedList : this.includeSuppressions ?
allRecordsList :
diagnosticsList;
}
}
Expand Down
7 changes: 7 additions & 0 deletions Engine/ScriptAnalyzer.format.ps1xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@
<Width>60</Width>
<Label>Justification</Label>
</TableColumnHeader>
<TableColumnHeader>
<Width>10</Width>
<Label>Kind</Label>
</TableColumnHeader>
</TableHeaders>
<TableRowEntries>
<TableRowEntry>
Expand All @@ -101,6 +105,9 @@
<TableColumnItem>
<PropertyName>Justification</PropertyName>
</TableColumnItem>
<TableColumnItem>
<PropertyName>Kind</PropertyName>
</TableColumnItem>
</TableColumnItems>
</TableRowEntry>
</TableRowEntries>
Expand Down
Loading