eslint icon indicating copy to clipboard operation
eslint copied to clipboard

feat: circular autofix/conflicting rules detection

Open Adrastopoulos opened this issue 11 months ago • 6 comments

Prerequisites checklist

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?

  1. Review how the circular fix warning is handled. I implemented it using process.emitWarning similar to @snitin315's suggestion. This includes emitting a warning and adding a debug log message. Let me know if this aligns with ESLint's best practices.

  2. Verify the new test cases added under the verifyAndFix namespace for both Flat Config (FlatConfigArray) and legacy Config setups.

Adrastopoulos avatar Dec 10 '24 10:12 Adrastopoulos

CLA Signed

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

eslint-github-bot[bot] avatar Dec 10 '24 10:12 eslint-github-bot[bot]

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

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Dec 10 '24 10:12 netlify[bot]

Have you considered (or should we consider) cycles of length greater than 2? (why not just keep a set of all previously seen results?)

cirex-web avatar Dec 13 '24 15:12 cirex-web

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.

Adrastopoulos avatar Dec 13 '24 21:12 Adrastopoulos

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.

nzakas avatar Dec 16 '24 15:12 nzakas

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.

github-actions[bot] avatar Dec 29 '24 22:12 github-actions[bot]

Hi @Adrastopoulos, there are some suggestions you can check out.

Tanujkanti4441 avatar Dec 30 '24 03:12 Tanujkanti4441

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.

github-actions[bot] avatar Jan 18 '25 22:01 github-actions[bot]

@Adrastopoulos are you still working on this?

nzakas avatar Jan 21 '25 17:01 nzakas

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

Adrastopoulos avatar Jan 23 '25 16:01 Adrastopoulos

Sounds good. Let us know if you'd like help finishing this up.

nzakas avatar Jan 24 '25 17:01 nzakas

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?

Adrastopoulos avatar Jan 25 '25 06:01 Adrastopoulos

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.

nzakas avatar Jan 27 '25 20:01 nzakas

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.

Adrastopoulos avatar Feb 03 '25 19:02 Adrastopoulos

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?

mdjermanovic avatar Feb 04 '25 13:02 mdjermanovic

Can you push the changes so we can figure out what's causing the problem?

Done.

Adrastopoulos avatar Feb 06 '25 00:02 Adrastopoulos

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.

github-actions[bot] avatar Feb 16 '25 22:02 github-actions[bot]

@Adrastopoulos there are a few more comments. Can you take a look?

nzakas avatar Feb 17 '25 17:02 nzakas

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.

github-actions[bot] avatar Feb 27 '25 22:02 github-actions[bot]

@Adrastopoulos would you like someone else to finish this work up?

nzakas avatar Mar 03 '25 18:03 nzakas

@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!

Adrastopoulos avatar Mar 05 '25 03:03 Adrastopoulos

No worries. @mdjermanovic do you want to pick this up?

nzakas avatar Mar 10 '25 15:03 nzakas

Yes, I'll take this.

mdjermanovic avatar Mar 10 '25 15:03 mdjermanovic

Continued in https://github.com/eslint/eslint/pull/19514.

mdjermanovic avatar Mar 13 '25 20:03 mdjermanovic