Skip to content
This repository has been archived by the owner on Nov 16, 2023. It is now read-only.

Commit

Permalink
Fixed issues with multiple suppressions
Browse files Browse the repository at this point in the history 
When multiple suppressions appear on the same line, or multiple
suppressionss of the same type were in a file, or a suppression and a
review were on the same line, there were bugs that caused either the
whole line to be suppressed by one suppression, or the other suppression
to be incorrectly inserted, depending on scenario
  • Loading branch information
@joshbw
joshbw committed Mar 14, 2017
1 parent 8e68559 commit 3305dbc
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 32 deletions.
4 changes: 3 additions & 1 deletion server/src/devskimObjects.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ export class DevSkimProblem {
public issueURL : string;
public replacement: string;
public fixes: DevSkimAutoFixEdit[];
public suppressedFindingRange : Range;

public overrides : string[]; //a collection of ruleIDs that this rule supercedes

Expand All @@ -186,7 +187,8 @@ export class DevSkimProblem {
this.issueURL = (issueURL !== undefined && issueURL.length > 0) ? issueURL : "";
this.replacement = (replacement !== undefined && replacement.length > 0) ? replacement : "";
this.range = (range !== undefined ) ? range : Range.create(0,0,0,0);
this.severity = severity;
this.severity = severity;
this.suppressedFindingRange = null;

}

Expand Down
34 changes: 22 additions & 12 deletions server/src/devskimWorker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,20 +249,20 @@ export class DevSkimWorker
let lineStart: number = this.getLineNumber(documentContents,match.index);
let columnStart : number = (lineStart == 0) ? match.index : match.index - documentContents.substr(0,match.index).lastIndexOf("\n") -1;

//since a match may span lines (someone who broke a long function invocation into multiple lines for example)
//it's necessary to see if there are any newlines WITHIN the match so that we get the line the match ends on,
//not just the line it starts on. Also, we use the substring for the match later when making fixes
let replacementSource : string = documentContents.substr(match.index, match[0].length);
let lineEnd : number = this.getLineNumber(replacementSource,replacementSource.length) + lineStart;

let range : Range = Range.create(lineStart,columnStart,lineEnd, columnStart + match[0].length);

//look for the suppression comment for that finding
if(!suppressionFinding.showFinding)
{
//since a match may span lines (someone who broke a long function invocation into multiple lines for example)
//it's necessary to see if there are any newlines WITHIN the match so that we get the line the match ends on,
//not just the line it starts on. Also, we use the substring for the match later when making fixes
let replacementSource : string = documentContents.substr(match.index, match[0].length);
let lineEnd : number = this.getLineNumber(replacementSource,replacementSource.length) + lineStart;

let range : Range = Range.create(lineStart,columnStart,lineEnd, columnStart + match[0].length);

let problem : DevSkimProblem = new DevSkimProblem(rule.description,rule.name,
rule.id, this.MapRuleSeverity(rule.severity), rule.replacement, rule.rule_info, range);

if(rule.overrides !== undefined && rule.overrides.length > 0)
{
problem.overrides = rule.overrides;
Expand All @@ -279,9 +279,16 @@ export class DevSkimWorker
else if(suppressionFinding.ruleColumn > 0)
{
//highlight suppression finding for context
let range : Range = Range.create(lineStart,columnStart + suppressionFinding.ruleColumn,lineStart, columnStart + suppressionFinding.ruleColumn + rule.id.length);
let suppressionRange : Range = Range.create(lineStart,columnStart + suppressionFinding.ruleColumn,lineStart, columnStart + suppressionFinding.ruleColumn + rule.id.length);
let problem : DevSkimProblem = new DevSkimProblem(rule.description,rule.name,
rule.id, DevskimRuleSeverity.Informational, rule.replacement, rule.rule_info, range);
rule.id, DevskimRuleSeverity.Informational, rule.replacement, rule.rule_info, suppressionRange);
problem.suppressedFindingRange = range;

if(rule.overrides !== undefined && rule.overrides.length > 0)
{
problem.overrides = rule.overrides;
}

problems.push(problem);

}
Expand Down Expand Up @@ -384,8 +391,11 @@ export class DevSkimWorker
for(let x : number = 0; x < problems.length; x++)
{
var matches = problems[x].ruleId.match(regexString);
let range : Range = (problem.suppressedFindingRange != null) ? problem.suppressedFindingRange : problem.range;

if((matches !== undefined && matches != null && matches.length > 0)
&& problems[x].range.start.line == problem.range.start.line )
&& problems[x].range.start.line == range.start.line &&
problems[x].range.start.character == range.start.character)
{
problems.splice(x,1);
}
Expand Down
42 changes: 23 additions & 19 deletions server/src/suppressions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,21 +184,22 @@ export class DevSkimSuppression
{
let action : DevSkimAutoFixEdit = Object.create(null);
let isDateSet = (daysOffset !==undefined && daysOffset !=null && daysOffset > 0);
let regex : RegExp = (isReviewRule) ? DevSkimSuppression.reviewRegEx : DevSkimSuppression.suppressionRegEx;

//these are the strings that appear on the lightbulb menu to the user.
//<TO DO> make this localizable. Right now these are the only hard coded strings in the app. The rest come from the rules files
//and we have plans to make those localizable as well
if(isReviewRule)
{
action.fixName = "DevSkim: Mark Finding as Reviewed";
action.fixName = "DevSkim: Mark "+ruleID+" as Reviewed";
}
else if(isDateSet)
{
action.fixName = "DevSkim: Suppress issue for "+daysOffset.toString(10)+" days";
action.fixName = "DevSkim: Suppress "+ruleID+" for "+daysOffset.toString(10)+" days";
}
else
{
action.fixName = "DevSkim: Suppress issue permenantly";
action.fixName = "DevSkim: Suppress "+ruleID+" permenantly";
}


Expand All @@ -212,8 +213,24 @@ export class DevSkimSuppression
let range : Range;
var match;

let newlinePattern : RegExp = /(\r\n|\n|\r)/gm;

if(match = XRegExp.exec(documentContents,newlinePattern,startCharacter))
{
let columnStart : number = (lineStart == 0) ? match.index : match.index - documentContents.substr(0,match.index).lastIndexOf("\n") -1;
range = Range.create(lineStart,columnStart ,lineStart, columnStart + match[0].length);
documentContents = documentContents.substr(0,match.index);
}
else
{
//replace with end of file
let columnStart : number = documentContents.length - startCharacter;
range = Range.create(lineStart,columnStart ,lineStart,columnStart);
}


//if there is an existing suppression that has expired (or is there for a different issue) then it needs to be replaced
if(match = XRegExp.exec(documentContents,DevSkimSuppression.suppressionRegEx,startCharacter))
if(match = XRegExp.exec(documentContents,regex,startCharacter))
{
let columnStart : number = (lineStart == 0) ? match.index : match.index - documentContents.substr(0,match.index).lastIndexOf("\n") -1;
range = Range.create(lineStart,columnStart ,lineStart, columnStart + match[0].length);
Expand All @@ -240,20 +257,7 @@ export class DevSkimSuppression
}
//if there is not an existing suppression then we need to find the newline and insert the suppression just before the newline
else
{
let newlinePattern : RegExp = /(\r\n|\n|\r)/gm;

if(match = XRegExp.exec(documentContents,newlinePattern,startCharacter))
{
let columnStart : number = (lineStart == 0) ? match.index : match.index - documentContents.substr(0,match.index).lastIndexOf("\n") -1;
range = Range.create(lineStart,columnStart ,lineStart, columnStart + match[0].length);
}
else
{
//replace with end of file
let columnStart : number = documentContents.length - startCharacter;
range = Range.create(lineStart,columnStart ,lineStart,columnStart);
}
{

if(isReviewRule || isDateSet)
{
Expand Down Expand Up @@ -324,7 +328,7 @@ export class DevSkimSuppression
finding.showFinding = true;
}
}
else //we have a match with the rule (or all rules), and now "until" date, so we should ignore this finding
else //we have a match with the rule (or all rules), and no "until" date, so we should ignore this finding
{
finding.showFinding = true;
}
Expand Down

0 comments on commit 3305dbc

Please sign in to comment.