Skip to content

Combine multiple suppressions applied to the same diagnostic #1699

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

Merged
merged 16 commits into from
Jul 23, 2021
Merged

Combine multiple suppressions applied to the same diagnostic #1699

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
05a275d
Ensure multiply suppressed diagnostics only create one output
rjmholt Jul 21, 2021
e486374
Fix suppression with no offset
rjmholt Jul 21, 2021
4efd640
Defend against NRE
rjmholt Jul 21, 2021
2874be3
Complete comment help
rjmholt Jul 21, 2021
abdaefd
Add error emission comment
rjmholt Jul 21, 2021
177ed88
Fix justification test
rjmholt Jul 21, 2021
37a0104
Add extra tests
rjmholt Jul 21, 2021
e4ea36f
Try error redirection
rjmholt Jul 21, 2021
5eccd72
Try to fix errors
rjmholt Jul 21, 2021
200f0ad
Go back to ErrorAction
rjmholt Jul 21, 2021
c942343
Fix tests
rjmholt Jul 22, 2021
6bfdd25
Fix tests
rjmholt Jul 22, 2021
f1391d3
Fix error variable
rjmholt Jul 23, 2021
99f146e
Try to filter out other diagnostics
rjmholt Jul 23, 2021
f93ad20
Remove redundant filters
rjmholt Jul 23, 2021
498a8c9
Use other API for attribute types
rjmholt 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: 7 additions & 8 deletions Engine/Generic/SuppressedRecord.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.Collections.Generic;
using System.Collections.ObjectModel;

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic
{
/// <summary>
Expand All @@ -9,22 +12,18 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic
public class SuppressedRecord : DiagnosticRecord
{
/// <summary>
/// The rule suppression of this record
/// The rule suppressions applied to this record.
/// </summary>
public RuleSuppression Suppression
{
get;
set;
}
public IReadOnlyList<RuleSuppression> Suppression { get; set; }

/// <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, IReadOnlyList<RuleSuppression> suppressions)
{
Suppression = suppression;
Suppression = new ReadOnlyCollection<RuleSuppression>(new List<RuleSuppression>(suppressions));
if (record != null)
{
RuleName = record.RuleName;
Expand Down
241 changes: 131 additions & 110 deletions Engine/Helper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1323,156 +1323,177 @@ internal List<RuleSuppression> GetSuppressionsConfiguration(ConfigurationDefinit
/// Suppress the rules from the diagnostic records list.
/// Returns a list of suppressed records as well as the ones that are not suppressed
/// </summary>
/// <param name="ruleSuppressions"></param>
/// <param name="diagnostics"></param>
/// <param name="ruleName">The name of the rule possibly being suppressed.</param>
/// <param name="ruleSuppressionsDict">A lookup table from rule name to suppressions of that rule.</param>
/// <param name="diagnostics">The list of all diagnostics emitted by the given rule.</param>
/// <param name="errors">Any errors that arise due to misconfigured suppression settings.</param>
/// <returns>A pair, with the first item being the list of suppressed diagnostics, and the second the list of unsuppressed diagnostics.</returns>
public Tuple<List<SuppressedRecord>, List<DiagnosticRecord>> SuppressRule(
string ruleName,
Dictionary<string, List<RuleSuppression>> ruleSuppressionsDict,
List<DiagnosticRecord> diagnostics,
out List<ErrorRecord> errorRecords)
out List<ErrorRecord> errors)
{
List<SuppressedRecord> suppressedRecords = new List<SuppressedRecord>();
List<DiagnosticRecord> unSuppressedRecords = new List<DiagnosticRecord>();
Tuple<List<SuppressedRecord>, List<DiagnosticRecord>> result = Tuple.Create(suppressedRecords, unSuppressedRecords);
errorRecords = new List<ErrorRecord>();
if (diagnostics == null || diagnostics.Count == 0)
var suppressedRecords = new List<SuppressedRecord>();
var unsuppressedRecords = new List<DiagnosticRecord>();
errors = new List<ErrorRecord>();
var result = new Tuple<List<SuppressedRecord>, List<DiagnosticRecord>>(suppressedRecords, unsuppressedRecords);

if (diagnostics is null || diagnostics.Count == 0)
{
return result;
}

if (ruleSuppressionsDict == null || !ruleSuppressionsDict.ContainsKey(ruleName)
|| ruleSuppressionsDict[ruleName].Count == 0)
if (ruleSuppressionsDict is null
|| !ruleSuppressionsDict.TryGetValue(ruleName, out List<RuleSuppression> ruleSuppressions)
|| ruleSuppressions.Count == 0)
{
unSuppressedRecords.AddRange(diagnostics);
unsuppressedRecords.AddRange(diagnostics);
return result;
}

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)
// We need to report errors for any rule suppressions with IDs that are not applied to anything
// Start by assuming all suppressions are unapplied and then we'll remove them as we apply them later
var unappliedSuppressions = new HashSet<RuleSuppression>();
foreach (RuleSuppression suppression 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)
if (!string.IsNullOrWhiteSpace(suppression.RuleSuppressionID))
{
startRecord += 1;
unappliedSuppressions.Add(suppression);
}

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

while (recordIndex < diagnostics.Count)
}

// We have suppressions and diagnostics
// For each diagnostic, collect all the suppressions that apply to it
// and if there are any, record the diagnostic as suppressed
foreach (DiagnosticRecord diagnostic in diagnostics)
{
Tuple<int, int> offsetPair = GetOffset(diagnostic);

var appliedSuppressions = new List<RuleSuppression>();
foreach (RuleSuppression suppression in ruleSuppressions)
{
DiagnosticRecord record = diagnostics[recordIndex];
var curOffset = offsetArr[recordIndex];

//if (record.Extent.EndOffset > ruleSuppression.EndOffset)
if (curOffset != null && curOffset.Item2 > ruleSuppression.EndOffset)
// Check that the diagnostic is within the suppressed extent, if we have such an extent
if (!(offsetPair is null)
&& (offsetPair.Item1 < suppression.StartOffset || offsetPair.Item2 > suppression.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 there's a rule suppression ID, check that it matches the diagnostic
if (!string.IsNullOrWhiteSpace(suppression.RuleSuppressionID)
&& !string.Equals(diagnostic.RuleSuppressionID, suppression.RuleSuppressionID, StringComparison.OrdinalIgnoreCase))
{
suppressed[recordIndex] = true;
suppressedRecords.Add(new SuppressedRecord(record, ruleSuppression));
suppressionCount += 1;
continue;
}

recordIndex += 1;

unappliedSuppressions.Remove(suppression);
appliedSuppressions.Add(suppression);
}

// 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 (appliedSuppressions.Count > 0)
{
// checks whether are given a string or a file path
if (String.IsNullOrWhiteSpace(diagnostics.First().Extent.File))
{
ruleSuppression.Error = String.Format(CultureInfo.CurrentCulture, Strings.RuleSuppressionErrorFormatScriptDefinition, ruleSuppression.StartAttributeLine,
String.Format(Strings.RuleSuppressionIDError, ruleSuppression.RuleSuppressionID));
}
else
{
ruleSuppression.Error = String.Format(CultureInfo.CurrentCulture, Strings.RuleSuppressionErrorFormat, ruleSuppression.StartAttributeLine,
System.IO.Path.GetFileName(diagnostics.First().Extent.File), String.Format(Strings.RuleSuppressionIDError, ruleSuppression.RuleSuppressionID));
}
errorRecords.Add(new ErrorRecord(new ArgumentException(ruleSuppression.Error), ruleSuppression.Error, ErrorCategory.InvalidArgument, ruleSuppression));
//this.outputWriter.WriteError(new ErrorRecord(new ArgumentException(ruleSuppression.Error), ruleSuppression.Error, ErrorCategory.InvalidArgument, ruleSuppression));
suppressedRecords.Add(new SuppressedRecord(diagnostic, appliedSuppressions));
}
else
{
unsuppressedRecords.Add(diagnostic);
}
}

for (int i = 0; i < suppressed.Length; i += 1)

// Do any error reporting for misused RuleSuppressionIDs here.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JamesWTruher elaborated on error reasoning here

// If the user applied a suppression qualified by a suppression ID,
// but that suppression didn't apply only because the suppression ID didn't match
// we let the user know in the form of an error.
string analyzedFile = diagnostics[0]?.Extent?.File;
bool appliedToFile = !string.IsNullOrEmpty(analyzedFile);
string analyzedFileName = appliedToFile ? Path.GetFileName(analyzedFile) : null;
foreach (RuleSuppression unappliedSuppression in unappliedSuppressions)
{
if (!suppressed[i])
string suppressionIDError = string.Format(Strings.RuleSuppressionIDError, unappliedSuppression.RuleSuppressionID);

if (appliedToFile)
{
unSuppressedRecords.Add(diagnostics[i]);
unappliedSuppression.Error = string.Format(
CultureInfo.CurrentCulture,
Strings.RuleSuppressionErrorFormat,
unappliedSuppression.StartAttributeLine,
analyzedFileName,
suppressionIDError);
}
else
{
unappliedSuppression.Error = string.Format(
CultureInfo.CurrentCulture,
Strings.RuleSuppressionErrorFormatScriptDefinition,
unappliedSuppression.StartAttributeLine,
suppressionIDError);
}

errors.Add(
new ErrorRecord(
new ArgumentException(unappliedSuppression.Error),
unappliedSuppression.Error,
ErrorCategory.InvalidArgument,
unappliedSuppression));
}

return result;
}

private Tuple<int,int>[] GetOffsetArray(List<DiagnosticRecord> diagnostics)
private Tuple<int, int> GetOffset(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++)
IScriptExtent extent = diagnostic.Extent;

if (extent is null)
{
var ext = diagnostics[k].Extent;
if (ext == null)
{
continue;
}
if (ext.StartOffset == 0 && ext.EndOffset == 0)
return null;
}

if (extent.StartOffset != 0 && extent.EndOffset != 0)
{
return Tuple.Create(extent.StartOffset, extent.EndOffset);
}

// We're forced to determine the offset ourselves from the lines and columns now
// Rather than counting every line in the file, we scan through the tokens looking for ones matching the offset

Token startToken = null;
int i = 0;
for (; i < Tokens.Length; i++)
{
Token curr = Tokens[i];
if (curr.Extent.StartLineNumber == extent.StartLineNumber
&& curr.Extent.StartColumnNumber == extent.StartColumnNumber)
{
// 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);
startToken = curr;
break;
}
else
}

if (startToken is null)
{
return Tuple.Create(0, 0);
}

Token endToken = null;
for (; i < Tokens.Length; i++)
{
Token curr = Tokens[i];
if (curr.Extent.EndLineNumber == extent.EndLineNumber
&& curr.Extent.EndColumnNumber == extent.EndColumnNumber)
{
// Extent has valid offsets
offsets[k] = GetTuple(ext.StartOffset, ext.EndOffset);
endToken = curr;
break;
}
}
return offsets;

if (endToken is null)
{
return Tuple.Create(0, 0);
}

return Tuple.Create(startToken.Extent.StartOffset, endToken.Extent.EndOffset);
}

public static string[] ProcessCustomRulePaths(string[] rulePaths, SessionState sessionState, bool recurse = false)
Expand Down
Loading