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.

Sign up for GitHub

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

Jump to bottom

Fix: prevent fixing to incorrect code (fixes #11069) #11076

Closed
wants to merge 11 commits into from
Closed

Fix: prevent fixing to incorrect code (fixes #11069) #11076

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9a8d8d6
Fix: prevent fixing to incorrect code (fixes #11069)
captain-yossarian Nov 11, 2018
a640595
Fix: prevent fixing to incorrect code (fixes #11069)
captain-yossarian Nov 11, 2018
20f8998
Merge branch 'issue11069' of github.com:SerhiiBilyk/eslint into issue…
captain-yossarian Nov 11, 2018
28eb7f8
Fix: prevent fixing to incorrect code (fixes #11069)
captain-yossarian Nov 11, 2018
cc00be7
Fix: prevent fixing to incorrect code (fixes #11069)
captain-yossarian Nov 11, 2018
1945162
Add unit tests
captain-yossarian Nov 12, 2018
51ed101
Add specific test case
captain-yossarian Nov 12, 2018
62289ee
Add one more unit test
captain-yossarian Nov 19, 2018
0772a0a
Get all variables from childScope and compare them
captain-yossarian Dec 14, 2018
0b3a3a4
Compute block scope declaration
captain-yossarian Dec 27, 2018
425f041
Fix eslint errors
captain-yossarian Feb 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 44 additions & 0 deletions lib/rules/no-else-return.js
View file
Open in desktop
Expand Up @@ -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
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -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
Copy link
Contributor

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

Copy link
Member

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!

Copy link
Contributor

Choose a reason for hiding this comment

The 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 childScope.variables.length !== 0. That's because function scoped declarations like var or function are hoisted to the containing function scope.

Copy link
Member

Choose a reason for hiding this comment

The 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 !== ";") {
Expand Down
26 changes: 26 additions & 0 deletions tests/lib/rules/no-else-return.js
View file
Open in desktop
Expand Up @@ -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; } }",
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aladdin-add Does my test code correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case should be fixable because the declaration in the else block doesn't shadow any other variable.

Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

The 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.
What the expected output here ?

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" }
]
}
]
});