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: prevent fixing to incorrect code (fixes #11069) #11076
Changes from 7 commits
9a8d8d6
a640595
20f8998
28eb7f8
cc00be7
1945162
51ed101
62289ee
0772a0a
0b3a3a4
425f041
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,20 @@ | |
const astUtils = require("../util/ast-utils"); | ||
const FixTracker = require("../util/fix-tracker"); | ||
|
||
//------------------------------------------------------------------------------ | ||
// Helpers | ||
//------------------------------------------------------------------------------ | ||
|
||
/** | ||
* @param {Token} token - The token to check | ||
* @returns {boolean} `true` if keyword let or const exist | ||
*/ | ||
function isVariableDeclaration(token) { | ||
const variableDeclaration = token.value === "let" || token.value === "const"; | ||
|
||
return token.type === "Keyword" && variableDeclaration; | ||
} | ||
|
||
//------------------------------------------------------------------------------ | ||
// Rule Definition | ||
//------------------------------------------------------------------------------ | ||
|
@@ -88,6 +102,17 @@ module.exports = { | |
} | ||
|
||
const endToken = sourceCode.getLastToken(node); | ||
|
||
/** | ||
* If else block includes block scope local variable declaration [let or const], | ||
* then it is not safe to remove else keyword [issue 11069] | ||
*/ | ||
const isDeclarationInside = sourceCode.getTokensBetween(startToken, endToken).findIndex(isVariableDeclaration) > -1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: You could use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @platinumazure I've used .findIndex, because it will stop iteration if it will find an element and .some method will iterate through whole array, even it will find an element There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SerhiiBilyk Not right. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @g-plane you are right, thank you. I will change it! |
||
|
||
if (isDeclarationInside) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this will evaluate to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ajafff Does it mean, that if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Block scoped declaration" means let foo = 1;
function canNotBeFixed() {
if (condition) {
return foo;
} else {
let foo = 2; // else-block contains block scoped declaration, can not be fixed
console.log(foo);
}
}
function canBeFixed() {
if (condition) {
return foo;
} else {
{
let foo = 2; // block scoped declaration is not directly in else-block, but in a nested block
console.log(foo);
}
}
} |
||
return null; | ||
} | ||
|
||
const lastTokenOfElseBlock = sourceCode.getTokenBefore(endToken); | ||
|
||
if (lastTokenOfElseBlock.value !== ";") { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -182,6 +182,24 @@ ruleTester.run("no-else-return", rule, { | |
output: "function foo21() { var x = true; if (x) { return x; } if (x === false) { return false; } }", | ||
options: [{ allowElseIf: false }], | ||
errors: [{ messageId: "unexpected", type: "IfStatement" }] | ||
}, | ||
{ | ||
code: "function foo() { if (true) { return bar; } else { const baz = 1; return baz; } }", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this case cannot be fixed? IMHO, it seems safe to do the fix. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aladdin-add Does my test code correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This case should be fixable because the declaration in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be okay to just not autofix any block scope declarations to be safe, but I think this one could be autofixed since none of the declarations share a name with the variable in the immediate upper scope. |
||
output: null, | ||
parserOptions: { ecmaVersion: 6 }, | ||
errors: [{ messageId: "unexpected", type: "BlockStatement" }] | ||
}, | ||
{ | ||
code: "function foo() { if (true) { return bar; } else { let baz = 1; return baz; } }", | ||
output: null, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be okay to just not autofix any block scope declarations to be safe, but I think this one could be autofixed since none of the declarations share a name with the variable in the immediate upper scope. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @platinumazure , I'm sorry for the delay. I have started to work with this issue. |
||
parserOptions: { ecmaVersion: 6 }, | ||
errors: [{ messageId: "unexpected", type: "BlockStatement" }] | ||
}, | ||
{ | ||
code: "function foo() { let bar = true; if (baz) { return; } else { let bar = false; return; } }", | ||
captain-yossarian marked this conversation as resolved.
Show resolved
Hide resolved
|
||
output: null, | ||
parserOptions: { ecmaVersion: 6 }, | ||
errors: [{ messageId: "unexpected", type: "BlockStatement" }] | ||
} | ||
] | ||
}); |
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 safe to remove the
else
keyword. it's not safe to remove the Block of theelse
branch