Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule auto-fix variable name conflicts #13706

Closed
fisker opened this issue Sep 22, 2020 · 11 comments
Closed

Rule auto-fix variable name conflicts #13706

fisker opened this issue Sep 22, 2020 · 11 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion

Comments

@fisker
Copy link
Contributor

fisker commented Sep 22, 2020

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

@fisker fisker added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Sep 22, 2020
@fisker fisker changed the title Rule auto-fix conflicts Rule auto-fix variable name conflicts Sep 22, 2020
@mdjermanovic mdjermanovic added question This issue asks a question about ESLint and removed core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Sep 22, 2020
@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 22, 2020

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
Copy link
Contributor Author

fisker commented Sep 22, 2020

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

@fisker fisker closed this as completed Sep 22, 2020
@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 22, 2020

@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
Copy link
Contributor Author

fisker commented Sep 27, 2020

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
Copy link
Member

mdjermanovic commented Sep 27, 2020

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

@mdjermanovic mdjermanovic reopened this Sep 27, 2020
@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed question This issue asks a question about ESLint labels Sep 27, 2020
@fisker
Copy link
Contributor Author

fisker commented Sep 27, 2020

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

@nzakas
Copy link
Member

nzakas commented Oct 1, 2020

@mdjermanovic what is the expected next step here?

@mdjermanovic
Copy link
Member

mdjermanovic commented Oct 6, 2020

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
Copy link
Member

nzakas commented Oct 6, 2020

Nice! So can we close this issue?

@mdjermanovic
Copy link
Member

mdjermanovic commented Oct 10, 2020

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

@nzakas
Copy link
Member

nzakas commented Oct 13, 2020

All right, closing.

@nzakas nzakas closed this as completed Oct 13, 2020
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Apr 12, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion
Projects
None yet
Development

No branches or pull requests

3 participants