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

Checking errors via @ts-expect-error? #65

Open
rauschma opened this issue Nov 26, 2022 · 6 comments
Open

Checking errors via @ts-expect-error? #65

rauschma opened this issue Nov 26, 2022 · 6 comments
Labels
status: in discussion Not yet ready for implementation or a pull request status: waiting on author Further information is requested type: feature New enhancement or request

Comments

@rauschma
Copy link

In my TypeScript book, I’m using @ts-expect-error with error messages to point out if there are errors – e.g.:

function func(value: unknown) {
  // @ts-expect-error: Object is of type 'unknown'.
  value.toFixed(2);

  // Type assertion:
  (value as number).toFixed(2); // OK
}

Benefit: Very similar to assert.throws() in that type checking doesn’t fail if everything is as expected.

All examples from my book: https://gist.github.com/rauschma/4d1ee06e47e03545d4c4b31b59700786

@JoshuaKGoldberg
Copy link
Owner

JoshuaKGoldberg commented Dec 4, 2022

Sorry for the delay - I like the idea! For visibility, microsoft/TypeScript#19139 is a long-standing feature request on the TypeScript side to add this kind of check natively. It's blocked because TypeScript's error codes and diagnostic message texts are not stable. Having a userland tool such as eslint-plugin-expect-type implement this functionality would be good.

A few things we should figure out...

  • Which format(s) should be supported for diagnostic codes? Examples:
    • // @ts-expect-error 1234 ...
    • // @ts-expect-error ts1234 ...
    • // @ts-expect-error(1234) ...
    • // @ts-expect-error(ts1234) ...
  • Which format(s) should be supported for message text? Examples:
    • // @ts-expect-error message
    • // @ts-expect-error message
    • // @ts-expect-error: message
    • // @ts-expect-error - message
    • // @ts-expect-error -- message
  • Should we support regular expressions on the text?
    • // @ts-expect-error /Object is of type '.+'/

My instinct is to make the rule strict by default so we don't have a plethora of odd user forms. Perhaps only the three following formats:

  • // @ts-expect-error: message
  • // @ts-expect-error(ts1234)
  • // @ts-expect-error(ts1234): message

...where if message starts with /, it's treated as a regular expression.

What do you think @rauschma?

Another side note: microsoft/TypeScript#38370 tracks quirks in the TypeScript side around parsing lines. Beware to any implementer of this issue that it's easy to get the edge cases wrong. (trust me, I know! 😄)

Edit 12/4: Oh, and...

  • @ts-ignore support as well. I'd say we include it. Sometimes folks need to @ts-ignore due to changes across specific TypeScript versions.
  • Adding these to $ExpectError. I'm in favor - it'd feel weird to leave the existing syntax out of the features.

@JoshuaKGoldberg JoshuaKGoldberg added type: feature New enhancement or request status: waiting on author Further information is requested status: in discussion Not yet ready for implementation or a pull request labels Dec 4, 2022
@rauschma
Copy link
Author

rauschma commented Dec 4, 2022

For reference, this is what an error message looks like in the TS Playground:

Property 'hello' does not exist on type '"abc"'.(2339)

This is what it looks like at the command line (tsc):

main.ts:1:7 - error TS2339: Property 'hello' does not exist on type '"abc"'.

Thoughts:

  • Supporting @ts-ignore: I don’t feel strongly about this. Why not!
  • “Adding these to $ExpectError. I'm in favor - it'd feel weird to leave the existing syntax out of the features.”
    • Do you mean same syntax as @ts-expect-error? Makes sense.
  • The error code should be optional and it should probably be in CLI format because that’s easier to web-search and it’s clearer what the number means.
    • I’m not sure if I want the error number to be a prefix or a suffix. I’m also unsure how often I’d include the error number at all.
  • For the error message, I would only support plain text matching (no regular expressions).
    • It should be possible to break up the error message across multiple lines. Sequences of whitespace could be normalized to a single space (needed for TypeScript’s output, too?).
  • I find it useful to indicate via [...] that I abbreviated an error message. Two options (I slightly prefer 1):
    1. The expected message must always match in full – unless there is an [...]. If there is an error code, completely omitting the error message would still be allowed.
    2. The actual error message must only start with the expected error message and the [...] is optional.
  • Mentioning error messages in this kind of check, will be slightly brittle, but I don’t mind that: Complaining too much about examples in documentation is not that problematic. It forces me to keep the examples up to date.

Examples (suffixed error numbers):

// @ts-expect-error: Property 'hello' does not exist on type '"abc"'. (TS2339)
// @ts-expect-error: (TS2339)
// @ts-expect-error: Property 'hello' does not exist [...] (TS2339)
// @ts-expect-error: Property 'hello' does not exist [...]

Examples (prefixed error numbers):

// @ts-expect-error TS2339: Property 'hello' does not exist on type '"abc"'.
// @ts-expect-error TS2339
// @ts-expect-error TS2339: Property 'hello' does not exist [...]
// @ts-expect-error: Property 'hello' does not exist [...]

What are your thoughts?

@bradzacher
Copy link

bradzacher commented Dec 4, 2022

Note that there's no api in TS to speculatively ask questions. This is important because it means that the diagnostics and types you get are what you get.

Put another way - if TS suppresses an error - it's gone.

You would need to manually re-check the code without the suppression in order to get the error. Which would make this a lot harder (and slower) to do than with a custom comment.

I'd assume this is why they used custom comments in the dtslint project.

@rauschma
Copy link
Author

rauschma commented Dec 4, 2022

@bradzacher Right! That reminds me of the workaround that I used when I implemented similar functionality via the tsc API: I replaced each @ts-expect-error in the code with %ts-expect-error and then collected errors.

@danvk
Copy link
Collaborator

danvk commented Dec 4, 2022

Here's the code in literate-ts that I used to match errors in Effective TypeScript. The sequence was:

  • look for an exact match
  • look for the error text in the comment to be a substring of the compiler error
  • try to get a match by letting "..." in the comment text match anything in the compiler error

IIRC, this never worked perfectly, but worked well enough to be useful in final editing.

https://github.com/danvk/literate-ts/blob/50393398f12252e3985b37d9e4b007a7a626ea4b/src/ts-checker.ts#L72-L138

@JoshuaKGoldberg
Copy link
Owner

This limitation is a little irksome. Filed microsoft/TypeScript#51749.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: in discussion Not yet ready for implementation or a pull request status: waiting on author Further information is requested type: feature New enhancement or request
Projects
None yet
Development

No branches or pull requests

4 participants