Skip to content

Commit 47afcf6

Browse files
authored
feat: correct preserve-caught-error edge cases (#20109)
* fix: update `preserve-caught-error` edge cases * allow multiple `cause` definitions when the last one is expected
1 parent 75b74d8 commit 47afcf6

File tree

2 files changed

+119
-36
lines changed

2 files changed

+119
-36
lines changed

lib/rules/preserve-caught-error.js

Lines changed: 60 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,15 @@
1010

1111
const astUtils = require("./utils/ast-utils");
1212

13-
//----------------------------------------------------------------------
13+
//------------------------------------------------------------------------------
14+
// Types
15+
//------------------------------------------------------------------------------
16+
17+
/** @typedef {import("estree").Node} ASTNode */
18+
19+
//------------------------------------------------------------------------------
1420
// Helpers
15-
//----------------------------------------------------------------------
21+
//------------------------------------------------------------------------------
1622

1723
/*
1824
* This is an indicator of an error cause node, that is too complicated to be detected and fixed.
@@ -32,9 +38,11 @@ const BUILT_IN_ERROR_TYPES = new Set([
3238
]);
3339

3440
/**
35-
* Finds and returns the ASTNode that is used as the `cause` of the Error being thrown
41+
* Finds and returns information about the `cause` property of an error being thrown.
3642
* @param {ASTNode} throwStatement `ThrowStatement` to be checked.
37-
* @returns {ASTNode | UNKNOWN_CAUSE | null} The `cause` of `Error` being thrown, `null` if not set.
43+
* @returns {{ value: ASTNode; multipleDefinitions: boolean; } | UNKNOWN_CAUSE | null}
44+
* Information about the `cause` of the error being thrown, such as the value node and
45+
* whether there are multiple definitions of `cause`. `null` if there is no `cause`.
3846
*/
3947
function getErrorCause(throwStatement) {
4048
const throwExpression = throwStatement.argument;
@@ -73,15 +81,21 @@ function getErrorCause(throwStatement) {
7381
return UNKNOWN_CAUSE;
7482
}
7583

76-
const causeProperty = errorOptions.properties.find(
84+
const causeProperties = errorOptions.properties.filter(
7785
prop =>
7886
prop.type === "Property" &&
7987
prop.key.type === "Identifier" &&
8088
prop.key.name === "cause" &&
8189
!prop.computed, // It is hard to accurately identify the value of computed props
8290
);
8391

84-
return causeProperty ? causeProperty.value : null;
92+
const causeProperty = causeProperties.at(-1);
93+
return causeProperty
94+
? {
95+
value: causeProperty.value,
96+
multipleDefinitions: causeProperties.length > 1,
97+
}
98+
: null;
8599
}
86100

87101
// Error options exist, but too complicated to be analyzed/fixed
@@ -290,14 +304,14 @@ module.exports = {
290304
}
291305

292306
// Check if there is a cause attached to the new error
293-
const thrownErrorCause = getErrorCause(throwStatement);
307+
const errorCauseInfo = getErrorCause(throwStatement);
294308

295-
if (thrownErrorCause === UNKNOWN_CAUSE) {
309+
if (errorCauseInfo === UNKNOWN_CAUSE) {
296310
// Error options exist, but too complicated to be analyzed/fixed
297311
return;
298312
}
299313

300-
if (thrownErrorCause === null) {
314+
if (errorCauseInfo === null) {
301315
// If there is no `cause` attached to the error being thrown.
302316
context.report({
303317
messageId: "missingCause",
@@ -439,42 +453,52 @@ module.exports = {
439453
return;
440454
}
441455

442-
// If there is an attached cause, verify that is matches the caught error
456+
const { value: thrownErrorCause } = errorCauseInfo;
457+
458+
// If there is an attached cause, verify that it matches the caught error
443459
if (
444460
!(
445461
thrownErrorCause.type === "Identifier" &&
446462
thrownErrorCause.name === caughtError.name
447463
)
448464
) {
449-
context.report({
450-
messageId: "incorrectCause",
451-
node: thrownErrorCause,
452-
suggest: [
453-
{
454-
messageId: "includeCause",
455-
fix(fixer) {
456-
/*
457-
* In case `cause` is attached using object property shorthand or as a method.
458-
* e.g. throw Error("fail", { cause });
459-
* throw Error("fail", { cause() { // do something } });
460-
*/
461-
if (
462-
thrownErrorCause.parent.method ||
463-
thrownErrorCause.parent.shorthand
464-
) {
465+
const suggest = errorCauseInfo.multipleDefinitions
466+
? null // If there are multiple `cause` definitions, a suggestion could be confusing.
467+
: [
468+
{
469+
messageId: "includeCause",
470+
fix(fixer) {
471+
/*
472+
* In case `cause` is attached using object property shorthand or as a method or accessor.
473+
* e.g. throw Error("fail", { cause });
474+
* throw Error("fail", { cause() { doSomething(); } });
475+
* throw Error("fail", { get cause() { return error; } });
476+
*/
477+
if (
478+
thrownErrorCause.parent
479+
.method ||
480+
thrownErrorCause.parent
481+
.shorthand ||
482+
thrownErrorCause.parent.kind !==
483+
"init"
484+
) {
485+
return fixer.replaceText(
486+
thrownErrorCause.parent,
487+
`cause: ${caughtError.name}`,
488+
);
489+
}
490+
465491
return fixer.replaceText(
466-
thrownErrorCause.parent,
467-
`cause: ${caughtError.name}`,
492+
thrownErrorCause,
493+
caughtError.name,
468494
);
469-
}
470-
471-
return fixer.replaceText(
472-
thrownErrorCause,
473-
caughtError.name,
474-
);
495+
},
475496
},
476-
},
477-
],
497+
];
498+
context.report({
499+
messageId: "incorrectCause",
500+
node: thrownErrorCause,
501+
suggest,
478502
});
479503
return;
480504
}

tests/lib/rules/preserve-caught-error.js

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,12 @@ ruleTester.run("preserve-caught-error", rule, {
8686
}`,
8787
options: [{ requireCatchParameter: false }],
8888
},
89+
/* Multiple cause properties are present and the last one is the expected caught error value. */
90+
`try {
91+
doSomething();
92+
} catch (error) {
93+
throw new Error("Something failed", { cause: anotherError, cause: error });
94+
}`,
8995
],
9096
invalid: [
9197
/* 1. Throws a new Error without cause, even though an error was caught */
@@ -722,5 +728,58 @@ ruleTester.run("preserve-caught-error", rule, {
722728
},
723729
],
724730
},
731+
/* 25. When multiple `cause` properties are present. */
732+
{
733+
code: `try {} catch (error) {
734+
throw new Error("Something failed", { cause: error, cause: anotherError });
735+
}`,
736+
errors: [{ messageId: "incorrectCause" }],
737+
},
738+
/* 26. Getters and setters as `cause`. */
739+
{
740+
code: `try {} catch (error) {
741+
throw new Error("Something failed", { get cause() { } });
742+
}`,
743+
errors: [
744+
{
745+
messageId: "incorrectCause",
746+
suggestions: [
747+
{
748+
messageId: "includeCause",
749+
output: `try {} catch (error) {
750+
throw new Error("Something failed", { cause: error });
751+
}`,
752+
},
753+
],
754+
},
755+
],
756+
},
757+
{
758+
code: `try {} catch (error) {
759+
throw new Error("Something failed", { set cause(value) { } });
760+
}`,
761+
errors: [
762+
{
763+
messageId: "incorrectCause",
764+
suggestions: [
765+
{
766+
messageId: "includeCause",
767+
output: `try {} catch (error) {
768+
throw new Error("Something failed", { cause: error });
769+
}`,
770+
},
771+
],
772+
},
773+
],
774+
},
775+
{
776+
code: `try {} catch (error) {
777+
throw new Error("Something failed", {
778+
get cause() { return error; },
779+
set cause(value) { error = value; },
780+
});
781+
}`,
782+
errors: [{ messageId: "incorrectCause" }],
783+
},
725784
],
726785
});

0 commit comments

Comments
 (0)