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

Trust variance checking during identity checking #30903

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Apr 12, 2019

This reverts the substantive change in #30877 in favor of an alternative fix that simply makes identity checking more efficient in many cases by always using the "variance" result, if present (I mentioned this in the comments in that PR),

We always used to do this until 3.4 where we realized variance checking was often inaccurate, and made it a fast path for the true case, but still fell back to structural checking - that change is ultimately what broke typesafe-joi. By retaining that behavior for identity checking, we retain our old performance characteristics there.

@RyanCavanaugh
Copy link
Member

@ahejlsberg thoughts on which should go into 3.4.4 (or both?)

@weswigham weswigham force-pushed the optimize-variance-checkign-in-identity-checking branch from 73ab894 to 0923a7c Compare April 12, 2019 22:19
@weswigham weswigham changed the title Optimize variance checking during identity checking Trust variance checking during identity checking Apr 12, 2019
@ahejlsberg
Copy link
Member

With #30877 the typesafe-joi project compiles successfully:

c:\tc\joi>tsc -diagnostics -skiplibcheck
Files:            94
Lines:         23789
Nodes:         95185
Identifiers:   30787
Symbols:       70221
Types:        103071
Memory used: 153280K
I/O read:      0.02s
I/O write:     0.01s
Parse time:    0.47s
Bind time:     0.31s
Check time:    1.37s
Emit time:     0.08s
Total time:    2.23s

With #30877 plus the bail out that always trusts variances with identity relations it gets slightly better:

c:\tc\joi>tsc -diagnostics -skiplibcheck
Files:            94
Lines:         23789
Nodes:         95185
Identifiers:   30787
Symbols:       59817
Types:         88985
Memory used: 138683K
I/O read:      0.02s
I/O write:     0.01s
Parse time:    0.46s
Bind time:     0.34s
Check time:    1.26s
Emit time:     0.08s
Total time:    2.14s

However, using this PR (i.e. isTypeIdenticalTo checks in getConditionalTypeWorker) makes it a lot worse and causes errors:

c:\tc\joi>tsc -diagnostics -skiplibcheck -pretty false
src/schema/alternative.ts(5,18): error TS2589: Type instantiation is excessively deep and possibly infinite.
src/schema/binary.ts(5,18): error TS2589: Type instantiation is excessively deep and possibly infinite.
src/schema/date.ts(6,18): error TS2589: Type instantiation is excessively deep and possibly infinite.
src/schema/function.ts(6,18): error TS2589: Type instantiation is excessively deep and possibly infinite.
src/schema/lazy.ts(5,18): error TS2589: Type instantiation is excessively deep and possibly infinite.
src/schema/number.ts(6,18): error TS2589: Type instantiation is excessively deep and possibly infinite.
src/schema/object.ts(7,18): error TS2589: Type instantiation is excessively deep and possibly infinite.
src/schema/string.ts(6,18): error TS2589: Type instantiation is excessively deep and possibly infinite.
src/schema/symbol.ts(5,18): error TS2589: Type instantiation is excessively deep and possibly infinite.
test/schema-defaults.spec.ts(13,50): error TS2345: Argument of type 'true' is not assignable to parameter of type 'false'.
test/schema-literal.spec.ts(19,110): error TS2345: Argument of type 'true' is not assignable to parameter of type 'false'.
test/schema-presence.spec.ts(13,74): error TS2345: Argument of type 'true' is not assignable to parameter of type 'false'.
test/schema-presence.spec.ts(15,86): error TS2345: Argument of type 'true' is not assignable to parameter of type 'false'.
test/schema-presence.spec.ts(27,85): error TS2345: Argument of type 'true' is not assignable to parameter of type 'false'.
test/schema-presence.spec.ts(29,97): error TS2345: Argument of type 'true' is not assignable to parameter of type 'false'.
test/schema-presence.spec.ts(41,77): error TS2345: Argument of type 'true' is not assignable to parameter of type 'false'.
test/schema-presence.spec.ts(43,89): error TS2345: Argument of type 'true' is not assignable to parameter of type 'false'.
Files:            94
Lines:         23789
Nodes:         95185
Identifiers:   30787
Symbols:      840743
Types:        170078
Memory used: 424667K
I/O read:      0.02s
I/O write:     0.01s
Parse time:    0.55s
Bind time:     0.37s
Check time:    3.90s
Emit time:     0.09s
Total time:    4.90s

So, my take is that #30877 should go in, possibly with the bail out that always trusts variance checks. I don't think we want this PR.

Copy link
Member

@ahejlsberg ahejlsberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this PR is ready for prime time as per my comments above.

@weswigham
Copy link
Member Author

#30416 essentially improves the same core area as this, so I'm going to close this.

@weswigham weswigham closed this Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants