feat: Per rule autofix configuration

[MorevM]

Summary

This RFC proposes adding support for controlling autofix behavior on a per-rule basis through shareable configuration.

Related Issues

eslint/eslint#18696

Notes

This is a continuation of the original RFC created to bring this to a conclusion.

The author of the original RFC didn't mind passing this along, so I created a fork and incorporated the changes discussed there, along with a proposed implementation plan.

I'd consider this a draft RFC, as it's written fairly informally and still contains a lot of "I think", "I'm not sure", "probably" and so on - but I believe it's a good starting point.

I hope this draft will receive comments so we can arrive at the right decisions and structure.
I'd appreciate any help from the team in shaping the document into its final form - working with code comes much more naturally to me than describing how it should behave.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: MorevM / name: Maxim Morev (59e0942, f309fb6, 72a6cd5, 2e892d3)
• ✅ login: Samuel-Therrien-Beslogic / name: Samuel T. (ef18a67, 41598f3, 7497a52, e4440f9, 903c57e, aa8bad8, 07a9016)

[michaelfaith]

How does extending these two options relate to allowing users to diasble autofux at the rule level? This seems like a separate concern, or at least it isn't quite clear to me.

[MorevM]

I'll agree that it conflicts with the RFC headline.

There was a discussion thread in the original RFC and the task seems related to me - I'd rather change the RFC title to something like "Control over autofix changes" than put the change in a separate RFC, as the steps would be quite similar.
Although it would be easier to focus, of course, even though the tasks are related.

Should I remove this suggestion now, or wait for other opinions?

[nzakas]

I think it's fine to leave it here given that these options can also produce autofixes that people might want to control.

[michaelfaith]

I feel like disableAutofix or disableFixers (with a default of false) might be slightly more intuitive. autofix: true on a rule that doesn't have fixers could be confusing on the surface.

[MorevM]

I took the latest posts from the thread as a starting point.

Generally this makes sense to me as well, I would advocate that boolean flags always default to false and require explicit enabling, this correlates with how I usually work.

But the proper name is open to me.
disableAutofix is fine, fix correlates with the Node API and CLI, but there are already precedents with the no prefix for negated checks - --no-warn-ignored, --no-color and a few others.

I was also going to suggest shortening it to disableFix, but then I thought that "auto" might be meaningful here, as if disableFix felt more restrictive.

I would choose between disableAutofix and noAutofix - one more explicitly reflects the intent, the other relates to the existing API.

[nzakas]

We try to avoid negative booleans for clarity disableAutofix: true requires some cognitive overhead vs. autofix: false is clear from the start.

(I know there is precedent with things like noInlineConfig, but we're trying to make better choices going forward.)

[KuSh]

And, if for a particular rule (beta or with many false positives), the autofix feature is disabled by default, it seems to me disableAutofix: false doesn't provide the same meaning as autofix: true.
That the disabling is canceled might not mean that the enabling is activated.

[MorevM]

@KuSh that make sense, the only point:

the autofix feature is disabled by default

We do not provide the ability (at least not yet discussed) for rule authors to control this behavior, for backwards compatibility the autofix is always applied if there is one.

*I don't really think that such a feature should be given - it can be solved with existing tools.
If the rule has faults - make them suggestions and indicate in the description that it is unstable, there's no need for a fixer

[nzakas]

Thanks for picking this up. I left a few comments.

[nzakas]

We try to avoid negative booleans for clarity disableAutofix: true requires some cognitive overhead vs. autofix: false is clear from the start.

(I know there is precedent with things like noInlineConfig, but we're trying to make better choices going forward.)

[nzakas]

I think it's fine to leave it here given that these options can also produce autofixes that people might want to control.

[nzakas]

I'm not 100% sure this is technically possible or desirable. Autofixing is CLI-specific and files can be linted without the CLI (i.e., in the browser), in which case this doesn't mean anything.

Need a few more opinions on this.

[mdjermanovic]

I think it's okay to support this, to allow rules to be configured the same way everywhere.

[nzakas]

Not sure if this is needed. We could leave this off and add later if we get requests.

[MorevM]

I think it makes sense to provide full feature parity for all possible use cases, but I won't insist - I've actually never run ESLint with rules specified that way, and I imagine it's a pretty rare use case.

[JoshuaKGoldberg]

I also don't think this is needed for the initial version. Specifying complex options on the CLI is not a great DX. I don't think it's something that should be encouraged, or is worth much investment.

[mdjermanovic]

I believe this will just work by itself, without any modification needed in the CLI code. --rule's input is parsed as an object, and whatever the result is, we just insert it in overrideConfig and let FlatConfigArray validate it and use it in the same way as rules configured in config files.

https://github.com/eslint/eslint/blob/f4fa40eb4bd6f4dba3b2e7fff259d0780ef6becf/lib/cli.js#L197

[nzakas]

This is a key detail of implementation that needs fleshing out in this RFC. Figuring out how to efficiently normalize configs so we're not creating a ton of extra objects is important for the implementation.

[MorevM]

@nzakas The normalization happens in a fairly limited number of places and is easy to implement, it's a pretty straightforward operation. The harder part is identifying and updating all the consumers to work with the normalized object instead of RuleSeverity | [RuleSeverity, ...unknown[]].

Or are you referring to refactoring the code to minimize or even eliminate the use of the non-normalized format altogether, like in this example?

[nzakas]

The implementation itself needs more thought than I see currently in this RFC. I mentioned this previously: I don't think normalizing all rule configs into an object is the right way to go. It creates A LOT of extra objects in memory that are probably not needed and can slow things down.

It would be helpful if you could update this RFC with the specific implementation you have in mind. Just saying that certain functions need to be considered doesn't give a clear vision of what you think the implementation will look like.

[MorevM]

Is it okay if I fork the repo, try out the suggested changes, and share the link here?
I think it might be easier to show what I mean directly with code instead of trying to explain everything in words.

We can always come back to describing it properly later if the general approach looks reasonable, I'm not propose it as a final solution - just as a way to keep the discussion moving.

[nzakas]

I'm unclear what this question means. What is "legacy format"?

[MorevM]

@nzakas I mean adding support for a new rule configuration format to the .eslintrc-style
configuration (I'm not yet deeply involved in how support for this format is implemented, so maybe this question isn't all that relevant).

While looking through the code, I noticed that the eslint package itself relies on some functions from the @eslint/eslintrc package (example), and those would need to be updated for the new format to work properly.

What confused me, though, is that the repository says: "This package is frozen except for critical bug fixes as ESLint moves to a new config system", and this RFC change doesn't fall under that category.

[mdjermanovic]

.eslintrc-style configuration format should not support the new rule configuration format.

While looking through the code, I noticed that the eslint package itself relies on some functions from the @eslint/eslintrc package (example), and those would need to be updated for the new format to work properly.

We should update the eslint package to not use functions from the @eslint/eslintrc package in code that handles the new flat config format.

[nzakas]

We should update the eslint package to not use functions from the @eslint/eslintrc package in code that handles the new flat config format.

I think I got most of this with the Config refactor. At the very least, I believe the relevant methods now exist on Config.

While looking through the code, I noticed that the eslint package itself relies on some functions from the @eslint/eslintrc package (example), and those would need to be updated for the new format to work properly.

I'll take a look at this.

[JoshuaKGoldberg]

What about switching specific suggestions to fixes? This comes up fairly regularly, e.g. typescript-eslint/typescript-eslint#11223.

Even if that's out of scope for this work, I'd think it should at least be mentioned as something that was thought about / could be a next step.

[MorevM]

Personally, I'd prefer to keep this as a separate task - there's already enough room for mistakes as it is.

[JoshuaKGoldberg]

I don't find "plugins are sometimes broken" to be very motivating personally 🙂. If a plugin rule is so buggy that its fixes are unsafe, I wouldn't trust it at all.

IMO a more compelling motivation is the "happy path" use case alluded to in the last sentence. What do you mean by "project-specific or personal preferences"?

[MorevM]

I agree the phrasing is a bit vague - and in practice, if an autofix isn't reliable, it usually ends up either being rewritten manually or suppressed with eslint-disable-next-line.
There's little reason to leave a persistent warning in that case.

What do you mean by "project-specific or personal preferences"?

I'm not sure about "project-specific", but I do have a concrete example of "personal preferences":

In most cases when I disable autofix, it's because it interferes with my development flow.

Take the prefer-const rule as an example:

I start some code with let because I plan to reassign the variable later.

let myVar = null;
// there is nothing else here

However, I have autofix-on-save enabled (which I believe is a common and reasonable setup).
I press Ctrl+S and let is replaced by const, because it's not (yet) overwritten anywhere.
But I intended to do that - I just haven't gotten to it yet :) I find this kind of premature autofix frustrating.
Still, I definitely want the warning to appear after I finish the logic, if it's still relevant.

Another example is unicorn/prefer-string-slice

I agree with the intent behind the rule, but its autofix often feels unnecessarily verbose.
It rewrites s.substring(0, endPosition) into s.slice(0, Math.max(0, endPosition)) - technically correct, but adds extra complexity that feels like a workaround.
Sure, if you remember that substring() coerces negative values to 0, and slice() does not, the fix makes sense.
But unless you're actively thinking about that - and I usually don't - the rewritten code just feels clunky.
In most of my real-world code, I'm confident the values are non-negative, so this autofix adds noise without much benefit.

---

I could rewrite this section based on that, but it's hard for me and would probably be too, well, "personal" and less scalable :)

[Samuel-Therrien-Beslogic]

Off the top of my head for an example of a rule that worked but the autofix was broken: angular-eslint/angular-eslint#1725 (even after being fixed, because the fix was only available for newer versions of angular and we needed to support a wider range, I couldn't just blindly enable the rule for all version, because of the broken autofix on previous versions)

I think I also had examples from eslint-plugin-etc and eslint-plugin-unicorn but I don't remember them by hearth.

And of course the best thing to do is report the broken behaviour, but having a way to temporarily work around it whilst still flagging the violation and allowing manual fixes was a nice to have (thanks to eslint-plugin-noautofix, which this feature is inspired from)

[JoshuaKGoldberg]

: boolean

Feature request: what about targeting specific message IDs?

[MorevM]

I think it would complicate things too much, both for end users and rule authors - you'd have to list in the documentation all the possible..

I see your scenario with specific rules, but it seems to me that it's a pretty rare scenario where you need to selectively disable some individual fixes.

But I'm ok with any coordinated changes, just saying :)

[fasttime]

Feature request: what about targeting specific message IDs?

This would be useful but I think it's fine to get started with rule-based settings. Probably it won't last long until users start asking for the ability to make this feature even more configurable.

[nzakas]

messageIds are not a user-facing feature and should not be exposed through config.

[JoshuaKGoldberg]

ACK that they're not currently a user-facing feature, but I think there's potential value in being able to target specific reports within a rule. +1 to @fasttime on not tackling that as part of this RFC. Just mentioning that in the RFC as being explicitly out of scope seems reasonable to me.

[mdjermanovic]

Unused directives produce lint messages, like rules, so there should be no problems with adding suggestions to those lint messages in linter/apply-disabled-directives.js. Perhaps some IDEs currently don't expect suggestions on lint messages that have ruleId: null, so the suggestions might be ignored by those IDEs at first (I think that was the case with autofixes for unused disable directives in VS Code), but that's something that should be updated in IDEs.

[mdjermanovic]

If eslint.calculateConfigForFile(filePath) would be returning rule configs normalized to the new object format, I think that could be considered a breaking change,

[nzakas]

It would be, and as I've mentioned elsewhere, this is why I think we need to have a larger discussion around what config normalization looks like for this RFC. I really don't like the idea of changing the format of everything our APIs are working with.

[mdjermanovic]

What happens when the severity is omitted, should we throw an error?

[MorevM]

I think it's reasonable to default the severity to error in this case.
If the user explicitly configures the rule - using the full form - that likely indicates they intend to enable it; otherwise, why bother configuring it at all?
*If the severity was already set, we'll inherit that value

[Samuel-Therrien-Beslogic]

If the severity was already set, we'll inherit that value

++

When extending configs, but disabling an autofix for a rule, I'd want the rule configs and severity to stay the same.

[fasttime]

If we want to allow options without severity, I think the default severity for a rule should remain "off" regardless of other settings. This would simplify the merging logic and avoid confusion if the same rule is configured in multiple places.

Alternatively we could require severity to be always specified when options is present, but at that point I'm not sure if it would be easier to have a single property for both severity and options, as their values would be otherwise concatenated in one array.

[nzakas]

I think it's okay not to require severity. If, after merging all the rule's configs, there is no severity specified, then I think we should throw an error because it's clearly a mistake.

[mdjermanovic]

We should specify what is the resulting configuration for a rule in case when two or more config objects have a configuration for it.

For example:

export default [
  {
    rules: {
      'eqeqeq': {
        severity: 'error',
        options: ['always'],
      }
    }
  },
  {
    rules: {
      'eqeqeq': {
        autofix: false
      }
    }
  }
];

I think the objects should be merged, so that the resulting configuration for the eqeqeq rule becomes:

{
  severity: 'error',
  options: ['always'],
  autofix: false
}

For properties that exist in both objects, the value from the second one is used, i.e., it overwrites the first one.

I think this would be aligned with the current behavior, where specifying just severity overwrites the severity from a previous config object but keeps the options:

https://eslint.org/docs/latest/use/configure/rules#using-configuration-files

[Samuel-Therrien-Beslogic]

I think the objects should be merged, so that the resulting configuration for the eqeqeq rule becomes:

Agreed, and that's what I would expect as a user from the new flat configs.

#134 (comment)

When extending configs, but disabling an autofix for a rule, I'd want the rule configs and severity to stay the same.

[fasttime]

Feature request: what about targeting specific message IDs?

This would be useful but I think it's fine to get started with rule-based settings. Probably it won't last long until users start asking for the ability to make this feature even more configurable.

[fasttime]

If we want to allow options without severity, I think the default severity for a rule should remain "off" regardless of other settings. This would simplify the merging logic and avoid confusion if the same rule is configured in multiple places.

Alternatively we could require severity to be always specified when options is present, but at that point I'm not sure if it would be easier to have a single property for both severity and options, as their values would be otherwise concatenated in one array.

[fasttime]

Is the recommendation here to update rule config normalization in a separate PR, before introducing autofix disabling logic? Can this be clarified?

[MorevM]

I definitely believe that it would take (at least) two PRs to accomplish the task, as it's at least easier to review.
But I'm not sure how to describe it here, and I'm not really sure if it makes sense for end users. PRs after all can only be consecutive, and the first PR with the updated format seems like it would be a breaking change, and as if one would want to show users the real reason for the change right away, i.e. do the release when both are ready.

Should I declare that each sub-item of the Impementation plan section is a separate PR?
Please help me with specific wording, or I can try it myself for further consideration

[fasttime]

I can imagine that some third-party tools, just like ESLint's own packages @eslint/core, @eslint/plugin-kit and probably more, will need an update in order to support the new rule config format without throwing errors. typescript-eslint will for sure. This makes me wonder if it would make sense to treat this feature as breaking.

[MorevM]

I'm not sure what's going on inside typescript-eslint, but in general I agree that this is a potentially breaking change not for end users directly, but for authors of solutions on top of the config.

I can imagine some utility wrapper function over the config, say setSeverityToWarning, that won't expect the updated format in the same way that the utility functions in ESLint itself don't.

Considering also this comment, it seems as if there are enough arguments to consider this change breaking.

I will put it in the document.

[fasttime]

This would need a separate RFC. I'd leave that out.

[nzakas]

I left this comment inline but also posting here for better visibility: We need to think long and hard about what config normalization looks like in this context. I don't like the idea of normalizing every rule config into an object because most rules won't have an autofix setting and therefore won't need any adjustment. Creating an extra object for every rule config regardless of whether autofix is set will create A LOT of extra objects in memory, and every allocation contributes to a slowing down of the entire operation.

I'm also not a fan of introducing breaking changes for this purpose. What we are talking about here is an edge case and I don't think it rationalizes breaking APIs or anything else. There are far too many consumers expecting a specific config format (Config Inspector, for one) and I don't see enough benefit for this RFC to break the ecosystem.

[MorevM]

My two cents, but perhaps it will help further the discussion

I'm generally not a fan of these kinds of micro-optimizations ([Severity, Record<string, any>] vs { severity: Severity, options: Record<string, any> }).
I'm not claiming to be a performance expert, but I did wonder whether having more objects instead of arrays would really matter - so I put together a quick test (codesandbox playground, run with node --expose-gc index.js):

Access speed (less is better):
- Array:   22.43 ms
- Object:  21.22 ms
---
Memory usage (100_000 elements):
- Array:   10243.50 KB
- Object:  7900.42 KB

Surprisingly, objects used less memory than arrays.
I honestly expected the opposite - maybe V8 is doing something clever with objects of known shape, or maybe I made a mistake (feel free to correct me if the issue is obvious).

Of course, this is a synthetic benchmark, and real-world performance may differ.
But realistically, how many rules do we usually have in a config? 1000? 2000? Does it really matter that much if we normalize the values once and reuse them afterward?

And frankly, wouldn't we gain more from having a consistent object shape to work with? How many CPU cycles are we spending on repeated checks like Array.isArray(value) ? value[0] : value across the codebase? (update: we have to do these checks anyway, since we expect rule configurations not only in the config, but also in the CLI and directives)

*This really needs proper testing by people who deeply understand performance optimization - I'm just experimenting here.
But at least for now, the performance argument feels a bit overstated to me.

---

As for backward compatibility - that's a much more serious concern.

I fully understand the reluctance to introduce a major version bump (especially to the nice, round 10) for something so minor - nobody wants an underwhelming announcement like "ESLint 10: mostly internal fixes, please update anyway".

But maybe we can compromise?
For example, calculateConfigForFile() and similar APIs could return data in the old format until v10, and we could expose the new normalized format behind a flag or even an environment variable (assuming, again, that the performance impact of switching to object form is as minimal as I think).

Of course, we could just avoid normalizing entirely - stash autofix data somewhere else, pick it in when needed, expose through a new API method, etc.
But honestly, that feels like a workaround. The object form is more extensible, and sooner or later we're going to need something like it anyway. People have already asked for similar functionality - and we can all imagine more use cases down the road.

Tooling like Config Inspector will require tweaking anyway, no matter how we expose autofix data - and if tweaking is required anyway, does it matter what kind of tweaking it will be?

[nzakas]

@MorevM your benchmark doesn't match the situation I'm describing. The concern is not the access speed of items in an object vs. an array. The concern is that we'll be created both an array and a object for every rule entry, thus significantly increasing our memory footprint.

The more I think about this problem, the more I'm convinced that we cannot change the config object format that is exposed publicly. As I already stated, what we're looking to address in this RFC is an edge case, and I don't think that rationalizes such a significant breaking change.

An alternate approach would be to allow this new object rule entry format only in defineConfig() as a convenience for end users and under the covers, use an unfixable top-level key like Ruff:

{
    "unfixable": {
        "no-console": true, 
        "prefer-const": true
    },
}

That would allow us to introduce this as a non-breaking change while maintaining the better end-user experience.

[MorevM]

@nzakas Now I see your point, thanks.

If we compare something like [0, 'option'] with { severity: 0, options: ['option'] }, then yes - the second one uses about 10% more memory in my tests:

Memory usage (100_000 elements):
- First:   7143.99 KB
- Second:  7925.84 KB

That said, the options key can be omitted during normalization if there are no options.
So when comparing [0] with { severity: 0 }, I see the following:

Memory usage (100_000 elements):
- First:   6362.73 KB
- Second:  4019.59 KB

If we average it out for a typical config, I'd estimate around a 5-8% increase in memory usage.
Personally, I think that's a reasonable trade-off for extensibility - especially considering that we're not operating with 100_000 rules, but something closer to two orders of magnitude fewer.

*I'm not insisting on anything - just sharing my findings. It's up to the team to decide.

---

Regarding the top-level property proposal:

This was suggested in the original RFC, but after that suggestion, the discussion went away to the per-rule syntax as a more convenient option, even you agreed with it (perhaps without realizing at the time how much of a breaking change it would be).

Personally, I find the per-rule approach much more usable. It's really helpful to see a rule declaration and immediately know whether autofix is enabled or not, without needing to search through and reconcile separate pieces of config.

I can imagine myself eventually writing a wrapper function for config that supports the proposed per-rule syntax, which would internally transform full-form declarations into the current format - and then attach a top-level property if we go this way.

Could we maybe consider this logic as part of core, so users can write config in the new style while rules are still stored and exposed as they are today? Maybe inside defineConfig() for instance?
I really want to support this kind of "syntax sugar" even if we decide not to normalize rule configs in core.

[nzakas]

This isn't a conversation about whether an array or an object takes up more memory. The conversation is about having just an array (as we do today after normalization) vs. having an array AND an object for each rule. That's what we'd end up with following the current state of the RFC. That's what I don't want.

Could we maybe consider this logic as part of core, so users can write config in the new style while rules are still stored and exposed as they are today? Maybe inside defineConfig() for instance?

Please re-read my previous comment. This is exactly what I proposed. 😄

[MorevM]

@nzakas
I saw the top-level property in the code sample and that was enough to second guess everything else :)
That's a bit embarrassing, sorry for that.

I'm actually not against it, but would we then provide this property for configuration explicitly?

// Which of the declarations should take precedence?
// It is not at all obvious to me how we should proceed in this case.

export default defineConfig([
  {
    autofix: {
      'eqeqeq': false,
    },
    rules: {
      'eqeqeq': { 
        severity: 'error', 
        autofix: true,
      },
    },
  },
]);

I would try to avoid this question because multiple sources of truth introduce too much confusion.

Either don't provide the top-level autofix/unfixable property for end users, limit the feature to full form of rule record and implicitly "pop" these things to the top-level internally.

export default defineConfig([
  {
    autofix: { // this will be omitted as if it doesn't exist
      'no-console': false,
    },
    rules: {
      'no-console': {  // this is the only way to use the feature
        severity: 'error', 
        autofix: true,
      },
    },
  },
]);

The current public API for getting the config will return something like this (this is also the form in which we store data in the core after merging and normalization):

{
  "rules": { // current API is unchanged
    "no-console": [0]
  },
  "autofix": { // but third-party tools can take account of new feature
    "no-console": true
  }
}

However, this will restrict users who do not use defineConfig from using the feature, which doesn't seem right (is there any reason NOT to use it?).

Either expose top-level property AND object form of rule record, and form rules for merging, but I would expect that whatever solution is adopted, someone will find it wrong. Two sources of truth doesn't seem like the right solution.

[nzakas]

My thought is that we could disallow the top-level autofix in defineConfig(). It would be a bit weird to have something in the underlying config not supported by defineConfig(), but oh well. That's what abstractions are for. We could always add it later if people come clamoring for it.

I'm not too concerned about users who choose not to use defineConfig(). They're opting-in to the less user-friendly approach and all that entails.