eslint-plugin-eslint-comments will be broken in 4.6.0

[not-an-aardvark]

eslint-plugin-eslint-comments is a plugin created by @mysticatea to solve the issue in #6190 of linting for unused /* eslint-disable */ comments. Since ESLint rules aren't supposed to be aware of other rules, a plugin like this technically isn't supposed to be possible. However it was successfully implemented by monkeypatching a private API context.eslint.report), and has been useful to a lot of people for avoiding unused disable comments.

I just realized that eslint-plugin-eslint-comments will be broken in v4.6.0 as a result of d672aef, which refactors ESLint's internal reporting logic and removes the private API that was being monkeypatched. With the current version of ESLint on master, it is no longer possible to implement something like eslint-plugin-eslint-comments, because context objects in rules don't have a reference to the underlying Linter instance anymore. We explicitly don't make any guarantees about the stability of undocumented APIs, but it would still be nice to avoid breaking peoples' linting builds if possible.

So we have a few things to consider:

• Should we revert d672aef for now?
• Should we reconsider adding a core feature to warn on unused disable comments? Personally, I don't think using a plugin for this is a good solution in the long-term, because it breaks the boundaries between rules and prevents us from refactoring internal APIs.

cc @ilyavolodin @mysticatea

[mysticatea]

Thank you for this issue.

It was a bad thing that I'm using the internal API. But it's helpful to developers if they can know unused directive comments because the remaining directive comments can hide reports unintentionally in future.

I'm glad if I can maintain eslint-plugin-eslint-comments with public API.

[not-an-aardvark]

I agree. For now, I think maybe it would be best to revert d672aef until we have a better way to lint for unused directive comments.

I tried to update eslint-plugin-eslint-comments to use the public API by loading rules and running them within the plugin, but I don't think it's possible to make it fully work right now without accessing context.eslint/context._linter. It can check comments like // eslint-disable-line no-undef by running the no-undef rule separately, but it can't check comments like // eslint-disable-line that apply to all rules, because it doesn't know which rules are enabled.

In the long term, I think the best solution would be to check for unused directive comments in core, using something similar to what was discussed in #6190.

[mysticatea]

Should we revert d672aef for now?

I'm happy if we revert d672aef to keep detectable for unused directive comments.

Should we reconsider adding a core feature to warn on unused disable comments? Personally, I don't think using a plugin for this is a good solution in the long-term, because it breaks the boundaries between rules and prevents us from refactoring internal APIs.

Yes, I think.

However, in my experience, a rule to warn unused disable comments was good. It tells me the unused disable comments in real-time on editors. Also, it can stop CI builds. It's configurable with .eslintrc.
There are best practices around directive comments (e.g. specify a rule to disable), so I thought reasonable that eslint-plugin-eslint-comments plugin aggregates rules to enforce the best practices.

If..., I understand that it's opposite of ESLint philosophy, but if a public API to catch reports from other rules, it's useful. For example:

exports.create = (context) => {
    return {
        onReport(report) {
            console.log(report.ruleId, report.message, report.node, report.line, report.column)

            // Cancel the report to return false.
            return false
        }
    }
}

This is a generalized system of context.markVariableAsUsed(node).

The context.markVariableAsUsed(node) API cancels some reports of no-unused-vars rule. But onReport handler has more controllability. react/jsx-uses-vars, eslint-comments/no-unused-disable, and node/this-in-event-handlers will be feasible with the API. Possibly, also overwriting of a part of indentation rule.

[not-an-aardvark]

All of the eslint-plugin-eslint-comments tests pass after applying #9195 and #9196.

[not-an-aardvark]

For node/this-in-event-handlers, I think it would be best to use rule composition. For example:

module.exports = {
    create(context) {
        const eslint = require("eslint");
        const linter = new eslint.Linter();

        linter.verify(context.getSourceCode(), { rules: { "no-invalid-this": "error" } })
            .filter(problem => !isInEventHandler(problem))
            .forEach(problem => {
                context.report({ message: problem.message, loc: { line: problem.line, column: problem.column - 1 });
            });
        });
    }
};

Then users could easily use node/no-invalid-this as a replacement for no-invalid-this and eslint-plugin-node wouldn't need to copy the logic from no-invalid-this.

To avoid needing to do another traversal, maybe it would be possible to extract the listeners manually and call them as part of the node/no-invalid-this listeners, then filter out reports in the Program:exit listener.