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

[3.9] Complex types truncated in declaration files #38557

Closed
ostrowr opened this issue May 13, 2020 · 15 comments
Closed

[3.9] Complex types truncated in declaration files #38557

ostrowr opened this issue May 13, 2020 · 15 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@ostrowr
Copy link

ostrowr commented May 13, 2020

TypeScript Version: 3.9.0+ (works in 3.8.x, broken in 3.9.x, 4.0.x)

Search Terms:
truncated, "...", declaration

Code

I'm still working on getting a scoped repro here, and will update this ticket when I have one. But – in TS 3.8 (and earlier), I have a complex type that produces the following code in a .d.ts file:

<-snip->
removed: (import("ts-json-validator/dist/json-schema").JsonValue[] & {
                [k: string]: unknown;
            }[]) | (import("ts-json-validator/dist/json-schema").JsonValue[] & {
                [k: string]: unknown;
            }[] & string) | (import("ts-json-validator/dist/json-schema").JsonValue[] & {
                [k: string]: unknown;
            }[] & number) | (import("ts-json-validator/dist/json-schema").JsonValue[] & {
                [k: string]: unknown;
            }[] & false) | (import("ts-json-validator/dist/json-schema").JsonValue[] & {
                [k: string]: unknown;
            }[] & true) | (import("ts-json-validator/dist/json-schema").JsonValue[] & {
                [k: string]: unknown;
            }[] & {
                [property: string]: import("ts-json-validator/dist/json-schema").JsonValue;
            }) | (import("ts-json-validator/dist/json-schema").JsonValue[] & {
                [k: string]: unknown;
            }[] & readonly import("ts-json-validator/dist/json-schema").JsonValue[]);
<-snip->

In 3.9+, this gets truncated as though we're in an editor environment:

removed: (import("ts-json-validator/dist/json-schema").JsonValue[] & {
                [k: string]: unknown;
            }[]) | (import("ts-json-validator/dist/json-schema").JsonValue[] & {
                ...;
            }[] & string) | ... 4 more ... | (import("ts-json-validator/dist/json-schema").JsonValue[] & ... 1 more ... & readonly import("ts-json-validator/dist/json-schema").JsonValue[]);

Expected behavior:
Types aren't truncated in .d.ts files

Actual behavior:
Types are truncated, producing syntactically invalid .d.ts files

Related Issues:

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label May 14, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.9.2 milestone May 14, 2020
@RyanCavanaugh
Copy link
Member

How big was your .d.ts output in 3.8? We introduced a cap on type display to avoid printing impractically-large types (> 1 million chars)

@ostrowr
Copy link
Author

ostrowr commented May 14, 2020

Almost 2.8 million chars, generated from about 50 lines of code in the corresponding .ts file...

Many of these characters are of the form import("ts-json-validator/dist/json-schema")., so assigning that to some intermediate value would probably save a lot of characters. I don't know if .d.ts supports that.

That's what I get for ridiculous recursive types. The file at issue defines three schemas from https://github.com/ostrowr/ts-json-validator; I'll try splitting them into three files. But 3.9 makes that library pretty useless if there's a cap on type size. Is this value configurable?

@weswigham
Copy link
Member

weswigham commented May 14, 2020

so assigning that to some intermediate value would probably save a lot of characters

We don't do it in the emitter ourselves, however if you write
import * as js from "ts-json-validator/dist/json-schema" at the top of the input file (or specific imports for each of the referenced types!), we'll use the js reference that now exists over serializing the full thing, which can be a workaround for now, if you think that's enough to shave enough characters to get back under the limit.

@ostrowr
Copy link
Author

ostrowr commented May 14, 2020

Thanks! That workaround fixes the issue in my codebase (with some ts-expect-error and eslint-ignores around unused variables) even though the .d.ts is still more than a million characters. Naturally, this isn't a super satisfying fix.

Can you point me to the PR or discussion around introducing this limit? I don't understand why it was introduced, but I'm sure there are plenty of reasons. In any case, compilation should probably fail with a nice error message when the limit is reached, not emit invalid code.

Philosophically, this is similar to my comment here: #35533 (comment) – it's OK for typescript not to be able to do something, but when it can't do something (and is able to detect that it's in that situation) it should yell at the user whenever possible!

@weswigham
Copy link
Member

#37461 - you are correct in that we should consider issuing an error when this occurs, rather than relying on people noticing that the output itself contains a truncation.

@ostrowr
Copy link
Author

ostrowr commented May 15, 2020

Ah, makes sense why your workaround fixed it even though the doc is over a million characters. Looks like the real maxLength is 2 * 1 million.

I'll consider diving in to try to issue an error in this situation, but can't promise I'll get to it soon! It looks a little less trivial than I expected since it seems that this code implements shared functionality for when truncation really is needed.

(Also makes sense why --noErrorTruncation didn't fix the issue for me.)

@weswigham
Copy link
Member

Looks like the real maxLength is 2 * 1 million.

Node truncation starts at 1 million estimated length (and thus stop building synthetic AST nodes), the final string is just also hardcapped at 2 million, in case the estimate and the real final size are very different.

@RyanCavanaugh RyanCavanaugh added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Needs Investigation This issue needs a team member to investigate its status. labels May 20, 2020
@RyanCavanaugh RyanCavanaugh assigned weswigham and unassigned weswigham May 20, 2020
@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels May 20, 2020
@RyanCavanaugh RyanCavanaugh removed this from the TypeScript 3.9.2 milestone May 20, 2020
@ostrowr
Copy link
Author

ostrowr commented May 21, 2020

Re the new labels – I totally understand that a truncated .d.ts file is easier to debug than an OOM – but there are plenty of projects that do generate large (but still very useful) .d.ts files.

I think tsc should never generate invalid code – in fact, I'd rather it crashed.

$0.02 from me: #37461 should be reverted at least until truncation comes with a nice error message (but maybe forever.) Why limit the power of tsc on machines that can handle large-ish files?

If you can't revert, this should added to the list of "breaking changes" in the release notes :)

@Arlen22
Copy link

Arlen22 commented May 25, 2020

Looks like the real maxLength is 2 * 1 million.

Node truncation starts at 1 million estimated length (and thus stop building synthetic AST nodes), the final string is just also hardcapped at 2 million, in case the estimate and the real final size are very different.

Can you point me to the file where this happens? I would like to experiment with some stuff. I am very certain that I've run into this before reaching 1 million characters, even with noErrorTruncation true.

@FSM1
Copy link

FSM1 commented Oct 12, 2020

I recently ran in to this issue thanks to a PartialDeep<I*Override> where IOverrides was using the CSSProperties type

PartialDeep is from typefest

export interface IAvatarOverride {
  root?: CSSProperties
  sizes?: {
    large?: CSSProperties
    medium?: CSSProperties
    small?: CSSProperties
  }
  variants?: {
    square?: CSSProperties
    circle?: CSSProperties
  }
}

which generated

        Avatar?: {
            root?: {
                alignContent?: (string & {}) | "-moz-initial" | "inherit" | "initial" | "revert" | "unset" | "space-around" | "space-between" | "space-evenly" | "stretch" | "center" | "end" | "flex-end" | "flex-start" | "start" | "baseline" | "normal" | undefined;
                alignItems?: (string & {}) | "-moz-initial" | "inherit" | "initial" | "revert" | "unset" | "stretch" | "center" | "end" | "flex-end" | "flex-start" | "start" | "baseline" | "normal" | "self-end" | "self-start" | undefined;
                alignSelf?: (string & {}) | "-moz-initial" | "inherit" | "initial" | "revert" | "unset" | "stretch" | "center" | "end" | "flex-end" | "flex-start" | "start" | "baseline" | "normal" | "self-end" | "self-start" | "auto" | undefined;
                animationDelay?: (string & {}) | "-moz-initial" | "inherit" | "initial" | "revert" | "unset" | undefined;
...

This causes the truncation described in the issue.

I have temporarily reverted that package to TS3.8 and the file generated is ~53MB.

Any ideas for a workaround?

@weswigham
Copy link
Member

Add an explicit type annotation with the type you want, rather than letting the complier print it out in full, for one. Two, the latest version of TS should be issuing an error any time this truncation would occur, so this should be fixed in 4.1 (in the sense that you'll no longer quietly get a broken million plus character declaration file).

@FSM1
Copy link

FSM1 commented Oct 12, 2020

thanks for the suggestions @weswigham. Could you elaborate on the "adding an explicit type declaration" suggestion?

This project is still on TS 4.0.3, so that explains why I didn't get any errors.

@weswigham
Copy link
Member

We only emit types like this when we can't use a type annotation you've written. Wherever the generated type appears in the output, go to the corresponding place in your input, and add a type annotation.

@FSM1
Copy link

FSM1 commented Oct 12, 2020

The type annotations are all there. Its when passing the IAvatarOverride through PartialDeep that these types get expanded.

For now I have loosened the properties that were typed as CSSProperties to Record<string, any>.

@weswigham
Copy link
Member

Somewhere you need to actually write the PartialDeep<IAvatarOverride> annotation yourself, rather than letting the compiler infer it and print it back (where it prints back the expanded form). I'm also going to close this thread, since we do issue an error when this truncation occurs now~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants