From 9a8d8d6a292c4cf522d67b21d100bfd6bfd3d713 Mon Sep 17 00:00:00 2001 From: Serhjii Bilyk Date: Sun, 11 Nov 2018 17:48:24 +0100 Subject: [PATCH 1/8] Fix: prevent fixing to incorrect code (fixes #11069) --- lib/rules/no-else-return.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/lib/rules/no-else-return.js b/lib/rules/no-else-return.js index eebdec76e0e..44b5454c808 100644 --- a/lib/rules/no-else-return.js +++ b/lib/rules/no-else-return.js @@ -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; + + if (isDeclarationInside) { + return null; + } + const lastTokenOfElseBlock = sourceCode.getTokenBefore(endToken); if (lastTokenOfElseBlock.value !== ";") { From a640595595d10533db73183bb4d4426be2b2805f Mon Sep 17 00:00:00 2001 From: Serhii Bilyk Date: Sun, 11 Nov 2018 17:48:24 +0100 Subject: [PATCH 2/8] Fix: prevent fixing to incorrect code (fixes #11069) --- lib/rules/no-else-return.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/lib/rules/no-else-return.js b/lib/rules/no-else-return.js index eebdec76e0e..44b5454c808 100644 --- a/lib/rules/no-else-return.js +++ b/lib/rules/no-else-return.js @@ -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; + + if (isDeclarationInside) { + return null; + } + const lastTokenOfElseBlock = sourceCode.getTokenBefore(endToken); if (lastTokenOfElseBlock.value !== ";") { From 1945162630fd620d3cd276f13d1aeae433293abb Mon Sep 17 00:00:00 2001 From: Serhjii Bilyk Date: Mon, 12 Nov 2018 18:24:54 +0100 Subject: [PATCH 3/8] Add unit tests --- tests/lib/rules/no-else-return.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/lib/rules/no-else-return.js b/tests/lib/rules/no-else-return.js index 6111b55da65..4b5ef20510b 100644 --- a/tests/lib/rules/no-else-return.js +++ b/tests/lib/rules/no-else-return.js @@ -182,6 +182,18 @@ 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; } }", + 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, + parserOptions: { ecmaVersion: 6 }, + errors: [{ messageId: "unexpected", type: "BlockStatement" }] } ] }); From 51ed101f32a6e275109354043ff44a62410795f5 Mon Sep 17 00:00:00 2001 From: Serhjii Bilyk Date: Mon, 12 Nov 2018 20:25:08 +0100 Subject: [PATCH 4/8] Add specific test case --- tests/lib/rules/no-else-return.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/lib/rules/no-else-return.js b/tests/lib/rules/no-else-return.js index 4b5ef20510b..e1c9f6e38a0 100644 --- a/tests/lib/rules/no-else-return.js +++ b/tests/lib/rules/no-else-return.js @@ -194,6 +194,12 @@ ruleTester.run("no-else-return", rule, { output: null, parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "unexpected", type: "BlockStatement" }] + }, + { + code: "function foo() { let bar = true; if (baz) { return; } else { let bar = false; return; } }", + output: null, + parserOptions: { ecmaVersion: 6 }, + errors: [{ messageId: "unexpected", type: "BlockStatement" }] } ] }); From 62289ee740a3bd74654ba470106aeea53d0987dd Mon Sep 17 00:00:00 2001 From: Serhjii Bilyk Date: Mon, 19 Nov 2018 07:15:38 +0100 Subject: [PATCH 5/8] Add one more unit test --- tests/lib/rules/no-else-return.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/lib/rules/no-else-return.js b/tests/lib/rules/no-else-return.js index e1c9f6e38a0..6dd0add8ffd 100644 --- a/tests/lib/rules/no-else-return.js +++ b/tests/lib/rules/no-else-return.js @@ -200,6 +200,14 @@ ruleTester.run("no-else-return", rule, { output: null, parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "unexpected", type: "BlockStatement" }] + }, + { + code: "function foo() { let bar = true; if (baz) { return; } else { { let bar = false; } return; } }", + output: null, + parserOptions: { ecmaVersion: 6 }, + errors: [ + { messageId: "unexpected", type: "BlockStatement" } + ] } ] }); From 0772a0ab327562215579b78e339916aff7cba21f Mon Sep 17 00:00:00 2001 From: Serhjii Bilyk Date: Fri, 14 Dec 2018 09:09:26 +0100 Subject: [PATCH 6/8] Get all variables from childScope and compare them --- lib/rules/no-else-return.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/lib/rules/no-else-return.js b/lib/rules/no-else-return.js index 44b5454c808..0d13aaf9a1e 100644 --- a/lib/rules/no-else-return.js +++ b/lib/rules/no-else-return.js @@ -109,6 +109,23 @@ module.exports = { */ const isDeclarationInside = sourceCode.getTokensBetween(startToken, endToken).findIndex(isVariableDeclaration) > -1; + const childScopeVars = (childScopes, store = {}, level = 0) => + childScopes.reduce((acc, block) => { + if (block.childScopes.length >= 0) { + acc[level] = block.variables; + let index = level + 1 + return childScopeVars(block.childScopes, acc, index); + } + return acc + }, store) + + const shadowedVariables = scope => { + const result = childScopeVars(scope.childScopes); + return scope.variables.filter(elem => result[0].findIndex(vars => vars.name === elem.name) > -1) + }; + const shadowed = shadowedVariables(context.getScope()); + console.log('test', shadowed) + if (isDeclarationInside) { return null; } From 0b3a3a44fa1625735e4f6cb05471d2d289e6659f Mon Sep 17 00:00:00 2001 From: Serhjii Bilyk Date: Thu, 27 Dec 2018 19:58:38 +0100 Subject: [PATCH 7/8] Compute block scope declaration --- lib/rules/no-else-return.js | 66 ++++++++++++++++--------------- tests/lib/rules/no-else-return.js | 4 +- 2 files changed, 36 insertions(+), 34 deletions(-) diff --git a/lib/rules/no-else-return.js b/lib/rules/no-else-return.js index 0d13aaf9a1e..f3058d9b4e8 100644 --- a/lib/rules/no-else-return.js +++ b/lib/rules/no-else-return.js @@ -16,15 +16,8 @@ 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"; +const BLOCK_SCOPED_DECLARATION = /^(?:VariableDeclarator|ClassDeclaration)$/; - return token.type === "Keyword" && variableDeclaration; -} //------------------------------------------------------------------------------ // Rule Definition @@ -104,31 +97,40 @@ 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] + * 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 isDeclarationInside = sourceCode.getTokensBetween(startToken, endToken).findIndex(isVariableDeclaration) > -1; - - const childScopeVars = (childScopes, store = {}, level = 0) => - childScopes.reduce((acc, block) => { - if (block.childScopes.length >= 0) { - acc[level] = block.variables; - let index = level + 1 - return childScopeVars(block.childScopes, acc, index); - } - return acc - }, store) - - const shadowedVariables = scope => { - const result = childScopeVars(scope.childScopes); - return scope.variables.filter(elem => result[0].findIndex(vars => vars.name === elem.name) > -1) - }; - const shadowed = shadowedVariables(context.getScope()); - console.log('test', shadowed) - - if (isDeclarationInside) { - return null; - } + 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 + + }); + }; + + if (blockScopeDeclaration(context.getScope())) { + return null; + } const lastTokenOfElseBlock = sourceCode.getTokenBefore(endToken); diff --git a/tests/lib/rules/no-else-return.js b/tests/lib/rules/no-else-return.js index 6dd0add8ffd..a3b2fb6b156 100644 --- a/tests/lib/rules/no-else-return.js +++ b/tests/lib/rules/no-else-return.js @@ -202,8 +202,8 @@ ruleTester.run("no-else-return", rule, { errors: [{ messageId: "unexpected", type: "BlockStatement" }] }, { - code: "function foo() { let bar = true; if (baz) { return; } else { { let bar = false; } return; } }", - output: null, + 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" } From 425f04191a9f5751d8593fba70b8bd883886634a Mon Sep 17 00:00:00 2001 From: Serhjii Bilyk Date: Sun, 10 Feb 2019 12:09:07 +0100 Subject: [PATCH 8/8] Fix eslint errors --- lib/rules/no-else-return.js | 60 ++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 35 deletions(-) diff --git a/lib/rules/no-else-return.js b/lib/rules/no-else-return.js index f3058d9b4e8..484413066ef 100644 --- a/lib/rules/no-else-return.js +++ b/lib/rules/no-else-return.js @@ -18,6 +18,28 @@ const FixTracker = require("../util/fix-tracker"); const BLOCK_SCOPED_DECLARATION = /^(?:VariableDeclarator|ClassDeclaration)$/; +/** + * + * @param {*} scope current scope + * @param {Token} startToken s + * @param {Token} endToken s + * @returns {boolean} s + */ +function blockScopeDeclaration(scope, startToken, endToken) { + + 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 @@ -96,41 +118,9 @@ 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 - - }); - }; - - if (blockScopeDeclaration(context.getScope())) { - return null; - } + if (blockScopeDeclaration(context.getScope(), startToken, endToken)) { + return null; + } const lastTokenOfElseBlock = sourceCode.getTokenBefore(endToken);