eslint-plugin-expect-type icon indicating copy to clipboard operation
eslint-plugin-expect-type copied to clipboard

Checking errors via `@ts-expect-error`?

Open rauschma opened this issue 2 years ago • 6 comments
trafficstars

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

rauschma avatar Nov 26 '22 12:11 rauschma

Sorry for the delay - I like the idea! For visibility, https://github.com/microsoft/TypeScript/issues/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: https://github.com/microsoft/TypeScript/issues/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 avatar Dec 04 '22 05:12 JoshuaKGoldberg

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?

rauschma avatar Dec 04 '22 12:12 rauschma

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.

bradzacher avatar Dec 04 '22 20:12 bradzacher

@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.

rauschma avatar Dec 04 '22 20:12 rauschma

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

danvk avatar Dec 04 '22 22:12 danvk

This limitation is a little irksome. Filed https://github.com/microsoft/TypeScript/issues/51749.

JoshuaKGoldberg avatar Dec 04 '22 22:12 JoshuaKGoldberg