-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Comments
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'); |
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"); |
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 Looking at the example, if when computing the type of 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 However, if the opposite happens, i.e. signature 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. In the case of signature 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. |
Re: how having |
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"); |
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:
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! |
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 I upgraded a project from TS 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);
Is there a suggested work-around for this issue at this time? |
Like @dylangrandmont, I’ve been encountering this issue pretty frequently in my own code in the case of a 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
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; |
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 tsc:
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. |
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:
tsc
reports this error:* 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
⏯ 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.🙂 Expected behavior
tsc
should not consider above code to have an error.tsc
.Analysis
I'm not sure why but this seems related to the use of the union
HTMLCanvasElement | OffscreenCanvas
for the variablecanvas
.Both interfaces in the union contain a set of method overloads for
getContext
...lib.dom.d.ts
Based on these method overload signatures I'd expect the statement
canvas.getContext('webgl')
to use the union of these two overrloads:Which I believe would reduce down to look like this:
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 beRenderingContext | null
.RenderingContext | null
is returned by the base method signature for getContext() in HTMLCanvasElement:So it would seem
tsc
somehow is inferring the wrong override while TS Language Server / VSCode is inferring the correct one.The text was updated successfully, but these errors were encountered: