`no-unused-disable` autofixing does not properly work

[ehmicky]

This problem appeared in 3.1.0, i.e. was introduced by #13.

Input:

'use strict'

var values = [1, 2]

/* eslint-disable max-nested-callbacks */
values.map(function(value) {
  return values.map(function(otherValue) {
    return value + otherValue
  })
})
/* eslint-enable max-nested-callbacks */

Then eslint --fix file.js. Output:

'use strict'

var values = [1, 2]

values.map(function(value) {
  return values.map(function(otherValue) {
    return value + otherValue
  })
})
/* eslint-enable max-nested-callbacks */

Note that the following works:

'use strict'

var values = [1, 2]

// eslint-disable max-nested-callbacks
values.map(function(value) {
  return values.map(function(otherValue) {
    return value + otherValue
  })
})

.eslintrc.yml:

plugins: [eslint-comments]

rules:
  eslint-comments/no-unused-disable: 2

Versions:

$ eslint --version
v5.13.0
$ npm ls eslint-plugin-eslint-comments
my-package@1.2.4 /home/me/my-package
└── eslint-plugin-eslint-comments@3.1.0
$ node --version
v11.9.0 

[mysticatea]

Thank you for the report.

Looks like intentional behavior. If max-nested-callbacks error was not reported, eslint-comments/no-unused-disable rule delete the unnecessary directive comment: /* eslint-disable max-nested-callbacks */

This means, in the following case, because max-nested-callbacks error was reported, the /* eslint-disable max-nested-callbacks */ will not be removed.

/* eslint max-nested-callbacks: [error, {max: 1}] */
'use strict'

var values = [1, 2]

/* eslint-disable max-nested-callbacks */
values.map(function(value) {
  return values.map(function(otherValue) {
    return value + otherValue
  })
})
/* eslint-enable max-nested-callbacks */

However, in the following case, because max-nested-callbacks error was not reported, the /* eslint-disable max-nested-callbacks */ will be removed.

/* eslint max-nested-callbacks: [error, {max: 2}] */
'use strict'

var values = [1, 2]

/* eslint-disable max-nested-callbacks */
values.map(function(value) {
  return values.map(function(otherValue) {
    return value + otherValue
  })
})
/* eslint-enable max-nested-callbacks */

[ehmicky]

Hi @mysticatea,

One problem is that the related eslint-enable should probably be removed as well, otherwise eslint --fix leaves the code in a state that requires further fixes.

Another problem is that eslint-disable actually gets removed even when it is needed, i.e. when the max-nested-callbacks is reported. It only happens when using a pair of /* eslint-disable */ + /* eslint-enable */, but not when using a single // eslint-disable. Let me get a working example demonstrating this in few minutes.

[ehmicky]

I managed to reproduce it (it took me a while :) ).

'use strict'

// eslint-disable-next-line max-params
module.exports = function(context, type, promise, secondPromiseValue) {
  return true
}

Fixing produces:

'use strict'

module.exports = function(context, type, promise, secondPromiseValue) {
  return true
}

My .eslintrc.yml:

plugins: [eslint-comments]

rules:
  eslint-comments/no-unused-disable: 2
  max-params: 2

What's odd is that I can only reproduce this when fixing through eslint.CLIEngine not on the command line. You can reproduce (using same versions as above) with:

const { CLIEngine } = require('eslint')

const eslintConfig = {
  rules: { 'eslint-comments/no-unused-disable': 2, 'max-params': [ 'off' ] },
  plugins: [ 'eslint-comments' ],
  fix: true,
  useEslintrc: false,
}
const engine = new CLIEngine(eslintConfig)
const text = `'use strict'

// eslint-disable-next-line max-params
module.exports = function(context, type, promise, secondPromiseValue) {
  return true
}`
const report = engine.executeOnText(text)
console.log(report.results[0].output)

For information eslint.CLIEngine is used by many tools including prettier-eslint, which in turn is used by VS Code, which is where the behavior happened for me. But the problem is reproducible without VS Code nor prettier-eslint.

[mysticatea]

One problem is that the related eslint-enable should probably be removed as well, otherwise eslint --fix leaves the code in a state that requires further fixes.

Okay, now I understand your problem. Please write both the actual behavior and the expected behavior to report bugs in the future.

So, this is correct behavior for now, because eslint-comments/no-unused-enable rule has not supported autofix yet. PR is welcome :smile:

Another problem is that eslint-disable actually gets removed even when it is needed ...

Your CLIEngine example works correctly because you have 'max-params': [ 'off' ] setting. This means max-params error was never reported, so // eslint-disable-next-line max-params is unnecesary.

[ehmicky]

My original problem is this: in VS Code, I have some files that remove ESLint comments when I save them. However those ESLint comments are followed by lines that actually report ESLint errors. I.e. removing those comments make ESLint report those lines. There is definitely an issue there, this is not the expected behavior, and it started appearing with the 3.1.0 release of eslint-plugin-eslint-comments.

This is hard to debug because VS Code uses vscode-eslint which itself uses prettier-eslint (when that option is enabled) which uses eslint which uses eslint-plugin-eslint-comments.

I tried to reduce the original problem to a simpler problem to make the GitHub issue easier to fix, but it turns out I'm having a hard time reducing it to the actual problem.

Now based on your comment above I think I've boiled it down to the following problem: prettier-eslint is turning off unfixable rules before running eslint --fix. This leads eslint-comments/no-unused-disable to think some // eslint-disable comments can be removed, even though they have only "artificially" been removed by prettier-eslint.

I don't think this is an issue with prettier-eslint as I can see why they would want to turn off unfixable rules for their use case. I am wondering if the problem is not using the typical fixer helper.

file.js:

// eslint-disable-next-line no-empty-function
module.exports = function () {}

.eslintrc.yml:

plugins: [eslint-comments]

rules:
  eslint-comments/no-unused-disable: 2
  no-empty-function: 2

Running prettier-eslint file.js produces:

module.exports = function () {}

But the expected output is:

// eslint-disable-next-line no-empty-function
module.exports = function () {}

Same versions as above. prettier-eslint version is 4.7.1.

[mysticatea]

prettier-eslint is turning off unfixable rules before running eslint --fix.

That is the problem.

[ehmicky]

I think they might have some legitimate reasons to do so, i.e. if I open a GitHub issue to the prettier-eslint repository, they might send me back here.

Would there be a way to make prettier-eslint compatible with eslint-comments/no-unused-disable autofixing? It's actually quite commonly used, and often integrated within IDEs like VS Code and Atom.

[mysticatea]

The result of eslint-comments/no-unused-disable rule is affected by the reports of other rules. If the rule settings are changed temporary, eslint-comments/no-unused-disable rule cannot work properly.

I'd like to recommend to use eslint-plugin-prettier instead.

[ehmicky]

Using eslint-plugin-prettier (or eslint-config-prettier) fixes this issue, thanks.

For VS Code users reading this issue: prettier-eslint must be disabled with the Prettier: ESLint integration setting.

This conflict makes it impossible to code (it creates quite a difficult developer experience) without either disabling eslint-comments/no-unused-disable or using a prettier-eslint alternative. Now I do think there might be quite many Atom and VS Code users enabling this option. I am not sure everyone will see this GitHub issue, i.e. even though technically prettier-eslint is at fault, would this be a good idea to revert autofixing for eslint-comments/no-unused-disable?

[mysticatea]

I'm under considering.

[mysticatea]

After considering, I have determined to revert that autofix feature.

1. It conflicts with prettier-eslint.
2. The removal of unused errors is not the right solution always. Maybe people want to rename the typo in some cases.
3. After the autofix, empty lines are stayed sometimes. Because ESLint autofix is silent, people cannot know the empty lines from eslint --fix command.

It was reverted in 3cd7256c48e4b92c942faa93e4cd655f9dd1eb2c.