eslint
Help us know when we have conflicting rules: 10 passes should throw an error
ESLint version
v8.5.0
What problem do you want to solve?
Between eslint, prettier, import, and @typescript/eslint, it's really easy to accidentally include rules with fixes that overlap or conflict with other rules we have enabled.
This can cause eslint --fix significantly longer to run as keeps re-fixing the same files up to 10 times.
It might also means users are not necessarily getting the output they desired if the wrong rule is the last to run in that loop of 10 attempts.
What do you think is the correct solution?
If eslint --fix needs to do 10 passes, then throw error and say which plugins are possibly conflicting.
- Baseline: Throw an error and say which plugins are possibly conflicting.
- Even nicer: Show the rule names and their options.
- Even more helpful: Show the HTML after each pass so we can visually see what's going on.
- Super helpful but not necessary: Show the differences as a diff.
- Might be nice: In the fix loops, cache the previous versions, so we can detect as soon as we see a repeating pattern.
Participation
- [X] I am willing to submit a pull request for this change.
Additional comments
This is the code where I am seeing the 10 passes not be treated as an error: https://github.com/eslint/eslint/blob/main/lib/linter/linter.js#L2079-L2105
Also, I'm not sure if this is considered a breaking change, so it might need to wait for v9. Maybe a compromise is to warn in v8 but throw an error in v9?
I've made contributions to open source projects, but not eslint. I'm not sure if this is a good first issue, but I'm happy to try.
Thanks for reporting this. Can you give us some more context? How many files are you running on?
Of the plugins you mentioned, I'd guess that Prettier is the culprit because it casts such a wide net over the file.
If eslint --fix needs to do 10 passes, then throw error and say which plugins are possibly conflicting.
I'm not sure I understand what you're asking for here. What type of output would you be expecting?
In any case, I don't believe that throwing an error is the correct default behavior. If you're lint-fixing 100 files and one of them goes through ten passes, that doesn't seem like an error case to me. Do you really want to throw away all of the other results in that case? Maybe what you'd rather have is a way to debug when ESLint is running slowly?
Hi @nzakas, thanks for the quick response and helpful questions!
Culprit
The culprit when I created the issue was two conflicting rules both from the import plugin, caused by a a bug with prefer-inline.
The repo that I noticed the slow linting had this happen in about about 7000 files, and it was happening in nearly all of them (our React patterns monorepo).
Debugging the issue
I was using DEBUG="eslint:linter" but the output was verbose for me, especially with that many files, so it was hard to notice the pattern of repeating passes. I stopped using DEBUG and just added my own console logs that included the filename, which plugin caused the fix, and the changed code. The DEBUG made it easy to figure out where I should look to add my own console logs.
What output would I expect?
Ideally, something like this, though the fix comparison might be impractical or need to be trimmed to make sure it's readable.
Error: Conflicting rules are causing a potentially infinite loop while fixing the code:
Original code:
import type React from 'react';
import { useId, useState } from 'react';
Fix from: import/no-duplicates:
import type React, { useId , useState } from 'react';
Fix from: import/consistent-type-specifier-style:
import type React from 'react';
import { useId, useState } from 'react';
This was stopped after repeating 10 times in: monorepo-code/packages/ui/src/code/Code.tsx
To remediate this, disable one or more of the rules.
Also, maybe it doesn't need to throw a fatal error, but as a lint error, so this can be visible in IDE's.
Thanks, that's very helpful. We are somewhat limited in what we can output, as we either need to throw an error and basically throw away all linting results, or we need to fit the message in with the rest of the linting results so it can be formatted correctly. Those are the only ways that the results will be visible in both an IDE and on the command line.
So, I'm thinking we may need some other CLI flag to enable collecting and monitoring the extra data and then outputting a report.
Would it be possible for you to create a minimal repro case that we could look at? I'd like to see how you got stuck in this loop to see what kind of state it got stuck in. I'm guessing that two rules were conflicting such that if you autofix one then the other flags a violation and autofixes that, but it would be helpful to see an actual example.
Hi @nzakas - Here's a working reproduction that you can test in your browser.
https://stackblitz.com/edit/node-ugx5i6?file=module.ts,.eslintrc.js,index.tsx
Run yarn lint or npm run lint in the terminal to see the results.
Results from terminal
~/projects/node-ugx5i6
❯ npm install
added 213 packages in 2s
89 packages are looking for funding
run `npm fund` for details
~/projects/node-ugx5i6 2s
❯ npm run lint
> [email protected] lint
> DEBUG=eslint:linter eslint . --fix
eslint:linter Linting code for /home/projects/node-ugx5i6/index.tsx (pass 1) +0ms
eslint:linter Verify +0ms
eslint:linter With ConfigArray: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing: /home/projects/node-ugx5i6/index.tsx +2ms
eslint:linter Parsing successful: /home/projects/node-ugx5i6/index.tsx +748ms
eslint:linter Scope analysis: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Scope analysis successful: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Generating fixed text for /home/projects/node-ugx5i6/index.tsx (pass 1) +108ms
eslint:linter Linting code for /home/projects/node-ugx5i6/index.tsx (pass 2) +0ms
eslint:linter Verify +0ms
eslint:linter With ConfigArray: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing successful: /home/projects/node-ugx5i6/index.tsx +13ms
eslint:linter Scope analysis: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Scope analysis successful: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Generating fixed text for /home/projects/node-ugx5i6/index.tsx (pass 2) +39ms
eslint:linter Linting code for /home/projects/node-ugx5i6/index.tsx (pass 3) +0ms
eslint:linter Verify +0ms
eslint:linter With ConfigArray: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing successful: /home/projects/node-ugx5i6/index.tsx +9ms
eslint:linter Scope analysis: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Scope analysis successful: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Generating fixed text for /home/projects/node-ugx5i6/index.tsx (pass 3) +32ms
eslint:linter Linting code for /home/projects/node-ugx5i6/index.tsx (pass 4) +0ms
eslint:linter Verify +0ms
eslint:linter With ConfigArray: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing successful: /home/projects/node-ugx5i6/index.tsx +8ms
eslint:linter Scope analysis: /home/projects/node-ugx5i6/index.tsx +1ms
eslint:linter Scope analysis successful: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Generating fixed text for /home/projects/node-ugx5i6/index.tsx (pass 4) +32ms
eslint:linter Linting code for /home/projects/node-ugx5i6/index.tsx (pass 5) +0ms
eslint:linter Verify +0ms
eslint:linter With ConfigArray: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing successful: /home/projects/node-ugx5i6/index.tsx +11ms
eslint:linter Scope analysis: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Scope analysis successful: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Generating fixed text for /home/projects/node-ugx5i6/index.tsx (pass 5) +40ms
eslint:linter Linting code for /home/projects/node-ugx5i6/index.tsx (pass 6) +0ms
eslint:linter Verify +0ms
eslint:linter With ConfigArray: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing successful: /home/projects/node-ugx5i6/index.tsx +10ms
eslint:linter Scope analysis: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Scope analysis successful: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Generating fixed text for /home/projects/node-ugx5i6/index.tsx (pass 6) +31ms
eslint:linter Linting code for /home/projects/node-ugx5i6/index.tsx (pass 7) +0ms
eslint:linter Verify +0ms
eslint:linter With ConfigArray: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing successful: /home/projects/node-ugx5i6/index.tsx +9ms
eslint:linter Scope analysis: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Scope analysis successful: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Generating fixed text for /home/projects/node-ugx5i6/index.tsx (pass 7) +28ms
eslint:linter Linting code for /home/projects/node-ugx5i6/index.tsx (pass 8) +0ms
eslint:linter Verify +0ms
eslint:linter With ConfigArray: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing successful: /home/projects/node-ugx5i6/index.tsx +7ms
eslint:linter Scope analysis: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Scope analysis successful: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Generating fixed text for /home/projects/node-ugx5i6/index.tsx (pass 8) +24ms
eslint:linter Linting code for /home/projects/node-ugx5i6/index.tsx (pass 9) +0ms
eslint:linter Verify +0ms
eslint:linter With ConfigArray: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing successful: /home/projects/node-ugx5i6/index.tsx +8ms
eslint:linter Scope analysis: /home/projects/node-ugx5i6/index.tsx +1ms
eslint:linter Scope analysis successful: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Generating fixed text for /home/projects/node-ugx5i6/index.tsx (pass 9) +24ms
eslint:linter Linting code for /home/projects/node-ugx5i6/index.tsx (pass 10) +0ms
eslint:linter Verify +0ms
eslint:linter With ConfigArray: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing successful: /home/projects/node-ugx5i6/index.tsx +7ms
eslint:linter Scope analysis: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Scope analysis successful: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Generating fixed text for /home/projects/node-ugx5i6/index.tsx (pass 10) +23ms
eslint:linter Verify +0ms
eslint:linter With ConfigArray: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Parsing successful: /home/projects/node-ugx5i6/index.tsx +6ms
eslint:linter Scope analysis: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Scope analysis successful: /home/projects/node-ugx5i6/index.tsx +0ms
eslint:linter Linting code for /home/projects/node-ugx5i6/module.ts (pass 1) +22ms
eslint:linter Verify +0ms
eslint:linter With ConfigArray: /home/projects/node-ugx5i6/module.ts +0ms
eslint:linter Parsing: /home/projects/node-ugx5i6/module.ts +0ms
eslint:linter Parsing successful: /home/projects/node-ugx5i6/module.ts +3ms
eslint:linter Scope analysis: /home/projects/node-ugx5i6/module.ts +0ms
eslint:linter Scope analysis successful: /home/projects/node-ugx5i6/module.ts +0ms
eslint:linter Generating fixed text for /home/projects/node-ugx5i6/module.ts (pass 1) +4ms
/home/projects/node-ugx5i6/index.tsx
10:1 error Import "Example" is only used as types @typescript-eslint/consistent-type-imports
✖ 1 problem (1 error, 0 warnings)
1 error and 0 warnings potentially fixable with the `--fix` option.
Thank you! I'll take a look.
I'm also wondering at this point: would it be better to just detect that we are repeating fixes and abort before we get to 10, rather than trying to debug exactly which rules are conflicting? It seems like we could keep a record of the fixed versions and then if the fixed version after run n is the same as after run n-2, we should just stop.
I appreciate your idea because I'm guessing most eslint users are not in charge of their rules, so this will speed up their workflow without them having to do any work. Maybe, say, 10 passes to maybe just 3.
Perhaps a compromise is to use your idea, but, if possible, put a message in the console about the conflicting rules when eslint is complete. Like when Jest is done and is does a warning message like:
Jest did not exit one second after the test run has completed.
This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with
--detectOpenHandlesto troubleshoot this issue.
Of course this only helps when running on the terminal or when somebody looks at a CI log, but maybe that will hit the right audience: those who manage the rules, and those looking to speed up their eslint checks.
Cool, I'll mark this as accepted so we can see about aborting early if the output is remaining the same. I think that will address the problem sufficiently for most people.
Tracking conflicting rules is another level of complexity that I'm honestly not sure is worth the work. We'd need to keep at least three passes worth of violation data in order to do that which could cause memory issues in large projects, never mind calculating the overlaps between the violations to find the exact rules.
My theory is that knowing which rules are conflicting is actually not important because the primary concern is the amount of time ESLint is taking.
How about using rule metadata for this purpose? I could imagine two additional fields in the metadata for conflicting rules:
-
conflictsWith: array of rule ids for which the rule conflicts. Should be a configuration error. Useful for "extension" rules for TypeScript and Vue ("extension" rules use a base rule and adapt for the language, e.g. support other node types insemi). Also helpful for formatting rules. -
fixConflictsWith: if both rules create fixes only apply one
The fixConflictsWith does not really help as the same problem arises, when the user initiates the next fix but there is no longer unnecessary work being done. Another issue is the rule id as with flat configs there no longer is a static plugin id and therefore no static rule id.
I'm not sure if aborting early without any additional info for the user would be helpful. It reduces the amount of time ESLint takes when there are conflicting rules in the config, but the problem with conflicting rules in the config remains and must be fixed because otherwise there would always be some lint errors in the code, and when it's fixed the slow linting problem will just disappear.
@DMartens it sounds like you're suggesting that a rule definition should declare which rules it conflicts with? If so, that solution doesn't work. Core rules, by definition, never conflict with each other, and those are the only rules we can ever reliably document as conflicting with one another. With a huge ecosystem of custom rules, there's just no way to document every rule that conflicts with every other rule.
@mdjermanovic I'm not sure why that's a problem. Autofix isn't guaranteed to fix every issue, and stopping early will still report any remaining rule violations that you need to handle on your own. Rule conflicts that you can't figure out on your own seem like a pretty extreme edge case to me, so I'm not sure it's worth the extra work to track fixes through multiple rounds to try and identify possible conflicts.
@mdjermanovic I'm not sure why that's a problem. Autofix isn't guaranteed to fix every issue, and stopping early will still report any remaining rule violations that you need to handle on your own. Rule conflicts that you can't figure out on your own seem like a pretty extreme edge case to me, so I'm not sure it's worth the extra work to track fixes through multiple rounds to try and identify possible conflicts.
I didn't suggest that we identify possible conflicts (which would be nice, but I agree that it might not be worth the extra effort). I only think that stopping early alone doesn't bring any significant benefits because it just improves performance in an edge case that is invalid by itself and should be fixed by updating the configuration to remove conflicting rules, so I'm not sure if it's worth the effort either.
@nzakas Yes that is what I am suggesting. My idea was that core rules should not add this but as I said "extension" rules from e.g. TypeScript. In other words core rules would not change and there is no increased maintenance burden for ESLint as the whole data would be provided by plugin developers. Further users could raise an issue for the specific plugin when conflicting rules are detected. In the case that two custom rules conflict with each other, only one rule needs to have the metadata. The goal is also not to document every possible rule combination, but to give plugin developers a way to express it.
@mdjermanovic Yes you are right that fixConflictsWith does not really help. But the conflictsWith in the proposal would generate a configuration warning (and as such must be fixed).
Okay, if stopping alone isn't enough, and we won't be displaying which rules conflict, what do you suggest?
My proposal is one potential solution to this problem but also catches other configuration problems.
As such it is not limited to this fixing problem.
When the configuration is validated (with other checks like can the rule be resolved or are the rule options valid), I propose to add another check based on the rule metadata conflictsWith and print an error of conflicting rules.
The user would have to manually fix this problem (delete one rule configuration) and because the conflicting rules are never run at the same time, this fixing problem is also solved.
@DMartens I'm sorry, I was actually addressing @mdjermanovic with my comment. I think your comment posted just before I hit the button. :)
I don't think a solution that requires constant updating of metadata is a good idea. We need something that works automatically.
Okay, if stopping alone isn't enough, and we won't be displaying which rules conflict, what do you suggest?
I'd suggest either no change, because the presence of any fixable errors/warnings after running ESLint with --fix is already an indicator that there are conflicting rules, or perhaps we could add a lint warning that there are conflicts between the rules when we detect that the source code is the same as in any of the previous iterations, for the case that users may not be aware of the problem.
As for detecting which rules are causing conflicts, I agree that it doesn't seem worth the complexity. I believe It can be easily figured out in IDEs that allow applying individual fixes (VS Code, for example).
I'd suggest either no change, because the presence of any fixable errors/warnings after running ESLint with --fix is already an indicator that there are conflicting rules, or perhaps we could add a lint warning that there are conflicts between the rules when we detect that the source code is the same as in any of the previous iterations, for the case that users may not be aware of the problem.
By no change, does that mean you'd agree with me that we can just short-circuit the fix passes without printing any additional info to the console?
By no change, does that mean you'd agree with me that we can just short-circuit the fix passes without printing any additional info to the console?
We could do that, but I'm not sure if optimizing the execution for conflicting configurations that should be fixed anyway is worth the effort and the added complexity.
My theory is that knowing which rules are conflicting is actually not important because the primary concern is the amount of time ESLint is taking
I don't think the primary concern is performance, but the configuration that should be fixed by removing or reconfiguring conflicting rules.
By no change, does that mean you'd agree with me that we can just short-circuit the fix passes without printing any additional info to the console?
We could do that, but I'm not sure if optimizing the execution for conflicting configurations that should be fixed anyway is worth the effort and the added complexity.
I don't think this is complex, all we'd need to do is just check to see if the result of fix pass n matches the result of fix pass n-2 and abort if so. That seems like a low level of complexity to save folks some time.
My theory is that knowing which rules are conflicting is actually not important because the primary concern is the amount of time ESLint is taking
I don't think the primary concern is performance, but the configuration that should be fixed by removing or reconfiguring conflicting rules.
The point of this issue was to improve performance, so I believe this is the primary concern. If we can't do any further fixes after pass 3 but we continue to do all 11 passes, that really adds up across a codebase.
As I already said (and it seemed like you agreed?), there is no simple way for us to detect when two rules conflict. Going down that path seems like a lot of work for very little benefit, whereas the performance improvement of cutting off fixes when we detect we're going in circles is a tangible benefit.
The point of this issue was to improve performance, so I believe this is the primary concern.
I think this is where our opinions on the problem (and therefore the possible solution) differ. My understanding is that the point of the issue is to provide help in fixing the configuration, or at least a hint that the configuration has conflicting rules, while performance is a secondary concern because the goal should be to fix the configuration and once the configuration is fixed the performance problem goes away. I don't think users would want to keep a configuration with conflicting rules even if we provide better autofix performance in such cases, because conflicting rules cause the codebase to be permanently in a lint-incorrect state.
I'm going off the original comment:
This can cause eslint --fix significantly longer to run as keeps re-fixing the same files up to 10 times.
As I mentioned, i think this is easily fixable by short-circuiting the fix runs if we end up with the same output more twice.
What you are talking about, alerting people that they have conflicting rules, seems like a much more difficult problem to solve. If you want to solve it, I think that's fine, but I haven't seen any suggestion of how that might be done in an efficient way.
I also don't think that negates the benefits of short-circuiting fix passes that aren't doing anything useful.
I should note that if you currently have rules with conflicting or broken autofixes (but where you can manually arrange your code to satisfy both), you can disable some autofixes using https://www.npmjs.com/package/eslint-plugin-no-autofix . Although I'd prefer seeing that feature be part of the core ESLint tool, as eslint-plugin-no-autofix isn't a perfect solution and doesn't work 100% of the time.
@Samuel-Therrien-Beslogic I don't think this is about conflicting autofixes but about conflicting rules, in which case disabling one of the autofixes wouldn't help because one of the rules should be disabled entirely.
@nzakas
What you are talking about, alerting people that they have conflicting rules, seems like a much more difficult problem to solve. If you want to solve it, I think that's fine, but I haven't seen any suggestion of how that might be done in an efficient way.
I suggest only adding a lint warning saying that autofixing is going in circles possibly because of conflicting rules.
Otherwise, I don't think that just optimizing performance for configurations with conflicting rules justifies the added complexity. Because those configurations need to be fixed anyway.
I suggest only adding a lint warning saying that autofixing is going in circles possibly because of conflicting rules.
Otherwise, I don't think that just optimizing performance for configurations with conflicting rules justifies the added complexity. Because those configurations need to be fixed anyway.
I feel like we are going in circles here. What I'm suggesting is that we check to see if a previous autofix run results in the same output as a current autofix run and just exit. I don't want to message the user about how many autofix cycles have been done because, as we always say, this is an implementation detail and not something the user should be concerned about.
What is it, exactly, that you're suggesting we do?