TypeScript Language Server False Positive Error Reporting

[DScheglov]

Does this issue occur when all extensions are disabled?: Yes

• VS Code Version: 1.87.0
• OS Version: Ubuntu 22.04

Steps to Reproduce:

1. Clone the project git clone https://github.com/DScheglov/res-test.git
2. Open the VS Code: code res-test
3. Install dependencies (in terminal): npm i
4. Open file ./src/index.ts

The following errors are shown:

These errors are false positive:

1. project is configured to use the same typescript compiler as a project. See .vscode/settings.json
2. project is compiled successfully via terminal: npx tsc (no errors)
3. errors are gone after TS Language Server is restared (Ctrl + Shift + P , TypeScript: Restart TS Server)
4. errors go back after any touch of the file (just add a space).

There is not such problem in WebStorm

(Sorry, I have to check)

Errors are not displayed in the TS Playground as well:
Playground

[RyanCavanaugh]

Minimized repro

export interface Result<T, E> {
  mapErr<F>(fn: (error: E) => F): Result<T, F>;
  [Symbol.iterator](): Generator<E, T>;
}

declare const okIfObject: (value: unknown) => Result<Record<string, unknown>, 'ERR_NOT_AN_OBJECT'>;
declare const okIfInt: (value: unknown) => Result<number, 'ERR_NOT_AN_INT'>;
export declare function Do2<T, E>(job: () => Generator<E, T>): void;

declare let value: unknown;
Do2(function* () {
  const object = yield* okIfObject(value).mapErr(
    error => 0
  );

  const age = yield* okIfInt(object.age).mapErr(
    error => 0
  );

  return { age };
});

[DScheglov]

@RyanCavanaugh

I've met similar error from eslint:

Shoud I register the bug in @typescript-eslint? Or there it will be fixed after this one?

[RyanCavanaugh]

I'm not sure how their rule works. If it depends on type information (looks like it does) then that error should go away after we fix this one.

[Andarist]

Analyzing Ryan's simplified repro the problem comes from encodedSemanticClassifications-full that is called before semantic diagnostics. It breaks the regular order of type checking since while collecting tokens it calls reclassifyByType on object's declaration. That reads its type and to do that it has to resolve the type of the yield expression. That is a function call that has to be inferred (speaking about .mapErr here) and that depends on the contextual type of the yield operand, that in turn requires the return type of the containing function to be computed which leads to computing type of the return expression and that depends on... object's type. That's how the circularity arises.

Failing test case for this situation:

/// <reference path='fourslash.ts'/>

// @strict: true
// @target: esnext
// @lib: esnext

//// export interface Result<T, E> {
////   mapErr<F>(fn: (error: E) => F): Result<T, F>;
////   [Symbol.iterator](): Generator<E, T>;
//// }
////
//// declare const okIfObject: (
////   value: unknown,
//// ) => Result<Record<string, unknown>, "ERR_NOT_AN_OBJECT">;
////
//// declare const okIfInt: (value: unknown) => Result<number, "ERR_NOT_AN_INT">;
////
//// export declare function Do2<T, E>(job: () => Generator<E, T>): void;
////
//// declare let value: unknown;
////
//// Do2(function* () {
////   const object = yield* okIfObject(value).mapErr((error) => 0);
////   const age = yield* okIfInt(object.age).mapErr((error) => 0);
////   return { age };
//// });

verify.encodedSemanticClassificationsLength('2020', 132);
verify.getSemanticDiagnostics([]);

[Andarist]

I spent some time on this to learn what the proper fix could be. I concluded that the only proper fix for this has to be about making the cycle resolution logic smarter - or for the "out of order" type resolution to alter its behavior somehow.

No extra flags passed down by the caller can fix this because the compiler doesn't always control the caller. For instance, I managed to somewhat improve the situation locally for the test case above but that didn't fix quick info problem at the very same location:

/// <reference path="fourslash.ts" />

// @strict: true
// @target: esnext
// @lib: esnext

//// export interface Result<T, E> {
////   mapErr<F>(fn: (error: E) => F): Result<T, F>;
////   [Symbol.iterator](): Generator<E, T>;
//// }
////
//// declare const okIfObject: (
////   value: unknown,
//// ) => Result<Record<string, unknown>, "ERR_NOT_AN_OBJECT">;
////
//// declare const okIfInt: (value: unknown) => Result<number, "ERR_NOT_AN_INT">;
////
//// export declare function Do2<T, E>(job: () => Generator<E, T>): void;
////
//// declare let value: unknown;
////
//// Do2(function* () {
////   const object/*1*/ = yield* okIfObject(value).mapErr((error) => 0);
////   const age = yield* okIfInt(object.age).mapErr((error) => 0);
////   return { age };
//// });

verify.quickInfoAt("1", "const object: Record<string, unknown>");

One of those calls checker.getTypeAtLocation while the other one calls checker.getTypeOfSymbolAtLocation. The problem with any attempt to fix this by passing extra information through callers/links/whatever is that it won't fix raw API calls like this: checker.getTypeOfSymbol(checker.getSymbolAtLocation(node)).

Perhaps there is a way to fix this by overriding resolutionStart at some moment conditionally but I don't know what exact moment that would be or what the condition guarding this should be 😉 A part of the idea could be that outer inference should always start before type resolution for a node nested in it under regular circumstances - maybe this fact can be leveraged to manipulate the resolution arrays or the resolutionStart variable

[RyanCavanaugh]

Additional repro noted at #57429 (comment)

[DScheglov]

@RyanCavanaugh
I've found one more bug in TypeScript, but I'm not sure if it is related to this one. Could you please check:
#57903

[mikearnaldi]

Tracking as https://github.com/Effect-TS/effect is also hit by this

[DScheglov]

@mikearnaldi

Yep, I was surprised that I found the bug before the Effect team did.
Could you please check if #57903 is also relevant for Effect-TS?

[mikearnaldi]

@mikearnaldi

Yep, I was surprised that I found the bug before the Effect team did. Could you please check if #57903 is also relevant for Effect-TS?

the reason we didn't find the bug before is that we were using an adapter function so instead of yield* x it would have been yield* $(x) for some obscure reason the adapter variant doesn't raise the issue

[mikearnaldi]

#57903 doesn't affect Effect as we don't use async generators (provided I've understood the issue correctly)

[jakebailey]

I tried an absolutely terrible idea; in getTypeOfNode, I checked to see if the node is a child of a generator function, then do getReturnTypeOfSignature(getSignatureFromDeclaration(containingFunction)). This fixed the test case in #57585 (comment) without breaking any other tests, and is likely not slower because that info is cached (we're just forcing it to resolve early). But that doesn't work for the test case from #57429 (comment), and I'm sure there are other paths that could hit that.

[mikearnaldi]

I tried an absolutely terrible idea; in getTypeOfNode, I checked to see if the node is a child of a generator function, then do getReturnTypeOfSignature(getSignatureFromDeclaration(containingFunction)). This fixed the test case in #57585 (comment) without breaking any other tests, and is likely not slower because that info is cached (we're just forcing it to resolve early). But that doesn't work for the test case from #57429 (comment), and I'm sure there are other paths that could hit that.

Would it be possible to get a snapshot with this idea to test in the effect codebase if it solves all the instances of this issue?

[jakebailey]

Sure; watch #58323 for a build once the pipeline completes.

[mikearnaldi]

Unfortunately it seems like it doesn't fix the issue, rather it does but it makes it appear in a different place, the return of the generator

It does appear to be the case only if there is no specified return, adding a return keyword makes the issue disappear

[jakebailey]

Interestingly, a modified #57585 (comment) to omit the return like the above is also fixed by my hack, which implies that the error being produced above is a cycle caused by some other query than semantic tokens. tsserver trace logs ("typescript.tsserver.log": "verbose") would be interesting here to figure out what path happened to be turned into a test case.

[mikearnaldi]

the full code to repro this is:

import { Effect } from "effect"

Effect.gen(function* () {
  const a = yield* Effect.succeed(0)
  const b = yield* Effect.succeed(1)
})

the bug presents while you edit, like adding a new line, making changes in general

[mikearnaldi]

Not sure if helpful but you can find the log from tsserver here: https://gist.github.com/mikearnaldi/fc2c4f15135b034d62f0625de82557f4

[jakebailey]

Yeah, that log's really long (the error doesn't happen until message 1020), so would need something shorter. That and you're using a language service plugin, which we generally treat as "voiding all warranties" when it comes to us trying to figure out what's wrong. But I don't think it matters in this case (but do try without it).

I can't get that example to reproduce myself, though, only with the return present...

[mikearnaldi]

Much shorter log without the plugin: https://gist.github.com/mikearnaldi/70037e879777f1f3243d0496559c2a99

To reproduce delete the second line:

const b = yield* Effect.succeed(1)

and re-type it manually, error should appear.

I wonder if I can use replay.io to record a session here cc @Andarist

[jakebailey]

That log looks truncated; the main reason for the log is to find the steps to get to the bad error, so we need steps from the first message all the way to the error to be able to write an equivalent test which touches everything in the right way. (Hence why a full log and a short one is helpful.)

I still wasn't able to make it reproduce when typing it out by hand, though... It's possible my editor did not make the same sequence of calls, of course.

[mikearnaldi]

Try a few times to delete and retype, it's sporadic, doesn't happen always. Will provide a full log tomorrow

[Andarist]

Is there a strong need to provide an extra repro for this if we already have 3 compiler tests written for this here? ;p

[jakebailey]

I figured that would be helpful given I fixed the cases that looked effect-y, but it didn't help in the real world? That to me implied that there are still more cases missing, but sorry for the noise.

[ahejlsberg]

I working on a fix for this, I'll put something up later this afternoon.