feat: warn by default for unused disable directives

[bmish]

Prerequisites checklist

• I have read the contributing guidelines.

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)

Fixes #15466. This is part two of that issue. Part one was in #17212.

The goal here is to warn by default for reporting unused disable directives, only when using the new flat config.

TODO:

• Fix 35 tests in tests/lib/linter/linter.js
• Update docs
• Add new test

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

1. Is this the right place to change the default?
2. Where does the default need to be documented?

[netlify]

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	07352ef
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/658caff9cb3f6500083855ff

[mdjermanovic]

1. Is this the right place to change the default?

I was thinking about setting this default in default-config.js. For example, in its first object. That way, it will appear in --print-config, which is good I believe.

     languageOptions: {
         sourceType: "module",
         ecmaVersion: "latest",
         parser: require("espree"),
         parserOptions: {}
     },
+    linterOptions: {
+        reportUnusedDisableDirectives: 1
+    }

[mdjermanovic]

@bmish are you still working on this? This is one of the last changes planned for the upcoming v9.0.0-alpha.0.

• Fix 35 tests in tests/lib/linter/linter.js

Could we maybe fix those tests by adding linterOptions: { reportUnusedDisableDirectives: 0 } to the configs they're passing to linter.verify()?

[mdjermanovic]

2. Where does the default need to be documented?

I think here:

https://eslint.org/docs/head/use/configure/configuration-files#reporting-unused-disable-directives

Also here (at some point in the future, we'll maybe clear this redundancy we now have in the docs):

https://eslint.org/docs/head/use/configure/rules#report-unused-eslint-disable-comments

And maybe here, at the end of the Default Value line we could add (defaults to "warn"):

https://eslint.org/docs/head/use/command-line-interface#--report-unused-disable-directives-severity

[bmish]

I moved the default to default-config.js and fixed the existing tests. Thanks for the tips.

I still need to add a test for the new default and update some more docs. Will try to do this today.

[bmish]

I have updated the docs and added tests. This is ready for review.

[nzakas]

Overall LGTM. Just a note on changing rule tests.

[nzakas]

I think in this and the other rule tests, the intent was to check if directive comments were handled correctly.

Rather than changing all directive comments to non-directive comments, could we instead just turn off reporting of unused directives in those files?

[mdjermanovic]

Looks like the old rule tests aren't actually failing now, because FlatRuleTester has its own baseConfig, where linterOptions.reportUnusedDisableDirectives isn't set.

eslint/lib/rule-tester/flat-rule-tester.js

Lines 538 to 586 in d1018fc

	const baseConfig = [
	{ files: ["**"] }, // Make sure the default config matches for all files
	{
	plugins: {
	// copy root plugin over
	"@": {
	/*
	* Parsers are wrapped to detect more errors, so this needs
	* to be a new object for each call to run(), otherwise the
	* parsers will be wrapped multiple times.
	*/
	parsers: {
	...defaultConfig[0].plugins["@"].parsers
	},
	/*
	* The rules key on the default plugin is a proxy to lazy-load
	* just the rules that are needed. So, don't create a new object
	* here, just use the default one to keep that performance
	* enhancement.
	*/
	rules: defaultConfig[0].plugins["@"].rules
	},
	"rule-to-test": {
	rules: {
	[ruleName]: Object.assign({}, rule, {
	// Create a wrapper rule that freezes the `context` properties.
	create(context) {
	freezeDeeply(context.options);
	freezeDeeply(context.settings);
	freezeDeeply(context.parserOptions);
	// freezeDeeply(context.languageOptions);
	return rule.create(context);
	}
	})
	}
	}
	},
	languageOptions: {
	...defaultConfig[0].languageOptions
	}
	},
	...defaultConfig.slice(1)
	];

Now the question is shall we leave it that way?

[bmish]

Good points. I reverted the rule tests.

On the one hand, it seems like rule tests should generally use the default config? But on the other hand, reporting disable directives seems like it would be unnecessary and even annoying during rule tests, so I'd probably prefer to leave that option off...

[nzakas]

I think reporting unused directives is probably best left off for rule tests -- I can't imagine people wanting those warnings intermixed with rule errors.

[nzakas]

LGTM. Would like @mdjermanovic to confirm before merging.

[mdjermanovic]

LGTM, thanks!