feat: Per rule autofix configuration

[Samuel-Therrien-Beslogic]

Summary

This feature aims to make it possible to control autofixes through shareable configuration on a per-rule basis.

Related Issues

eslint/eslint#18696

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Samuel-Therrien-Beslogic / name: Samuel T. (07a9016, 41598f3, aa8bad8, ef18a67, 7497a52, 903c57e, e4440f9)

[eslint-github-bot]

Hi @Samuel-Therrien-Beslogic!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
• There should be a space following the initial tag and colon, for example 'feat: Message'.
• The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

[eslint-github-bot]

Hi @Samuel-Therrien-Beslogic!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
• There should be a space following the initial tag and colon, for example 'feat: Message'.
• The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

[ljharb]

What about instead of disabling autofixes, it turns it into suggestions?

[Samuel-Therrien-Beslogic]

What about instead of disabling autofixes, it turns it into suggestions?

That is what I would intend. I don't think suggestions need to be disabled as they're not "automated". I'll try to update my RFC to make this clearer.

One of the open question is whether suggestions could also be disabled (my proposal says no, but I've left the door open). The only case I could see it being valid is if a plugin has really bad suggestions and the maintainers are not updating it.

[ljharb]

I think this RFC, where it only allows "demoting" autofixes to suggestions on a per-rule basis, is excellent and more than sufficient.

[nzakas]

@Samuel-Therrien-Beslogic please be sure to sign the CLA.

[Samuel-Therrien-Beslogic]

@Samuel-Therrien-Beslogic please be sure to sign the CLA.

I sent the Corporate Contributor CLA to my employer (I know this is just a doc, and not code, but they preferred it given it was done under work hours). The person in charge told me he received the email. So hopefully that should be done soon :)

[michaelfaith]

I think this RFC, where it only allows "demoting" autofixes to suggestions on a per-rule basis, is excellent and more than sufficient.

To that end, if we're only using the autofixes portion of the config to turn off autofixes for specific rules, I'd propose having the config just be an array of rule names to turn autofix off, rather than it being a Record of rule name to boolean. Maybe disableAutofixes as the name? That eliminates this question from your RFC entirely:

Whether enabling autofixes for a rule that doesn't exist should warn, error or be silent.

[michaelfaith]

I guess you might need more than just an array in order to re-enable autofixes in later configs. You have it described in the RFC with overrides, but overrides isn't a thing in the new configuration format. I'd consider revising that to use the new config syntax. Probably something like this?

export default [
  {
    autofixes: {
      // We don't want this to autofix, as a rule suddenly not failing should require human attention
      '@eslint-community/eslint-comments/no-unused-disable': false,
    },
    rules: {
      '@eslint-community/eslint-comments/no-unused-disable': 'error',
    },
  },
  {
    files: ['*.spec.js'],
    autofixes: {
      // Let's pretend we want this to be autofixed in tests, for the sake of the RFC
      '@eslint-community/eslint-comments/no-unused-disable': true,
    },
  }
];

[nzakas]

Thanks for getting this started. I left a note regarding providing implementation details, which is required for an RFC.

[nzakas]

You're missing details on how this will be implemented. Please take a look at the code and see how you expect this feature to be implemented.

[Samuel-Therrien-Beslogic]

If I really need to learn about how ESLint is implemented I can, but idk when I'll get to it. I'd be more than happy if an existing contributor is willing to fill in this section for me as I am not personally concerned with implementation details.

[michaelfaith]

Looking at the code a bit, I would say this is probably a good place to start exploring: https://github.com/eslint/eslint/blob/main/lib/eslint/eslint.js#L386-L394

It's what's being used to determine if a rule should have a fix applied to it, based on a few different conditions. Based on the response from that, a "fixer" is passed back to the linter to use as part of its fix attempt loop.

[nzakas]

@Samuel-Therrien-Beslogic exploring the implementation is part of the RFC process. We can't really evaluate any proposal without it.

[michaelfaith]

Thinking more on the name, maybe something like disableAutofixes or suppressAutofixes and reverse the boolean condition would make its purpose more clear. It would also remove the concern about "turning on" an autofix that doesn't exist. You'd just be "not disabling the autofix", if that makes sense. Thoughts?

export default [
  {
    disableAutofixes: {
      // We don't want this to autofix, as a rule suddenly not failing should require human attention
      '@eslint-community/eslint-comments/no-unused-disable': true,
    },
    rules: {
      '@eslint-community/eslint-comments/no-unused-disable': 'error',
    },
  },
  {
    files: ['*.spec.js'],
    disableAutofixes: {
      // Let's pretend we want this to be autofixed in tests, for the sake of the RFC
      '@eslint-community/eslint-comments/no-unused-disable': false,
    },
  }
];

[Samuel-Therrien-Beslogic]

You have it described in the RFC with overrides, but overrides isn't a thing in the new configuration format. I'd consider revising that to use the new config syntax. Probably something like this?

Thanks. I am not familiar with flat-configs yet (too many plugins haven't migrated to v9, and I will need to rewrite our entire configs)

Thinking more on the name, maybe something like disableAutofixes or suppressAutofixes and reverse the boolean condition would make its purpose more clear. It would also remove the concern about "turning on" an autofix that doesn't exist. You'd just be "not disabling the autofix", if that makes sense. Thoughts?

Agreed. This is also closer to the inspiration cited with Ruff.

To that end, if we're only using the autofixes portion of the config to turn off autofixes for specific rules, I'd propose having the config just be an array of rule names to turn autofix off, rather than it being a Record of rule name to boolean.
[...]
I guess you might need more than just an array in order to re-enable autofixes in later configs.

Indeed a record seems cleaner to be able to re-enable. One could allow an array to disableAutofixes, but that would result in additional complexity on the implementation side with marginal benefits to the user.

I've added an entry about this in the FAQ

[mdjermanovic]

An alternative could be to expand rule config values to allow objects in which this would be a property. For example:

export default [
    {
        rules: {
            "@eslint-community/eslint-comments/no-unused-disable": {
                severity: "error",
                options: [/* ... */],
                disableAutofixes: true
            }
        }
    }
];

The advantages of this approach are that all the configurations for the rule can be in one place, and it would be easier to add more "meta" options if needed in the future.

[aladdin-add]

I like it! 👍

[nzakas]

We considered this a while back, even without trying to turn off autofixing. I'm not a fan of forcing rules to have to write out "severity" and "options", which is why we stuck with just an array.

This would also complicate rule configuration merging.

[aladdin-add]

Given a large number of config in the community are already in array format, I don't think we can drop it; instead, we can support both:

"@eslint-community/eslint-comments/no-unused-disable": ["error", {...}]

// is as the same as:

"@eslint-community/eslint-comments/no-unused-disable": {
    severity: "error",
    options: [/* ... */],
    disableAutofixes: false, // the default
}

[nzakas]

I don't think anyone was suggesting dropping the current format. I just think an object format adds additional complexity when merging rule configurations that isn't necessary. Plus, to modify a rule to disable autofix, you'd first need to convert the array into an object vs. adding a new key elsewhere.

[fasttime]

An alternative could be to expand rule config values to allow objects in which this would be a property. For example:

export default [
    {
        rules: {
            "@eslint-community/eslint-comments/no-unused-disable": {
                severity: "error",
                options: [/* ... */],
                disableAutofixes: true
            }
        }
    }
];

The advantages of this approach are that all the configurations for the rule can be in one place, and it would be easier to add more "meta" options if needed in the future.

Another advantage is that autofixes could be turned off from the CLI (with --rule 'some-rule: { disableAutofixes: true, ...}') or inline with /* eslint */ config comments without introducing additional syntax.

[Samuel-Therrien-Beslogic]

Changing the rule config format was a considered option (although not too much thoughts was put into what exactly that change would entail).
I like your suggestion too as it would also be backwards compatible and imo feels like a natural place for the config to live. But I went for the approach that reduces maintenance as much as possible.
At the end of the day it's really not my decision and both would satisfy my needs. So I'm leaving this one to the maintainers :)

[nzakas]

@fasttime this is additional syntax, though. Config comments and the command line don't already support this object notation.

[fasttime]

@fasttime this is additional syntax, though. Config comments and the command line don't already support this object notation.

Well, true. I should have written "with the same new syntax". My point is that rules can be configured in multiple ways, but the suggested approach only allows disabling autofixes inside a config.

I think while we are here, we should at least ask the question if it's sensible to disable autofixes from the CLI and in config comments. If the answer is no, we can go with the suggested approach and be assured that we won't have to rethink the design later.

[Samuel-Therrien-Beslogic]

disable autofixes from the CLI and in config comments

Config comments I wanna say no. Wanting to autofix or not is something to decide at the time of use. So when a user action is triggered (autofix on save in editor, which is already configurable in VSCode) or when calling the CLI (either on a CI or locally) by reading configuration files.

I find it really hard to imagine a rule that's you'd want to sometimes autofix, and sometimes not. Especially since autofixes don't change whether your linting "failed/passed" as a whole (I mean in the general idea that either something is reported or not, not about the return code 0/1). If a rule is "sometimes too fragile to fix", it should never be autofixed.

If it's incidentally supported through inline configs, sure, but I wouldn't put any effort towards supporting it.

---

Whether this should be configurable from the CLI: Prior art says yes. Looking at Ruff https://docs.astral.sh/ruff/configuration/#full-command-line-interface, their CLI accepts --unfixable which allows overriding their config https://docs.astral.sh/ruff/settings/#lint_unfixable

But even then with ESLint's way of having any config overridable on a per-file-selector basis (which Ruff doesn't have) and 9.12's Implement alternate config lookup (eslint/eslint#18742). I can hardly see a scenario where the CLI flag needs to be passed to disable specific rules' autofixes. Over simply using file selectors or nested configs in subfolders. and/or passing the paths to lint on the CLI.

[fasttime]

An alternative could be to expand rule config values to allow objects in which this would be a property. For example:

export default [
    {
        rules: {
            "@eslint-community/eslint-comments/no-unused-disable": {
                severity: "error",
                options: [/* ... */],
                disableAutofixes: true
            }
        }
    }
];

The advantages of this approach are that all the configurations for the rule can be in one place, and it would be easier to add more "meta" options if needed in the future.

Another advantage is that autofixes could be turned off from the CLI (with --rule 'some-rule: { disableAutofixes: true, ...}') or inline with /* eslint */ config comments without introducing additional syntax.

[Samuel-Therrien-Beslogic]

I just learned about https://eslint.org/docs/latest/use/configure/configuration-files#reporting-unused-disable-directives . Which I believe should be possible to disable the autofix just like any other rules as proposed by this RFC. However, it doesn't expose itself as a rule. How should this be handled?

I could:

1. add a property

export default [{
  linterOptions: {
    reportUnusedDisableDirectives: "error",
    autofixUnusedDisableDirectives: false
  }
}];

2. Change the config value to accept an object (just like regular rules)

export default [{
  linterOptions: {
    reportUnusedDisableDirectives: ["error", {autofix: false}]
  }
}];

3. Pretend reportUnusedDisableDirectives is a rule name and special-case it:

export default [{
  disableAutofixes: {
    // We don't want this to autofix, as a rule suddenly not failing should require human attention
    "reportUnusedDisableDirectives": true,
  },
}];

4. Accept that reportUnusedDisableDirectives's autofix should be configurable, but not handle it in this RFC

[Samuel-Therrien-Beslogic]

Should disableAutofixes go in linterOptions ? (as a note, if we do, I need to update the documentation location provided in this RFC)