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.

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

Conversation

Yiwei-Ding
Copy link
Contributor

This PR is to add suppression support for the sarif formatter of ESLint.

Suppression support in ESLint

In eslint/eslint#15459, the suppression support feature was merged into ESLint. When disabling suppressions by inline comments, the violations will be put into suppressedMessages in the output instead of being discarded. The text after two or more adjacent dashes will be considered as the justification. Detailed spec can be seen in rfcs/2021-suppression-support.

For example:

foo=1 // eslint-disable-line no-undef -- for test
> eslint -f json 1.js
[
  {
    ...
    "messages":[],
    "suppressedMessages":[{
      "ruleId":"no-undef",
      "severity":2,
      "message":"'foo' is not defined.",
      "line":1,
      "column":1,
      "nodeType":"Identifier",
      "messageId":"undef",
      "endLine":1,
      "endColumn":4,
      "suppressions":[{"kind":"directive","justification":"for test"}]}],
    ...
  }
]

NOTE: currently only inline suppressions are supported, so kind of suppressions is always "directive". Reason for why "directive" not "inSource" could be seen in this comment.

What this PR does

In this PR, I modify the formatter to convert suppressedMessages from ESLint to results with the suppressions property in the format of SARIF.

As for the example above, if we run eslint -f @microsoft/sarif 1.js, we will get:

{
  "version": "2.1.0",
  "$schema": "http://json.schemastore.org/sarif-2.1.0-rtm.5",
  "runs": [
    {
      "tool": {
        "driver": {
          "name": "ESLint",
          "informationUri": "https://eslint.org",
          "rules": [
            {
              "id": "no-undef",
              "helpUri": "https://eslint.org/docs/rules/no-undef",
              "properties": {},
              "shortDescription": {
                "text": "disallow the use of undeclared variables unless mentioned in `/*global */` comments"
              }
            }
          ],
          "version": "8.6.0"
        }
      },
      "artifacts": [
        {
          "location": {
            "uri": "file:///D:/test/1.js"
          }
        }
      ],
      "results": [
        {
          "level": "error",
          "message": {
            "text": "'foo' is not defined."
          },
          "locations": [
            {
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "file:///D:/test/1.js",
                  "index": 0
                },
                "region": {
                  "startLine": 1,
                  "startColumn": 1
                }
              }
            }
          ],
          "ruleId": "no-undef",
          "ruleIndex": 0,
          "suppressions": [
            {
              "kind": "inSource",
              "justification": "for test"
            }
          ]
        }
      ]
    }
  ]
}


result.messages.forEach(message => {
const messages = result.messages.concat(result.suppressedMessages);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is suppressedMessages guaranteed to be not undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The official doc is here.

_distinguishSuppressedMessages() in linter is to preserve the latest suppressed message list, which is initially an empty list instead of undefined or null. This comment could also corroborate this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @Yiwei-Ding. Any backwards compatibility concerns? Would our formatter ever receive output from an earlier version of ESLint that does not output suppressedMessages property?

Everything else looks great. Thanks for the contribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reminding! It's good for me to add incompatibility check.

let messages = result.messages;

if (result.suppressedMessages) {
    messages = messages.concat(result.suppressedMessages);
}

...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for addressing that!

@@ -220,6 +222,15 @@ module.exports = function (results, data) {
};
}

if (message.suppressions) {
Copy link
Member

@michaelcfanning michaelcfanning Feb 3, 2022

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.

Copy link
Collaborator

@EasyRhinoMSFT EasyRhinoMSFT left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@michaelcfanning michaelcfanning left a comment

Choose a reason for hiding this comment

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

:shipit:

@michaelcfanning
Copy link
Member

Nice improvement! Thanks for the contribution!

@michaelcfanning michaelcfanning merged commit d9f8070 into microsoft:main Feb 8, 2022
yongyan-gh added a commit that referenced this pull request Feb 8, 2022
Add suppression support (#2435)

* Add suppression support

* Add incompatibility check and make suppressions non-null

Co-authored-by: Eddy Nakamura <eddynaka@gmail.com>

Update releasehistory

fix couple test cases
@Yiwei-Ding Yiwei-Ding deleted the ESLintSuppressionSupport branch February 9, 2022 02:55
michaelcfanning added a commit that referenced this pull request Mar 2, 2022
)

* Add new visitor to get deterministic SARIF log by sorting results

* Fix dotnet format issue

* updating format

* remove unnecessary using

Format & minor fixes

* Add Run Comparer to support sorting logs with multiple runs.

* Add command argument unit tests

fix dotnet format

* use ContainsKey to avoid allocating variable

* Fixing `AnalyzeCommandBase` and `MultithreadedAnalyzeCommandBase` artifacts generation (#2433)

* Fixing multithreaded artifacts generation

* Fixing tests and flags

* Loosing precision.

* Applying fix for AnalyzeCommandBase

* Enabling tests

* Updating test case and release history

* Creating const to prevent magical numbers everywhere

* Rebaselining tests

* Creating Artifacts flag to keep previous behavior

* Addressing PR feedback.

* Rollback changes

* Update SARIF2012.ProvideRuleProperties_Invalid.sarif

* updating back

* Ordering deprecated names

* `SarifLogger` now emits an artifacts table entry if `artifactLocation` is not null for tool configuration and tool execution notifications. (#2437)

* Fixing artifacts generation when logging notifications

* Updating release history.

* Updating ReleaseHistory

* Fix `ArgumentException` when recurse is enabled and two file target specifiers generates the same file paths (#2438)

* Fixing ArgumentException when passing two filePaths that generates duplicated file analysis

* Fixing dotnet-format issues and updating releasehistory

* Removing comments

* Addressing PR feedback

* Addressing PR feedback

* Addressing PR review issues

Add suppression support (#2435)

* Add suppression support

* Add incompatibility check and make suppressions non-null

Co-authored-by: Eddy Nakamura <eddynaka@gmail.com>

Update releasehistory

fix couple test cases

* Fix issues in PR review

* Add xml comments

* Fix test issues

* fix dotnet format

* Addressing review feedbacks

* Fix tests

* Update extension methods names

* Change xml doc comments to normal comments

Co-authored-by: Eddy Nakamura <eddynaka@gmail.com>
Co-authored-by: Michael C. Fanning <michael.fanning@microsoft.com>
@scalvert
Copy link

Sorry to be late to reviewing this. We've moved the source to the sarif-js-sdk, but I was remiss in removing the source from this repo in favor of that, and republishing from that repo.

We should determine a plan to:

  • decommission the source from this repo
  • publish a new version from that repo (it can likely just be a patch, since it'll only be changing the repository path

This should not break/interrupt consumers, since the packages will be functionally the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants