From 8fa5756f9bdcaebb18025916d5805456440af9c5 Mon Sep 17 00:00:00 2001 From: Guilherme Gois Date: Sun, 27 Nov 2022 19:54:34 -0300 Subject: [PATCH 1/2] fix: no-useless-return false negative in try statement Promote up the useless returns in BlockStatement inside TryStatement when ESLint is going up in the tree during traversing the AST. Fixes #7481 --- lib/rules/no-useless-return.js | 40 ++++++++++++++++++++++- tests/lib/rules/no-useless-return.js | 48 ++++++++++++++++++++++++---- 2 files changed, 81 insertions(+), 7 deletions(-) diff --git a/lib/rules/no-useless-return.js b/lib/rules/no-useless-return.js index be8d4dfd3a5..739aae7aa99 100644 --- a/lib/rules/no-useless-return.js +++ b/lib/rules/no-useless-return.js @@ -38,6 +38,15 @@ function isRemovable(node) { return astUtils.STATEMENT_LIST_PARENTS.has(node.parent.type); } +/** + * Checks if given node is a try statement or not. + * @param {ASTNode} node The node to check. + * @returns {boolean} `true` if the node is a try statement. + */ +function isTryStatement(node) { + return node.type === "TryStatement"; +} + /** * Checks whether the given return statement is in a `finally` block or not. * @param {ASTNode} node The return statement node to check. @@ -49,7 +58,7 @@ function isInFinally(node) { currentNode && currentNode.parent && !astUtils.isFunction(currentNode); currentNode = currentNode.parent ) { - if (currentNode.parent.type === "TryStatement" && currentNode.parent.finalizer === currentNode) { + if (isTryStatement(currentNode.parent) && currentNode.parent.finalizer === currentNode) { return true; } } @@ -57,6 +66,19 @@ function isInFinally(node) { return false; } +/** + * Checks whether the given node is the last one in the parentNode. + * @param {ASTNode} node The node to check. + * @param {ASTNode} parentNode The node that possibly contains the given `node`. + * @param {any} sourceCode The source code + * @returns {boolean} `true` if the node is the last one in the `parentNode` children. + */ +function isLastNode(node, parentNode, sourceCode) { + const lastNodeInParent = parentNode.body[parentNode.body.length - 1]; + + return astUtils.equalTokens(node, lastNodeInParent, sourceCode); +} + //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -274,6 +296,22 @@ module.exports = { } scopeInfo.uselessReturns.push(node); }, + "BlockStatement:exit"(node) { + if ( + !isTryStatement(node.parent) || + !scopeInfo.upper || + !scopeInfo.uselessReturns + ) { + return; + } + + const lastUseselessReturn = scopeInfo.uselessReturns[scopeInfo.uselessReturns.length - 1]; + + + if (lastUseselessReturn && isLastNode(lastUseselessReturn, node, sourceCode)) { + scopeInfo.upper.uselessReturns.push(lastUseselessReturn); + } + }, /* * Registers for all statement nodes except FunctionDeclaration, BlockStatement, BreakStatement. diff --git a/tests/lib/rules/no-useless-return.js b/tests/lib/rules/no-useless-return.js index 434700cc2d8..4850a975e3d 100644 --- a/tests/lib/rules/no-useless-return.js +++ b/tests/lib/rules/no-useless-return.js @@ -386,12 +386,48 @@ ruleTester.run("no-useless-return", rule, { } ` }, - - /* - * FIXME: Re-add this case (removed due to https://github.com/eslint/eslint/issues/7481): - * https://github.com/eslint/eslint/blob/261d7287820253408ec87c344beccdba2fe829a4/tests/lib/rules/no-useless-return.js#L308-L329 - */ - + { + code: ` + function foo() { + try { + foo(); + return; + } catch (err) { + return 5; + } + } + `, + output: ` + function foo() { + try { + foo(); + + } catch (err) { + return 5; + } + } + ` + }, + { + code: ` + function foo() { + try { + return; + } catch (err) { + foo(); + } + } + `, + output: ` + function foo() { + try { + + } catch (err) { + foo(); + } + } + ` + }, { code: ` function foo() { From 414c472898ba693f99597b757c2a89de41ddff13 Mon Sep 17 00:00:00 2001 From: Guilherme Gois Date: Sat, 3 Dec 2022 21:28:28 -0300 Subject: [PATCH 2/2] fix: no-useless-return false positive in try statement Do not promote up the useless returns if the try statement isn't the last one --- lib/rules/no-useless-return.js | 18 +++++++++++++----- tests/lib/rules/no-useless-return.js | 9 +++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/lib/rules/no-useless-return.js b/lib/rules/no-useless-return.js index 739aae7aa99..a945736a9c1 100644 --- a/lib/rules/no-useless-return.js +++ b/lib/rules/no-useless-return.js @@ -74,7 +74,13 @@ function isInFinally(node) { * @returns {boolean} `true` if the node is the last one in the `parentNode` children. */ function isLastNode(node, parentNode, sourceCode) { - const lastNodeInParent = parentNode.body[parentNode.body.length - 1]; + if (!parentNode.body) { + return false; + } + + const lastNodeInParent = Array.isArray(parentNode.body) + ? parentNode.body[parentNode.body.length - 1] + : parentNode.body; return astUtils.equalTokens(node, lastNodeInParent, sourceCode); } @@ -297,19 +303,21 @@ module.exports = { scopeInfo.uselessReturns.push(node); }, "BlockStatement:exit"(node) { + const parentNode = node.parent; + if ( - !isTryStatement(node.parent) || + !(isTryStatement(parentNode) && isLastNode(parentNode, parentNode.parent, sourceCode)) || !scopeInfo.upper || !scopeInfo.uselessReturns ) { return; } - const lastUseselessReturn = scopeInfo.uselessReturns[scopeInfo.uselessReturns.length - 1]; + const lastUselessReturn = scopeInfo.uselessReturns[scopeInfo.uselessReturns.length - 1]; - if (lastUseselessReturn && isLastNode(lastUseselessReturn, node, sourceCode)) { - scopeInfo.upper.uselessReturns.push(lastUseselessReturn); + if (lastUselessReturn && isLastNode(lastUselessReturn, node, sourceCode)) { + scopeInfo.upper.uselessReturns.push(lastUselessReturn); } }, diff --git a/tests/lib/rules/no-useless-return.js b/tests/lib/rules/no-useless-return.js index 4850a975e3d..1ba3477dd33 100644 --- a/tests/lib/rules/no-useless-return.js +++ b/tests/lib/rules/no-useless-return.js @@ -96,6 +96,15 @@ ruleTester.run("no-useless-return", rule, { } } `, + ` + function foo() { + try { + bar(); + return; + } catch (err) {} + baz(); + } + `, ` function foo() { return;