Skip to content

fix: insert cause outside wrapping parens in preserve-caught-error#21062

Merged
fasttime merged 3 commits into
eslint:mainfrom
MahinAnowar:fix/preserve-caught-error-paren-args
Jul 8, 2026
Merged

fix: insert cause outside wrapping parens in preserve-caught-error#21062
fasttime merged 3 commits into
eslint:mainfrom
MahinAnowar:fix/preserve-caught-error-paren-args

Conversation

@MahinAnowar

@MahinAnowar MahinAnowar commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Prerequisites checklist

AI acknowledgment

  • I did not use AI to generate this PR.
  • (If the above is not checked) I have reviewed the AI-generated content before submitting.

What is the purpose of this pull request? (put an "X" next to an item)

[X] Bug fix

What changes did you make? (Give an overview)

The preserve-caught-error suggestions insert , { cause: err } with fixer.insertTextAfter(argumentNode, ...). AST node ranges exclude wrapping parentheses, so for a parenthesized argument the text lands inside the parens and produces a sequence expression:

try { foo(); } catch (err) { throw new Error(("wrap")); }
// suggestion output before this fix:
throw new Error(("wrap", { cause: err }));

That makes the options object the error message ("[object Object]") and attaches no cause at all — re-linting the suggestion output reports the same problem again. The new AggregateError((errs)) variant is worse: the suggested code throws a runtime TypeError because the sequence expression's value isn't iterable.

This adds a small getLastArgumentToken helper that walks past the argument's wrapping close-paren tokens (stopping before the call's own closing paren) and uses it in the three argument-appending suggestion paths. Suggestion output is now:

throw new Error(("wrap"), { cause: err });
throw new AggregateError((errs), "", { cause: err });

Same bug class as #21045 (no-implicit-coercion, merged a few days ago).

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

Three new RuleTester cases assert the exact suggestion output for parenthesized message, parenthesized errors array, and parenthesized message on AggregateError; they fail on the current code and pass with this change. All 52 rule tests pass.

@MahinAnowar
MahinAnowar requested a review from a team as a code owner July 6, 2026 15:19
@github-project-automation github-project-automation Bot moved this to Needs Triage in Triage Jul 6, 2026
@eslint-github-bot eslint-github-bot Bot added the bug ESLint is working incorrectly label Jul 6, 2026
@netlify

netlify Bot commented Jul 6, 2026

Copy link
Copy Markdown

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 270c2b4
🔍 Latest deploy log https://app.netlify.com/projects/docs-eslint/deploys/6a4dfcd8a04744000782e121
😎 Deploy Preview https://deploy-preview-21062--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

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

@linux-foundation-easycla

linux-foundation-easycla Bot commented Jul 6, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: MahinAnowar / name: Mahin Anowar (fe1f3d1)

@github-actions github-actions Bot added the rule Relates to ESLint's core rules label Jul 6, 2026
}`,
],
invalid: [
/* Parenthesized arguments: insertions must land outside the wrapping parentheses (see gh suggestion corruption) */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These added test cases should not be the first test cases as they are special cases for the fixer.
Further the note from the comment "(see gh suggestion corruption)" seems to be missing context. I think "Parenthesized arguments" is enough in this context.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved them to the end of the invalid cases and shortened the comment to just "Parenthesized arguments" — you're right, the extra note didn't add anything.

@DMartens

DMartens commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

In the future please follow our AI policy which requires an accepted issue first for AI-assisted contributions.

@MahinAnowar

Copy link
Copy Markdown
Contributor Author

Understood, sorry about that — I read the acceptance criteria too quickly. I'll open an issue and wait for it to be accepted before any future PR here.

@fasttime fasttime moved this from Needs Triage to Triaging in Triage Jul 7, 2026
@fasttime

fasttime commented Jul 7, 2026

Copy link
Copy Markdown
Member

I'm able to reproduce this in ESLint v10.6.0, so marking as accepted.

@fasttime fasttime added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jul 7, 2026
@fasttime fasttime moved this from Triaging to Evaluating in Triage Jul 7, 2026

@fasttime fasttime Jul 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test case for when there's a trailing token after the parenthesized argument, like this?

try {
    doSomething();
} catch (err) {
    throw new Error(("Something failed"),);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added — the fixer inserts after the wrapping paren and before the trailing comma, so it comes out as throw new Error(("Something failed"), { cause: err },); which parses fine. Test asserts that exact output.

@fasttime fasttime moved this from Evaluating to Implementing in Triage Jul 7, 2026

@fasttime fasttime left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@fasttime
fasttime merged commit 0d7d70c into eslint:main Jul 8, 2026
42 checks passed
@github-project-automation github-project-automation Bot moved this from Implementing to Complete in Triage Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

3 participants