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. (e4440f9, 41598f3, ef18a67, 07a9016, 7497a52)

[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]

The new config system doesn't support an overrides key.

[nzakas]

I'm assuming you mean in all situations when autofixes are applied? That would include using the ESLint.outputFixes() or SourceCodeFixer APIs.

[Samuel-Therrien-Beslogic]

I am not familiar with ESLint's API or implementation, how would you rephrase this? I wrote this purely from an ESLint user's PoV because, well, that's what I am ^^

[mdjermanovic]

Technically, Linter (or report-translator?) could just create a new object in the suggestions array and move the fix into it. We'd just have to figure out what to set as desc (the text that appears in IDEs as an action to apply the suggestion).

https://eslint.org/docs/latest/integrate/nodejs-api#-lintmessage-type

[Samuel-Therrien-Beslogic]

I'm surprised that anything with an an autofix available isn't also automatically available as an editor suggestion. Do plugins have to provide both separately? If not, what is the "desc" equivalent to here? Or maybe ESLint just autofills suggestions from the autofix ?
(the example below is an autofixable rule that shows up as an editor suggestion, I just want to disable autofixes w/o removing the ability to manually trigger the fix from this menu)

If it's the case that they're provided separately, only the entry from LintMessage.fix needs to be dropped.

[ljharb]

That is a good point :-)

[mdjermanovic]

"Fix this autofix/prefer-template problem" is a text VS Code generates by itself when there's LintMessage#fix property. On click, VS Code applies the code change described in LintMessage#fix.

For LintMessage#suggestions, VS Code shows suggestion.desc as the text. On click, VS Code applies the code change described in suggestion.fix.

So, IDEs use both LintMessage#fix and LintMessage#suggestions to generate editor suggestions. Rules don't provide the same fix in both LintMessage#fix and LintMessage#suggestions. Furthermore, it probably never makes sense for a rule to provide LintMessage#suggestions when it provides LintMessage#fix.

[ljharb]

I think it makes sense to provide both sometimes - an autofix can be partial, and suggestions can go farther.

[Samuel-Therrien-Beslogic]

@mdjermanovic Is there no way to provide a non-automatic fix (ie: a suggestion) using a default message ?

Oh yeah, look at that, I can see the implication of your explanation where the editor hint for the fix is completely lost. Using the no-autofix plugin I do loose suggestions (although maybe it's because of how that plugin highjack rules, I don't have a better example with more "vanilla" rules off the top of my head)

vs

That's a behaviour I'd like to improve as part of this RFC over how the no-autofix plugin works. So I would prefer "promoting" the disabled autofix to a suggestion.

If there's no way to provide a "use default message", can we just manually pass

{
  suggestion: {
    fix: lintMessage.fix,
    // Something like this, with translation support?
	desc: `Fix this ${ruleName} problem`,
    // or
    desc: `Apply the disabled autofix for ${ruleName}`,
  },
  fix: undefined,
}

or maybe allowing an empty desc to fallback to a default "fix this" message should be a requirement for this RFC.

Thoughts on how to handle this? I'm pretty convinced on "promoting" the fix to suggestion, but the description if still an open question.

[mdjermanovic]

Maybe just desc: 'Apply autofix'?

desc: 'Apply the disabled autofix' also sounds good to me.

I think there's no need for the rule name in desc as IDEs display the rule name with the suggestion.

or maybe allowing an empty desc to fallback to a default "fix this" message should be a requirement for this RFC.

Suggestions should generally provide a descriptive message, so I think it's better to not implement a fallback.

[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.

[fasttime]

If a new top-level property like disableAutofixes is added to configuration objects then it should be documented in the eslint repo in docs/src/use/configure/configuration-files.md. Additionally we may want to add a note to docs/src/extend/custom-rules.md to mention that some autofixes will be converted automatically into suggestions when the new feature is used.

[Samuel-Therrien-Beslogic]

Thank you. I've added that. Any input on whether the main feature's documentation should be a subsection of Configuring Rules or on the same level ?

[fasttime]

Any input on whether the main feature's documentation should be a subsection of Configuring Rules or on the same level ?

I think a new section inside the "Configure Rules" page is fine.

[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.