Skip to content

Commit

Permalink
feat: correct no-useless-return behaviour in try statements (#16996)
Browse files Browse the repository at this point in the history 
* 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 <github@fasttime.org>
  • Loading branch information
@snitin315 @fasttime
snitin315 and fasttime committed May 23, 2023
1 parent 37432f2 commit b8448ff
Show file tree
Hide file tree
Showing 2 changed files with 198 additions and 20 deletions.
42 changes: 35 additions & 7 deletions lib/rules/no-useless-return.js
View file
Expand Up @@ -82,7 +82,6 @@ module.exports = {

create(context) {
const segmentInfoMap = new WeakMap();
const usedUnreachableSegments = new WeakSet();
const sourceCode = context.sourceCode;
let scopeInfo = null;

Expand Down Expand Up @@ -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<CodePathSegment>} 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;
});
}

/**
Expand All @@ -188,7 +207,7 @@ module.exports = {
scopeInfo
.codePath
.currentSegments
.forEach(markReturnStatementsOnSegmentAsUsed);
.forEach(segment => markReturnStatementsOnSegmentAsUsed(segment, new Set()));
}

//----------------------------------------------------------------------
Expand All @@ -202,6 +221,7 @@ module.exports = {
scopeInfo = {
upper: scopeInfo,
uselessReturns: [],
traversedTryBlockStatements: [],
codePath
};
},
Expand Down Expand Up @@ -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.
Expand Down
176 changes: 163 additions & 13 deletions tests/lib/rules/no-useless-return.js
View file
Expand Up @@ -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; }",
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
`
],

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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
))
});

0 comments on commit b8448ff

Please sign in to comment.