Skip to content

Bug: [only-throw-error] add an 'allow' option using the TypeOrValueSpecifier shape #9525

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
4 tasks done
andsouto opened this issue Jul 9, 2024 · 7 comments · Fixed by #10221
Closed
4 tasks done
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@andsouto
Copy link

andsouto commented Jul 9, 2024

Before You File a Bug Report Please Confirm You Have Done The Following...

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=5.5.2&fileType=.tsx&code=PQKhCgAIUglBTADgJ3gZ3gOwC5stgC3kiIEMATeZSAYwHtMAzASwHMBXZU7ZhyOxDwZ5GdaqUyQAEgBUZABUioAju3TYAdFBgBJTGmwSa6SKVSRmAW0vtDAIwA28DZACydcs0YBPZplaQlvCEHnio2JySpLQODPDk2hb6hpjGkADuzIT4RLQEEqzOkDK5Ymx%2BpA78dgBW8DTYFniY8ABuVHkF8VrQiQACiOyOzDQAgojM2sDg8AAeiGKNlDQOZsQrpGh4UtjYiFLwFFR4AN5QkKAQkNe6OFSYlYGkiPyMkLHpVDSbxGSU1A8gnhsHRIK1Kmo0D0btNrihmODsL9Dv80ABuc6Xc7QSB6JHIB5VSzPV7vOifZDfDDkEgojqAkwgnLETBiYkOZgAL3i2JgomQllJhBZpCCkAAFMLIPzBRgsNLmMgDABKaHQWGQeGIllsypc%2BIAOVF6Ax1yx1xxAGE6JZEE4kcz3qROd4kllmHrOdxeJI6G9CMw8HRavVGuKWvF4pA7PB%2BcRUBQ-KxVbyNVruMRVi69FlTRcwLzIABFNRqGnsRDkDPA0Exp74j0c7k0qUtWaNPzuz3ehhqkBp5AIjNOl0AVUrGbzl2u1uSyHYDTw0Ra6WkckUfw6wbqDQyWQIjtYCPl4IckI0-fO9DnC5ByHFm6VAH4AFyQAyD-yQAA%2BkDOFsgABtBk3w-JMAF1QOwT8Al-TB2EsGNqF-cUwK-OCEKQ5VAPAvMAF8f2kOklWVKcCwtGBLSIGgAGsRDESA5kDJFUmIP1TEgI92kkR8%2B0LAYzFFSAGWKXJH2E418FBGhqJo6UGKYgwsGMPiKMgPpwkiPAZHnYgvEdcTFNwAAaaVKgwfhhWQTIMD7DV8jQcNjSgmDlTfOw6DoJwJDIq4bjgYJBzaRlchYJVGlPNRSWiLj5V43l%2BMQQTBREkpkSOAFjVU-yNOCLTROISLiDQgJ9KlQzZmYtBTPghwqjoKybPgVNzkKbAnKCFykzc99oKTQjaocXzCwQPrguBXIGSDf0xOIqEErU3KIgJPBRneZjSXEqa7POGj4G8RyepKnDhrU0agvaJd1oMUkivo8ROOPHjiOynEBK4FLJLS2kMoksVGGQG0MgIEYDyZcILsKiF0FemAlvytaStu6G8DK2bfqM6rhPYOrLKIazA2cFrrja0Y6o6%2BAuv8I6%2Bv8HCBpxobMXI-zxkQLByCulcwWhqTHSM-qMEadi7vkh7H0LCQaU0lbmUFPwOJWOIhVKQcj0JJIDCMImKMS5K-uIb6tsk-lgdBvnnnZzAW1yO6%2B3UpKPp5s9Ddt3mmUtjnYfUmX9EgNalZaFXiFkBQftRaod0aTJsilIrTEQK3KBbUEpVi56Mp265PetimqdYUyivzwjjvAnqdj2A4MvRZm-JxABlYIg2oSwPC8ZgTGiePTZip7w46BXokDtiZrYtWKiqPwtdY%2B2dFH-vxAcBNyFdTHTKyPB48DJQkFWYwaRjsHcnT521ELBWpV9qNt1Db33qE1L0f%2BA37fvwV4%2B%2B%2BOGNFpkhf4ag6DtGQIOSgYtDx93irrRavtVoxGVuxKUocNxzUjqGPcsdJrwHSA4V0f9xJFSzu%2BYIedeowULtDYuv5S7l12PsOap1-IABF4D2hMKLHuj1uILySIrWIQcEGq3KBrKeKQVILRyo7B%2BX0n70iynrJ2H83Yu3-qfRkoJKCsLAWnCBL1xFvRgf7OB-D55IIXkGEMu5D7gK4fHDRwRujE0gHYpEJCSrkJdq%2BUh-UqG01YDhGhld6HnHTA6Yk3gYyN2wAaXUTZDTGjzCEvSmBczBMHNqWgAhvAADFAaWASWk4cw98lDgdJbHB44qxImKekoWVd-gAFEcBBRrnhcAMx5iLCcfUVY5gsAIWkLQ%2Bp3FsAyG8OzP8tdCzfRUJCaOmwiE4H4LYfgQDHSZFQIQxuiyAC8kAAAMxlJlqVGJICssQKCakBqwVAWxGLDIyPM1AxhjzkG9tEpEb5voAAMsnBBkgAIVILRDmXynF0HQJgAA5I0NAFYFjIEaCgOg1z0BhCQJ0vgZy6AUHmhRDU45znkHkFcm5eBdkAEZDlmhZjiaZ6AFj6GKoYCIeB6CgKlmYjIVB4z1HgC8whCA0AMowHUjouyABMVL8x1xgGtcg5JMCEsuci0ldysBzLRc89orzHGMIVYS4lKrUWQF2QAZileafy31GCMx3kK4QSSVjsE8F%2BKUHkV4PM1Xy7VAr6UOpNZAAALBamlsraDsAMEDYKiyAZAwkEkfExhBAMQYtEOwQK9rW0IaODA1BdkAFZwCtPafCpY3S1imDsB%2BIFjQNi3IrogQVwr4CAosv%2BaVhYyZVBuc2sxhDl4MBwWYt8DbRVKgYTiJt-qtYsoyZQftKJB24OZRGt88FEJUAnTAGQcwIiPEoGgGgg5BA%2BlJD26dK7WUeHgKZSgjBSA41wHzAA8gAaW9nq4SDUulJ34JIAMuKYTnAHYq5d3AI07vbPnLdkBRywAADLBztXQTgaQIYd21aZBig0LBvFZI0Ug4JmCrEcDrID1wQNDs4A4Sh2M6owYAOpECso6GdEa53EHvXVVG-7cjitmLMJQXQF0UCXfwGi7lPLeUwDB0Z4yBE8vtYymqZhAafBToxfcHQpQ2txuexlKjyq6LxcBxdoH8BjMpgMvYQz1VyecFOgzv4G22ZwPZjQjmRXERg-XCsVAAC014Py3gYj3PTfrGWAd6Gpb6aAkxOE1Mlex4gaBJqRDSbe8bOw8G7EIHimwCAuBOa6JF7MEUdzQIWBTdre1JS2FGKyxBMi40AVQEBxApR3ofQ4CKKNCFBegiF%2B82W3ztuuI%2BNAnjR1BIAmxyba7MKbuxL1cDaBIPYE8SVPM1xqObd8fhW9sZuvYHrpezx66kKHfvY%2B07q31t7dchiYtcxS1dI2OYOt2xBnALEJ54gu6OZfb2H91telbROCCDgPA9SfvUHbZR10IldkACIXOw7%2B8jvMCPAiotIIUaDpnRPmba2IN8EhXQYXo0c-yMPAZKlMOYFoqy6A0VIN4Uy0bgbyilHNjjTQeFSn44J2FqXjVcH8GR9UhP5XmZZwGrjGA8wDfnA0MQ4oRsTIAiT5AnjyfbbMVN2hY6a6zbOwtjdyB9dzfuwTgCu3beQDwqRItbTwA2tSHl-A6hxTKk1zkNTwksHWcQLT37EWMDihOE7p7bSkQGB92iIAA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6AeyfkNvwAto7AO71og6OiiIx7CeDABfEAqA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

Repro Code

export class HttpErrorResponse implements Error {
  readonly name = "HttpErrorResponse";
  readonly message: string;
  readonly error: any | null;
  readonly ok = false;
  constructor(public status: number) {}
}

function test() {
  throw new HttpErrorResponse(404);
}

test();

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/only-throw-error": "error"
  },
};

tsconfig

{
  "compilerOptions": {
    "strictNullChecks": true
  }
}

Expected Result

I would expect an error is not detected

Actual Result

Expected an error object to be thrown. 11:9 - 11:35

Additional Info

The code of the linked playgroung is copied from angular type declarations. It seems angular is implementing instead of extending the Error interface for their HttpErrorResponse and that eslint is not recognising this. AFAIK rule should not trigger an error as thrown object is implemeting the Error interface.

@andsouto andsouto added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Jul 9, 2024
@kirkwaiblinger kirkwaiblinger changed the title Bug: [rule name here] <short description of the issue> Bug: [only-throw-error] should allow throwing objects that implement the Error interface Jul 9, 2024
@kirkwaiblinger
Copy link
Member

Hi @andsouto, thanks for the report! This is an interesting edge case.

My initial reaction is to regard it as a bug that the Error interface is implemented, not extended, see also #9370. But, I realize that that's something that occurs in a third party library, not one which you have control over modifying...

It may be reasonable to address this by creating an option allowing a list of types to specified as permissible to be thrown, similar to what we've done recently for no-floating-promises (see https://typescript-eslint.io/rules/no-floating-promises/#allowforknownsafepromises).

We'll see what other team members say.

@Josh-Cena
Copy link
Member

We should probably add an allow option for it. There are similar examples such as Remix throwing redirect responses.

@JoshuaKGoldberg
Copy link
Member

such as Remix throwing redirect responses

...why would they do this?

I can see an argument for allowing something that happens to implement a shape like Error - an ErrorLike, if you will. But what possible good reason would one have for throwing something else?

@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented Jul 9, 2024

...why would they do this?

This reminds me of https://h3.unjs.io/guide/event-handler as well (which, for instance, powers Nuxt.js), where throwing is basically used for relatively ordinary control flow, with custom data. Now, it looks like createError does return an Error subclass in this case (see the source code), but I don't think it needs to for their usage, and I'd regard that as an implementation detail subject to change. In other words, I think it would be reasonable for someone to want to throw createError({}) in this context even if createError() didn't return an Error subclass (or even if it didn't implement Error/ErrorLike at all and were just a specific kind of object).

@Josh-Cena
Copy link
Member

Josh-Cena commented Jul 9, 2024

But what possible good reason would one have for throwing something else?

There are two reasons I can think of, in general:

  1. You want to have a two-way distinction between completion values, but you don't want to force everyone into the tagged union pattern.
  2. You want to let the value pierce through the call stack to someone who cares (algebraic-effect-style).

For example, see https://remix.run/docs/en/main/route/error-boundary

@JoshuaKGoldberg 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 Aug 22, 2024
@JoshuaKGoldberg JoshuaKGoldberg changed the title Bug: [only-throw-error] should allow throwing objects that implement the Error interface Bug: [only-throw-error] add an 'allow' option using the TypeOrValueSpecifier shape Aug 22, 2024
@kirkwaiblinger
Copy link
Member

Note that there was a duplicate under the old rule name #6226

@github-actions 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 Nov 12, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants