From b8448ff1ae1adf26a81dea07f340caa5b5c2f257 Mon Sep 17 00:00:00 2001 From: Nitin Kumar Date: Tue, 23 May 2023 15:57:44 +0530 Subject: [PATCH] feat: correct no-useless-return behaviour in try statements (#16996) * feat: correct `no-useless-return` behaviour in try statements * test: add more test cases for no-useless-return * refactor: fix formatting * refactor: fix formatting * fix: check all try block statements in the list * Fix a false positive --------- Co-authored-by: Francesco Trotta --- lib/rules/no-useless-return.js | 42 +++++-- tests/lib/rules/no-useless-return.js | 176 +++++++++++++++++++++++++-- 2 files changed, 198 insertions(+), 20 deletions(-) diff --git a/lib/rules/no-useless-return.js b/lib/rules/no-useless-return.js index db1ccae9739..f89523153d4 100644 --- a/lib/rules/no-useless-return.js +++ b/lib/rules/no-useless-return.js @@ -82,7 +82,6 @@ module.exports = { create(context) { const segmentInfoMap = new WeakMap(); - const usedUnreachableSegments = new WeakSet(); const sourceCode = context.sourceCode; let scopeInfo = null; @@ -152,24 +151,44 @@ module.exports = { * This behavior would simulate code paths for the case that the return * statement does not exist. * @param {CodePathSegment} segment The segment to get return statements. + * @param {Set} usedUnreachableSegments A set of segments that have already been traversed in this call. * @returns {void} */ - function markReturnStatementsOnSegmentAsUsed(segment) { + function markReturnStatementsOnSegmentAsUsed(segment, usedUnreachableSegments) { if (!segment.reachable) { usedUnreachableSegments.add(segment); segment.allPrevSegments .filter(isReturned) .filter(prevSegment => !usedUnreachableSegments.has(prevSegment)) - .forEach(markReturnStatementsOnSegmentAsUsed); + .forEach(prevSegment => markReturnStatementsOnSegmentAsUsed(prevSegment, usedUnreachableSegments)); return; } const info = segmentInfoMap.get(segment); - for (const node of info.uselessReturns) { + info.uselessReturns = info.uselessReturns.filter(node => { + if (scopeInfo.traversedTryBlockStatements && scopeInfo.traversedTryBlockStatements.length > 0) { + const returnInitialRange = node.range[0]; + const returnFinalRange = node.range[1]; + + const areBlocksInRange = scopeInfo.traversedTryBlockStatements.some(tryBlockStatement => { + const blockInitialRange = tryBlockStatement.range[0]; + const blockFinalRange = tryBlockStatement.range[1]; + + return ( + returnInitialRange >= blockInitialRange && + returnFinalRange <= blockFinalRange + ); + }); + + if (areBlocksInRange) { + return true; + } + } + remove(scopeInfo.uselessReturns, node); - } - info.uselessReturns = []; + return false; + }); } /** @@ -188,7 +207,7 @@ module.exports = { scopeInfo .codePath .currentSegments - .forEach(markReturnStatementsOnSegmentAsUsed); + .forEach(segment => markReturnStatementsOnSegmentAsUsed(segment, new Set())); } //---------------------------------------------------------------------- @@ -202,6 +221,7 @@ module.exports = { scopeInfo = { upper: scopeInfo, uselessReturns: [], + traversedTryBlockStatements: [], codePath }; }, @@ -275,6 +295,14 @@ module.exports = { scopeInfo.uselessReturns.push(node); }, + "TryStatement > BlockStatement.block:exit"(node) { + scopeInfo.traversedTryBlockStatements.push(node); + }, + + "TryStatement:exit"() { + scopeInfo.traversedTryBlockStatements.pop(); + }, + /* * Registers for all statement nodes except FunctionDeclaration, BlockStatement, BreakStatement. * Removes return statements of the current segments from the useless return statement list. diff --git a/tests/lib/rules/no-useless-return.js b/tests/lib/rules/no-useless-return.js index 434700cc2d8..5b090dbb365 100644 --- a/tests/lib/rules/no-useless-return.js +++ b/tests/lib/rules/no-useless-return.js @@ -19,7 +19,6 @@ const rule = require("../../../lib/rules/no-useless-return"), const ruleTester = new RuleTester(); ruleTester.run("no-useless-return", rule, { - valid: [ "function foo() { return 5; }", "function foo() { return null; }", @@ -96,6 +95,26 @@ ruleTester.run("no-useless-return", rule, { } } `, + ` + function foo() { + try { + bar(); + return; + } catch (err) {} + baz(); + } + `, + ` + function foo() { + if (something) { + try { + bar(); + return; + } catch (err) {} + } + baz(); + } + `, ` function foo() { return; @@ -176,6 +195,19 @@ ruleTester.run("no-useless-return", rule, { } console.log(arg); } + `, + + // https://github.com/eslint/eslint/pull/16996#discussion_r1138622844 + ` + function foo() { + try { + bar(); + return; + } finally { + baz(); + } + qux(); + } ` ], @@ -386,12 +418,120 @@ 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() { + if (something) { + try { + bar(); + return; + } catch (err) {} + } + } + `, + output: ` + function foo() { + if (something) { + try { + bar(); + + } catch (err) {} + } + } + ` + }, + { + code: ` + function foo() { + try { + return; + } catch (err) { + foo(); + } + } + `, + output: ` + function foo() { + try { + + } catch (err) { + foo(); + } + } + ` + }, + { + code: ` + function foo() { + try { + return; + } finally { + bar(); + } + } + `, + output: ` + function foo() { + try { + + } finally { + bar(); + } + } + ` + }, + { + code: ` + function foo() { + try { + bar(); + } catch (e) { + try { + baz(); + return; + } catch (e) { + qux(); + } + } + } + `, + output: ` + function foo() { + try { + bar(); + } catch (e) { + try { + baz(); + + } catch (e) { + qux(); + } + } + } + ` + }, { code: ` function foo() { @@ -438,11 +578,21 @@ ruleTester.run("no-useless-return", rule, { { code: "function foo() { return; return; }", output: "function foo() { return; }", - errors: [{ - messageId: "unnecessaryReturn", - type: "ReturnStatement", - column: 18 - }] + errors: [ + { + messageId: "unnecessaryReturn", + type: "ReturnStatement", + column: 18 + } + ] } - ].map(invalidCase => Object.assign({ errors: [{ messageId: "unnecessaryReturn", type: "ReturnStatement" }] }, invalidCase)) + ].map(invalidCase => + Object.assign( + { + errors: [ + { messageId: "unnecessaryReturn", type: "ReturnStatement" } + ] + }, + invalidCase + )) });