Ensure common supertype retains nullability in inference #49981

jakebailey · 2022-07-21T04:08:47Z

The common supertype of a bunch of inference candidates should include null and undefined of those candidates can be null or undefined.

#49119 and #49330 changed the way this information was stored and propagated. It turns out that the old code only worked because of a fluke related to those flags. In v4.7.4, given the types in the playground link above:

The candidates are B and D | undefined. We declare that these are both "primary types" because neither are exactly null or undefined.
We look for the supertype of these, but because neither are supertypes of each other (because of the undefined), we end up picking B as it's the leftmost option.
But then, we would rescan the input types for the types whose falsy flags include null or undefined, but using a function that is recursive, so we find D | undefined, when the intent was to just find exactly null or undefined.
We union that with B to get B | D | undefined, which reduces to D | undefined, and we get the supertype that everyone had expected all along, even though that's maybe not what the code was written to do!

Instead, we can just compare the candidates without null or undefined, then add null or undefined back on again if the supertype needs it; this means that the cases we supported in 4.7.4 work (and now work the same regardless of argument order).

jakebailey · 2022-07-21T18:08:26Z

Well, I narrowed this down to operate strictly on only unions and direct types, so it shouldn't have that build problem anymore (and I encoded it as a test).

jakebailey · 2022-07-21T18:08:42Z

@typescript-bot test this
@typescript-bot user test this inline
@typescript-bot pack this

typescript-bot · 2022-07-21T18:08:43Z

Heya @jakebailey, I've started to run the extended test suite on this PR at a817435. You can monitor the build here.

typescript-bot · 2022-07-21T18:08:43Z

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at a817435. You can monitor the build here.

Update: The results are in!

typescript-bot · 2022-07-21T18:08:43Z

Heya @jakebailey, I've started to run the tarball bundle task on this PR at a817435. You can monitor the build here.

typescript-bot · 2022-07-21T18:29:09Z

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/130164/artifacts?artifactName=tgz&fileId=32A768F7B0A1960DB14C832B8F12F2972D738FB3A5CC18B69F843EBC2D78CBD902&fileName=/typescript-4.8.0-insiders.20220721.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

typescript-bot · 2022-07-21T18:29:49Z

Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here.

typescript-bot · 2022-07-21T18:30:11Z

@jakebailey
Great news! no new errors were found between main..refs/pull/49981/merge

jakebailey · 2022-07-21T18:48:41Z

Demo as of this current commit: Playground Link

jakebailey · 2022-07-22T18:28:03Z

RE: the RWC tests, I think there might be one error in there that's real, but I'm having trouble understanding what's going on there because it looks like a bunch of unrelated things are showing up in the diff that shouldn't be there if the diff was running on the same version of the test codebases.

The error sort of implies that void should also be considered, though...

jakebailey · 2022-07-22T19:41:53Z

@typescript-bot perf test this faster

typescript-bot · 2022-07-22T19:41:56Z

Heya @jakebailey, I've started to run the abridged perf test suite on this PR at a817435. You can monitor the build here.

Update: The results are in!

typescript-bot · 2022-07-22T19:56:19Z

@jakebailey
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..49981

_Metric	_main	₄₉₉₈₁	_Delta	_Best	_Worst
_{Angular - node (v14.15.1, x64)}
_{Memory used}	_{335,954k (± 0.00%)}	_{335,952k (± 0.01%)}	_{-2k (- 0.00%)}	_335,900k	_336,022k
_{Parse Time}	_{2.05s (± 0.52%)}	_{2.07s (± 0.48%)}	_{+0.01s (+ 0.58%)}	_2.04s	_2.09s
_{Bind Time}	_{0.89s (± 0.84%)}	_{0.90s (± 0.91%)}	_{+0.00s (+ 0.45%)}	_0.88s	_0.92s
_{Check Time}	_{5.81s (± 0.52%)}	_{5.83s (± 0.40%)}	_{+0.02s (+ 0.34%)}	_5.76s	_5.87s
_{Emit Time}	_{6.36s (± 0.30%)}	_{6.38s (± 0.61%)}	_{+0.02s (+ 0.31%)}	_6.29s	_6.46s
_{Total Time}	_{15.12s (± 0.28%)}	_{15.17s (± 0.44%)}	_{+0.06s (+ 0.38%)}	_14.98s	_15.30s
_{Compiler-Unions - node (v14.15.1, x64)}
_{Memory used}	_{193,134k (± 0.02%)}	_{193,102k (± 0.01%)}	_{-32k (- 0.02%)}	_193,059k	_193,144k
_{Parse Time}	_{0.85s (± 0.73%)}	_{0.85s (± 1.04%)}	_{+0.01s (+ 0.59%)}	_0.84s	_0.88s
_{Bind Time}	_{0.57s (± 0.64%)}	_{0.57s (± 0.97%)}	_{+0.01s (+ 1.06%)}	_0.56s	_0.58s
_{Check Time}	_{6.71s (± 0.59%)}	_{6.71s (± 0.31%)}	_{+0.00s (+ 0.06%)}	_6.66s	_6.75s
_{Emit Time}	_{2.49s (± 1.02%)}	_{2.51s (± 1.37%)}	_{+0.02s (+ 0.76%)}	_2.46s	_2.63s
_{Total Time}	_{10.61s (± 0.41%)}	_{10.65s (± 0.45%)}	_{+0.04s (+ 0.34%)}	_10.59s	_10.82s
_{Monaco - node (v14.15.1, x64)}
_{Memory used}	_{325,660k (± 0.00%)}	_{325,628k (± 0.01%)}	_{-31k (- 0.01%)}	_325,579k	_325,675k
_{Parse Time}	_{1.58s (± 0.39%)}	_{1.58s (± 0.56%)}	_{+0.01s (+ 0.38%)}	_1.57s	_1.61s
_{Bind Time}	_{0.78s (± 0.60%)}	_{0.78s (± 0.46%)}	_{+0.00s (+ 0.51%)}	_0.78s	_0.79s
_{Check Time}	_{5.68s (± 0.49%)}	_{5.69s (± 0.54%)}	_{+0.02s (+ 0.30%)}	_5.64s	_5.76s
_{Emit Time}	_{3.34s (± 0.47%)}	_{3.36s (± 0.66%)}	_{+0.01s (+ 0.45%)}	_3.31s	_3.42s
_{Total Time}	_{11.38s (± 0.22%)}	_{11.42s (± 0.43%)}	_{+0.04s (+ 0.38%)}	_11.34s	_11.56s
_{TFS - node (v14.15.1, x64)}
_{Memory used}	_{288,788k (± 0.01%)}	_{288,806k (± 0.01%)}	_{+18k (+ 0.01%)}	_288,743k	_288,856k
_{Parse Time}	_{1.32s (± 1.67%)}	_{1.34s (± 1.72%)}	_{+0.03s (+ 1.97%)}	_1.30s	_1.41s
_{Bind Time}	_{0.77s (± 4.97%)}	_{0.77s (± 4.64%)}	_{-0.01s (- 1.16%)}	_0.73s	_0.86s
_{Check Time}	_{5.35s (± 0.38%)}	_{5.37s (± 0.38%)}	_{+0.02s (+ 0.36%)}	_5.33s	_5.42s
_{Emit Time}	_{3.58s (± 1.99%)}	_{3.58s (± 2.15%)}	_{-0.00s (- 0.00%)}	_3.42s	_3.71s
_{Total Time}	_{11.02s (± 0.79%)}	_{11.05s (± 0.78%)}	_{+0.03s (+ 0.25%)}	_10.86s	_11.19s
_{material-ui - node (v14.15.1, x64)}
_{Memory used}	_{446,695k (± 0.01%)}	_{446,605k (± 0.05%)}	_{-90k (- 0.02%)}	_445,630k	_446,780k
_{Parse Time}	_{1.87s (± 0.37%)}	_{1.89s (± 0.47%)}	_{+0.01s (+ 0.80%)}	_1.87s	_1.91s
_{Bind Time}	_{0.72s (± 0.80%)}	_{0.72s (± 0.92%)}	_{-0.00s (- 0.00%)}	_0.71s	_0.74s
_{Check Time}	_{13.12s (± 0.50%)}	_{13.30s (± 0.73%)}	_{+0.18s (+ 1.37%)}	_13.04s	_13.57s
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_{0.00s ( NaN%)}	_0.00s	_0.00s
_{Total Time}	_{15.72s (± 0.46%)}	_{15.91s (± 0.64%)}	_{+0.20s (+ 1.25%)}	_15.64s	_16.20s
_{xstate - node (v14.15.1, x64)}
_{Memory used}	_{541,302k (± 0.00%)}	_{541,298k (± 0.01%)}	_{-4k (- 0.00%)}	_541,241k	_541,370k
_{Parse Time}	_{2.60s (± 0.40%)}	_{2.60s (± 0.64%)}	_{+0.00s (+ 0.04%)}	_2.57s	_2.64s
_{Bind Time}	_{1.17s (± 1.43%)}	_{1.14s (± 1.57%)}	_{-0.02s (- 1.89%)}	_1.11s	_1.19s
_{Check Time}	_{1.55s (± 0.53%)}	_{1.55s (± 0.49%)}	_{+0.01s (+ 0.45%)}	_1.54s	_1.57s
_{Emit Time}	_{0.07s (± 0.00%)}	_{0.07s (± 4.66%)}	_{+0.00s (+ 4.29%)}	_0.07s	_0.08s
_{Total Time}	_{5.38s (± 0.42%)}	_{5.37s (± 0.30%)}	_{-0.01s (- 0.19%)}	_5.34s	_5.42s

System

_{Machine Name}	_ts-ci-ubuntu
_Platform	_{linux 4.4.0-210-generic}
_Architecture	_x64
_{Available Memory}	_{16 GB}
_{Available Memory}	_{15 GB}
_CPUs	_{4 × 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)

_Benchmark	_Name	_Iterations
_Current	₄₉₉₈₁	₁₀
_Baseline	_main	₁₀

Developer Information:

Download Benchmark

DanielRosenwasser · 2022-07-22T20:56:14Z

src/compiler/checker.ts

+            function someHas(type: Type, flags: TypeFlags): boolean {
+                return someType(type, t => !!(t.flags & flags));


This feels common enough that I would rather us lift it up to someTypeHasFlags

I can do that, yeah, though I think most places just entirely inline it, so I could just do that too.

jakebailey · 2022-07-24T03:10:57Z

#50021 is a more complete fix than mine now, so, closing.

jakebailey added 3 commits July 20, 2022 12:31

Add test case with temporary name

6c2e62b

Rename and revamp test

b288657

Ensure common supertype retains nullability

30d0cdc

typescript-bot assigned jakebailey Jul 21, 2022

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jul 21, 2022

This comment was marked as outdated.

Sign in to view

Avoid producing brand new types in getCommonSupertype

1b96117

This comment was marked as outdated.

Sign in to view

jakebailey marked this pull request as draft July 21, 2022 17:17

Only check for undefined or null directly or in unions

a817435

jakebailey marked this pull request as ready for review July 22, 2022 18:28

jakebailey requested a review from ahejlsberg July 22, 2022 18:28

typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jul 22, 2022

DanielRosenwasser mentioned this pull request Jul 22, 2022

Design Meeting Notes, 7/22/2022 #50008

Closed

DanielRosenwasser reviewed Jul 22, 2022

View reviewed changes

ahejlsberg mentioned this pull request Jul 23, 2022

Properly handle null and undefined in getCommonSupertype #50021

Merged

jakebailey closed this Jul 24, 2022

jakebailey deleted the fix-49938 branch October 20, 2022 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure common supertype retains nullability in inference #49981

Ensure common supertype retains nullability in inference #49981

jakebailey commented Jul 21, 2022 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

jakebailey commented Jul 21, 2022

jakebailey commented Jul 21, 2022

typescript-bot commented Jul 21, 2022 •

edited

Loading

typescript-bot commented Jul 21, 2022 •

edited

Loading

typescript-bot commented Jul 21, 2022 •

edited

Loading

typescript-bot commented Jul 21, 2022 •

edited

Loading

typescript-bot commented Jul 21, 2022

typescript-bot commented Jul 21, 2022

jakebailey commented Jul 21, 2022

jakebailey commented Jul 22, 2022 •

edited

Loading

jakebailey commented Jul 22, 2022

typescript-bot commented Jul 22, 2022 •

edited

Loading

typescript-bot commented Jul 22, 2022

DanielRosenwasser Jul 22, 2022

jakebailey Jul 22, 2022

jakebailey commented Jul 24, 2022

		function someHas(type: Type, flags: TypeFlags): boolean {
		return someType(type, t => !!(t.flags & flags));

Ensure common supertype retains nullability in inference #49981

Ensure common supertype retains nullability in inference #49981

Conversation

jakebailey commented Jul 21, 2022 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

jakebailey commented Jul 21, 2022

jakebailey commented Jul 21, 2022

typescript-bot commented Jul 21, 2022 • edited Loading

typescript-bot commented Jul 21, 2022 • edited Loading

typescript-bot commented Jul 21, 2022 • edited Loading

typescript-bot commented Jul 21, 2022 • edited Loading

typescript-bot commented Jul 21, 2022

typescript-bot commented Jul 21, 2022

jakebailey commented Jul 21, 2022

jakebailey commented Jul 22, 2022 • edited Loading

jakebailey commented Jul 22, 2022

typescript-bot commented Jul 22, 2022 • edited Loading

typescript-bot commented Jul 22, 2022

DanielRosenwasser Jul 22, 2022

Choose a reason for hiding this comment

jakebailey Jul 22, 2022

Choose a reason for hiding this comment

jakebailey commented Jul 24, 2022

jakebailey commented Jul 21, 2022 •

edited

Loading

typescript-bot commented Jul 21, 2022 •

edited

Loading

typescript-bot commented Jul 21, 2022 •

edited

Loading

typescript-bot commented Jul 21, 2022 •

edited

Loading

typescript-bot commented Jul 21, 2022 •

edited

Loading

jakebailey commented Jul 22, 2022 •

edited

Loading

typescript-bot commented Jul 22, 2022 •

edited

Loading