Combine multiple suppressions applied to the same diagnostic #1699
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
Show file tree
Hide file tree
Changes from all commits
File filter
Filter by extension
Filter by extension
.cs
(3)
.ps1
(1)
All 2 file types selected
Viewed files
Viewed files
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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="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> errors) | ||
| { | ||
| 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 is null | ||
| || !ruleSuppressionsDict.TryGetValue(ruleName, out List<RuleSuppression> ruleSuppressions) | ||
| || ruleSuppressions.Count == 0) | ||
| { | ||
| unsuppressedRecords.AddRange(diagnostics); | ||
| return result; | ||
| } | ||
|
|
||
| // We need to report errors for any rule suppressions with IDs that are not applied to anything | ||
|
Comment thread
rjmholt marked this conversation as resolved.
|
||
| // 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) | ||
| { | ||
| if (!string.IsNullOrWhiteSpace(suppression.RuleSuppressionID)) | ||
| { | ||
| unappliedSuppressions.Add(suppression); | ||
| } | ||
| } | ||
|
|
||
| // 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) | ||
| { | ||
| // 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)) | ||
| { | ||
| continue; | ||
|
Comment thread
rjmholt marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // 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)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| unappliedSuppressions.Remove(suppression); | ||
| appliedSuppressions.Add(suppression); | ||
| } | ||
|
|
||
| if (appliedSuppressions.Count > 0) | ||
| { | ||
| suppressedRecords.Add(new SuppressedRecord(diagnostic, appliedSuppressions)); | ||
| } | ||
| else | ||
| { | ||
| unsuppressedRecords.Add(diagnostic); | ||
| } | ||
| } | ||
|
|
||
| // Do any error reporting for misused RuleSuppressionIDs here. | ||
|
Comment thread
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment. James Truher (@JamesWTruher) elaborated on error reasoning here
Sorry, something went wrong. |
||
| // 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) | ||
| { | ||
| string suppressionIDError = string.Format(Strings.RuleSuppressionIDError, unappliedSuppression.RuleSuppressionID); | ||
|
|
||
| if (appliedToFile) | ||
| { | ||
| 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> GetOffset(DiagnosticRecord diagnostic) | ||
| { | ||
| IScriptExtent extent = diagnostic.Extent; | ||
|
|
||
| if (extent is null) | ||
| { | ||
| 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) | ||
| { | ||
| startToken = curr; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| 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) | ||
| { | ||
| endToken = curr; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| 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
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.