Enhancement: [only-throw-error] allow re-throwing a caught error #10376

ronami · 2024-11-23T12:20:58Z

Before You File a Proposal Please Confirm You Have Done The Following...

I have searched for related issues and found none that match my proposal.
I have searched the current rule list and found no rules that match my proposal.
I have read the FAQ and my problem is not listed.

My proposal is suitable for this project

I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/only-throw-error

Description

only-throw-error (and prefer-promise-reject-errors) disallow throwing non-error values. only-throw-error (and with #10375 prefer-promise-reject-errors too) can be configured to allow throwing unknown and any types in addition to error values.

It's necessary to enable allowThrowingUnknown so re-throwing a cached error is valid (or any in some tsconfig.json configurations):

function test() {
  try {
    // ...
  } catch (error) {
    if (x) {
      throw new Error()
    }

    // this would otherwise fail
    throw error;
  }
}

While configuring allowThrowingUnknown makes the above pass, it allows potentially unsafe throwing of an unknown type:

function foo(): unknown {
  // not necessarily returns an `Error` object
}

function test() {
  // this should fail
  throw foo();
}

This issue proposes an additional option, a stricter form of allowThrowingUnknown, which will allow re-throwing a caught error.

This is relevant both to only-throw-error and prefer-promise-reject-errors.

Fail

function foo(): unknown {
  // not necessarily returns an `Error` object
}

function test() {
  // this should fail
  throw foo();
}

Pass

function test() {
  try {
    // ...
  } catch (error) {
    if (x) {
      throw new Error()
    }

    throw error;
  }
}

Additional Info

No response

The text was updated successfully, but these errors were encountered:

bradzacher · 2024-11-23T13:10:02Z

Counter argument to your request - it's no safer really and can lead to ugly behaviour:

try {
  // imagine this is in a 3rd party library
  throw 'lol oops';
} catch (e) {
  // this seems safe and wouldn't error in the rule despite it being unsafe.
  throw e;
}

ronami · 2024-11-23T17:40:42Z

Counter argument to your request - it's no safer really and can lead to ugly behaviour:

try {
  // imagine this is in a 3rd party library
  throw 'lol oops';
} catch (e) {
  // this seems safe and wouldn't error in the rule despite it being unsafe.
  throw e;
}

Yes, it's not as safe as using the rule without any options, but I think it is safer than having allowThrowingUnknown (or allowThrowingAny in some cases) enabled. It allows a specific error handling pattern rather than allowing throwing any unknown type, regardless of where it came from.

I don't know if it's too niche, though.

kirkwaiblinger · 2024-11-23T18:08:50Z

I am +1.

Partially handling/logging, then rethrowing, an error is a valid pattern that does happen. In other languages (for more technical reasons to do with stack traces) there is even a dedicated throw; or raise statement (taking no operand) for this purpose, and if JS had such a thing, we probably wouldn't think twice about the validity of using it. Propagating a potentially non-error exception is a different - and much less dev-friendly to avoid - thing than creating one.

This is largely analogous to #8538, where propagating a bad void is treated differently than creating a bad void.

This probably makes sense as an option here too.

JoshuaKGoldberg · 2024-11-24T16:50:40Z

I like this idea in theory, but don't think it can be done well practically. Rules targeting specific syntax uses are brittle to refactors. There's no real difference in developer intent between re-throwing directly inside a catch vs. re-throwing indirectly, such as inside a helper:

function doDangerousWork(work: () => void) {
  try {
    work()
  } catch (error) {
    logAndRethrow(error);
  }
}

function logAndRethrow(error: unknown) {
  console.log("oh no", error);
  throw error;
}

For that reason I'm -1 on this issue. My suggestion would be to reuse the existing allowThrowingAny and allowThrowingUnknown options as described in the OP.

kirkwaiblinger · 2024-11-25T15:27:09Z

I like this idea in theory, but don't think it can be done well practically. Rules targeting specific syntax uses are brittle to refactors. There's no real difference in developer intent between re-throwing directly inside a catch vs. re-throwing indirectly, such as inside a helper:
function doDangerousWork(work: () => void) {
  try {
    work()
  } catch (error) {
    logAndRethrow(error);
  }
}

function logAndRethrow(error: unknown) {
  console.log("oh no", error);
  throw error;
}
For that reason I'm -1 on this issue. My suggestion would be to reuse the existing allowThrowingAny and allowThrowingUnknown options as described in the OP.

I get what you're saying here, but am not swayed. The trouble is allowThrowingUnknown: false is basically just unusable for a large class of valid patterns as-is.

Why should a user who is trying to do this

function doDangerousWork(work) {
  try {
    work()
  } catch (error: unknown) {
    console.error('critical thing failed!')
    throw error;
  }
}

be forced to pick between working around the option for an easily-detectable valid pattern

function doDangerousWork() {
  try {
    work()
  } catch (error: unknown) {
    console.error('critical thing failed!')

    // yuck
    if (error instanceof Error) {
      throw error;
    }
    throw new Error("error wasn't an error", { cause: error });
  }
}

or disabling the option and allowing unrelated, bad code

function doDangerousWork(args: unknown) {
    throw args;
}

?

In effect, it encourages eslint-disable-averse users to write a helper to rethrow, which is objectively worse code.

function rethrow(error: unknown): never {
  // eslint-disable-next-line @typescript-eslint/only-throw-error -- utility to allow rethrowing without linter complaints
  throw error;
}

function doDangerousWork(work) {
  try {
    work()
  } catch (error: unknown) {
    console.error('critical thing failed!')
    rethrow(error);
  }
}

function somethingElse(x: unknown) {
  rethrow(x); // oops - used rethrowing utility outside of error-handling context.
}

Note that another pattern that this messes up is the ability to do conditional behavior in a finally block based on whether an exception was encountered.

function doDangerousWork() {
   let didThrow = false;
   try { 
     work();
   } catch (x) {
      didThrow = true;
      // why do i have to do this when i am making no attempt to 
      // handle the error or throw a new exception? I'm just trying
      // to observe whether there was an exception.
      if (error instanceof Error) {
         throw error;
      } else {
         throw new Error('error wasnt an error', { cause: error });
      }
   } finally {
     doConditionalCleanup(didThrow);
   }
}

I think there's a meaningful distinction between whether an exception is being created or whether it's being rethrown because the language forces you to, and I don't think we should put the responsibility to repair a possibly-invalid exception during an obviously-detectable rethrowing pattern. Especially since exceptions never have valid types in a catch block.

Taking the counterargument ad absurdum, we could even say this line should be flagged since it possibly throws an exception that isn't an error

function doWork() {
   doesntThrow();
}

and therefore every function call in the program should be fixed to

function doWork() {
  try { 
    doesntThrow();
  } catch (e) { 
    if (e instanceof Error) {
      throw e;
    } else {
      throw new Error("non-error Exception", { cause: e });
    }
  }
}

kirkwaiblinger · 2024-11-25T15:34:40Z

So, in conclusion, I'd rather take the downside of having a very, very natural and understandable syntax heuristic (which is probably the only correct syntax anyway to get technically clean stack traces), than the downside of flagging on literally every rethrow.

JoshuaKGoldberg · 2024-12-07T16:12:36Z

Yeah, that's fair. I'm still not in favor of this but I'm not opposed. It does make sense that re-throwing inside an else is a reasonable & reasonably common strategy. Thanks for the pushback 😄.

Accepting PRs, with the note that docs should very clearly describe why the option is super limited.

ronami · 2024-12-07T21:05:21Z

Would it make sense to have this option enabled by default?

I've enabled both only-throw-error and prefer-promise-reject-errors on a couple of projects, and I had to add an eslint-disable-next-line comment on top of several of such cases in each project (or opt-out with the unsafer allowThrowingUnknown option).

This is similar to the original plan to have the option added in #8538 enabled by default in a future major. I think this is a significant pain point in using the rule on a somewhat large codebase.

JoshuaKGoldberg · 2024-12-07T21:20:57Z

🤔 maybe enabled in recommended, disabled in strict? 🤷 I wouldn't be upset by just always having it enabled by default.

kirkwaiblinger · 2024-12-07T23:18:15Z

+1 to just enabled by default always.

@ronami are you asking because you plan to work on this issue? I started working on this today, so I figure I'll share my notes if so; my branch is here: main...kirkwaiblinger:typescript-eslint:only-throw-error-rethrow

ronami · 2024-12-08T09:57:12Z

+1 to just enabled by default always.

@ronami are you asking because you plan to work on this issue? I started working on this today, so I figure I'll share my notes if so; my branch is here: main...kirkwaiblinger:typescript-eslint:only-throw-error-rethrow

Oh, not at all. Please go ahead and continue. This will be helpful to me too on my own projects. Thanks for taking this!

I really enjoyed reading your reasoning in this discussion. I think you've explained why this option is important really well (especially on the difference between creating a bad throw and propagating an existing one). I didn't add any comments because I didn't have anything useful to add, thanks! ❤️

ronami added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Nov 23, 2024

JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Dec 7, 2024

kirkwaiblinger mentioned this issue Apr 15, 2025

feat(eslint-plugin): [only-throw-error] add option allowRethrowing #11075

Merged

3 tasks

kirkwaiblinger mentioned this issue Apr 23, 2025

Enhancement: [prefer-promise-reject-errors] option to allow 'rethrow' of signal reasons or caught values #11095

Open

4 tasks

JoshuaKGoldberg closed this as completed in #11075 May 5, 2025

github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label May 13, 2025

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: [only-throw-error] allow re-throwing a caught error #10376

Enhancement: [only-throw-error] allow re-throwing a caught error #10376

Enhancement: [only-throw-error] allow re-throwing a caught error #10376

Enhancement: [only-throw-error] allow re-throwing a caught error #10376

Comments

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

Link to the rule's documentation

Description

Fail

Pass

Additional Info