Skip to content

Out of memory checking recursive mapped type with two constraint type references #21048

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
mquandalle opened this issue Jan 6, 2018 · 18 comments
Labels
Bug A bug in TypeScript
Milestone

Comments

@mquandalle
Copy link

mquandalle commented Jan 6, 2018

edit: better description of the issue in #21048 (comment)


Hello,

I have the following Node error in one of my project using Typescript:

<--- Last few GCs --->

[6455:0x3fcc3c0]    44547 ms: Mark-sweep 1410.5 (1465.1) -> 1410.4 (1449.1) MB, 1625.7 / 0.0 ms  (+ 0.0 ms in 0 steps since start of marking, biggest step 0.0 ms, walltime since start of marking1626 ms) last resort GC in old space requested
[6455:0x3fcc3c0]    46098 ms: Mark-sweep 1410.4 (1449.1) -> 1410.5 (1449.1) MB, 1551.3 / 0.0 ms  last resort GC in old space requested


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x2198fd4a5ee1 <JSObject>
    2: getTypeListId(aka getTypeListId) [/home/project/node_modules/typescript/lib/typescript.js:~30911] [pc=0x3ee74d14a111](this=0x28fe3da82311 <undefined>,types=0x29df387ac9c9 <JSArray[4]>)
    3: /* anonymous */(aka /* anonymous */) [/home/project/node_modules/typescript/lib/typescript.js:~31668] [pc=0x3ee74d35e07d](this=0x28fe3da82311 <undefined>,t...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory
 1: node::Abort() [node]
 2: 0x121a2cc [node]
 3: v8::Utils::ReportOOMFailure(char const*, bool) [node]
 4: v8::internal::V8::FatalProcessOutOfMemory(char const*, bool) [node]
 5: v8::internal::Factory::NewRawOneByteString(int, v8::internal::PretenureFlag) [node]
 6: v8::internal::Factory::NewStringFromOneByte(v8::internal::Vector<unsigned char const>, v8::internal::PretenureFlag) [node]
 7: v8::internal::Factory::NumberToString(v8::internal::Handle<v8::internal::Object>, bool) [node]
 8: v8::internal::Runtime_NumberToString(int, v8::internal::Object**, v8::internal::Isolate*) [node]
 9: 0x3ee74d00463d

I've spend a lot of time trying to fix it by using various versions of Node, Typescript (including latest dev version), and Webpack. Is there any method I can use to understand this issue?

@electricessence
Copy link

Would be helpful to have a bit more context. What, when, where?
Do you have any tsconfig.json settings?
Can you point to what you are trying to compile/run?

@mhegazy mhegazy added the Needs More Info The issue still hasn't been fully clarified label Jan 8, 2018
@mquandalle
Copy link
Author

Yes sorry I wasn't able to provide a more detailed bug report in the first place, I was more looking at techniques to deal with this kind of Node error related to the Typescript compilation.

I have now identified the file and the typing dependency (@types/ramda) causing the issue in my project. I made a minimal clonable repository demonstrating the issue: https://github.com/mquandalle/typescript-issue-21048

@mhegazy mhegazy added Bug A bug in TypeScript and removed Needs More Info The issue still hasn't been fully clarified labels Jan 24, 2018
@mhegazy mhegazy added this to the TypeScript 2.7.1 milestone Jan 24, 2018
@sandersn
Copy link
Member

sandersn commented Jan 25, 2018

Here is a smaller repro that doesn't depend on ramda and is easier for me to read:

interface A {
  x: PartialDeep<A>;
  calls: number | { };
}
type PartialDeep<T> = { [P in keyof T]?: T[P] & PartialDeep<T[P]> };

declare var deep: PartialDeep<A>;
declare var xdeep: { x: PartialDeep<A> };
deep = xdeep;

@sandersn
Copy link
Member

sandersn commented Jan 25, 2018

I haven't spent much time in the debugger yet, but notably

  1. calls needs to have a type that's a union with an object type.
  2. The template type of PartialDeep needs to be T[P] & PartialDeep<T[P]>.
  3. [Updated] There must be some assignment of type { x: PartialDeep<A> } to PartialDeep<A>.

@sandersn
Copy link
Member

The basic type of problem is checking assignability of an infinitely expanding type, in this case "is <T2>(b: T2) => { x: Deep } & T2 assignable to F?". We try to make this work in two ways:

  1. Recognising when further expansion of a recursive type will fail to inform the result of assignability.
  2. Giving up after a certain recursion depth.

In both approaches, assignability checking continues to the next property, having failed to obtain a positive or negative result from checking the infinitely expanding one. However, (1) only works in specific situations; in general it's hard to say that a comparison will not produce additional information. One example is below. (2) only works when there is one (or a few) infinite expansion. (1) definitely isn't applying here, and (2) looks to be defeated because of the first two points in my previous comment: a reasonably complex type A and a mapped type that expands infinitely for every property.

It might be possible to come up with a rule that falls under category (1), something like "when comparing PartialDeep<A> ==> PartialDeep<PartialDeep<A>>, continue to the next property because you won't learn anything from this comparison."

@sandersn
Copy link
Member

In the meantime, a workaround is to change the partial-deep type:

type PartialDescriptor<T> = { [P in keyof T]?: T[P] & PartialDescriptor<T[P]> };
// remove T[P]
type PartialDescriptor<T> = { [P in keyof T]?: PartialDescriptor<T[P]> };

The first definition is wrong anyway, and is the source of the infinite expansion.

PartialDescriptor<{ x: { y: number } }> is equivalent to { x?: { y: number } & { y?: number } }, but this type requires y. I assume this definition is written to work around the deep-mapped type skipping signatures:

{ x: { f(): void } would become { x?: { f?: {} } with the simple definition vs { x?: { f(): void } & { f?: {} } } with the complex one.

But there is no way to make only non-methods become optional in the current version of typescript. I think it is possible with conditional types, which might be in 2.8.

By the way, if the current, incorrect definition was good enough to not notice the incorrectness, a non-deep mapped type is even better: you can just use Partial.

@electricessence
Copy link

electricessence commented Jan 27, 2018

So I've installed [email protected] and (still) when my build runs it takes forever and then displays the fatal error. I'm guessing this is because (as you may be suggesting) there is some recursion in the type checking. What can I do to avoid this?

I'm pretty confident I know where this is happening in my code, but it would require changing the code in a way that is undesirable.

In order to maintain modularity and not have to load the Linq library but still provide the convenience of having a .linq property on all collections. I had to mirror the interfaces used by the library. Maybe I'm doing this all wrong. If anyone would be willing to critique, it might be helpful:

Here are the interfaces:
https://github.com/electricessence/TypeScript.NET/blob/master/source/System.Linq/Enumerable.d.ts

Here is the actual classes:
https://github.com/electricessence/TypeScript.NET/blob/master/source/System.Linq/Linq.ts

Here is where the interfaces are consumed:
https://github.com/electricessence/TypeScript.NET/blob/master/source/System/Collections/CollectionBase.ts#L17

I'm pretty confident if there wasn't a need for the interfaces themselves then this issue would go away for me. The way to make this all work the way I'm intending is that the .linq property should still expose the types inside Linq.ts, but not end up loading the classes unless used.

See the .linq and .linqAsync methods here:
https://github.com/electricessence/TypeScript.NET/blob/master/source/System/Collections/CollectionBase.ts#L429-L494

It ends up being quite painful keeping the classes in sync with the interfaces.

@mquandalle
Copy link
Author

My particular issue is now solved, the T[P] & PartialDeep<T[P]> wasn't needed in my case.

@sandersn sandersn changed the title JavaScript heap out of memory Out of memory checking recursive mapped type with two constraint type references Feb 12, 2018
@sandersn
Copy link
Member

@electricessence Do you have a self-referential mapped type, particularly one with a template type that contains two references to the constraint type? What I remember from looking at Typescript.net was that the interfaces and classes you reference are all (1) large (2) self-referential and (3) almost identical, but not quite. This is indeed a recipe for breaking our structural type checking, but it’s a different issue than this specific mapped type problem. The general solution would be one of the two approaches I described above, although approach (1), recognizing general relations between types, is probably only feasible if we missed something in our earlier caching rules, because there aren’t a lot of algebraic relations between OO-style classes+interfaces.

@electricessence
Copy link

@sandersn I think you are correct. As I mentioned, I wish i knew a good hack to not have to actually import references but still have their types. :/ Maybe my brain isn't working and it's just something I missed.

If I remove the interfaces, the compilation problem goes away, but I need the interfaces to prevent the classes from automatically importing. :(

@sandersn
Copy link
Member

I took another look at the code for .linq.
@Andy-MS pointed out that if your users import the classes directly and only use them as types, then no import statement gets emitted. However, the classes will still be available for construction at any time, and if the user does happen to construct one, then a real import statement gets emitted. Maybe you could make it hard to construct the classes directly, for example by making constructors private?

@sandersn
Copy link
Member

Thanks @mquandalle. I'll keep this bug open to track the explosion of types that occurs with a mapped type of this form.

@electricessence
Copy link

@sandersn Sorry if this ends up polluting the thread as I still think it's related. Are you saying that if you don't call 'new' then the types don't get imported? If so that would be great. That's what I need. I need to import without actually importing the file.

@ghost
Copy link

ghost commented Feb 16, 2018

Any import not used as a value will be elided. There's some info at https://www.typescriptlang.org/docs/handbook/modules.html .

@electricessence
Copy link

So easy. This worked. THANK YOU. Solved everything. :)

@sandersn
Copy link
Member

@electricessence if you don't mind keeping a bad branch (or tag) of TypeScript.NET around, it's a very useful stress test for "large classic OO library". In other words, we would like to use it as a test case if we can ever compile it without crashing. :)

@electricessence
Copy link

@sandersn...

I missed tagging it but this is the last commit before the fixes: electricessence/TypeScript.NET@fbb5f9d

So to clarify, this was a management nightmare because I thought the interfaces had to exist to prevent importing the classes. Maintaining the interfaces was incredibly painful. That decision was a long time ago. It is doing now what I really wanted which was to import the types without the classes/code.
The current compilation time is now VERY FAST. It's only the silly addition of the interfaces references that killed it. Huge thank you. I'm now officially on 2.7.2 :)

Anything I can do to help, just ask.

sandersn added a commit that referenced this issue Mar 15, 2018
Recursive mapped types usually lead to the error "excess stack depth
comparing types" because of a new type relation rule added in #19564
which says that "A source type T is related to a target type { [P in
keyof T]: X } if T[P] is related to X".

Unfortunately, with self-recursive mapped types like

```ts
D<T> = { [P in keyof T]: D<T[P]> }
```
we get infinite recursion when trying to assign a type
parameter T to D<T>, as T[P] is compared to D<T[P]>, T[P][P] is compared
to D<T[P][P]>, and so on.

We can avoid many of these infinite recursions by replacing occurrences
of D in the template type with its type argument. This works because
mapped types will completely cover the tree, so checking assignability
of the top level implies that checking of lower level would succeed,
even if there are infinitely many levels.

For example:

```ts
D<T> = { [P in keyof T]: D<T[P]> | undefined }
<T>(t: T, dt: D<T>) => { dt = t }
```

would previously check that `T[P]` is assignable to `D<T[P]> |
undefined`. Now, after replacement, it checks that `T[P]` is assignable
to `T[P] | undefined`.

This implementation suffers from 3 limitations:
1. I use aliasSymbol to detect whether a type reference is a
self-recursive one. This only works when the mapped type is at the top
level of a type alias.
2. Not all instances of D<T> are replaced with T, just those in
intersections and unions. I think this covers almost all uses.
3. This doesn't fix #21048, which tries to assign an "off-by-one"
partial-deep type to itself.

Mostly fixes #21592. One repro there has a type alias to a union, and a
mapped type is a member of the union. But this can be split into two
aliases:

```ts
type SafeAnyMap<T> = { [K in keyof T]?: SafeAny<T[K] };
type SafeAny<T> = SafeAnyMap<T> | boolean | string | symbol | number | null | undefined;
```
@mquandalle
Copy link
Author

The simple reproduction provided in #21048 (comment), now raise the following clean error instead of crashing the compiler, so I consider the problem solved:

error TS2615: Type of property 'x' circularly references itself in mapped type 'PartialDeep<PartialDeep<A>>'.

9 deep = xdeep;
  ~~~~~~~~~~~~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants