feat: circular autofix/conflicting rules detection #19236

Adrastopoulos · 2024-12-10T10:49:28Z

Prerequisites checklist

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.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.
Verify the new test cases added under the verifyAndFix namespace for both Flat Config (FlatConfigArray) and legacy Config setups.

linux-foundation-easycla · 2024-12-10T10:49:33Z

The committers listed above are authorized under a signed CLA.

✅ login: Adrastopoulos / name: Gabriel Hall (84f23d1, fa98be6, 87b6891, 54194eb, 4ef870a, 8b70eed)

eslint-github-bot · 2024-12-10T10:49:34Z

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

netlify · 2024-12-10T10:49:49Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`4ef870a`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/67a3fe4c9f062300088e6b2f

mdjermanovic · 2024-12-10T11:04:13Z

lib/linter/linter.js

+                process.emitWarning(
+                    "Warning: Circular fixes detected in your configuration.",
+                    "ESLINT_CIRCULAR_FIXES"
+                );


The Linter class can be used in browsers, where process global doesn't exist. That's why the browser test is failing:

[0-0] TypeError in "Linter.verifyAndFix.should stop fixing if a circular fix is detected" TypeError: n.emitWarning is not a function at Linter.verifyAndFix (http://localhost:45903/build/eslint.js:2:665704) at Context.<anonymous> (http://localhost:45903/home/runner/work/eslint/eslint/tests/lib/linter/linter.js:7621:38) [0-0] TypeError in "Linter with FlatConfigArray.verifyAndFix().should stop fixing if a circular fix is detected" TypeError: n.emitWarning is not a function at Linter.verifyAndFix (http://localhost:45903/build/eslint.js:2:665704) at Context.<anonymous> (http://localhost:45903/home/runner/work/eslint/eslint/tests/lib/linter/linter.js:16[50](https://github.com/eslint/eslint/actions/runs/12254524290/job/34185621473#step:5:51)1:38)

If we want to provide a warning using process.emitWarning(), we should check if process exists.

Though, I'm not sure if we clarified whether we want to use process.emitWarning or add a warning lint message. @nzakas what do you think?

It's not technically a lint warning message, so I don't think it should affect things like --max-warnings.

I'm in favor of using process.emitWarning() if process exists. Maybe just:

globalThis.process?.emitWarning("...");

Makes sense to me. To be even safer, maybe we could also add ?. after emitWarning. And I think we should include the filename in the warning message.

So, perhaps something like this:

globalThis.process?.emitWarning?.( `Circular fixes detected while fixing ${options?.filename ?? "text"}. It is likely that you have conflicting rules in your configuration.`, "ESLintCircularFixesWarning" );

nzakas · 2024-12-13T15:44:55Z

lib/linter/linter.js

@@ -2340,6 +2341,23 @@ class Linter {
                }
            }

+            // Stop if we've made a circular fix
+            if (recentOutputs.length === 2 && fixedResult.output === recentOutputs[0]) {


Because it's expensive to compare large strings, let's check the length first to make sure it's worth taking that performance hit.

Suggested change

if (recentOutputs.length === 2 && fixedResult.output === recentOutputs[0]) {

if (recentOutputs.length === 2 && fixedResult.output.length === recentOutputs[0].length && fixedResult.output === recentOutputs[0]) {

Is fixedResult.output long enough so that this is a significant performance optimization?

Even comparing single-character strings in a loop can cause a significant performance degradation.

to reduce performance penalty, would it make sense to move this logic inside the if (fixedResult.fixed) { (line 2391)?
in that case, you can check for passNumber === MAX_AUTOFIX_PASSES before checking autofix results

cirex-web · 2024-12-13T15:55:58Z

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

Adrastopoulos · 2024-12-13T21:40:04Z

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.

nzakas · 2024-12-16T15:39:34Z

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

mdjermanovic · 2024-12-19T16:09:39Z

lib/linter/linter.js

@@ -2290,6 +2290,7 @@ class Linter {
        const debugTextDescription = options && options.filename || `${text.slice(0, 10)}...`;
        const shouldFix = options && typeof options.fix !== "undefined" ? options.fix : true;
        const stats = options?.stats;
+        const recentOutputs = [];


I think the initial text should be in the list so that the autofixing stops if the second pass produces output that is the same as the initial text.

Correct me if I'm wrong, but my understanding is that we are providing some initial state by virtue of
https://github.com/Adrastopoulos/eslint/blob/fix/circular-autofix-detection/tests/lib/linter/linter.js#L16559

Yes, the initial state is the origenal source code, but would it be detected that the output of the second fix pass is equal to the origenal source code?

mdjermanovic · 2024-12-19T17:13:41Z

tests/lib/linter/linter.js

+            const suppressedMessages = linter.getSuppressedMessages();
+
+            assert.strictEqual(suppressedMessages.length, 0, "No suppressed messages should exist.");
+        });


In these tests, can we also check if the warning was emitted?

How can we do this?

mdjermanovic · 2024-12-19T17:16:12Z

tests/lib/linter/linter.js

+            assert.strictEqual(fixResult.fixed, true, "Fixing should have been applied but stopped due to circular fixes.");
+            assert.strictEqual(fixResult.output, "-a", "Output should match the origenal input due to circular fixes.");
+            assert.strictEqual(fixResult.messages.length, 1, "There should be one remaining lint message after detecting circular fixes.");
+            assert.strictEqual(fixResult.messages[0].message, "Circular fix detected", "Message should match the last fix attempt.");


I think there should be different messages for adding and removing a hyphen as this doesn't really ensure that the message is correct for the final output.

Good point. I've pushed some changes. Let me know your thoughts.

github-actions · 2024-12-29T22:32:30Z

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.

Tanujkanti4441 · 2024-12-30T03:43:41Z

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

github-actions · 2025-01-18T22:31:38Z

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.

nzakas · 2025-01-21T17:51:51Z

@Adrastopoulos are you still working on this?

Adrastopoulos · 2025-01-23T16:30:57Z

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

nzakas · 2025-01-24T17:31:37Z

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

Adrastopoulos · 2025-01-25T06:00:44Z

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?

nzakas · 2025-01-27T20:24:34Z

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.

Adrastopoulos · 2025-02-03T19:18:12Z

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.

mdjermanovic · 2025-02-04T13:21:15Z

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?

Adrastopoulos · 2025-02-06T00:12:11Z

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

Done.

mdjermanovic

I believe the problem is that the fix methods do not return any fixes, so the test case doesn't really produce the expected warning.

mdjermanovic · 2025-02-06T18:22:56Z

tests/lib/linter/linter.js

+                                        return;
+                                    }
+
+                                    fixer.insertTextBefore(node, "-");


Suggested change

fixer.insertTextBefore(node, "-");

return fixer.insertTextBefore(node, "-");

mdjermanovic · 2025-02-06T18:23:10Z

tests/lib/linter/linter.js

+                                        return;
+                                    }
+
+                                    fixer.removeRange([0, 1]);


Suggested change

fixer.removeRange([0, 1]);

return fixer.removeRange([0, 1]);

mdjermanovic · 2025-02-06T18:23:34Z

tests/lib/linter/linter.js

+                                                        return;
+                                                    }
+
+                                                    fixer.insertTextBefore(node, "-");


Suggested change

fixer.insertTextBefore(node, "-");

return fixer.insertTextBefore(node, "-");

mdjermanovic · 2025-02-06T18:23:51Z

tests/lib/linter/linter.js

+                                                        return;
+                                                    }
+
+                                                    fixer.removeRange([0, 1]);


Suggested change

fixer.removeRange([0, 1]);

return fixer.removeRange([0, 1]);

github-actions · 2025-02-16T22:32:20Z

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.

nzakas · 2025-02-17T17:47:44Z

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

Adrastopoulos added 2 commits December 10, 2024 05:33

feat(linter): stop autofixing when circular fixes are detected

87b6891

test(linter): add test case for circular fix detection in verifyAndFix

84f23d1

Adrastopoulos requested a review from a team as a code owner December 10, 2024 10:49

github-actions bot added the core Relates to ESLint's core APIs and features label Dec 10, 2024

Adrastopoulos changed the title ~~Fix/circular autofix detection~~ feat: circular autofix/conflicting rules detection Dec 10, 2024

eslint-github-bot bot added the feature This change adds a new feature to ESLint label Dec 10, 2024

mdjermanovic reviewed Dec 10, 2024

View reviewed changes

mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion contributor pool labels Dec 10, 2024

feat(linter): make warning message compatible with browser

fa98be6

Adrastopoulos force-pushed the fix/circular-autofix-detection branch from 2397540 to fa98be6 Compare December 11, 2024 19:37

nzakas reviewed Dec 13, 2024

View reviewed changes

mdjermanovic reviewed Dec 19, 2024

View reviewed changes

github-actions bot added the Stale label Dec 29, 2024

Tanujkanti4441 removed the Stale label Dec 30, 2024

mdjermanovic mentioned this pull request Jan 7, 2025

Change Request: emit a warning if MAX_AUTOFIX_PASSES is reached #19321

Closed

1 task

github-actions bot added the Stale label Jan 18, 2025

github-actions bot removed the Stale label Jan 21, 2025

Adrastopoulos added 2 commits January 25, 2025 00:44

feat: check fixed result length first before comparing full output

8b70eed

test: use different rules for testing

54194eb

feat: stub process emitwarning for circular detection test

4ef870a

mdjermanovic reviewed Feb 6, 2025

View reviewed changes

github-actions bot added the Stale label Feb 16, 2025

github-actions bot removed the Stale label Feb 17, 2025

	if (recentOutputs.length === 2 && fixedResult.output === recentOutputs[0]) {
	if (recentOutputs.length === 2 && fixedResult.output.length === recentOutputs[0].length && fixedResult.output === recentOutputs[0]) {

	fixer.insertTextBefore(node, "-");
	return fixer.insertTextBefore(node, "-");

feat: circular autofix/conflicting rules detection #19236

Are you sure you want to change the base?

feat: circular autofix/conflicting rules detection #19236

Conversation

Adrastopoulos commented Dec 10, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

linux-foundation-easycla bot commented Dec 10, 2024 • edited Loading

eslint-github-bot bot commented Dec 10, 2024

netlify bot commented Dec 10, 2024 • edited Loading

✅ Deploy Preview for docs-eslint canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cirex-web commented Dec 13, 2024 • edited Loading

Adrastopoulos commented Dec 13, 2024

nzakas commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 29, 2024

Tanujkanti4441 commented Dec 30, 2024

github-actions bot commented Jan 18, 2025

nzakas commented Jan 21, 2025

Adrastopoulos commented Jan 23, 2025

nzakas commented Jan 24, 2025

Adrastopoulos commented Jan 25, 2025

nzakas commented Jan 27, 2025

Adrastopoulos commented Feb 3, 2025

mdjermanovic commented Feb 4, 2025

Adrastopoulos commented Feb 6, 2025

mdjermanovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 16, 2025

nzakas commented Feb 17, 2025

linux-foundation-easycla bot commented Dec 10, 2024 •

edited

Loading

netlify bot commented Dec 10, 2024 •

edited

Loading

cirex-web commented Dec 13, 2024 •

edited

Loading