eslint
eslint copied to clipboard
feat: circular autofix/conflicting rules detection
Prerequisites checklist
- [x] I have read the contributing guidelines.
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update [ ] Bug fix (template) [ ] New rule (template) [ ] Changes an existing rule (template) [ ] Add autofix to a rule [ ] Add a CLI option [ ] Add something to the core [x] Other, please explain: #17609
What changes did you make? (Give an overview)
This addresses an issue with ESLint's autofix mechanism, where it could enter a loop in cases of conflicting rules/circular fixes (e.g., alternating fixes that add and remove the same text).
The fix ensures that the verifyAndFix method detects circular fixes by comparing the output of the current fix pass to the output two passes ago. If a circular pattern is detected, the autofix process halts and shows a warning.
Both the Flat Config and legacy Config tests are updated to handle this.
Fixes #17609.
Is there anything you'd like reviewers to focus on?
-
Review how the circular fix warning is handled. I implemented it using
process.emitWarningsimilar to @snitin315's suggestion. This includes emitting a warning and adding adebuglog message. Let me know if this aligns with ESLint's best practices. -
Verify the new test cases added under the
verifyAndFixnamespace for both Flat Config (FlatConfigArray) and legacy Config setups.
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: Adrastopoulos / name: Gabriel Hall (87b6891c174b4946e5bd93a4f8354e984d56efaa, 54194eb7c2017fc376dd92c73f4a353f3ae49775, 84f23d151c34d50c146baed69264a06eeb0509b7, fa98be624cc5fd5bc6fcf63c2916610db8ad5182, 8b70eed2b8a7cfb5f0147a866f6a425c0b350385, 4ef870a75c500016554781cb5ab754c0a7984df8, 643fe5ed7654acd4f02655ec631a15d4403b5ccc)
Hi @Adrastopoulos!, 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
Deploy Preview for docs-eslint ready!
| Name | Link |
|---|---|
| Latest commit | 643fe5ed7654acd4f02655ec631a15d4403b5ccc |
| Latest deploy log | https://app.netlify.com/sites/docs-eslint/deploys/67c7c13e864a740008880604 |
| Deploy Preview | https://deploy-preview-19236--docs-eslint.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Have you considered (or should we consider) cycles of length greater than 2? (why not just keep a set of all previously seen results?)
Have you considered (or should we consider) cycles of length greater than 2? (why not just keep a set of all previously seen results? (or the hash of every result, if the lengths are too long))
If this is the case, we could use a set to track all previous results rather than only checking n vs n-2, terminating when the set size does not increase.
Keeping a set of all previously seen results increases memory usage. I don't think there's a good reason to keep more than two based on our discussion in the original issue.
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.
Hi @Adrastopoulos, there are some suggestions you can check out.
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.
@Adrastopoulos are you still working on this?
@Adrastopoulos are you still working on this?
I'll have some time to look at this this week. School started up and things have been hectic.
Sounds good. Let us know if you'd like help finishing this up.
I've addressed the feedback now. Assuming nothing else comes up, it will suffice to check if the warning was emitted in text to complete this PR. I am not sure how to implement that. @mdjermanovic any pointers?
Take a look at https://github.com/eslint/eslint/blob/main/tests/lib/cli.js. There are multiple tests in there that check for output from the CLI.
Take a look at https://github.com/eslint/eslint/blob/main/tests/lib/cli.js. There are multiple tests in there that check for output from the CLI.
I am stuck. I created a stub for the emitWarning method akin to cli.js:
let processStub;
beforeEach(() => {
sinon.restore();
processStub = sinon.stub(process, "emitWarning");
});
afterEach(() => {
processStub.restore();
});
But this stub is never being called in the test.
I am stuck. I created a stub for the emitWarning method akin to
cli.js:let processStub; beforeEach(() => { sinon.restore(); processStub = sinon.stub(process, "emitWarning"); }); afterEach(() => { processStub.restore(); });But this stub is never being called in the test.
Can you push the changes so we can figure out what's causing the problem?
Can you push the changes so we can figure out what's causing the problem?
Done.
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.
@Adrastopoulos there are a few more comments. Can you take a look?
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.
@Adrastopoulos would you like someone else to finish this work up?
@Adrastopoulos would you like someone else to finish this work up?
Yes. I'm unable to get the tests to capture the emitted warning. Perhaps someone more experienced can fix it up. Thanks everyone for being patient!
No worries. @mdjermanovic do you want to pick this up?
Yes, I'll take this.
Continued in https://github.com/eslint/eslint/pull/19514.