feat: add `preserve-caught-error` rule #19913

Amnish04 · 2025-07-04T19:43:26Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request?

[ ] Documentation update
[ ] Bug fix (template)
[X] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

fixes #19844

What changes did you make?

I've done some work to implement an initial version of preserve-caught-error rule.

On top of what my plugin already supports, I've made some improvements as discussed in #19844.

The rule now also:

Detects and flags multiple throw statements without a cause within a CatchClause.
Reports with a special message when the caught error is being ignored at the parameter level. See https://github.com/Amnish04/eslint/blob/8c3120c0fb2743db5164ceb127c81ffbd8a9bc95/tests/lib/rules/preserve-caught-error.js#L188.
Checks for all the in-built Error types that support the cause option by default. Accepts a customErrorTypes option, that is a list of user-defined error types to extend what Error types to look for.
Offers suggestions to auto-fix instead of direct fixers.
All the new features are accompanies with corresponding tests.
Documents the rule details in docs/src/rules/preserve-caught-error.md.

Is there anything you'd like reviewers to focus on?

Happy to iterate and make any changes/fixes required :)

netlify · 2025-07-04T19:43:48Z

✅ Deploy Preview for docs-eslint ready!

Name	Link
🔨 Latest commit	`6986a62`
🔍 Latest deploy log	https://app.netlify.com/projects/docs-eslint/deploys/6872b1cb7c33300008664dec
😎 Deploy Preview	https://deploy-preview-19913--docs-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Amnish04 · 2025-07-04T20:14:34Z

Not sure why, I am getting CI issues with my further_reading links:

Problem writing Eleventy templates:
[11ty] 1. Having trouble writing to "./_site/rules/preserve-caught-error.html" from "./src/rules/preserve-caught-error.md" (via EleventyTemplateError)
[11ty] 2. (./src/_includes/layouts/doc.html)
[11ty]  (/home/runner/work/eslint/eslint/docs/src/_includes/components/docs-index.html)
[11ty]  (/home/runner/work/eslint/eslint/docs/src/_includes/components/search.html)
[11ty]   EleventyNunjucksError: Error with Nunjucks shortcode `link` (via Template render error)
[11ty] 3. Data missing for https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause

Although I see MDN links being used in docs of some existing rules:
https://github.com/Amnish04/eslint/blob/99b3c2926152b972782a4eec8f06dea7177ccb04/docs/src/rules/class-methods-use-this.md#L4-L7

fasttime

Thanks for the pull request. Can you also add a type definition for the new rule to lib/types/rules.d.ts? This is a good example:

eslint/lib/types/rules.d.ts

Lines 2447 to 2453 in bb370b8

    
           "no-extend-native": Linter.RuleEntry< 
        
           	[ 
        
           		Partial<{ 
        
           			exceptions: string[]; 
        
           		}>, 
        
           	] 
        
           >;

After that, you can run node tools/update-rule-type-headers.js locally to autogenerate the TSDoc comment from the rule metadata.

docs/src/rules/preserve-caught-error.md

Amnish04 · 2025-07-06T21:56:56Z

@fasttime Thanks for your help!

As per you review, I have:

Added a type definition for preserve-caught-error rule and generated a tsdoc comment for it.
Added my "further reading" links metadata to further_reading_links.json file.
Updated the error message for when further links metadata is missing.

All CI checks are passing and the branch is up-to-date with upstream main. Please let me know if you see any more issues!

Amnish04 · 2025-07-06T22:06:28Z

I've also marked this rule as recommended as it is widely applicable and a general best practice to adhere to. But please let me know if people have different thoughts and I'll undo it.

lib/rules/preserve-caught-error.js

docs/src/rules/preserve-caught-error.md

packages/js/src/configs/eslint-recommended.js

JoshuaKGoldberg

A lovely start! fasttime left some very good review comments around ESLint's infrastructure that I don't have a lot to add to. I also have some separate suggestions around the options and default behavior. Cheers!

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

JoshuaKGoldberg · 2025-07-08T13:56:26Z

lib/rules/preserve-caught-error.js

+				properties: {
+					customErrorTypes: {
+						type: "array",
+						items: { type: "string" },


😄 #16540 is relevant in core, at long last!

The problem with an array of plain strings is that it's easy for users to accidentally use unrelated names that coincidentally match the strings. The strings just encode the raw name, not any other metadata such as location / package the targeted construct comes from.

For example, suppose a project has a SafeError class:

// src/utils/SafeError.js export class SafeError extends Error { /* ... */ }

It might configure the rule like:

"preserve-caught-errors": ["error", { customErrorTypes: ["SafeError"] }]

If a file happens to create its own SafeError, then oops, the rule won't report:

class SafeError { /* ... */ } export function doSomethingDangerous() { // ... try { // ... } catch (error) { throw new SafeError(error); } }

If this option were a must-have for the rule, I would ask that we talk about adopting some kind of standard locator format - e.g. typescript-eslint's TypeOrValueSpecifier. But per my comment later in the file, I think we can omit this option for now.

Yeah, this looks like a problem. @nzakas @fasttime If you also agree, I'll remove this option for now.

I don't feel strongly about this. @nzakas what do you think?

Let's remove it for now. We can always revisit later.

Sounds good, I've removed the option for now.

We can revisit when we have something like TypeOrValueSpecifier implemented in core. @JoshuaKGoldberg Do we have an issue filed for it?

Not that I know of. I'm also not confident it would make sense in core, since core doesn't have any concept of types.

lib/rules/preserve-caught-error.js

JoshuaKGoldberg · 2025-07-08T14:07:14Z

lib/rules/preserve-caught-error.js

+			return (
+				throwStatement.argument.type === "NewExpression" &&
+				throwStatement.argument.callee.type === "Identifier" &&
+				allErrorTypes.has(throwStatement.argument.callee.name)


[Bug] Speaking of shared helpers, there's no way for a core ESLint rule like this one to know what type a helper function throws:

"preserve-caught-errors": ["error", { customErrorTypes: ["SafeError"] }]

declare function throwSafeError(cause: Error): SafeError; try { // ... } catch (error) { throwSafeError(error); }

See also my earlier comment on standalone strings not being ideal for these options.

Proposal: I think it'd be best to omit the customErrorTypes logic for now. Instead, I think the rule should err on the side of permissiveness. If the catch block passes its caught error as an argument to any call or new expression, the rule should consider the caught error as used.

If the catch block passes its caught error as an argument to any call or new expression, the rule should consider the caught error as used.

I like this idea. 👍

I also like this idea as this allows us to cover all types of cases in a generic way.

But, a couple of thoughts:

Going with the above approach would mean cases like the following would be considered valid even though we're losing error context there. In other words, this would solve one problem (helper functions) but break the rule for multiple other important cases.

try { doSomething(); } catch (err) { if (err.code === "A") { throw new Error("Type A", { cause: err }); } throw new TypeError("Fallback error"); }

I recently switched my approach to start with ThrowStatement listeners and track parent CatchClause, as it allows reaching every throw statement without dealing with searching tonnes of possible JavaScript constructs as discussed in feat: add preserve-caught-error rule #19913 (comment). So in order to answer the question - "If the caught error has been used at least once", we'll still have to do the manual search which we were trying to avoid. (Maybe there is a better way I can't think of)

Proposal: I think it'd be best to omit the customErrorTypes logic for now. Instead, I think the rule should err on the side of permissiveness. If the catch block passes its caught error as an argument to any call or new expression, the rule should consider the caught error as used.

By this logic, we wouldn't be able to check the error type to make sure it allows the cause option to be specified. Many custom errors, like Node.js' AssertionError, don't provide a way to specify a cause, and there is no point in reporting a violation if such an error is created without including the caught error. I think it's better to restrict the rule on reporting specific error types only.

try { doSomething(); } catch (err) { if (err.code === "A") { throw new Error("Type A", { cause: err }); } throw new TypeError("Fallback error"); }

I think the above code should be invalid because the thrown TypeError has no cause option.

For the sake of simplicity and getting a first version of this rule, let's keep it simple and just error for the built-in error types. I agree with @fasttime that throw new TypeError("Fallback error") should be flagged as invalid.

I agree, this should work in the current state then. We can ignore helper functions for now.

Amnish04 · 2025-07-08T20:07:22Z

@fasttime @JoshuaKGoldberg Thanks for the detailed reviews!

I have made the following changes:

Fixed the further reading links error message.
Removed the rule from recommended config (will file a separate issue once this gets landed), and enabled in eslint-config-eslint.
Fixed some edge cases that @fasttime found. See feat: add preserve-caught-error rule #19913 (comment) and feat: add preserve-caught-error rule #19913 (comment).
Pretty formatted the code and output in test cases, instead of having the entire code on one line.
Fixed some typos.

@JoshuaKGoldberg I've left some ideas on your comment. Please let me know what you think!

PS: Adding this rule to eslint-config-eslint has started flagging error cause issues in CI. Gotta fix those as well.

Amnish04 · 2025-07-09T19:44:26Z

Ok, I've made some changes to handle another case for throws that use a function's return value.

It makes sure that such function accept the caught error as one of it's arguments.

Added the following test cases.

Valid:

`try {
	doSomething();
} catch (err) {
	// passing the caught error directly
	throw createExtraneousResultsError("test", err);
}`,

	`try {
	doSomething();
} catch (err) {
	// passing the caught error as a property in an object
	throw createExtraneousResultsError("test", { err });
}`,

	`try {
	doSomething();
} catch (err) {
	// caught error passed with explicit property name
	throw createExtraneousResultsError("test", { error: err });
}`,

Invalid:

/* 14. Throw uses a function's return value, but the function does not use the `cause` to construct an error. */
{
	code: `try {
	doSomething();
	} catch (err) {
		throw createExtraneousResultsError("Something went wrong");
	}`,
	errors: [
		{
			messageId: "missingCauseInFunction",
		},
	],
},

CI is green again, and I've rebased this on upstream main.

fasttime · 2025-07-10T05:37:45Z

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

+		{
+			code: `try {
+			doSomething();
+			} catch (err) {
+				throw createExtraneousResultsError("Something went wrong");
+			}`,
+			errors: [
+				{
+					messageId: "missingCauseInFunction",
+				},
+			],
+		},


I think this case should be considered valid because createExtraneousResultsError could return a different error type than those of built-in errors. When not sure, it's better not to report possible false positives.

Regardless of the error type, err is never referenced, so it seems like it should be invalid?

The main problem is that not all errors support a cause. For example, in this code, the suggestion of using the caught error as a cause doesn't apply:

import { AssertionError } from "node:assert"; function createAssertionError(message) { return new AssertionError({ message }); } try { testSomething(); } catch (error) { throw createAssertionError("Test failed"); }

DOMException is another example of a common error that doesn't support a cause, as is SuppressedError (a built-in global as of ES2026).

Understood. My point is that if error is present, then I think it must be used to avoid a violation. This code could just as easily be rewritten as:

import { AssertionError } from "node:assert"; function createAssertionError(message) { return new AssertionError({ message }); } try { testSomething(); } catch { throw createAssertionError("Test failed"); }

In this case, it's clear the intent is to ignore the thrown error.

Ah, I see. Sorry for the misunderstanding. Here's another example that should be clearer:

import { AssertionError } from "node:assert"; function createAssertionError(message) { return new AssertionError({ message }); } try { testSomething(); } catch (error) { if (error.code !== "ENOENT") { throw createAssertionError("Test failed"); } }

In this case the error error is caught and used, but it seems legitimate not to reference it in the call expression in the throw statement.

If an error is caught but not used, like in my example in #19913 (comment), then the no-unused-vars should report it already (Playground link), so I think it's okay not to handle that case additionally in this rule.

Right so in the current implementation:

If there are no throws inside a catch block, that is a valid case.

If there is a throw and it does not use the cause error, that's an invalid case. This could be a function or a direct instantiation of an Error. If the function creates an error that does not support cause property, the rule could be disabled for that line.

If the cause property is used, but it is not same as caught error, that's an invalid case.

If there are throw statements, and the caught error is not being accessed, that's an invalid case.

Makes sense generally, but I'm not sure if it's a good idea to report on generic function or constructor calls. That means users will have to use a disable comment each time an error that doesn't support a cause is thrown in a catch block. If we want to enable the rule on arbitrary error types besides built-in errors, I think it would be better to use an inclusion/exclusion list to allow exceptions for certain error types with a central setting.

fasttime · 2025-07-10T06:34:51Z

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

AggregateError expects options as a third parameter, so this case should be valid:

try { throw new Error("Original error"); } catch (error) { throw new AggregateError([], "Lorem ipsum", { cause: error }); }

this should be invalid:

try { throw new Error("Original error"); } catch (error) { throw new AggregateError([], { cause: error }); }

Yes, I totally forgot about this. I've made some changes to handle the args differently specifically for AggregateError, but if this is too common, we might have to implement a more intelligent way. Also added a test case.

fasttime · 2025-07-10T06:46:00Z

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

For this case the rule is reporting a suggestion with syntax errors:

try { doSomething(); } catch (error) { throw new Error(); }

I think it's okay not to report a suggestion in this case. Otherwise, the suggestion should set an empty message (throw new Error("", { cause: error })).

Ahh, nice catch!

I've fixed this use case, and added the following test case:

{ code: `try { } catch (err) { { throw new Error(); } }`, errors: [ { messageId: "missingCause", suggestions: [ { messageId: "includeCause", output: `try { } catch (err) { { throw new Error("", { cause: err }); } }`, }, ], }, ], }

Thanks, can we also add a test case for this code?

try { } catch (err) { throw new AggregateError(); }

…output`

Amnish04 · 2025-07-13T22:10:51Z

@fasttime @nzakas I've made some changes addressing recent comments. Please let me know if we need to handle some cases differently.

snitin315 · 2025-07-15T12:28:32Z

docs/src/rules/preserve-caught-error.md

+
+Examples of **incorrect** code for this rule:
+
+```js


Suggested change

```js

```js

/* eslint presever-caught-error: "error" */

snitin315 · 2025-07-15T12:28:48Z

docs/src/rules/preserve-caught-error.md

+
+Examples of **correct** code for this rule:
+
+```js


Suggested change

```js

```js

/* eslint presever-caught-error: "error" */

snitin315 · 2025-07-15T12:38:48Z

lib/rules/preserve-caught-error.js

@@ -0,0 +1,307 @@
+/**


Right now we report the following which is a false positive:

try { // ... } catch (err) { const opts = { cause: err } throw new Error("msg", { ...opts }); // Error: There is no `cause` attached to the symptom error being thrown // or throw new Error("msg", opts) }

I think in such cases we can just ignore the node instead of reporting a false positive, because it might be a lot of work to detect if the object referred has a cause property or not.

Amnish04 requested a review from a team as a code owner July 4, 2025 19:43

github-project-automation bot added this to Triage Jul 4, 2025

github-project-automation bot moved this to Needs Triage in Triage Jul 4, 2025

eslint-github-bot bot added the feature This change adds a new feature to ESLint label Jul 4, 2025

github-actions bot added the rule Relates to ESLint's core rules label Jul 4, 2025

Amnish04 force-pushed the feat/preserve-caught-error-rule branch from f15c629 to ee5a39c Compare July 4, 2025 20:23

fasttime moved this from Needs Triage to Triaging in Triage Jul 5, 2025

fasttime reviewed Jul 5, 2025

View reviewed changes

docs/src/rules/preserve-caught-error.md Show resolved Hide resolved

fasttime moved this from Triaging to Implementing in Triage Jul 5, 2025

fasttime added accepted There is consensus among the team that this change meets the criteria for inclusion contributor pool labels Jul 5, 2025

fasttime reviewed Jul 7, 2025

View reviewed changes

lib/rules/preserve-caught-error.js Outdated Show resolved Hide resolved

lib/rules/preserve-caught-error.js Outdated Show resolved Hide resolved

lib/rules/preserve-caught-error.js Outdated Show resolved Hide resolved

docs/src/rules/preserve-caught-error.md Show resolved Hide resolved

fasttime reviewed Jul 7, 2025

View reviewed changes

packages/js/src/configs/eslint-recommended.js Outdated Show resolved Hide resolved

JoshuaKGoldberg requested changes Jul 8, 2025

View reviewed changes

github-actions bot added cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features labels Jul 9, 2025

Amnish04 force-pushed the feat/preserve-caught-error-rule branch from f4dbd84 to 3b40bf8 Compare July 9, 2025 19:44

fasttime reviewed Jul 10, 2025

View reviewed changes

Amnish04 added 4 commits July 12, 2025 14:59

Write documentation for preserve-caught-error rule

b3fbe5e

Remove unsupported headings from docs

d72e15a

Setup initial unit test cases

eee04b6

Implement preserve-caught-error rule

7c0264f

Amnish04 added 27 commits July 12, 2025 14:59

Fix incorrect formatting

9f6131a

Do not use bare urls in docs

4113461

Add proposal-error-cause to further_reading

761c5c6

Add further reading links metadata

1c7b3a8

Improve error message for when further links metadata is missing

fb90f01

Add type definition for preserve-caught-error

b85b0ca

Auto-generate tsdoc comment

d587584

Mark preserve-caught-error as recommended

cdfe925

Add preserve-caught-error to recommended config

1f89490

Update rule tsdoc

153e450

Only use file path in further reading links error message

6e4a0a6

Only enable the rule in eslint-plugin-eslint for now

fdddb1d

Fix edge cases and add tests

0495d60

Fix formatting

a6d25eb

Pretty format test cases with consistent indents between code and `…

fc2e152

…output`

Get rid of manual throw statement search

35b307f

Add back lost comment

5c0bb21

Remove customErrorTypes option for now

f47371f

Fix issues caught by preserve-caught-error

3979858

Require function calls with throws to use the caught error

afbda4f

Fix createExtraneousResultsError to accept a cause error

2b7f199

Add error creating functions case to docs

4504483

Handle autofix case where there is no message argument

04396a2

Fix formatting

70e1e88

Handle AggregateError arg positioning exception

58c862c

Fix AggregateError suggestion

7f19321

Fix AggregateError test case

6986a62

Amnish04 force-pushed the feat/preserve-caught-error-rule branch from d4231db to 6986a62 Compare July 12, 2025 19:04

snitin315 reviewed Jul 15, 2025

View reviewed changes

	"no-extend-native": Linter.RuleEntry<
	[
	Partial<{
	exceptions: string[];
	}>,
	]
	>;

Uh oh!

feat: add preserve-caught-error rule #19913

Are you sure you want to change the base?

feat: add preserve-caught-error rule #19913

Conversation

Amnish04 commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites checklist

What is the purpose of this pull request?

What changes did you make?

Is there anything you'd like reviewers to focus on?

Uh oh!

netlify bot commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for docs-eslint ready!

Uh oh!

Amnish04 commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fasttime left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Amnish04 commented Jul 6, 2025

Uh oh!

Amnish04 commented Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fasttime Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amnish04 commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amnish04 commented Jul 9, 2025

Uh oh!

fasttime Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feat: add `preserve-caught-error` rule #19913

feat: add `preserve-caught-error` rule #19913

Amnish04 commented Jul 4, 2025 •

edited

Loading

netlify bot commented Jul 4, 2025 •

edited

Loading

Amnish04 commented Jul 4, 2025 •

edited

Loading

Amnish04 commented Jul 6, 2025 •

edited

Loading

fasttime Jul 9, 2025 •

edited

Loading

Amnish04 commented Jul 8, 2025 •

edited

Loading

fasttime Jul 10, 2025 •

edited

Loading

fasttime Jul 10, 2025 •

edited

Loading