Proposal: reportUnusedDisableDirectives in config files

[mysticatea]

The version of ESLint you are using.

• 4.8.0

The problem you want to solve.

As a great work of @not-an-aardvark, #9371 added --reportUnusedDisableDirectives CLI option. It's useful in order to detect unused disable directives which can hide errors unintentionally.

But unfortunately, we cannot enable the feature with config files (e.g. .eslintrc.json). As a result, there is no simple way to enable the feature equally on CI and the editors of team members. I have to write npm-scripts and config files of several editors.

It's convenient if we can configure the feature with config files.

Your take on the correct solution to problem.

This proposal adds a top-level property: reportUnusedDisableDirectives. It has a number or a string as their value as same as rules.

• 0 or "off" ... ESLint does not report unused disable directives. This is default.
• 1 or "warn" ... ESLint reports unused disable directives as warning.
• 2 or "error" ... ESLint reports unused disable directives as error. (exit with non-zero if the error exists)

{
    "extends": "eslint:recommended",
    "rules": {
        "indent": "error",
        "semi": "error",
        "quotes": "error"
    },
    "reportUnusedDisableDirectives": "error"
}

• Shareable configs can include the setting. This feature does not have confusable content such as relative paths. It's the essentially same as rule settings, it's turning warnings on/off. Also, it's needed for the migration path from eslint-comments/no-unused-disable rule.
• overrides field values can include the setting.
• Config files override the setting of base configs.
• CLI options and the constructor option of Linter/CLIEngine override the setting of config files.

I'll champion and implement this.

[not-an-aardvark]

I'm fine with having a config option for this, but I'm a bit worried about allowing shareable configs to set this option, due to the warning here. It seems like if a user inherits this value from a shareable config, they might not expect their build to get broken by a patch release of ESLint. When a command-line flag is used, this isn't a problem because each project opts into the warning.

[platinumazure]

Does this need to go on the TSC agenda?

And I'd be 100% 👍 for it myself, if we only allowed it in configurations with root: true. As it is, I'm 50/50 due to potential for confusion in either sharable configurations, plugins, or configurations in project subfolders.

[not-an-aardvark]

Does this need to go on the TSC agenda?

It will need to go on the agenda eventually in order to be accepted, yes. In general I think it's better if we continue discussion on the issue, and then add it to the TSC agenda when the discussion is finished (after we either reach an agreement, or we are unable to resolve our disagreement, or there just hasn't been any discussion for awhile). Otherwise, we can end up in a situation where we're still working out a lot of the details during the TSC meeting itself, and so we're less likely to reach consensus than if we had discussed the details on the issue.

Of course, you're free to add issues to the agenda at any point -- this is just my personal view of how the discussion would be most effective.

[ilyavolodin]

I thought we discussed this limitation during TSC meeting, and we're fine with it?

[not-an-aardvark]

The resolution was to accept the option as a command-line flag now, but discuss improving things for editor integrations afterwards.

Could you clarify what "limitation" you're referring to? Are you talking about the fact that the option can't currently be used in a config file, or are you talking about the fact that it can cause additional errors to be reported as a result of bugfixes?

[ilyavolodin]

I mean config file limitation.

[mysticatea]

Though I don't think reportUnusedDisableDirectives in sharable configs is a problem because of my experience with eslint-comments/no-unused-disable rule, I'm OK if there is a limitation that sharable configs can have reportUnusedDisableDirectives with only off or warn in order to prevent break CI builds by patch releases.

[mysticatea]

TSC Summary: This issue proposes adding reportUnusedDisableDirectives property into the configuration (e.g. .eslintrc). This would help to set the setting for all of command-line, CI builds, and the editor/IDE of developers at once.

TSC Question:

1. Should we accept this feature?
2. Should we make this setting available in sharable configs? @mysticatea thinks it's necessary, but @not-an-aardvark has a concern.
   I have a compromise that only off and warn are available in sharable configs to avoid breaking CI builds by the sharable config's setting and patch releases of ESLint.

[not-an-aardvark]

The TSC did not reach a consensus on this issue in today's meeting, so I'm leaving it on the agenda for next meeting.

[not-an-aardvark]

By the way, I created an alternative proposal which would make it possible to declare command-line options for a project without being able to inherit them from shareable configs.

[kaicataldo]

TSC Resolution: We will not accept this proposal for now until we can come up with better way of handing CLI option configurations.

[mysticatea]

Now I'm investigating another way..., implementing eslint-comments/no-unused-disable rule without private API. However, it's pretty hard.

One idea is that the rule uses require("eslint").CLIEngine and it verifies the code and checks the report. But the rule cannot get necessary information such as rulePaths. Also, I should give up if context.getFilename() returns <input>...

const CLIEngine = require("eslint").CLIEngine

function create(context) {
        const filePath = context.getFilename()
        if (filePath === "<input>") {
            // The code came from stdin.
            // TODO: what config should I use?
        } else {
            const eslint = new CLIEngine() // TODO: `rulePaths` is...?
            const report = eslint.executeOnFiles([filePath])

            // Check the report...
        }
}

Could somebody advise me?

[not-an-aardvark]

I don't think it's possible to do this. (I had also tried to refactor eslint-plugin-eslint-comments with to use the public API before I created #9193, but it's not possible in general because rules aren't supposed to know about other rules.)