Bug: Comment directives with just severity overrides options

[matwilko]

Environment

Node version: v18.16.0
npm version: v9.5.1
Local ESLint version: v8.45.0 (Currently used)
Global ESLint version: Not found
Operating System: win32 10.0.23493

What parser are you using?

Default (Espree)

What did you do?

Configuration

module.exports = [
  {
    rules: {
      "no-use-before-define": ["error", { functions: false, classes: true }]
    }
  }
];

/* eslint no-use-before-define: warn */

const a = A();
const b = new B();

function A() {}
class B {}

What did you expect to happen?

Given the documented behaviour for rule configuration overrides/merges is to retain options if only a severity is specified in an override/later merged config element, I'd expect the same to be true of comment directives.

In the above example, no-use-before-define generates only a single error highlighting the use of the class B if the comment directive is not present.

(...)\someFile.js
  4:15  error  'B' was used before it was defined  no-use-before-define

✖ 1 problem (1 error, 0 warnings)

I'd expect adding in the comment directive with only the severity specified to turn that error into a warning, e.g.

(...)\someFile.js
  4:15  warning  'B' was used before it was defined         no-use-before-define

✖ 1 problems (0 errors, 1 warnings)

What actually happened?

Adding in the comment directive creates two warnings, because the default options of no-use-before-define have overridden my original configuration:

(...)\someFile.js
  3:11  warning  'someFunc' was used before it was defined  no-use-before-define
  4:15  warning  'B' was used before it was defined         no-use-before-define

✖ 2 problems (0 errors, 2 warnings)

Link to Minimal Reproducible Example

https://eslint.org/play/#eyJ0ZXh0IjoiLyotLSBlc2xpbnQgbm8tdXNlLWJlZm9yZS1kZWZpbmU6IHdhcm4gKi9cblxuY29uc3QgYSA9IHNvbWVGdW5jKCk7XG5jb25zdCBiID0gbmV3IEIoKTtcblxuZnVuY3Rpb24gc29tZUZ1bmMoKSB7fVxuY2xhc3MgQiB7fSIsIm9wdGlvbnMiOnsiZW52Ijp7ImVzNiI6dHJ1ZX0sInBhcnNlck9wdGlvbnMiOnsiZWNtYUZlYXR1cmVzIjp7fSwiZWNtYVZlcnNpb24iOiJsYXRlc3QiLCJzb3VyY2VUeXBlIjoic2NyaXB0In0sInJ1bGVzIjp7Im5vLXVzZS1iZWZvcmUtZGVmaW5lIjpbImVycm9yIix7ImZ1bmN0aW9ucyI6ZmFsc2UsImNsYXNzZXMiOnRydWV9XX19fQ==

Participation

• I am willing to submit a pull request for this issue.

Additional comments

I think this can be fixed by updating linter.js#1325 and linter.js#1625 to use the flatConfigSchema.rules.merge rather than Object.assign.

[mdjermanovic]

I couldn't find any mention of this scenario in issues or PRs, nor a test case confirming that the current behavior is intentional.

However, given that the feature to change the severity of a rule without changing the options is only documented in Configuration Files and is under "The rules property can do any of the following", it seems that this feature is indeed designed to apply in configuration files, but not in configuration comments.

[matwilko]

Yeah, I couldn't find anything that explicitly documented this as the expected and designed behaviour, and it seemed like an unintuitive result to me, and something easy to overlook when implementing/reviewing originally, which is why I filed it as a bug 😄

At this point, it would be a breaking change to "fix" the behaviour now, so it'd have to wait for v9, but it seems to me like something that should be better aligned with the new config system when it comes into force.

To also contrast it with eslint-enable/eslint-disable, the behaviour is "correct" there, i.e.

// code here is checked against the configured options for `no-use-before-define`.

/* eslint-disable no-use-before-define */

// some code that is not checked for `no-use-before-define`

/* eslint-enable no-use-before-define */

// code here is checked against the configured options for `no-use-before-define`.

whereas the "equivalent" here silently changes the configured options for the remainder of the file:

// code here is checked against the configured options for `no-use-before-define`.

/* eslint no-use-before-define: off */

// some code that is not checked for `no-use-before-define`

/* eslint no-use-before-define: error */

// code here is checked against the _default_ options for `no-use-before-define`.

I think the problem here is that at the moment, there is no way to express "I just want to change the severity of this rule" without knowing in advance exactly what the options for that rule are going to be in that file (which is asking for config drift in the future).

I don't think there's really a case where a user would purposefully want to revert to a rule's default options, and if they did, it would probably be best that they explicitly did so inline (i.e. embed the rule's default config in the directive). If there were really a need to do so, something like /* eslint rule-name: error default */ could work as a syntax that is not currently valid, but communicates intent?

[nzakas]

I think this is a bug. The intent was always for configuration comments to act the same as the configuration file, so I think if a configuration comment only has a severity then the other options should remain the same.

This would be a breaking change, but since we are going to be doing v9 planning soon, this seems like a good time to discuss.

[github-actions]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

[Rec0iL99]

Not stale. Waiting for feedback.

[nzakas]

Thanks, we will talk about this during v9 planning.

[github-actions]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

[snitin315]

If a configuration comment only has a severity then the other options should remain the same.

I agree with this behavior, I think with v9 we can update this.

[mdjermanovic]

I'm working on this.