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

Add suppression support #2435

Merged
merged 3 commits into from
Feb 8, 2022
Merged

Add suppression support #2435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ec8dd0
Add suppression support
Yiwei-Ding Jan 30, 2022
9ce18f9
Add incompatibility check and make suppressions non-null
Yiwei-Ding Feb 4, 2022
b7825dd
Merge branch 'main' into ESLintSuppressionSupport
eddynaka Feb 4, 2022
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
19 changes: 16 additions & 3 deletions src/ESLint.Formatter/sarif.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,11 @@ module.exports = function (results, data) {
console.log(err);
}
}
if (result.messages.length > 0) {

result.messages.forEach(message => {
const messages = result.suppressedMessages ? result.messages.concat(result.suppressedMessages) : result.messages;

if (messages.length > 0) {
messages.forEach(message => {

const sarifRepresentation = {
level: getResultLevel(message),
Expand Down Expand Up @@ -208,7 +210,7 @@ module.exports = function (results, data) {
}
if (message.column > 0) {
sarifRepresentation.locations[0].physicalLocation.region.startColumn = message.column;
};
}
}

if (message.source) {
Expand All @@ -220,6 +222,17 @@ module.exports = function (results, data) {
};
}

if (message.suppressions) {
Copy link
Member

@michaelcfanning michaelcfanning Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

message.suppressions

An issue to keep in mind: in the case of any suppression utilization, SARIF requires that all messages populate this element (with an empty array in cases the result is unsuppressed. Spec text follows.

Many systems only conditionally emit or otherwise process suppression details. The .NET SuppressMessageAttribute is a good example: this in-source data is only produced in the presence of a certain #define at compilation time.

If ESLint always provides a suppression mechanism, one choice you have is to always emit an empty array for every result (at the expense of increasing log file size).

Alternately, if there's an efficient way to determine that no message in an analysis is suppressed, you can omit this data.

From 3.27.23

The suppressions values for all result objects in theRun SHALL be either all null or all non-null.
NOTE: The rationale is that an engineering system will generally evaluate all results for suppression, or none of them. Requiring that the suppressions values be either all null or all non-null enables a consumer to determine whether suppression information is available for the run by examining a single result object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks.

sarifRepresentation.suppressions = message.suppressions.map(suppression => {
return {
kind: suppression.kind === "directive" ? "inSource" : "external",
justification: suppression.justification
}
});
} else {
sarifRepresentation.suppressions = [];
}

if (message.ruleId) {
sarifResults.push(sarifRepresentation);
} else {
Expand Down
172 changes: 158 additions & 14 deletions src/ESLint.Formatter/test/sarif.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ describe("formatter:sarif", () => {
describe("when passed no messages", () => {
const code = [{
filePath: sourceFilePath1,
messages: []
messages: [],
suppressedMessages: []
}];

it("should return a log with files, but no results", () => {
Expand All @@ -103,6 +104,94 @@ describe("formatter:sarif", () => {

describe("formatter:sarif", () => {
describe("when passed one message", () => {
const code = [{
filePath: sourceFilePath1,
messages: [{
message: "Unexpected value.",
severity: 2,
ruleId: testRuleId
}],
suppressedMessages: []
}];

it("should return a log with one file and one result", () => {
const log = JSON.parse(formatter(code, { rulesMeta: rules }));

assert(log.runs[0].artifacts[0].location.uri.startsWith(uriPrefix));
assert(log.runs[0].artifacts[0].location.uri, "/" + sourceFilePath1);
assert.isDefined(log.runs[0].results);
assert.lengthOf(log.runs[0].results, 1);
assert.strictEqual(log.runs[0].results[0].level, "error");
assert.isDefined(log.runs[0].results[0].message);
assert.strictEqual(log.runs[0].results[0].message.text, code[0].messages[0].message);
assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.startsWith(uriPrefix));
assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.endsWith("/" + sourceFilePath1));
assert.strictEqual(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.index, 0);
assert.isDefined(log.runs[0].results[0].suppressions);
assert.lengthOf(log.runs[0].results[0].suppressions, 0);
});
});

describe("when passed one suppressedMessage", () => {
const code = [{
filePath: sourceFilePath1,
messages: [],
suppressedMessages: [{
message: "Unexpected value.",
severity: 2,
ruleId: testRuleId,
suppressions: [{ kind: "directive", justification: "foo" }]
}]
}];

it("should return a log with one file and one result", () => {
const log = JSON.parse(formatter(code, { rulesMeta: rules }));

assert(log.runs[0].artifacts[0].location.uri.startsWith(uriPrefix));
assert(log.runs[0].artifacts[0].location.uri, "/" + sourceFilePath1);
assert.isDefined(log.runs[0].results);
assert.lengthOf(log.runs[0].results, 1);
assert.strictEqual(log.runs[0].results[0].level, "error");
assert.isDefined(log.runs[0].results[0].message);
assert.strictEqual(log.runs[0].results[0].message.text, code[0].suppressedMessages[0].message);
assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.startsWith(uriPrefix));
assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.endsWith("/" + sourceFilePath1));
assert.strictEqual(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.index, 0);
assert.lengthOf(log.runs[0].results[0].suppressions, 1);
assert.strictEqual(log.runs[0].results[0].suppressions[0].kind, "inSource");
assert.strictEqual(log.runs[0].results[0].suppressions[0].justification, code[0].suppressedMessages[0].suppressions[0].justification);
});
});

describe("when passed one suppressedMessage with multiple suppressions", () => {
const code = [{
filePath: sourceFilePath1,
messages: [],
suppressedMessages: [{
message: "Unexpected value.",
severity: 2,
ruleId: testRuleId,
suppressions: [
{ kind: "directive", justification: "foo" },
{ kind: "directive", justification: "bar" }
]
}]
}];

it("should return a log with one file and one result", () => {
const log = JSON.parse(formatter(code, { rulesMeta: rules }));

assert.isDefined(log.runs[0].results[0].message);
assert.strictEqual(log.runs[0].results[0].message.text, code[0].suppressedMessages[0].message);
assert.lengthOf(log.runs[0].results[0].suppressions, 2);
assert.strictEqual(log.runs[0].results[0].suppressions[0].kind, "inSource");
assert.strictEqual(log.runs[0].results[0].suppressions[0].justification, code[0].suppressedMessages[0].suppressions[0].justification);
assert.strictEqual(log.runs[0].results[0].suppressions[1].kind, "inSource");
assert.strictEqual(log.runs[0].results[0].suppressions[1].justification, code[0].suppressedMessages[0].suppressions[1].justification);
});
});

describe("when passed one message and no suppressedMessages array", () => {
const code = [{
filePath: sourceFilePath1,
messages: [{
Expand All @@ -114,7 +203,7 @@ describe("formatter:sarif", () => {

it("should return a log with one file and one result", () => {
const log = JSON.parse(formatter(code, { rulesMeta: rules }));

assert(log.runs[0].artifacts[0].location.uri.startsWith(uriPrefix));
assert(log.runs[0].artifacts[0].location.uri, "/" + sourceFilePath1);
assert.isDefined(log.runs[0].results);
Expand All @@ -125,6 +214,8 @@ describe("formatter:sarif", () => {
assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.startsWith(uriPrefix));
assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.endsWith("/" + sourceFilePath1));
assert.strictEqual(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.index, 0);
assert.isDefined(log.runs[0].results[0].suppressions);
assert.lengthOf(log.runs[0].results[0].suppressions, 0);
});
});
});
Expand All @@ -138,7 +229,8 @@ describe("formatter:sarif", () => {
message: "Unexpected value.",
ruleId: ruleid,
source: "getValue()"
}]
}],
suppressedMessages: []
}];

it("should return a log with one rule", () => {
Expand All @@ -162,7 +254,8 @@ describe("formatter:sarif", () => {
message: "Unexpected value.",
ruleId: testRuleId,
line: 10
}]
}],
suppressedMessages: []
}];

it("should return a log with one result whose location region has only a startLine", () => {
Expand All @@ -184,7 +277,8 @@ describe("formatter:sarif", () => {
ruleId: testRuleId,
line: 10,
column: 0
}]
}],
suppressedMessages: []
}];

it("should return a log with one result whose location contains a region with line # and no column #", () => {
Expand All @@ -206,7 +300,8 @@ describe("formatter:sarif", () => {
ruleId: testRuleId,
line: 10,
column: 5
}]
}],
suppressedMessages: []
}];

it("should return a log with one result whose location contains a region with line and column #s", () => {
Expand All @@ -229,7 +324,8 @@ describe("formatter:sarif", () => {
line: 10,
column: 5,
source: "getValue()"
}]
}],
suppressedMessages: []
}];

it("should return a log with one result whose location contains a region with line and column #s", () => {
Expand All @@ -251,7 +347,8 @@ describe("formatter:sarif", () => {
severity: 2,
ruleId: testRuleId,
source: "getValue()"
}]
}],
suppressedMessages: []
}];

it("should return a log with one result whose location contains a region with line and column #s", () => {
Expand All @@ -264,6 +361,39 @@ describe("formatter:sarif", () => {
});
});



describe("formatter:sarif", () => {
describe("when passed one message and one suppressedMessage", () => {
const code = [{
filePath: sourceFilePath1,
messages: [{
message: "Unexpected value.",
severity: 2,
ruleId: testRuleId,
source: "getValue()"
}],
suppressedMessages: [{
message: "Unexpected value.",
severity: 2,
ruleId: testRuleId,
source: "getValue()",
suppressions: [{ kind: "directive", justification: "foo" }]
}]
}];

it("should return a log with two results, one of which has suppressions", () => {
const log = JSON.parse(formatter(code, rules));

assert.lengthOf(log.runs[0].results, 2)
assert.lengthOf(log.runs[0].results[0].suppressions, 0);
assert.lengthOf(log.runs[0].results[1].suppressions, 1);
assert.strictEqual(log.runs[0].results[1].suppressions[0].kind, "inSource");
assert.strictEqual(log.runs[0].results[1].suppressions[0].justification, code[0].suppressedMessages[0].suppressions[0].justification);
});
});
});

describe("formatter:sarif", () => {
describe("when passed two results with two messages each", () => {
const ruleid1 = "no-unused-vars";
Expand Down Expand Up @@ -291,7 +421,8 @@ describe("formatter:sarif", () => {
line: 10,
column: 5,
source: "doSomething(thingId)"
}]
}],
suppressedMessages: []
},
{
filePath: sourceFilePath2,
Expand All @@ -305,7 +436,8 @@ describe("formatter:sarif", () => {
message: "Custom error.",
ruleId: ruleid3,
line: 42
}]
}],
suppressedMessages: []
}];

it("should return a log with two files, three rules, and four results", () => {
Expand Down Expand Up @@ -385,6 +517,11 @@ describe("formatter:sarif", () => {
assert.strictEqual(log.runs[0].results[2].locations[0].physicalLocation.region.startLine, 18);
assert.isUndefined(log.runs[0].results[2].locations[0].physicalLocation.region.startColumn);
assert.isUndefined(log.runs[0].results[2].locations[0].physicalLocation.region.snippet);

assert.lengthOf(log.runs[0].results[0].suppressions, 0);
assert.lengthOf(log.runs[0].results[1].suppressions, 0);
assert.lengthOf(log.runs[0].results[2].suppressions, 0);
assert.lengthOf(log.runs[0].results[3].suppressions, 0);
});
});
});
Expand All @@ -404,7 +541,8 @@ describe("formatter:sarif", () => {
};
const code = [{
filePath: sourceFilePath1,
messages: [ ]
messages: [],
suppressedMessages: []
},
{
filePath: sourceFilePath2,
Expand All @@ -418,7 +556,8 @@ describe("formatter:sarif", () => {
message: "Custom error.",
ruleId: ruleid3,
line: 42
}]
}],
suppressedMessages: []
}];

it("should return a log with two files, two rules, and two results", () => {
Expand Down Expand Up @@ -464,6 +603,9 @@ describe("formatter:sarif", () => {
assert.strictEqual(log.runs[0].results[0].locations[0].physicalLocation.region.startLine, 18);
assert.isUndefined(log.runs[0].results[0].locations[0].physicalLocation.region.startColumn);
assert.isUndefined(log.runs[0].results[0].locations[0].physicalLocation.region.snippet);

assert.lengthOf(log.runs[0].results[0].suppressions, 0);
assert.lengthOf(log.runs[0].results[1].suppressions, 0);
});
});
});
Expand All @@ -476,7 +618,8 @@ describe("formatter:sarif", () => {
message: "Internal error.",
severity: 2,
// no ruleId property
}]
}],
suppressedMessages: []
}];

it("should return a log with no rules, no results, and a tool execution notification", () => {
Expand Down Expand Up @@ -526,7 +669,8 @@ describe("formatter:sarif", () => {
message: "Custom error.",
ruleId: ruleid,
line: 42
}]
}],
suppressedMessages: []
}];
it("should return a log with one file, one rule, and one result", () => {
const log = JSON.parse(formatter(code, { rulesMeta: rules }));
Expand Down