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

Use global NonNullable type for nonnull types #22096

Merged

Conversation

weswigham
Copy link
Member

Also changes NonNullableAssertions to no longer be constraint locations, as it is no longer required to work with the appropriate members/type relationships with this change.

This PR uses the global NonNullable type alias if it can be found, and failing finding it (ie, when an old lib is used), it will manufacture an anonymous conditional type of the appropriate shape to use in its place (which will behave the same way, it just won't be pretty in quick info or declaration emit).

Fixes #21317

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

  1. How fast is this? Seems like it could be faster because of the caching, or slower because of the work in instantiating conditional types.
  2. This seems like a decent solution to NonNullable being missing, while still allowing user overrides. But our other built-ins that come from lib.d.ts don't behave this way yet. The worst are Array and Promise, which don't error but have bad behaviour. As part of the general push to switch the compiler to rely more on conditional types from lib.d.ts, we should revisit our handling of types like this.

@weswigham
Copy link
Member Author

weswigham commented Feb 22, 2018

  1. I think probably faster (instantiation cache is a good thing, since it also causes relationships to be cached and stuff)... but do any of our perf tests have strictNullChecks on, even?

@sandersn
Copy link
Member

No, although turning it on and obtaining a baseline from master would work, I think. That's what I did to test #22006.

@weswigham weswigham force-pushed the nonnull-not-constraint-position branch from f61f6b7 to 67014b0 Compare February 26, 2018 21:49
@weswigham
Copy link
Member Author

weswigham commented Feb 26, 2018

In my unscientific (ie, 10 iteration) tests, I'm not seeing any real difference just on the codebase using strictNullChecks. I think this is a very pay-for-play change - you only do much more expensive things when you're in a situation where more complex behavior is needed to behave correctly (eg, generics), which just don't happen often in your average (or at least our) codebase (but are still frustrating when they don't work) - usually just a handful of places. And the potential increase in work in the non-generic case (since the assignability checks done in getConditionalType are more complex than the checking filterType does) is clearly at least offset by the presence of an instantiation cache.

Metric master nonnull-not-constraint-position Delta Best Worst
Compiler - Unions - node (v9.5.0, x64)
Memory used 208,801k (± 0.02%) 208,790k (± 0.05%) -11k (- 0.01%) 208,554k 208,921k
Parse Time 0.68s (± 2.28%) 0.68s (± 1.41%) -0.01s (- 1.02%) 0.66s 0.70s
Bind Time 0.69s (± 7.00%) 0.64s (± 3.06%) -0.05s (- 7.22%) 0.61s 0.71s
Check Time 17.58s (± 1.37%) 17.47s (± 2.06%) -0.10s (- 0.59%) 16.99s 18.76s
Emit Time 2.15s (± 0.99%) 2.13s (± 2.49%) -0.01s (- 0.65%) 2.06s 2.30s
Total Time 21.10s (± 1.27%) 20.92s (± 1.97%) -0.18s (- 0.85%) 20.36s 22.40s
Compiler - Unions - node (v9.4.0, x64)
Memory used 208,792k (± 0.03%) 208,835k (± 0.03%) +43k (+ 0.02%) 208,661k 208,983k
Parse Time 0.67s (± 0.87%) 0.66s (± 0.89%) -0.00s (- 0.45%) 0.65s 0.67s
Bind Time 0.64s (± 1.05%) 0.63s (± 0.64%) -0.01s (- 1.26%) 0.62s 0.64s
Check Time 17.31s (± 0.50%) 17.28s (± 0.36%) -0.03s (- 0.17%) 17.10s 17.39s
Emit Time 2.14s (± 1.14%) 2.15s (± 2.57%) +0.01s (+ 0.47%) 2.08s 2.30s
Total Time 20.75s (± 0.48%) 20.72s (± 0.31%) -0.03s (- 0.14%) 20.53s 20.85s

@weswigham weswigham merged commit 2f0a13c into microsoft:master Mar 5, 2018
@weswigham weswigham deleted the nonnull-not-constraint-position branch March 5, 2018 22:52
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants