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 10 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,13 @@ | |
const astUtils = require("../util/ast-utils"); | ||
const FixTracker = require("../util/fix-tracker"); | ||
|
||
//------------------------------------------------------------------------------ | ||
// Helpers | ||
//------------------------------------------------------------------------------ | ||
|
||
const BLOCK_SCOPED_DECLARATION = /^(?:VariableDeclarator|ClassDeclaration)$/; | ||
|
||
|
||
//------------------------------------------------------------------------------ | ||
// Rule Definition | ||
//------------------------------------------------------------------------------ | ||
|
@@ -88,6 +95,43 @@ module.exports = { | |
} | ||
|
||
const endToken = sourceCode.getLastToken(node); | ||
|
||
/** | ||
* Dig into `else` branch and receive all root level declarations. | ||
* Checks only if @var foo is block scope declaration, @not @var x | ||
* @example | ||
* function foo (){ | ||
* if(baz){ | ||
* return 42 | ||
* } else { | ||
* let foo=1; | ||
* { | ||
* const x=10; | ||
* } | ||
* return foo | ||
* } | ||
* } | ||
* | ||
*/ | ||
const blockScopeDeclaration = scope => { | ||
return scope.childScopes.some(childScope => { | ||
const { | ||
start, | ||
end | ||
} = childScope.block; | ||
|
||
const elseBranch = | ||
start === startToken.start && end === endToken.end; | ||
|
||
return elseBranch ? childScope.variables.some(variable => BLOCK_SCOPED_DECLARATION.test(variable.defs[0].node.type)) : false | ||
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. I don't know exactly how ESLint's scope handling works, but it should work if you only check 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. I agree that it's worth trying out this simplification, although I think the real problem is that we need to track all variables available in the containing scope and look for declarations with the same name (which are shadowing before the autofix, but redeclaring after). |
||
|
||
}); | ||
}; | ||
|
||
if (blockScopeDeclaration(context.getScope())) { | ||
return null; | ||
} | ||
|
||
const lastTokenOfElseBlock = sourceCode.getTokenBefore(endToken); | ||
|
||
if (lastTokenOfElseBlock.value !== ";") { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -182,6 +182,32 @@ 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" }] | ||
}, | ||
{ | ||
code: "function foo() { let bar = true; if (baz) { return baz; } else { { let bar = false; } return bar; } }", | ||
output: "function foo() { let bar = true; if (baz) { return baz; } { let bar = false; } return bar; }", | ||
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.
seems like there's a missing semicolon at the end of the line
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.
Please fix the missing semicolon on this line, as well as any other lint errors. Thanks!