Skip to content

Commit b8448ff

Browse files
snitin315fasttime
snitin315
and
fasttime
authored
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 <[email protected]>
1 parent 37432f2 commit b8448ff

2 files changed

Lines changed: 198 additions & 20 deletions

File tree

‎lib/rules/no-useless-return.js‎

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,6 @@ module.exports = {
8282

8383
create(context) {
8484
const segmentInfoMap = new WeakMap();
85-
const usedUnreachableSegments = new WeakSet();
8685
const sourceCode = context.sourceCode;
8786
let scopeInfo = null;
8887

@@ -152,24 +151,44 @@ module.exports = {
152151
* This behavior would simulate code paths for the case that the return
153152
* statement does not exist.
154153
* @param {CodePathSegment} segment The segment to get return statements.
154+
* @param {Set<CodePathSegment>} usedUnreachableSegments A set of segments that have already been traversed in this call.
155155
* @returns {void}
156156
*/
157-
function markReturnStatementsOnSegmentAsUsed(segment) {
157+
function markReturnStatementsOnSegmentAsUsed(segment, usedUnreachableSegments) {
158158
if (!segment.reachable) {
159159
usedUnreachableSegments.add(segment);
160160
segment.allPrevSegments
161161
.filter(isReturned)
162162
.filter(prevSegment => !usedUnreachableSegments.has(prevSegment))
163-
.forEach(markReturnStatementsOnSegmentAsUsed);
163+
.forEach(prevSegment => markReturnStatementsOnSegmentAsUsed(prevSegment, usedUnreachableSegments));
164164
return;
165165
}
166166

167167
const info = segmentInfoMap.get(segment);
168168

169-
for (const node of info.uselessReturns) {
169+
info.uselessReturns = info.uselessReturns.filter(node => {
170+
if (scopeInfo.traversedTryBlockStatements && scopeInfo.traversedTryBlockStatements.length > 0) {
171+
const returnInitialRange = node.range[0];
172+
const returnFinalRange = node.range[1];
173+
174+
const areBlocksInRange = scopeInfo.traversedTryBlockStatements.some(tryBlockStatement => {
175+
const blockInitialRange = tryBlockStatement.range[0];
176+
const blockFinalRange = tryBlockStatement.range[1];
177+
178+
return (
179+
returnInitialRange >= blockInitialRange &&
180+
returnFinalRange <= blockFinalRange
181+
);
182+
});
183+
184+
if (areBlocksInRange) {
185+
return true;
186+
}
187+
}
188+
170189
remove(scopeInfo.uselessReturns, node);
171-
}
172-
info.uselessReturns = [];
190+
return false;
191+
});
173192
}
174193

175194
/**
@@ -188,7 +207,7 @@ module.exports = {
188207
scopeInfo
189208
.codePath
190209
.currentSegments
191-
.forEach(markReturnStatementsOnSegmentAsUsed);
210+
.forEach(segment => markReturnStatementsOnSegmentAsUsed(segment, new Set()));
192211
}
193212

194213
//----------------------------------------------------------------------
@@ -202,6 +221,7 @@ module.exports = {
202221
scopeInfo = {
203222
upper: scopeInfo,
204223
uselessReturns: [],
224+
traversedTryBlockStatements: [],
205225
codePath
206226
};
207227
},
@@ -275,6 +295,14 @@ module.exports = {
275295
scopeInfo.uselessReturns.push(node);
276296
},
277297

298+
"TryStatement > BlockStatement.block:exit"(node) {
299+
scopeInfo.traversedTryBlockStatements.push(node);
300+
},
301+
302+
"TryStatement:exit"() {
303+
scopeInfo.traversedTryBlockStatements.pop();
304+
},
305+
278306
/*
279307
* Registers for all statement nodes except FunctionDeclaration, BlockStatement, BreakStatement.
280308
* Removes return statements of the current segments from the useless return statement list.

‎tests/lib/rules/no-useless-return.js‎

Lines changed: 163 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ const rule = require("../../../lib/rules/no-useless-return"),
1919
const ruleTester = new RuleTester();
2020

2121
ruleTester.run("no-useless-return", rule, {
22-
2322
valid: [
2423
"function foo() { return 5; }",
2524
"function foo() { return null; }",
@@ -96,6 +95,26 @@ ruleTester.run("no-useless-return", rule, {
9695
}
9796
}
9897
`,
98+
`
99+
function foo() {
100+
try {
101+
bar();
102+
return;
103+
} catch (err) {}
104+
baz();
105+
}
106+
`,
107+
`
108+
function foo() {
109+
if (something) {
110+
try {
111+
bar();
112+
return;
113+
} catch (err) {}
114+
}
115+
baz();
116+
}
117+
`,
99118
`
100119
function foo() {
101120
return;
@@ -176,6 +195,19 @@ ruleTester.run("no-useless-return", rule, {
176195
}
177196
console.log(arg);
178197
}
198+
`,
199+
200+
// https://github.com/eslint/eslint/pull/16996#discussion_r1138622844
201+
`
202+
function foo() {
203+
try {
204+
bar();
205+
return;
206+
} finally {
207+
baz();
208+
}
209+
qux();
210+
}
179211
`
180212
],
181213

@@ -386,12 +418,120 @@ ruleTester.run("no-useless-return", rule, {
386418
}
387419
`
388420
},
389-
390-
/*
391-
* FIXME: Re-add this case (removed due to https://github.com/eslint/eslint/issues/7481):
392-
* https://github.com/eslint/eslint/blob/261d7287820253408ec87c344beccdba2fe829a4/tests/lib/rules/no-useless-return.js#L308-L329
393-
*/
394-
421+
{
422+
code: `
423+
function foo() {
424+
try {
425+
foo();
426+
return;
427+
} catch (err) {
428+
return 5;
429+
}
430+
}
431+
`,
432+
output: `
433+
function foo() {
434+
try {
435+
foo();
436+
437+
} catch (err) {
438+
return 5;
439+
}
440+
}
441+
`
442+
},
443+
{
444+
code: `
445+
function foo() {
446+
if (something) {
447+
try {
448+
bar();
449+
return;
450+
} catch (err) {}
451+
}
452+
}
453+
`,
454+
output: `
455+
function foo() {
456+
if (something) {
457+
try {
458+
bar();
459+
460+
} catch (err) {}
461+
}
462+
}
463+
`
464+
},
465+
{
466+
code: `
467+
function foo() {
468+
try {
469+
return;
470+
} catch (err) {
471+
foo();
472+
}
473+
}
474+
`,
475+
output: `
476+
function foo() {
477+
try {
478+
479+
} catch (err) {
480+
foo();
481+
}
482+
}
483+
`
484+
},
485+
{
486+
code: `
487+
function foo() {
488+
try {
489+
return;
490+
} finally {
491+
bar();
492+
}
493+
}
494+
`,
495+
output: `
496+
function foo() {
497+
try {
498+
499+
} finally {
500+
bar();
501+
}
502+
}
503+
`
504+
},
505+
{
506+
code: `
507+
function foo() {
508+
try {
509+
bar();
510+
} catch (e) {
511+
try {
512+
baz();
513+
return;
514+
} catch (e) {
515+
qux();
516+
}
517+
}
518+
}
519+
`,
520+
output: `
521+
function foo() {
522+
try {
523+
bar();
524+
} catch (e) {
525+
try {
526+
baz();
527+
528+
} catch (e) {
529+
qux();
530+
}
531+
}
532+
}
533+
`
534+
},
395535
{
396536
code: `
397537
function foo() {
@@ -438,11 +578,21 @@ ruleTester.run("no-useless-return", rule, {
438578
{
439579
code: "function foo() { return; return; }",
440580
output: "function foo() { return; }",
441-
errors: [{
442-
messageId: "unnecessaryReturn",
443-
type: "ReturnStatement",
444-
column: 18
445-
}]
581+
errors: [
582+
{
583+
messageId: "unnecessaryReturn",
584+
type: "ReturnStatement",
585+
column: 18
586+
}
587+
]
446588
}
447-
].map(invalidCase => Object.assign({ errors: [{ messageId: "unnecessaryReturn", type: "ReturnStatement" }] }, invalidCase))
589+
].map(invalidCase =>
590+
Object.assign(
591+
{
592+
errors: [
593+
{ messageId: "unnecessaryReturn", type: "ReturnStatement" }
594+
]
595+
},
596+
invalidCase
597+
))
448598
});

0 commit comments

Comments
 (0)