Skip to content

Commit

Permalink
Fix: Add fixer for no-else-return (fixes #7863)
Browse files Browse the repository at this point in the history
  • Loading branch information
Xander Dumaine committed Jan 19, 2017
1 parent ca1f841 commit 4be98de
Show file tree
Hide file tree
Showing 3 changed files with 179 additions and 41 deletions.
2 changes: 2 additions & 0 deletions docs/rules/no-else-return.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

If an `if` block contains a `return` statement, the `else` block becomes unnecessary. Its contents can be placed outside of the block.

(fixable) The `--fix` option on the [command line](../user-guide/command-line-interface#fix) automatically fixes problems reported by this rule.

```js
function foo() {
if (x) {
Expand Down
109 changes: 84 additions & 25 deletions lib/rules/no-else-return.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ module.exports = {
recommended: false
},

schema: []
schema: [],

fixable: "code"
},

create(context) {
Expand All @@ -33,7 +35,56 @@ module.exports = {
* @returns {void}
*/
function displayReport(node) {
context.report({ node, message: "Unnecessary 'else' after 'return'." });
context.report({
node,
message: "Unnecessary 'else' after 'return'.",
fix: fixer => {
const sourceCode = context.getSourceCode();
const alternateStartToken = sourceCode.getFirstToken(node.parent.alternate);
const elseToken = sourceCode.getTokenBefore(alternateStartToken);
const source = sourceCode.getText(node.parent.alternate);
const lastIfToken = sourceCode.getTokenBefore(elseToken);
let fixedSource, firstTokenOfElseBlock;

if (alternateStartToken.type === "Punctuator" && alternateStartToken.value === "{") {
firstTokenOfElseBlock = sourceCode.getTokenAfter(alternateStartToken);
} else {
firstTokenOfElseBlock = alternateStartToken;
}

// If the if block does not have curly braces and does not end in a semicolon
// and the else block starts with (, [, /, +, ` or -, then it is not
// safe to remove the else keyword, because ASI will not add a semicolon
// after the if block
const ifBlockMaybeUnsafe = node.parent.consequent.type !== "BlockStatement" && lastIfToken.value !== ";";
const elseBlockUnsafe = /^[([/+`-]/.test(firstTokenOfElseBlock.value);

if (ifBlockMaybeUnsafe && elseBlockUnsafe) {
return null;
}

const endToken = sourceCode.getLastToken(node);
const lastTokenOfElseBlock = sourceCode.getTokenBefore(endToken);

if (lastTokenOfElseBlock.value !== ";") {
const nextToken = sourceCode.getTokenAfter(endToken);

const nextTokenUnsafe = nextToken && /^[([/+`-]/.test(nextToken.value);
const nextTokenOnSameLine = nextToken && nextToken.loc.start.line === lastTokenOfElseBlock.loc.start.line;

if (nextTokenUnsafe || (nextTokenOnSameLine && nextToken.value !== "}")) {
return null;
}
}

if (alternateStartToken.type === "Punctuator" && alternateStartToken.value === "{") {
fixedSource = source.slice(1, -1);
} else {
fixedSource = source;
}
return fixer.replaceTextRange([elseToken.start, node.end], fixedSource);
}
});
}

/**
Expand Down Expand Up @@ -121,35 +172,43 @@ module.exports = {
return checkForReturnOrIf(node);
}

/**
* Check the if statement
* @returns {void}
* @param {Node} node The node for the if statement to check
* @private
*/
function IfStatement(node) {
const parent = context.getAncestors().pop();
let consequents,
alternate;

// Only "top-level" if statements are checked, meaning the first `if`
// in a `if-else-if-...` chain.
if (parent.type === "IfStatement" && parent.alternate === node) {
return;
}

for (consequents = []; node.type === "IfStatement"; node = node.alternate) {
if (!node.alternate) {
return;
}
consequents.push(node.consequent);
alternate = node.alternate;
}

if (consequents.every(alwaysReturns)) {
displayReport(alternate);
}
}

//--------------------------------------------------------------------------
// Public API
//--------------------------------------------------------------------------

return {

IfStatement(node) {
const parent = context.getAncestors().pop();
let consequents,
alternate;

// Only "top-level" if statements are checked, meaning the first `if`
// in a `if-else-if-...` chain.
if (parent.type === "IfStatement" && parent.alternate === node) {
return;
}

for (consequents = []; node.type === "IfStatement"; node = node.alternate) {
if (!node.alternate) {
return;
}
consequents.push(node.consequent);
alternate = node.alternate;
}

if (consequents.every(alwaysReturns)) {
displayReport(alternate);
}
}
"IfStatement:exit": IfStatement

};

Expand Down
109 changes: 93 additions & 16 deletions tests/lib/rules/no-else-return.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,23 +30,100 @@ ruleTester.run("no-else-return", rule, {
"if (0) { if (0) {} else {} } else {}"
],
invalid: [
{ code: "function foo() { if (true) { return x; } else { return y; } }", errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }] },
{ code: "function foo() { if (true) { var x = bar; return x; } else { var y = baz; return y; } }", errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }] },
{ code: "function foo() { if (true) return x; else return y; }", errors: [{ message: "Unnecessary 'else' after 'return'.", type: "ReturnStatement" }] },
{ code: "function foo() { if (true) { if (false) return x; else return y; } else { return z; } }", errors: [{ message: "Unnecessary 'else' after 'return'.", type: "ReturnStatement" }, { message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }] },
{ code: "function foo() { if (true) { if (false) { if (true) return x; else w = y; } else { w = x; } } else { return z; } }", errors: [{ message: "Unnecessary 'else' after 'return'.", type: "ExpressionStatement" }] },
{ code: "function foo() { if (true) { if (false) { if (true) return x; else return y; } } else { return z; } }", errors: [{ message: "Unnecessary 'else' after 'return'.", type: "ReturnStatement" }] },
{ code: "function foo() { if (true) { if (false) { if (true) return x; else return y; } return w; } else { return z; } }", errors: [
{ message: "Unnecessary 'else' after 'return'.", type: "ReturnStatement" },
{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }
] },
{ code: "function foo() { if (true) { if (false) { if (true) return x; else return y; } else { w = x; } } else { return z; } }", errors: [
{ message: "Unnecessary 'else' after 'return'.", type: "ReturnStatement" },
{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }
] },
{
code: "function foo() {if (x) { return true; } else if (y) { return true; } else { notAReturn(); } }",
{
code: "function foo1() { if (true) { return x; } else { return y; } }",
output: "function foo1() { if (true) { return x; } return y; }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]
},
{
code: "function foo2() { if (true) { var x = bar; return x; } else { var y = baz; return y; } }",
output: "function foo2() { if (true) { var x = bar; return x; } var y = baz; return y; }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]
},
{
code: "function foo3() { if (true) return x; else return y; }",
output: "function foo3() { if (true) return x; return y; }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "ReturnStatement" }] },
{
code: "function foo4() { if (true) { if (false) return x; else return y; } else { return z; } }",
output: "function foo4() { if (true) { if (false) return x; return y; } return z; }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "ReturnStatement" }, { message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]
},
{
code: "function foo5() { if (true) { if (false) { if (true) return x; else { w = y; } } else { w = x; } } else { return z; } }",
output: "function foo5() { if (true) { if (false) { if (true) return x; w = y; } else { w = x; } } else { return z; } }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]
},
{
code: "function foo6() { if (true) { if (false) { if (true) return x; else return y; } } else { return z; } }",
output: "function foo6() { if (true) { if (false) { if (true) return x; return y; } } else { return z; } }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "ReturnStatement" }]
},
{
code: "function foo7() { if (true) { if (false) { if (true) return x; else return y; } return w; } else { return z; } }",
output: "function foo7() { if (true) { if (false) { if (true) return x; return y; } return w; } return z; }",
errors: [
{ message: "Unnecessary 'else' after 'return'.", type: "ReturnStatement" },
{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }
]
},
{
code: "function foo8() { if (true) { if (false) { if (true) return x; else return y; } else { w = x; } } else { return z; } }",
output: "function foo8() { if (true) { if (false) { if (true) return x; return y; } w = x; } else { return z; } }",
errors: [
{ message: "Unnecessary 'else' after 'return'.", type: "ReturnStatement" },
{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }
]
},
{
code: "function foo9() {if (x) { return true; } else if (y) { return true; } else { notAReturn(); } }",
output: "function foo9() {if (x) { return true; } else if (y) { return true; } notAReturn(); }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]
},
{
code: "function foo10() { if (foo) return bar; else (foo).bar(); }",
output: "function foo10() { if (foo) return bar; (foo).bar(); }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "ExpressionStatement" }]
},
{
code: "function foo11() { if (foo) return bar \nelse { [1, 2, 3].map(foo) } }",
output: "function foo11() { if (foo) return bar \nelse { [1, 2, 3].map(foo) } }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]
},
{
code: "function foo12() { if (foo) return bar \nelse { baz() } \n[1, 2, 3].map(foo) }",
output: "function foo12() { if (foo) return bar \nelse { baz() } \n[1, 2, 3].map(foo) }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]
},
{
code: "function foo13() { if (foo) return bar; \nelse { [1, 2, 3].map(foo) } }",
output: "function foo13() { if (foo) return bar; \n [1, 2, 3].map(foo) }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]
},
{
code: "function foo14() { if (foo) return bar \nelse { baz(); } \n[1, 2, 3].map(foo) }",
output: "function foo14() { if (foo) return bar \n baz(); \n[1, 2, 3].map(foo) }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]
},
{
code: "function foo15() { if (foo) return bar; else { baz() } qaz() }",
output: "function foo15() { if (foo) return bar; else { baz() } qaz() }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]
},
{
code: "function foo16() { if (foo) return bar \nelse { baz() } qaz() }",
output: "function foo16() { if (foo) return bar \nelse { baz() } qaz() }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]
},
{
code: "function foo17() { if (foo) return bar \nelse { baz() } \nqaz() }",
output: "function foo17() { if (foo) return bar \n baz() \nqaz() }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]
},
{
code: "function foo18() { if (foo) return function() {} \nelse [1, 2, 3].map(bar) }",
output: "function foo18() { if (foo) return function() {} \nelse [1, 2, 3].map(bar) }",
errors: [{ message: "Unnecessary 'else' after 'return'.", type: "ExpressionStatement" }]
}
]
});

0 comments on commit 4be98de

Please sign in to comment.