Skip to content

Fix equality narrowing and comparable relation for intersections with {} #50735

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

Merged
merged 4 commits into from
Sep 14, 2022

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Sep 12, 2022

This PR fixes both the original issue in #50706 and the issue reported here. It also fixes an issue in control flow analysis of the false branch of != (which should always yield the same result as the true branch of ==, but didn't). Specifically, in this test, x was wrongly narrowed to number in the last branch.

Fixes #50706.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Sep 12, 2022
@ahejlsberg
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this inline
@typescript-bot run dt
@typescript-bot perf test faster
@typescript-bot test top100

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 12, 2022

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 12, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 12, 2022

Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 7d9924d. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 12, 2022

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 7d9924d. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 12, 2022

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@ahejlsberg Here are the results of running the user test suite comparing main and refs/pull/50735/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..50735

Metric main 50735 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 338,828k (± 0.01%) 338,840k (± 0.01%) +12k (+ 0.00%) 338,766k 338,875k
Parse Time 2.05s (± 0.46%) 2.07s (± 0.88%) +0.01s (+ 0.63%) 2.03s 2.11s
Bind Time 0.79s (± 0.63%) 0.79s (± 0.82%) -0.00s (- 0.38%) 0.78s 0.80s
Check Time 5.83s (± 0.46%) 5.80s (± 0.20%) -0.04s (- 0.60%) 5.78s 5.83s
Emit Time 6.13s (± 0.47%) 6.14s (± 0.46%) +0.00s (+ 0.08%) 6.08s 6.19s
Total Time 14.82s (± 0.33%) 14.80s (± 0.25%) -0.02s (- 0.16%) 14.72s 14.88s
Compiler-Unions - node (v14.15.1, x64)
Memory used 192,624k (± 0.01%) 192,631k (± 0.01%) +7k (+ 0.00%) 192,602k 192,650k
Parse Time 0.86s (± 0.95%) 0.85s (± 0.56%) -0.01s (- 1.17%) 0.84s 0.86s
Bind Time 0.49s (± 0.98%) 0.48s (± 0.70%) -0.00s (- 0.82%) 0.48s 0.49s
Check Time 6.71s (± 0.65%) 6.71s (± 0.39%) -0.00s (- 0.06%) 6.66s 6.79s
Emit Time 2.41s (± 0.82%) 2.41s (± 0.84%) +0.00s (+ 0.21%) 2.37s 2.46s
Total Time 10.46s (± 0.42%) 10.44s (± 0.39%) -0.01s (- 0.11%) 10.36s 10.54s
Monaco - node (v14.15.1, x64)
Memory used 326,521k (± 0.01%) 326,545k (± 0.01%) +23k (+ 0.01%) 326,483k 326,580k
Parse Time 1.57s (± 0.47%) 1.58s (± 0.81%) +0.01s (+ 0.32%) 1.56s 1.61s
Bind Time 0.72s (± 0.55%) 0.73s (± 0.68%) +0.01s (+ 0.69%) 0.72s 0.74s
Check Time 5.71s (± 0.50%) 5.70s (± 0.58%) -0.02s (- 0.28%) 5.65s 5.82s
Emit Time 3.31s (± 0.82%) 3.30s (± 0.45%) -0.01s (- 0.30%) 3.26s 3.33s
Total Time 11.32s (± 0.43%) 11.30s (± 0.43%) -0.02s (- 0.13%) 11.22s 11.47s
TFS - node (v14.15.1, x64)
Memory used 289,654k (± 0.00%) 289,656k (± 0.01%) +2k (+ 0.00%) 289,595k 289,700k
Parse Time 1.31s (± 0.36%) 1.30s (± 0.72%) -0.01s (- 0.54%) 1.28s 1.32s
Bind Time 0.79s (± 0.66%) 0.79s (± 0.51%) +0.00s (+ 0.25%) 0.78s 0.80s
Check Time 5.33s (± 0.33%) 5.34s (± 0.32%) +0.02s (+ 0.30%) 5.30s 5.37s
Emit Time 3.57s (± 0.63%) 3.56s (± 0.62%) -0.00s (- 0.14%) 3.51s 3.62s
Total Time 10.99s (± 0.35%) 11.00s (± 0.30%) +0.01s (+ 0.08%) 10.93s 11.07s
material-ui - node (v14.15.1, x64)
Memory used 436,458k (± 0.00%) 436,476k (± 0.01%) +18k (+ 0.00%) 436,417k 436,533k
Parse Time 1.85s (± 0.41%) 1.85s (± 0.56%) -0.00s (- 0.11%) 1.83s 1.87s
Bind Time 0.58s (± 0.69%) 0.58s (± 0.69%) -0.00s (- 0.34%) 0.57s 0.59s
Check Time 12.84s (± 0.47%) 12.86s (± 0.62%) +0.03s (+ 0.19%) 12.77s 13.14s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.27s (± 0.38%) 15.29s (± 0.53%) +0.02s (+ 0.13%) 15.19s 15.57s
xstate - node (v14.15.1, x64)
Memory used 546,740k (± 0.00%) 546,746k (± 0.00%) +6k (+ 0.00%) 546,696k 546,796k
Parse Time 2.59s (± 0.39%) 2.59s (± 0.60%) +0.01s (+ 0.27%) 2.57s 2.64s
Bind Time 0.97s (± 0.60%) 0.97s (± 1.62%) +0.00s (+ 0.21%) 0.95s 1.02s
Check Time 1.52s (± 0.50%) 1.51s (± 0.34%) -0.01s (- 0.85%) 1.50s 1.52s
Emit Time 0.07s (± 0.00%) 0.07s (± 3.14%) +0.00s (+ 1.43%) 0.07s 0.08s
Total Time 5.15s (± 0.22%) 5.15s (± 0.23%) -0.01s (- 0.17%) 5.12s 5.17s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory15 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)
Benchmark Name Iterations
Current 50735 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

@ahejlsberg Here are the results of running the top-repos suite comparing main and refs/pull/50735/merge:

Everything looks good!

@andrewbranch andrewbranch added this to the TypeScript 4.8.4 milestone Sep 12, 2022
@ahejlsberg
Copy link
Member Author

@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 13, 2022

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

@typescript-bot
Copy link
Collaborator

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

@ahejlsberg
Copy link
Member Author

Tests and perf all look good. This one is ready to merge.

@ahejlsberg
Copy link
Member Author

@andrewbranch Ping for review.

Comment on lines -19194 to +19199
const constraints = sameMap((source as IntersectionType).types, getBaseConstraintOrType);
const constraints = sameMap((source as IntersectionType).types, t => t.flags & TypeFlags.Instantiable ? getBaseConstraintOfType(t) || unknownType : t);
if (constraints !== (source as IntersectionType).types) {
source = getIntersectionType(constraints);
if (source.flags & TypeFlags.Never) {
return Ternary.False;
}
Copy link
Member

Choose a reason for hiding this comment

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

Where do these changes come into play?

Copy link
Member Author

Choose a reason for hiding this comment

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

From the comment right above:

// Source is an intersection. For the comparable relation, if the target is a primitive type we hoist the
// constraints of all non-primitive types in the source into a new intersection. We do this because the
// intersection may further constrain the constraints of the non-primitive types. For example, given a type
// parameter 'T extends 1 | 2', the intersection 'T & 1' should be reduced to '1' such that it doesn't
// appear to be comparable to '2'.

The code previously didn't properly handle unconstrained type parameters, and it didn't properly handle the case where the hoisting causes the constraint to be never (meaning the types have no overlap).

@ahejlsberg ahejlsberg merged commit 4110b80 into main Sep 14, 2022
@ahejlsberg ahejlsberg deleted the fix50706 branch September 14, 2022 16:19
@andrewbranch
Copy link
Member

@typescript-bot cherry-pick this to release-4.8

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 14, 2022

Heya @andrewbranch, I've started to run the task to cherry-pick this into release-4.8 on this PR at 7d9924d. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @andrewbranch, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-4.8 manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4.8.2 regression: type narrowing bug
3 participants