Skip to content

[WIP] Contextual narrowing to object - #48576

Closed
Daniel Rosenwasser (DanielRosenwasser) wants to merge 6 commits into
mainfrom
contextualNarrowingToObject
Closed

[WIP] Contextual narrowing to object#48576
Daniel Rosenwasser (DanielRosenwasser) wants to merge 6 commits into
mainfrom
contextualNarrowingToObject

Conversation

@DanielRosenwasser

@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser) commented Apr 5, 2022

Copy link
Copy Markdown
Member

Related: #48468

@DanielRosenwasser

Copy link
Copy Markdown
MemberAuthor

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@DanielRosenwasser

Copy link
Copy Markdown
MemberAuthor

Seems like there's a slowdown from this - maybe a fluke?

TypeScript Bot (@typescript-bot) perf test faster

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 27, 2022

Copy link
Copy Markdown
Contributor

Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the abridged perf test suite on this PR at 1e2eb03. You can monitor the build here.

Update: The results are in!

Comment threadsrc/compiler/checker.ts Outdated
someType(type, isGenericTypeWithUnionConstraint) ||
// When the contextual type is 'unknown', we may need to narrow for compatibility with non-null targets.
// This allows some parity with a constraint of '{} | null | undefined'.
(type.flags & TypeFlags.Instantiable) && contextualType && isEmptyObjectType(contextualType);

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Should this be gated on strictNullChecks?

Comment threadsrc/compiler/checker.ts Outdated
@typescript-bot

Copy link
Copy Markdown
Contributor

Daniel Rosenwasser (@DanielRosenwasser)
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..48576

Metricmain48576DeltaBestWorst
Angular - node (v14.15.1, x64)
Memory used333,421k (± 0.01%)333,544k (± 0.01%)+124k (+ 0.04%)333,489k333,606k
Parse Time2.03s (± 0.43%)2.04s (± 0.57%)+0.01s (+ 0.54%)2.02s2.07s
Bind Time0.87s (± 0.74%)0.87s (± 0.56%)+0.00s (+ 0.23%)0.86s0.88s
Check Time5.63s (± 0.41%)5.73s (± 0.46%)+0.11s (+ 1.88%)5.67s5.78s
Emit Time6.30s (± 0.51%)6.31s (± 0.73%)+0.01s (+ 0.14%)6.21s6.42s
Total Time14.83s (± 0.33%)14.95s (± 0.47%)+0.13s (+ 0.85%)14.80s15.09s
Compiler-Unions - node (v14.15.1, x64)
Memory used192,431k (± 0.41%)192,049k (± 0.14%)-383k (- 0.20%)191,370k192,339k
Parse Time0.85s (± 0.58%)0.85s (± 0.70%)0.00s ( 0.00%)0.85s0.87s
Bind Time0.56s (± 1.57%)0.56s (± 0.40%)+0.00s (+ 0.18%)0.56s0.57s
Check Time7.50s (± 0.49%)7.56s (± 0.43%)+0.06s (+ 0.76%)7.50s7.65s
Emit Time2.52s (± 1.05%)2.50s (± 0.40%)-0.02s (- 0.79%)2.49s2.53s
Total Time11.44s (± 0.38%)11.48s (± 0.30%)+0.04s (+ 0.31%)11.41s11.56s
Monaco - node (v14.15.1, x64)
Memory used325,495k (± 0.00%)325,586k (± 0.01%)+91k (+ 0.03%)325,511k325,626k
Parse Time1.58s (± 0.79%)1.58s (± 0.94%)-0.00s (- 0.32%)1.56s1.62s
Bind Time0.78s (± 0.47%)0.78s (± 0.64%)-0.00s (- 0.13%)0.77s0.79s
Check Time5.56s (± 0.49%)5.62s (± 0.50%)+0.05s (+ 0.99%)5.57s5.69s
Emit Time3.32s (± 1.07%)3.31s (± 0.72%)-0.01s (- 0.27%)3.27s3.36s
Total Time11.25s (± 0.44%)11.29s (± 0.38%)+0.04s (+ 0.37%)11.20s11.40s
TFS - node (v14.15.1, x64)
Memory used289,058k (± 0.01%)289,197k (± 0.01%)+139k (+ 0.05%)289,101k289,262k
Parse Time1.37s (± 0.65%)1.37s (± 1.67%)+0.00s (+ 0.37%)1.35s1.43s
Bind Time0.73s (± 0.94%)0.72s (± 0.50%)-0.00s (- 0.28%)0.72s0.73s
Check Time5.17s (± 0.25%)5.31s (± 0.41%)+0.14s (+ 2.63%)5.26s5.35s
Emit Time3.52s (± 2.29%)3.50s (± 1.84%)-0.01s (- 0.40%)3.40s3.63s
Total Time10.78s (± 0.72%)10.91s (± 0.69%)+0.13s (+ 1.19%)10.76s11.08s
material-ui - node (v14.15.1, x64)
Memory used447,685k (± 0.06%)447,773k (± 0.06%)+87k (+ 0.02%)446,631k447,945k
Parse Time1.87s (± 0.56%)1.88s (± 0.64%)+0.01s (+ 0.37%)1.84s1.90s
Bind Time0.70s (± 0.93%)0.70s (± 0.80%)-0.00s (- 0.00%)0.69s0.71s
Check Time12.98s (± 0.67%)13.18s (± 0.64%)+0.19s (+ 1.49%)13.02s13.47s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time15.55s (± 0.61%)15.75s (± 0.49%)+0.21s (+ 1.32%)15.59s16.01s
xstate - node (v14.15.1, x64)
Memory used535,906k (± 0.00%)535,912k (± 0.00%)+7k (+ 0.00%)535,876k535,960k
Parse Time2.59s (± 0.35%)2.59s (± 0.36%)+0.00s (+ 0.04%)2.58s2.62s
Bind Time1.14s (± 0.70%)1.15s (± 0.66%)+0.00s (+ 0.26%)1.13s1.16s
Check Time1.51s (± 0.46%)1.53s (± 0.49%)+0.02s (+ 1.06%)1.51s1.54s
Emit Time0.07s (± 0.00%)0.07s (± 0.00%)0.00s ( 0.00%)0.07s0.07s
Total Time5.33s (± 0.30%)5.34s (± 0.27%)+0.01s (+ 0.24%)5.31s5.38s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory2 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
BenchmarkNameIterations
Current4857610
Baselinemain10

Developer Information:

Download Benchmark

@DanielRosenwasser

Copy link
Copy Markdown
MemberAuthor

@typescript-bot

This comment was marked as outdated.

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 28, 2022

Copy link
Copy Markdown
Contributor

Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the parallelized Definitely Typed test suite on this PR at ab3f21e. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 28, 2022

Copy link
Copy Markdown
Contributor

Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the extended test suite on this PR at ab3f21e. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 28, 2022

Copy link
Copy Markdown
Contributor

Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the tarball bundle task on this PR at ab3f21e. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 28, 2022

Copy link
Copy Markdown
Contributor

Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the parallelized community code test suite on this PR at ab3f21e. You can monitor the build here.

@typescript-bot

Copy link
Copy Markdown
Contributor

Daniel Rosenwasser (@DanielRosenwasser)
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..48576

Metricmain48576DeltaBestWorst
Angular - node (v14.15.1, x64)
Memory used333,420k (± 0.01%)333,410k (± 0.01%)-10k (- 0.00%)333,358k333,446k
Parse Time2.03s (± 0.77%)2.03s (± 0.47%)-0.01s (- 0.34%)2.01s2.05s
Bind Time0.88s (± 0.45%)0.88s (± 0.42%)-0.00s (- 0.46%)0.87s0.88s
Check Time5.63s (± 0.60%)5.60s (± 0.41%)-0.03s (- 0.59%)5.56s5.65s
Emit Time6.34s (± 1.13%)6.27s (± 0.58%)-0.07s (- 1.10%)6.20s6.34s
Total Time14.88s (± 0.66%)14.78s (± 0.33%)-0.11s (- 0.74%)14.68s14.88s
Compiler-Unions - node (v14.15.1, x64)
Memory used192,516k (± 0.37%)192,801k (± 0.51%)+285k (+ 0.15%)191,559k195,438k
Parse Time0.85s (± 0.47%)0.85s (± 0.47%)0.00s ( 0.00%)0.84s0.86s
Bind Time0.56s (± 0.71%)0.56s (± 0.59%)-0.00s (- 0.18%)0.55s0.57s
Check Time7.51s (± 0.38%)7.48s (± 0.74%)-0.03s (- 0.33%)7.40s7.67s
Emit Time2.53s (± 1.00%)2.52s (± 1.06%)-0.01s (- 0.24%)2.46s2.59s
Total Time11.45s (± 0.37%)11.41s (± 0.60%)-0.03s (- 0.30%)11.27s11.65s
Monaco - node (v14.15.1, x64)
Memory used325,497k (± 0.01%)325,523k (± 0.00%)+27k (+ 0.01%)325,496k325,544k
Parse Time1.58s (± 0.82%)1.56s (± 0.44%)-0.01s (- 0.76%)1.55s1.58s
Bind Time0.78s (± 0.86%)0.78s (± 0.75%)0.00s ( 0.00%)0.77s0.79s
Check Time5.56s (± 0.47%)5.54s (± 0.44%)-0.01s (- 0.23%)5.50s5.60s
Emit Time3.33s (± 0.71%)3.34s (± 0.73%)+0.01s (+ 0.27%)3.28s3.39s
Total Time11.24s (± 0.48%)11.22s (± 0.38%)-0.02s (- 0.13%)11.12s11.30s
TFS - node (v14.15.1, x64)
Memory used289,074k (± 0.01%)289,061k (± 0.01%)-14k (- 0.00%)289,033k289,110k
Parse Time1.37s (± 1.84%)1.35s (± 0.66%)-0.01s (- 0.88%)1.34s1.37s
Bind Time0.72s (± 0.66%)0.73s (± 1.43%)+0.01s (+ 0.69%)0.72s0.76s
Check Time5.21s (± 0.33%)5.19s (± 0.44%)-0.01s (- 0.21%)5.15s5.25s
Emit Time3.54s (± 2.00%)3.52s (± 2.17%)-0.02s (- 0.68%)3.38s3.63s
Total Time10.84s (± 0.89%)10.79s (± 0.75%)-0.04s (- 0.39%)10.63s10.91s
material-ui - node (v14.15.1, x64)
Memory used447,798k (± 0.00%)447,694k (± 0.06%)-103k (- 0.02%)446,660k447,862k
Parse Time1.87s (± 0.43%)1.87s (± 0.31%)-0.00s (- 0.11%)1.86s1.88s
Bind Time0.70s (± 0.74%)0.70s (± 0.83%)-0.00s (- 0.57%)0.69s0.71s
Check Time13.05s (± 0.82%)13.12s (± 0.79%)+0.07s (+ 0.52%)12.86s13.32s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time15.62s (± 0.69%)15.68s (± 0.66%)+0.06s (+ 0.38%)15.44s15.88s
xstate - node (v14.15.1, x64)
Memory used535,897k (± 0.00%)535,924k (± 0.00%)+27k (+ 0.01%)535,903k535,968k
Parse Time2.61s (± 0.63%)2.60s (± 0.68%)-0.01s (- 0.23%)2.57s2.66s
Bind Time1.15s (± 1.02%)1.14s (± 0.52%)-0.01s (- 0.61%)1.13s1.16s
Check Time1.51s (± 0.49%)1.51s (± 0.72%)+0.00s (+ 0.13%)1.49s1.54s
Emit Time0.07s (± 0.00%)0.07s (± 0.00%)0.00s ( 0.00%)0.07s0.07s
Total Time5.34s (± 0.40%)5.33s (± 0.32%)-0.01s (- 0.15%)5.31s5.38s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory2 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
BenchmarkNameIterations
Current4857610
Baselinemain10

Developer Information:

Download Benchmark

@DanielRosenwasser

Copy link
Copy Markdown
MemberAuthor

RWC reveals a few interesting things:

  1. Substituting unknown for the error messages is definitely a bit confusing.

  2. This gets worse with unions - e.g.

    letf=<T,UextendsUsefulType>(x: T|U): object=>x

    So whereas this used to report that T | U wasn't assignable to object, it now reports unknown isn't assignable to object.

Going to rerun with latest changes.

@ahejlsberg

Copy link
Copy Markdown
Member

Hmm, yeah, I don't much like the error message changes in the RWC baselines. The unknown makes it seem like the expression doesn't have a type at all.

@DanielRosenwasser

Copy link
Copy Markdown
MemberAuthor

Discussed with Anders Hejlsberg (@ahejlsberg) offline - this PR is a bit too corner-casey and produces worse messages when a type isn't narrowed. We're interested in seeing if we can leverage smarter intersection types with {} while narrowing generics, which is a change that will have to wait for 4.8 at the least. For now we'll have to live with the break, which people can get around by adding a constraint of {} | null | undefined.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Uncommitted BugPR for untriaged, rejected, closed or missing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@DanielRosenwasser@typescript-bot@ahejlsberg