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

feat: implement rfc 2021-suppression-support #15459

Merged
merged 14 commits into from Jan 27, 2022

Conversation

Yiwei-Ding
Copy link
Contributor

@Yiwei-Ding Yiwei-Ding commented Dec 28, 2021

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Implemented the RFC 2021-suppression-support.

Related issue #14784.

Is there anything you'd like reviewers to focus on?

@eslint-github-bot eslint-github-bot bot added feature triage labels Dec 28, 2021
@nzakas
Copy link
Member

@nzakas nzakas commented Dec 31, 2021

It looks like there are several file conflicts. Can you please take a look?

@Yiwei-Ding
Copy link
Contributor Author

@Yiwei-Ding Yiwei-Ding commented Jan 1, 2022

Conflicts resolved. Thanks.

Copy link
Member

@nzakas nzakas left a comment

Overall looks pretty good. A few things that jumped out to me.

lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
lib/linter/apply-disable-directives.js Show resolved Hide resolved
while (
nextDirectiveIndex < options.directives.length &&
compareLocations(options.directives[nextDirectiveIndex], problem) <= 0
) {
const directive = options.directives[nextDirectiveIndex++];
const suppression = { kind: "directive", justification: directive.unprocessedDirective.justification };
Copy link
Member

@nzakas nzakas Jan 5, 2022

Choose a reason for hiding this comment

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

Would it make sense to add location information to the suppression to show where the directive is?

Copy link
Contributor Author

@Yiwei-Ding Yiwei-Ding Jan 5, 2022

Choose a reason for hiding this comment

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

It's a good idea. The location info can be leveraged for auditing purposes as well. I also find a location property in suppressions in the SARIF format.

But I'd like to add location info to the suppressions in another PR because:

  1. The type location in suppressions is needed to be discussed. Shall we use the line and the column in unprocessedDirective directly or we use the similar structure in LintMessage (line/column/endLine/endColumn)?
  2. This PR is huge enough due to a lot of modification in core logic and tests.

@mdjermanovic mdjermanovic added accepted core and removed triage labels Jan 5, 2022
Copy link
Member

@mdjermanovic mdjermanovic left a comment

I think this doesn't work as expected with processors.

For example, I was testing with eslint-plugin-markdown.

.eslintrc.js:

module.exports = {
    plugins: ["markdown"],
    rules: {
        "no-undef": 2
    },
    overrides: [
        {
            files: ["**/*.md"],
            processor: "markdown/markdown"
        }
    ]
};

test.md:

```js
a;
```

```js
b;
```
D:\tmp\suppressions\eslint>npx eslint test.md

D:\tmp\suppressions\eslint\test.md
  2:1  error  'a' is not defined  no-undef
  6:1  error  'b' is not defined  no-undef

✖ 2 problems (2 errors, 0 warnings)

Then, if we suppress those two problems:

```js
a; // eslint-disable-line
```

```js
b; // eslint-disable-line
```
D:\tmp\suppressions\eslint>npx eslint test.md -f json > results.json

results.json:

[
  {
    "filePath": "D:\\tmp\\suppressions\\eslint\\test.md",
    "messages": [],
    "suppressedMessages": [
      {
        "ruleId": "no-undef",
        "severity": 2,
        "message": "'b' is not defined.",
        "line": 1,
        "column": 1,
        "nodeType": "Identifier",
        "messageId": "undef",
        "endLine": 1,
        "endColumn": 2,
        "suppressions": [
          {
            "kind": "directive",
            "justification": ""
          }
        ]
      }
    ],
    "errorCount": 0,
    "fatalErrorCount": 0,
    "warningCount": 0,
    "fixableErrorCount": 0,
    "fixableWarningCount": 0,
    "usedDeprecatedRules": []
  }
]

What doesn't look right in the output:

  1. There's only one suppressed message instead of two (slots.lastSuppressedMessages had only messages from the last code block).
  2. line and endLine should be 6 instead of 1 (the message was not postprocessed, so the location wasn't adjusted).

@Yiwei-Ding
Copy link
Contributor Author

@Yiwei-Ding Yiwei-Ding commented Jan 6, 2022

@mdjermanovic Thanks for pointing out! I moved distinguishSuppressedMessages() from applyDisableDirectives() to Linter#verify(), so that the linter with processors could preprocess - lint - postprocess - return non-suppressed messages.

@Yiwei-Ding Yiwei-Ding requested review from mdjermanovic and nzakas Jan 7, 2022
lib/linter/linter.js Outdated Show resolved Hide resolved
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
lib/linter/apply-disable-directives.js Show resolved Hide resolved
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jan 21, 2022

ESLint#getRulesMetaForResults() returns meta of rules that appear in messages. Per the description of this method, it "returns meta objects for each rule represented in the lint results".

eslint/lib/eslint/eslint.js

Lines 514 to 519 in 851f1f1

/**
* Returns meta objects for each rule represented in the lint results.
* @param {LintResult[]} results The results to fetch rules meta for.
* @returns {Object} A mapping of ruleIds to rule meta objects.
*/
getRulesMetaForResults(results) {

I think the expected behavior would be to include rules that appear in suppressedMessages.

@nzakas what do you think about this?

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jan 21, 2022

ESLint.getErrorResults() / CLIEngine.getErrorResults() filters out active (nonsuppressed) warnings, so it should probably filter out suppressed warnings as well. I think it would be unexpected to entirely omit active warnings from the results, but keep the suppressed ones.

Thoughts?

lib/linter/linter.js Outdated Show resolved Hide resolved
@nzakas
Copy link
Member

@nzakas nzakas commented Jan 25, 2022

I think the expected behavior would be to include rules that appear in suppressedMessages.

I agree. We want to be sure all rules represented in a formatter have access to the related meta info.

lib/cli-engine/cli-engine.js Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

Since this is a big change in the core, I'd certainly like someone else to review it too before merging.

@mdjermanovic mdjermanovic linked an issue Jan 27, 2022 that may be closed by this pull request
nzakas
nzakas approved these changes Jan 27, 2022
Copy link
Member

@nzakas nzakas left a comment

LGTM. Thanks so much for your hard work on this. Excellent job!

@mdjermanovic mdjermanovic merged commit 5d60812 into eslint:main Jan 27, 2022
14 checks passed
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jan 27, 2022

Thanks for contributing! This will be released tomorrow in ESLint v8.8.0

@Yiwei-Ding Yiwei-Ding deleted the SuppressionSupport branch Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted core feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants