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

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

Closed
wants to merge 11 commits into from
34 changes: 34 additions & 0 deletions lib/rules/no-else-return.js
Expand Up @@ -12,6 +12,35 @@
const astUtils = require("../util/ast-utils");
const FixTracker = require("../util/fix-tracker");

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------

const BLOCK_SCOPED_DECLARATION = /^(?:VariableDeclarator|ClassDeclaration)$/;

/**
*
* @param {*} scope current scope
* @param {Token} startToken s
* @param {Token} endToken s
* @returns {boolean} s
Copy link
Member

Choose a reason for hiding this comment

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

Could you please fill in these comments (this line, and the two lines above it)?

*/
function blockScopeDeclaration(scope, startToken, endToken) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused by this function name. What are we checking here? It looks like we might be trying to check if an else block has any block scope declarations?

I would suggest prefixing the function name with "is" or "has", if it is returning a boolean.

Copy link
Author

Choose a reason for hiding this comment

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

I have renamed it.
Yes, I'm checking here if else block has any scope declarations,


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


//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -88,6 +117,11 @@ module.exports = {
}

const endToken = sourceCode.getLastToken(node);

if (blockScopeDeclaration(context.getScope(), startToken, endToken)) {
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
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" }
]
}
]
});