Skip to content

Getting recursive type errors with TS 4.3.2 when they don't happen with TS 4.2.4 #44281

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

Closed
xaviergonz opened this issue May 26, 2021 · 20 comments · Fixed by #44615
Closed

Getting recursive type errors with TS 4.3.2 when they don't happen with TS 4.2.4 #44281

xaviergonz opened this issue May 26, 2021 · 20 comments · Fixed by #44615
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@xaviergonz
Copy link

xaviergonz commented May 26, 2021

Bug Report

Given (this is just the smaller repro I could come up with, not real code):

const p = <T>(fn: () => T): T => {return null as any}

const Base = <T>(val: T): {new(): T} => {
  return null as any;
}

class C extends Base({
 x: p<C[]>(() => [])
}) { }

With TS 4.3.2 it gives the errors:

Type 'C' recursively references itself as a base type.ts(2310)
'C' is referenced directly or indirectly in its own base expression.ts(2506)

where as there's no such error with TS 4.2.4

If the p function parameter is changed from fn: () => T to fn: () => any or x: p<C[]>(() => []) is changed to x: p<C[]>(() => [] as any) then it does not give that error anymore.

Real impact:

This impacts a library that used to be able to do self-referenced models:

class TreeNode extends Model({ children: prop<TreeNode[]>(() => []), x: prop(0) }) {}

but with the new version it is not possible anymore

Alternative repro:

abstract class Base2 {
    abstract root() : Derived
}

class Derived extends class extends Base2 {
  root() {
    return undefined as any
  }
}
{}

🕗 Version & Regression Information

  • This changed between versions 4.2.4 and 4.3.2

⏯ Playground Link

Playground Link

@xaviergonz
Copy link
Author

It seems this was introduced in 4.3.0-dev.20210428 (does not happen in 4.3.0-dev.20210427).

More specifically it seems to be introduced by the addition of this line

7748694#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R17241

from a commit from @andrewbranch

@xaviergonz
Copy link
Author

More specifically the call to getBaseTypes on this line seems to trigger some side effect that causes this:

7748694#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R20008

@xaviergonz
Copy link
Author

xaviergonz commented May 28, 2021

The error seems to originate in getBaseConstructorTypeOfClass <- resolveBaseTypesOfClass <- getBaseTypes <- getSingleBaseForNonAugmentingSubtype

  if (!pushTypeResolution(type, TypeSystemPropertyName.ResolvedBaseConstructorType)) {
    return errorType;
  }

I don't think my knowledge is deep enough to dig any deeper :( but my super-wild guess is that the array related optimization in that commit is maybe trying to resolve the class structure earlier than before and therefore the cached versions cannot keep up with it

@evelant
Copy link

evelant commented May 28, 2021

I think this is what's breaking my mobx-state-tree project when upgrading to TS 4.3. Not sure if this is affecting all MST projects or not but I ended up with a ton of recursive type errors after trying to update to TS 4.3.

@dsherret
Copy link
Contributor

Another example showing this bug is in #44284 (I've closed that issue in favour of this one). Just like this issue, it starts happening in 4.3.0-dev.20210428.

@mrgleba
Copy link

mrgleba commented Jun 1, 2021

Another repro

abstract class Base {
    abstract root() : Derived
}

class Derived extends class extends Base {
    root() {
        return undefined as any
    }
}
{   
}

@xaviergonz
Copy link
Author

@DanielRosenwasser

@DanielRosenwasser DanielRosenwasser added the Bug A bug in TypeScript label Jun 1, 2021
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.3.3 milestone Jun 1, 2021
@DanielRosenwasser
Copy link
Member

Unclear if this is working as intended, but it might be a regression.

@xaviergonz
Copy link
Author

xaviergonz commented Jun 10, 2021

has it been decided already if it is a regression or maybe something else?

@colinhacks
Copy link

Not to pile on, but this has broken Zod pretty comprehensively. That said, Zod uses some pretty heinous recursive type hacks so I suppose I wouldn't be surprised if this is intentional.

@vlazh
Copy link

vlazh commented Jun 17, 2021

Still happening with 4.3.3 when don't happen with 4.2.4. Playground

image

@vlazh
Copy link

vlazh commented Jun 17, 2021

Error disappearing if remove

  : A extends Uint8Array | Uint16Array | Uint32Array | Int8Array | Int16Array | Int32Array
  ? Array<number>

but compilation time is about 10x slower than 4.2.4.

@xaviergonz
Copy link
Author

The reproduction cases are still erroring for me after updating to 4.3.3...

@xaviergonz
Copy link
Author

Actually the merged fix code is not in TS4.3.3 at all...

mikkopiu added a commit to espoon-voltti/evaka that referenced this issue Jul 22, 2021
- I've been to hell and back, and haven't managed to pinpoint all of the exact dependencies that cause the build to leak (or more likely: attempt to over-allocate) memory but after extensive testing, this is the set of dependencies that manages to build in an environment with max 3GB of memory available
    - It could very well be that some of the dependencies blocked by this commit's locks simply take the build's memory consumption over the 3GB limit and aren't part of the same issue of over allocating memory -- but this is a decent compromise that ensures that the build should no longer intermittently fail in CI (which has 4GB, of which roughly 3,3GB is usable)
- The test setup:
    - `docker run --rm -it --volume $(pwd):/home/circleci/project --memory=3G cimg/node:14.15` (same image as in CI, and 3 GB of memory ensures that the build will always work on the 4 GB available in CI)
        1. `cd .. && cp -r project frontend` (just to ignore any file permission issues without touching the executor image too much)
        2. `rm -rf dist node_modules`
        3. `yarn install --immutable`
        4. `yarn build`
        5. -> repeat at least 3 times and if all pass (& CI passes) -> OK set of dependency versions
    - 8 parallel CI jobs running to increase sample size with `/sys/fs/cgroup/memory/memory.max_usage_in_bytes` logged
- Findings:
    - _Something_ (or a combination of things) attempts to over allocate memory (specifically new heap space) when running with `master`'s versions of frontend dependencies (likely culprits described below)
        - With the versions used in this commit, the build will use all memory available to it but GC is early enough to never actually go over the limit **but** something in the newer dependencies goes over the line, causing reaping of the build processes and crashing the build
        - The most likely explanation would be something "outside" the Node.js process (i.e. something creates additional processes), which doesn't obey the same heap limits
    - TypeScript 4.3 has a few issues with relating to complex types which lead to at least significantly higher memory usage (likely not the same over-allocation issue as below) -> **lock to ~4.2.x**
        - Related: microsoft/TypeScript#44299 and microsoft/TypeScript#44281
        - This is actually a good idea anyway as TS makes breaking changes in "minor" releases (e.g. 4.2.x -> 4.3.x) and we shouldn't blindly update it
    - webpack's dependency `terser-webpack-plugin@5.1.2` updates `jest-worker` 26 -> 27 which appears to cause memory over allocation, likely related to the way it creates new processes and/or threads for workers based on incorrect assumptions about the available amount of CPU cores inside a Docker container (`os.cpus()` vs. what's actually allocated in cgroups) -> **lock to 5.1.1**
        - There's also a big issue with shared dependencies with `@storybook/*` modules also using webpack (but *4.x.x* instead of 5.x.x), which makes using `resolutions` pretty difficult without locking absolutely everything
        - Related: webpack/webpack#13550 and webpack-contrib/terser-webpack-plugin#403
    - `@types/node@16.x.x` is incompatible with Jest 27 (jestjs/jest#11640) -> update only to 15.x.x
    - `webpack@5.38.0` updates _some_ dependency that either causes a similar memory over-allocation or simply increases the build's memory usage over the set 3GB limit
        - My money's on `webpack-sources` as based on webpack's changelogs it appears to have had some memory leak issues between the versions used by `webpack@5.37.1` and `webpack@5.46.0` (latest at the time of this commit)
     - `ts-node@10.x.x` could also be related but didn't manage to find anything conclusive here
- Additionally, update all dependencies _that don't break the build / cause memory issues_ to the latest non-major versions
    - Excluded Chart.js (& related) dependencies as that whole repo seems in a very weird place now
        - 3.x.x released a while ago and there hasn't been any release notes, tags or anything
        - 3.0.0 diff is pretty massive with very non-descriptive commit/PR messages
    - Excluded `react-select` and `react-datepicker` as they're going to be dropped at some point and both of them appeared to have pretty major changes
    - Excluded Testcafe as that's always a pain & we're going to drop it when everything's been re-written for Playwright
    - Update webpack loaders to their latest (incl. major) versions -- nothing breaking for us
quolpr added a commit to quolpr/harika that referenced this issue Jul 25, 2021
@vlazh
Copy link

vlazh commented Aug 16, 2021

Still happening with 4.4.1-rc.

@vlazh
Copy link

vlazh commented Aug 31, 2021

@ahejlsberg, the issue still happening with 4.4.2. Could you reopen the issue?

@xaviergonz
Copy link
Author

I think you might have better luck if you open a new issue with a playground link to the case that gets broken. as a matter of fact the two reproductions listed in the first post have been fixed in 4.4,so your issue might be because of another reason

@evelant
Copy link

evelant commented Aug 31, 2021

Yeah something is still wrong in 4.4.2 but I'm not certain if it's this issue. My mobx-state-tree models blow up with thousands of errors if I try to upgrade, same as 4.3. I'm still stuck on 4.2.4 for now.

@DanielRosenwasser
Copy link
Member

Would you each be able to open up a separate issue with a minimal repro? In the worst-case, we'll dedupe them.

@vlazh
Copy link

vlazh commented Sep 1, 2021

@DanielRosenwasser, separated #45672

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants