Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: correct no-useless-return behaviour in try statements #16996

Merged
merged 6 commits into from May 23, 2023
Merged

feat: correct no-useless-return behaviour in try statements #16996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
23c80b5
feat: correct `no-useless-return` behaviour in try statements
snitin315 Mar 16, 2023
87db81e
test: add more test cases for no-useless-return
snitin315 Mar 16, 2023
775f070
refactor: fix formatting
snitin315 Mar 16, 2023
2d96a7c
refactor: fix formatting
snitin315 Mar 16, 2023
c4eb538
fix: check all try block statements in the list
snitin315 Mar 18, 2023
80c7367
Fix a false positive
fasttime May 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 35 additions & 7 deletions lib/rules/no-useless-return.js
View file
Open in desktop
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
Open in desktop
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
))
});