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
Fix: prefer-const produces invalid autofix (fixes #11699) #11827
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for one suggestion to avoid needlessly breaking Typescript. Thanks!
lib/rules/prefer-const.js
Outdated
(node.parent.type === "ForInStatement" || node.parent.type === "ForOfStatement" || | ||
node.declarations.every(declaration => declaration.init)); | ||
|
||
const declarationKindToken = sourceCode.getFirstToken(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be better to use a predicate here, in case of Typescript declare let
or other potential token prefixes:
const declarationKindToken = sourceCode.getFirstToken(node); | |
const declarationKindToken = sourceCode.getFirstToken(node, t => t.value === node.kind); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this, it was inherited from checkGroup
, as well as the autofix for each variable which isn't necessary now when we have the whole declaration.
Speaking of TypeScript, it seems that @typescript-eslint/parser
allows let let = 1
, so one fix without the other might produce changing several tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fixed now within the second commit (multiple autofixes are also removed).
While it is most likely good to support potential prefixes, this particular example shouldn't produce a problem (like #11441), because only initialized variables are auto-fixed and the following is not a valid TypeScript:
declare let x = 1; // tsc error `Initializers are not allowed in ambient contexts`
Nevertheless, @typescript-eslint/parser
treats this as a valid code, so the autofix without this correction would indeed replace declare
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for nice refactoring.
As a non-blocking suggestion, for-of
instead of forEach()
is nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! (Sorry for letting this one sit.)
Actually, I believe the refactoring is ** not good **, please do not merge this fix yet. Although the tests are passing, after rereading the original code I realized I didn't fully understand intended functionality, in particular cases when destructuring happens outside of the declaration. For instance, with the option let a;
let b;
({a, b} = {});
b = 1; After this refactoring, this code would be incorrect. |
Hi @mdjermanovic, based on your last comment I've edited "WIP:" into the PR title. This helps our PR checks to show that there's more to do before we can merge. When you're ready for us to review and merge, simply edit the PR title to remove the WIP prefix. Thanks for your diligence, we really appreciate it! |
I rolled back the original code and made a small fix just to address the original issue + the TypeScript fix. As a summary, with this PR the following is done:
If you agree, this could close the original issue and I can open a new one:
|
Just a note that this is ready for a new review, and that the existing 2 approvals are no longer valid since it's a completely different code now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
What is the purpose of this pull request? (put an "X" next to item)
[X] Bug fix
issue #11699
What changes did you make? (Give an overview)
Fixed the autofix for the following cases:
by moving and modifying logic from
checkGroup()
toVariableDeclaration()
Is there anything you'd like reviewers to focus on?