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 canceled.

Name	Link
🔨 Latest commit	`b3d4cf5`
🔍 Latest deploy log	https://app.netlify.com/projects/docs-eslint/deploys/68b5d7ba5035960008ab1bcd

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.

Agreed, I've reverted the commit that started reporting cases where function return objects were thrown.

Regarding the inclusion/exclusion list, I did introduce a customErrorTypes option to pass the names of all Error types to be checked, but @JoshuaKGoldberg suggests that strings are not a reliable way to pass types, so I've reverted that for now.
#19913 (comment)

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

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.

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

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.

Yes, thanks for noticing! I did some work to detect all types of options argument including Identifiers, SpreadElements, and ObjectExpressions, but it's too complicated to auto-fix such cases even if detecting is manageable.

So I dropped the idea and started ignoring such cases. I am using a custom UNKNOWN_CAUSE Symbol to communicate such cases across the functions.

Amnish04 · 2025-07-18T21:15:08Z

I've done some work to:

Ignore the cases where function return objects are thrown.
Preserve the existing error options when fixing with suggestions.
Ignore the cases where the error options are too complicated to be analyzed/fixed. For example - Identifiers, SpreadElements etc. Only inline ObjectExpressions are supported in the current implementation.

Also added test cases for the above.

fasttime

LGTM, thanks for the patient work. Happy to see this new rule coming.

Amnish04 · 2025-09-02T16:42:57Z

@fasttime Thanks for all the detailed follow ups! Excited to see this in the next release ^^

* Write documentation for `preserve-caught-error` rule * Remove unsupported headings from docs * Setup initial unit test cases * Implement `preserve-caught-error` rule * Update messageIds in tests * Fix incorrect message id in one of the tests * Handle multiple throw statements and mandate looking at the caught error * Allow passing an option for custom error types * Remove unnecessary comments * Add another invalid case to docs * Remove MDN link from `further_reading` in docs * Add "Options" section to docs * Fix incorrect formatting * Do not use bare urls in docs * Add `proposal-error-cause` to `further_reading` * Add further reading links metadata * Improve error message for when further links metadata is missing * Add type definition for `preserve-caught-error` * Auto-generate tsdoc comment * Mark `preserve-caught-error` as recommended * Add `preserve-caught-error` to recommended config * Update rule tsdoc * Only use file path in further reading links error message * Only enable the rule in `eslint-plugin-eslint` for now * Fix edge cases and add tests * Fix formatting * Pretty format test cases with consistent indents between `code` and `output` * Get rid of manual throw statement search * Add back lost comment * Remove `customErrorTypes` option for now * Fix issues caught by `preserve-caught-error` * Require function calls with throws to use the caught error * Fix `createExtraneousResultsError` to accept a `cause` error * Add error creating functions case to docs * Handle autofix case where there is no message argument * Fix formatting * Handle `AggregateError` arg positioning exception * Fix `AggregateError` suggestion * Fix AggregateError test case * Revert "Require function calls with throws to use the caught error" This reverts commit afbda4f. * Add rule configuration comments to examples * Do not report/analyze complex error options * Preserve existing options when adding cause using fixer * Add test cases to avoid false positives, and existing options preservation * Fix formatting * Remove error factory case from docs * Update docs based on reviews * Don't report cases where throw is not related to caught error * Add `disallowUncaughtErrors` option and autofix for discarded errors * Fix rule type * Rename `disallowUncaughtErrors` to `requireCatchParameter` * Update the docs and use triple-colon fences * Improve examples in docs * Fix custom error false positives and unrelated-error fixer * Update TODO comment to limitation * Start reporting partially lost caught error * Report cases where caught error is shadowed by local declarations * Do not provide a suggestion on partially lost error * Do not provide suggestion when caught error is not referenced * Highlight throw statements individually when catch param is missing * Ignore cases with `SpreadElement` constructor arguments * Make sure existing comments are preserved with suggested fixes * Cleanup and follow up fixes * Handle value and method shorthand edge cases * Fix comment typo

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

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

fasttime reviewed Jul 10, 2025

View reviewed changes

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

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

Amnish04 added 15 commits September 1, 2025 13:28

Rename disallowUncaughtErrors to requireCatchParameter

0ceaebe

Update the docs and use triple-colon fences

8884ab4

Improve examples in docs

0056043

Fix custom error false positives and unrelated-error fixer

5d42e98

Update TODO comment to limitation

e52f51c

Start reporting partially lost caught error

1f41596

Report cases where caught error is shadowed by local declarations

891093a

Do not provide a suggestion on partially lost error

82b11d8

Do not provide suggestion when caught error is not referenced

dc4bdb8

Highlight throw statements individually when catch param is missing

372f26e

Ignore cases with SpreadElement constructor arguments

903a8f8

Make sure existing comments are preserved with suggested fixes

c0f78ae

Cleanup and follow up fixes

32ef6e2

Handle value and method shorthand edge cases

7e77582

Fix comment typo

b3d4cf5

Amnish04 force-pushed the feat/preserve-caught-error-rule branch from 079332a to b3d4cf5 Compare September 1, 2025 17:28

fasttime approved these changes Sep 2, 2025

View reviewed changes

fasttime merged commit e51ffff into eslint:main Sep 2, 2025
29 checks passed

github-project-automation bot moved this from Second Review Needed to Complete in Triage Sep 2, 2025

fisker mentioned this pull request Sep 5, 2025

Rule proposal: prefer-error-cause sindresorhus/eslint-plugin-unicorn#1342

Closed

dyc3 mentioned this pull request Sep 10, 2025

📎 Port preserve-caught-error from eslint biomejs/biome#7469

Open

github-actions bot mentioned this pull request Sep 16, 2025

Bump anchore/scan-action from 6.5.1 to 7.0.0 some-natalie/kubernoodles#410

Merged

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

Uh oh!

feat: add preserve-caught-error rule #19913

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?

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 canceled.

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

Uh oh!

fasttime left a comment

Choose a reason for hiding this comment

Uh oh!

Amnish04 commented Jul 6, 2025

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!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Uh oh!

Amnish04 commented Jul 9, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Amnish04 commented Jul 13, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amnish04 commented Jul 18, 2025

fasttime left a comment

Choose a reason for hiding this comment

Uh oh!

Amnish04 commented Sep 2, 2025

Labels

6 participants

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