Skip to content
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

Error in catch clause should be unknown #26174

Closed
4 tasks done
felixfbecker opened this issue Aug 3, 2018 · 5 comments
Closed
4 tasks done

Error in catch clause should be unknown #26174

felixfbecker opened this issue Aug 3, 2018 · 5 comments
Labels
Fixed A PR has been merged for this issue

Comments

@felixfbecker
Copy link
Contributor

Search Terms

catch unknown type

Suggestion

Today I had another situation where I would have wanted #13219, but a less complex feature that would have prevented the bug as well would have been if the error in catch clauses had the new unknown type instead of any, forcing me to narrow it down before accessing properties.

Ideally this would be the same for the Promise rejection type.

Use Cases

Making error handling more type safe. Currently any prevents type narrowing.

Examples

This is a real world example of a bug:

    let pipeline: Pipeline
    try {
        ;({ body: pipeline } = await buildkiteClient.post('organizations/sourcegraph/pipelines', {
            body: buildkitePipeline,
            json: true,
        }))
    } catch (err) {
        if (
            err.error &&
            Array.isArray(err.error.errors) &&
            err.errors[0] &&
            err.errors[0].field === 'name' &&
            err.errors[0].code === 'already_exists'
        ) {
            console.log(`Buildkite pipeline ${repoName} already exists, skipping creation`)
            pipeline = await buildkiteClient.get(`organizations/sourcegraph/pipelines/${repoName}`)
        } else {
            throw err
        }
    }

Very easy to miss in code review - it should have been err.error.errors. This is an error returned by a real API.

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript / JavaScript code (with a flag)
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. new expression-level syntax)
@mhegazy
Copy link
Contributor

mhegazy commented Aug 3, 2018

This is only possible if #25720, #10715, #25172, and #21732 are implemented. otherwise it is a massive breaking change.

@mhegazy mhegazy added Suggestion An idea for TypeScript Revisit An issue worth coming back to labels Aug 3, 2018
@donaldpipowitch
Copy link
Contributor

I'd love to see this as well. I just made a PR to get this for Reacts componentDidCatch (DefinitelyTyped/DefinitelyTyped#30068), because it would have prevented a real world bug in our code and looked for an equivalent issue for the native try/catch.

@falsandtru
Copy link
Contributor

I want to enable this change by a compiler option. Or want to declare only unknown type like catch (reason: unknow) { }.

@gerardolima
Copy link

gerardolima commented May 5, 2021

I believe the default inferred type for catch (err) { } should be unknown, which it is, instead of any which is unsafe and highly discouraged. unknown is safer and provides better semantics; I really don't understand why this is not the default for tsc.

please see: typescript-eslint/typescript-eslint#3348

@wbt
Copy link

wbt commented Dec 17, 2021

It appears this has been implemented in #41013 and should likely be closed.
https://devblogs.microsoft.com/typescript/announcing-typescript-4-4-beta/#use-unknown-catch-variables

@DanielRosenwasser DanielRosenwasser added Fixed A PR has been merged for this issue and removed Suggestion An idea for TypeScript Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

8 participants