Rule auto-fix variable name conflicts

[fisker]

The version of ESLint you are using.

v7.7.0

The problem you want to solve.

I have this code

function foo() {
	try {
	} catch (err) {
		console.log(err);

		if (test) {
			throw a;
		} else {
			throw b;
		}
	}
}

We added a rule unicorn/catch-error-name with auto-fix, it will fix err to error.

function foo() {
	try {
	} catch (error) {
		console.log(error);

		if (test) {
			throw a;
		} else {
			throw b;
		}
	}
}

We are adding a new rule prefer-ternary, we plan to fix this code to

function foo() {
	try {
	} catch (err) {
		console.log(err);

		const error = test ? a : b;
		throw error;
	}
}

Here is the problem, when we check variable name error in prefer-ternary rule and catch-error-name, they will both show error is not used, but when apply fixes, there will be conflicts.

function foo() {
	try {
	} catch (error) {
		console.log(error);

		const error = test ? a : b; // <-- error is already used.
		throw error;
	}
}

Any solution/suggestion?

Your take on the correct solution to problem.

N/A

Are you willing to submit a pull request to implement this change?

N/A

[mdjermanovic]

Hi, @fisker!

When renaming a variable or introducing a new one, you could replace the whole range that represents the scope of that variable. Then, ESLint will not replace overlapping ranges in the same iteration (--fix runs up to 10 iterations), so the other rule, for which the fix was not applied in the first iteration, will become aware of the variable name conflicts in the next iteration.

As an example, some builtin rules use FixTracker helper to extend the range and thus make sure that nothing within the extended range can be fixed by other rules in the same iteration.

[fisker]

@mdjermanovic Thanks for this information, I know that overlapping ranges won't apply fixes from other rule, but never thought we can extend the range to prevent this conflicts, I'll try this solution.

[mdjermanovic]

@fisker maybe I relabeled this issue as a "question" too early. If you think that something could be added to ESLint core, we can certainly reopen this issue or create a new one with a concrete proposal. I think that FixTracker#retainRange can solve the problem you're encountering, but FixTracker isn't part of the public API.

There was a lengthy discussion about this kind of problems in #7928, and the solution at the time was to internally try out the approach with the FixTracker helper first, before maybe adding something to the public API (in fact, the initial version of #8101 had new methods on the fixer, like fixer.keepRange). Perhaps we could revisit that now.

[fisker]

maybe adding something to the public API (in fact, the initial version of #8101 had new methods on the fixer, like fixer.keepRange)

That would be great.

[mdjermanovic]

Reopening to discuss if we could add something to ESLint's public API.

@fisker in the meantime, I'd suggest you maintain your own FixTracker instead of using require('eslint/lib/rules/utils/fix-tracker') because it isn't a public API, so it could be changed in an incompatible way in any of the future ESLint versions (including non-major versions).

[fisker]

@mdjermanovic I understand that, but I'm going to use it directly for now.

[nzakas]

@mdjermanovic what is the expected next step here?

[mdjermanovic]

I reopened this issue to discuss if it would be useful to add something to public API (since FixTracker isn't public), but now I realized there's a simple way to extend the fixed range using the already existing features.

If the rule wants to fix something inside a node and make sure there will be no other fixes inside the node in the same pass, the fix() can just add empty strings around the node:

*fix(fixer) {

    // yield some fixes inside `node` ...

    // extend the range to `node`:
    yield fixer.insertTextBefore(node, "");
    yield fixer.insertTextAfter(node, "");
}

Then, the report translator will merge fixes and the unchanged code into one big fix having the range of node.

[nzakas]

Nice! So can we close this issue?

[mdjermanovic]

I added a test to confirm that this works in #13748, and I'd agree to close this issue.

[nzakas]

All right, closing.