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

Inconsistent errors reported between tsc and VSCode (Unions across objects with method overload definitions) #53614

Open
frank-weindel opened this issue Mar 31, 2023 · 9 comments
Labels
Bug A bug in TypeScript Cursed? It's likely this is extremely difficult to fix without making something else much, much worse Help Wanted You can do this
Milestone

Comments

@frank-weindel
Copy link

frank-weindel commented Mar 31, 2023

Bug Report

This is similar to #52813, in spirit. However the errors, how it's reproduced, and the code causing it are pretty different.

Basically I have a code snippet that appears 100% error-free* (most of the time, see note below) in the VSCode IDE. Logically in my head the code should be error free too. However, when I run tsc I get an error inside the snippet.

Here's the code:

export function createWebGLContext(
  canvas: HTMLCanvasElement | OffscreenCanvas,
): WebGLRenderingContext | null {
  return (
    canvas.getContext('webgl')
  )
}

tsc reports this error:

createWebGLContext.ts:4:3 - error TS2322: Type 'RenderingContext | null' is not assignable to type 'WebGLRenderingContext | null'.
  Type 'CanvasRenderingContext2D' is missing the following properties from type 'WebGLRenderingContext': drawingBufferHeight, drawingBufferWidth, activeTexture, attachShader, and 428 more.

4   return (
    ~~~~~~~~
5     canvas.getContext('webgl')
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
6   )
  ~~~

* In the larger project that I originally discovered this issue, I actually do sometimes see the error in the IDE. I haven't figured out how to consistently cause it, but restarting the TS server or making a small change and saving the file generally clears the error.

🔎 Search Terms

inconsistency, inconsistent, errors, ts language server, tsc

🕗 Version & Regression Information

  • Attempted on 5.0.3 and latest dev build (typescript@5.1.0-dev.20230331)

⏯ Playground Link

With the Playground, since it uses elements of VSCode under the hood, the snippet appears valid with no errors.
Playground link with relevant code

You can also clone this repo or start a Github CodeSpace on it to reproduce the errors when running tsc:
https://github.com/frank-weindel/ts-53614-ide-tsc-inconsistency

🙁 Actual behavior

  • tsc considers the above code to have an error.
  • VSCode normally and properly does not show that error, but it occasionally can.

🙂 Expected behavior

  • tsc should not consider above code to have an error.
  • VSCode should be consistent with tsc.

Analysis

I'm not sure why but this seems related to the use of the union HTMLCanvasElement | OffscreenCanvas for the variable canvas.

Both interfaces in the union contain a set of method overloads for getContext...

lib.dom.d.ts

// ...
interface HTMLCanvasElement extends HTMLElement {
    // ...
    getContext(contextId: "2d", options?: CanvasRenderingContext2DSettings): CanvasRenderingContext2D | null;
    getContext(contextId: "bitmaprenderer", options?: ImageBitmapRenderingContextSettings): ImageBitmapRenderingContext | null;
    getContext(contextId: "webgl", options?: WebGLContextAttributes): WebGLRenderingContext | null;
    getContext(contextId: "webgl2", options?: WebGLContextAttributes): WebGL2RenderingContext | null;
    getContext(contextId: string, options?: any): RenderingContext | null;
    // ...
}
// ...
interface OffscreenCanvas extends EventTarget {
    // ...
    getContext(contextId: "2d", options?: any): OffscreenCanvasRenderingContext2D | null;
    getContext(contextId: "bitmaprenderer", options?: any): ImageBitmapRenderingContext | null;
    getContext(contextId: "webgl", options?: any): WebGLRenderingContext | null;
    getContext(contextId: "webgl2", options?: any): WebGL2RenderingContext | null;
    getContext(contextId: OffscreenRenderingContextId, options?: any): OffscreenRenderingContext | null;
    // ...
}
// ...

Based on these method overload signatures I'd expect the statement canvas.getContext('webgl') to use the union of these two overrloads:

getContext(contextId: "webgl", options?: any): WebGLRenderingContext | null; // from HTMLCanvasElement
getContext(contextId: "webgl", options?: WebGLContextAttributes): WebGLRenderingContext | null; // from OffscreenCanvas

Which I believe would reduce down to look like this:

getContext(contextId: "webgl", options?: any): WebGLRenderingContext | null;

Where the return type is WebGLRenderingContext | null. Which is what we see if we hover over the method name in the Playground or VSCode.

However, tsc inferred the return type of the method to be RenderingContext | null.

RenderingContext | null is returned by the base method signature for getContext() in HTMLCanvasElement:

getContext(contextId: string, options?: any): RenderingContext | null;

So it would seem tsc somehow is inferring the wrong override while TS Language Server / VSCode is inferring the correct one.

@RyanCavanaugh
Copy link
Member

It's worse than this; there's a tsc ordering sensitivity as well originating in the same problem

declare let cv: HTMLCanvasElement | OffscreenCanvas;
declare let oc: OffscreenCanvas;

// Swapping these two unrelated lines changes errorness
oc.getContext('webgl');
const g: WebGLRenderingContext | null = cv.getContext('webgl');

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Apr 7, 2023

Here's a reduced version that doesn't need the DOM types

interface Alpha {
    foo(type: "num", opts?: unknown): number;
    foo(type: "str", opts?: unknown): string;
    foo(type: string, opts?: unknown): number | string;
}

interface Beta {
    foo(type: "num", opts?: { a?: string }): number;
    foo(type: "str", opts?: { b?: string }): string;
    foo(type: "num" | "str", opts?: unknown): number | string;
}

declare let b: Beta;
// Commenting this line out causes an error on the bottom line
const b_res = b.foo("str");

declare let ab: Alpha | Beta;
const x: string = ab.foo("str");

@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript Cursed? It's likely this is extremely difficult to fix without making something else much, much worse labels Apr 7, 2023
@gabritto
Copy link
Member

gabritto commented Apr 10, 2023

The problem in the minimal repro is a combination of two things: first, that overloaded signatures are order-dependent, so depending on the order of the component types Alpha and Beta in the internal representation of the union Alpha | Beta, we get a different order for the resulting signatures of foo on type Alpha | Beta.

Looking at the example, if when computing the type of ab.foo, signature foo(type: "str", opts?: { b?: string }): string occurs before signature foo(type: "str", opts?: unknown): string in the union type, then everything works correctly and the error doesn't happen, because we end up with the following signature list for ab.foo (after processing each union component's signatures):

type CallSignatures2 = {
    (type: "num", opts?: { a?: string | undefined; } | undefined): number
    (type: "str", opts?: { b?: string | undefined; } | undefined): string
    (type: "num" | "str", opts?: unknown): string | number
    (type: "num", opts?: unknown): string | number
    (type: "str", opts?: unknown): string | number
}

So the ab.foo("str") call matches signature (type: "str", opts?: { b?: string | undefined; } | undefined): string and the return type is string, and everything is fine.

However, if the opposite happens, i.e. signature foo(type: "str", opts?: unknown): string occurs before foo(type: "str", opts?: { b?: string }): string in the union type Alpha | Beta, then we have an issue. We end up with the following signatures for foo:

type CallSignatures = {
    (type: "num", opts?: unknown): string | number // Notice the return type is now `string | number`
    (type: "str", opts?: unknown): string | number // Notice the return type is now `string | number`
    (type: "num", opts?: { a?: string | undefined; } | undefined): number
    (type: "str", opts?: { b?: string | undefined; } | undefined): string
    (type: "num" | "str", opts?: unknown): string | number
}

Here the second factor comes into play: how we process each component type's signatures in a union type to try and get a signature for the whole union.
When you have ab of type Alpha | Beta, and we need to figure out the type of ab.foo, we need to combine Alpha's signatures for foo with Beta's signatures for foo.
The way we do this for a union type, in general, is that:
For each signature s of each component type, we try to find a signature that matches s in each component type in the union. If we find those matching signatures, we combine them (by, among other things, unioning their return types together) into a new signature, and that new signature is what is present in the union type's signatures. If we don't find matching signatures in every component in the union, we discard that signature from the result.

In the case of signature foo(type: "str", opts?: unknown): string from type Alpha, we match it to itself in component Alpha of the union Alpha | Beta, and we match it to signature foo(type: "num" | "str", opts?: unknown): string | number. So as a result, we combine those two matches into signature foo(type: "str", opts?: unknown): string | number and put this result into the foo signatures on type Alpha | Beta. This is because, if you call foo on type Alpha and pass in an opt, you get something of type string, and if you do it on type Beta, you get something of type string | number. However, if you call foo on type Alpha and don't pass an opt argument (because it is optional), then you get type string, and if you call foo on type Beta and don't pass an opt argument, then you'd get type string as a result. But the way we combine signatures in a union isn't precise enough to capture that distinction. (If we split the signature with an optional parameter into two separate signatures, then the issue is fixed. See code here)

So tl;dr I think there are two issues here: (1) when combining signatures from a union type's component, we don't try to order them relative to other components' signatures by how specific their parameter types are, so in the resulting union type's signatures, a less specific signature can appear first, and (2) when finding signature matches for a signature with optional parameters to get a resulting signature in a union, we don't distinguish between that parameter being optional or not, resulting in a less precise signature.

@gabritto
Copy link
Member

Re: how having b.foo("str") changes things, it switches the order of Alpha and Beta in the internal representation of union Alpha | Beta, because it affects the order of the signatures in the union. The same thing happens if you comment that off but switch the order of the declarations Alpha and Beta: the error goes away.

@RyanCavanaugh
Copy link
Member

Further reduced. Will provide more commentary later

interface Alpha {
    foo(n: "str", opt?: boolean): string;
}
// Comment/uncomment to change errorness
type B = Beta['foo'];

interface Beta {
    foo(n: "str", opt?: true): string;
    foo(n: string): number | string;
//  ^^^ <- not the one we should pick to unify
//         with Alpha#foo, sort of.
}

declare let ab: Alpha | Beta;
const x: string = ab.foo("str");

@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Apr 12, 2023
@RyanCavanaugh RyanCavanaugh added the Help Wanted You can do this label Apr 12, 2023
@RyanCavanaugh
Copy link
Member

I was going to try to simplify @gabritto's comment but I think it already is as simple as it gets, modulo the slightly simpler example above.

There are basically only two possible fixes here:

  • Change the signature merging algorithm to be fully order-agnostic. We don't really have any ideas on what that might look like; the current algorithm is sound in both cases but generates different completeness depending on which signature set comes first
  • Change the union ordering algorithm to be more stable. We don't really have any ideas on what that might look like either; any sorting order is liable to introduce other observable instabilities (e.g. if we sorted alphabetically, then renaming a type might cause a type error when there previously wasn't one). Given that the signature merging algorithm is already order-sensitive, enforcing a new sort order here is also liable to be a breaking change in existing code since there may well be hidden dependencies on current behavior

If someone can come up with a good order-agnostic signature merging algorithm that has reasonable performance, we'd welcome a PR.

But given that this has been the behavior since at least 3.3 and probably earlier, and this is the first report, and we don't have any good solutions on the table, this will have to go to the Backlog.

Aside: A much more approachable fix is to just adjust the lib DOM types to make them not repro this problem, possibly by splitting out the optional parameters into separate signatures as suggested above. Also accepting PRs for that!

@dylangrandmont
Copy link

dylangrandmont commented Jun 28, 2023

But given that this has been the behavior since at least 3.3 and probably earlier, and this is the first report, and we don't have any good solutions on the table, this will have to go to the Backlog.

I don't mean to spam but I want to add some visibility to this issue. I'm surprised this hasn't been reported more often since it's quite common for rendering operations in TS to pass around a union like HTMLCanvasElement | OffscreenCanvas.

I upgraded a project from TS 4.3.4 (where I've never encountered this issue) to 5.0.4 where I encountered an equivalent issue from the following snippet

function createCanvas(width: number, height: number) {
  return supportsOffscreenCanvas() ? new OffscreenCanvas(width, height) : document.createElement('canvas');
}

const c = createCanvas();
c.getContext('2d')?.drawImage(img, 0, 0);
Type error: Property 'drawImage' does not exist on type 'OffscreenCanvasRenderingContext2D | RenderingContext'.
  Property 'drawImage' does not exist on type 'ImageBitmapRenderingContext'.

Is there a suggested work-around for this issue at this time?

@controversial
Copy link

Like @dylangrandmont, I’ve been encountering this issue pretty frequently in my own code in the case of a HTMLCanvasElement | OffscreenCanvas union.

I use classes of the form:

export default class Renderer {
  ctx: CanvasRenderingContext2D | OffscreenCanvasRenderingContext2D;

  constructor(
    public canvas: HTMLCanvasElement | OffscreenCanvas,
  ) {
    const ctx = canvas.getContext('2d');
    if (!ctx) throw new Error('Could not get 2D context from canvas');
    // fails unpredictably
    this.ctx = ctx;
  }

  draw() {
    this.ctx.fillStyle = 'red';
    this.ctx.fillRect(0, 0, 100, 100);
  }
}

throughout several work projects. Since, like the original commenter, the issue doesn’t always appear in my editor (vscode), my experience ends up usually being that a build fails basically at random and without warning, and it’s hard for me to predict whether TypeScript will infer the type of ctx

  • correctly, as CanvasRenderingContext2D | OffscreenCanvasRenderingContext2D | null or
  • too broadly, as OffscreenCanvasRenderingContext2D | RenderingContext | null

The best workaround I have is just to use type assertion:

// https://github.com/microsoft/TypeScript/issues/53614
// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
const ctx = canvas.getContext('2d') as CanvasRenderingContext2D | OffscreenCanvasRenderingContext2D | null;

@edobrb
Copy link

edobrb commented Mar 14, 2024

I've got a similar error on my project. VSCode seems to infer the correct type but tsc infer another type and throws an error.

from VSCode:
image

from tsc:

src/functions/wallet.functions.ts:103:13 - error TS2322: Type '{ _count: string; }' is not  assignable to type '"asc" | "desc" | undefined'.

103       const test1: typeof test = { _count: 'asc' }

The type is derived from a library like Typebox, zod, io-ts and is recursive

I've tried with versions 5.3.3 and 5.4.2. If needed I could provide more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Cursed? It's likely this is extremely difficult to fix without making something else much, much worse Help Wanted You can do this
Projects
None yet
Development

No branches or pull requests

6 participants