Skip to content

Improvements to strictSubtypeRelation and getNarrowedType #52282

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 12 commits into from
Feb 14, 2023
Merged

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Jan 18, 2023

This PR fixes a number of inconsistencies related to the strict subtype relation and user defined type predicate narrowing. Specifically:

  • In the strict subtype relation, unknown is related to any but any is not related to unknown. Previously, the two were mutually related.
  • In the strict subtype relation, a non-fresh object type is not related to a another object type if the first type is missing an index signature. Previously, it was possible for such types to be mutually related.
  • In the subtype and strict subtype relations, given a signature S1 of the form (...args: A) => R, where A is any, any[], never, or never[], and R is any or unknown, and a signature S2 not of that form, S2 is related to S1 but S1 is not related to S2. Previously, it was possible for such signatures to be mutually related.
  • The assignment expression x = y is has the same type as y with any object literal freshness preserved. Previously, object literal freshness was stripped.
  • Narrowing by a user defined type predicate preserves the original type in the false branch for non-identical mutual subtypes.

The strict subtype relation primarily controls subtype reduction of union types. Subtype reduction occurs in control flow analysis and other situations that require determining a common supertype for multiple types. For example, given expressions a and b of type A and B, the element type of an array literal [a, b] is A if B is a subtype of A, B if A is a subtype of B, or A | B if neither is a subtype of the other. However, if A and B are mutual subtypes, the type chosen may ultimately depend on declaration order, which isn't desired. The goal of the strict subtype relation is to impose a stable ordering by eliminating mutual subtypes, and the first three rules above further that goal.

Fixes #50916.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 18, 2023
@ahejlsberg ahejlsberg changed the title Fix50916 Improvements to strictSubtypeRelation and getNarrowedType Jan 18, 2023
@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 Jan 18, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 18, 2023

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 18, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 18, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 18, 2023

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

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@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.

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..52282

Metric main 52282 Delta Best Worst
Angular - node (v16.17.1, x64)
Memory used 358,311k (± 0.01%) 358,322k (± 0.01%) +11k (+ 0.00%) 358,263k 358,361k
Parse Time 4.10s (± 0.58%) 4.12s (± 0.59%) +0.01s (+ 0.33%) 4.09s 4.16s
Bind Time 1.25s (± 0.35%) 1.25s (± 0.45%) -0.00s (- 0.03%) 1.25s 1.26s
Check Time 9.49s (± 0.50%) 9.49s (± 0.18%) -0.00s (- 0.02%) 9.47s 9.51s
Emit Time 7.96s (± 0.25%) 7.94s (± 0.44%) -0.02s (- 0.22%) 7.90s 8.00s
Total Time 22.81s (± 0.20%) 22.80s (± 0.18%) -0.00s (- 0.01%) 22.72s 22.83s
Compiler-Unions - node (v16.17.1, x64)
Memory used 195,555k (± 0.96%) 191,823k (± 0.03%) -3,732k (- 1.91%) 191,735k 191,901k
Parse Time 1.80s (± 1.25%) 1.81s (± 0.39%) +0.01s (+ 0.31%) 1.80s 1.82s
Bind Time 0.85s (± 0.61%) 0.85s (± 0.72%) +0.00s (+ 0.03%) 0.84s 0.86s
Check Time 10.24s (± 0.56%) 10.09s (± 0.99%) -0.15s (- 1.44%) 9.94s 10.20s
Emit Time 3.13s (± 5.36%) 3.06s (± 2.08%) -0.07s (- 2.26%) 3.01s 3.18s
Total Time 16.02s (± 1.05%) 15.81s (± 0.60%) -0.21s (- 1.33%) 15.64s 15.91s
Monaco - node (v16.17.1, x64)
Memory used 345,118k (± 0.01%) 345,147k (± 0.01%) +29k (+ 0.01%) 345,115k 345,172k
Parse Time 3.10s (± 1.05%) 3.10s (± 0.76%) +0.00s (+ 0.01%) 3.08s 3.14s
Bind Time 1.11s (± 0.87%) 1.11s (± 0.79%) -0.00s (- 0.38%) 1.10s 1.12s
Check Time 7.87s (± 0.09%) 7.93s (± 1.08%) +0.05s (+ 0.66%) 7.85s 8.09s
Emit Time 4.57s (± 1.09%) 4.57s (± 2.05%) +0.01s (+ 0.14%) 4.50s 4.76s
Total Time 16.66s (± 0.39%) 16.71s (± 1.09%) +0.05s (+ 0.32%) 16.56s 17.07s
TFS - node (v16.17.1, x64)
Memory used 299,919k (± 0.01%) 299,918k (± 0.00%) -1k (- 0.00%) 299,911k 299,924k
Parse Time 2.43s (± 1.35%) 2.42s (± 1.01%) -0.01s (- 0.41%) 2.40s 2.46s
Bind Time 1.26s (± 1.06%) 1.26s (± 0.62%) +0.00s (+ 0.26%) 1.25s 1.27s
Check Time 7.41s (± 0.40%) 7.44s (± 0.43%) +0.03s (+ 0.38%) 7.40s 7.49s
Emit Time 4.23s (± 0.38%) 4.25s (± 0.52%) +0.01s (+ 0.31%) 4.21s 4.27s
Total Time 15.34s (± 0.49%) 15.38s (± 0.31%) +0.03s (+ 0.23%) 15.31s 15.45s
material-ui - node (v16.17.1, x64)
Memory used 475,709k (± 0.00%) 475,705k (± 0.00%) -4k (- 0.00%) 475,666k 475,734k
Parse Time 3.65s (± 0.39%) 3.66s (± 0.26%) +0.01s (+ 0.25%) 3.65s 3.68s
Bind Time 1.01s (± 0.51%) 1.01s (± 0.50%) +0.00s (+ 0.40%) 1.00s 1.02s
Check Time 17.86s (± 0.22%) 18.05s (± 0.69%) +0.18s (+ 1.01%) 17.90s 18.26s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 22.53s (± 0.17%) 22.72s (± 0.52%) +0.19s (+ 0.85%) 22.57s 22.92s
xstate - node (v16.17.1, x64)
Memory used 543,991k (± 0.02%) 544,002k (± 0.01%) +11k (+ 0.00%) 543,961k 544,099k
Parse Time 4.57s (± 0.37%) 4.60s (± 0.72%) +0.03s (+ 0.65%) 4.57s 4.66s
Bind Time 1.79s (± 0.38%) 1.80s (± 0.18%) +0.00s (+ 0.19%) 1.79s 1.80s
Check Time 2.93s (± 0.44%) 2.93s (± 0.54%) +0.00s (+ 0.04%) 2.91s 2.95s
Emit Time 0.08s (± 6.09%) 0.08s (± 6.08%) +0.00s (+ 0.10%) 0.08s 0.09s
Total Time 9.38s (± 0.20%) 9.42s (± 0.26%) +0.03s (+ 0.37%) 9.39s 9.45s
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-135-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
Benchmark Name Iterations
Current 52282 6
Baseline main 6

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/52282/merge:

Something interesting changed - please have a look.

Details

marmelab/react-admin

22 of 23 projects failed to build with the old tsc and were ignored

packages/ra-core/tsconfig.json

ReactiveX/rxjs

8 of 13 projects failed to build with the old tsc and were ignored

src/tsconfig.cjs.spec.json

src/tsconfig.esm5.rollup.json

src/tsconfig.types.json

vuejs/vue

7 of 8 projects failed to build with the old tsc and were ignored

tsconfig.json

@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 Jan 19, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 19, 2023

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 19, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 19, 2023

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 19, 2023

Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at d7a80c4. 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/52282/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..52282

Metric main 52282 Delta Best Worst
Angular - node (v16.17.1, x64)
Memory used 358,348k (± 0.01%) 358,145k (± 0.01%) -203k (- 0.06%) 358,095k 358,205k
Parse Time 4.10s (± 0.43%) 4.09s (± 0.25%) -0.02s (- 0.37%) 4.07s 4.10s
Bind Time 1.25s (± 0.39%) 1.25s (± 0.71%) +0.00s (+ 0.03%) 1.24s 1.27s
Check Time 9.47s (± 0.41%) 9.51s (± 0.38%) +0.04s (+ 0.42%) 9.46s 9.56s
Emit Time 7.97s (± 0.26%) 7.97s (± 0.68%) -0.00s (- 0.05%) 7.92s 8.06s
Total Time 22.80s (± 0.15%) 22.82s (± 0.37%) +0.03s (+ 0.11%) 22.73s 22.96s
Compiler-Unions - node (v16.17.1, x64)
Memory used 193,926k (± 0.03%) 192,283k (± 0.73%) -1,643k (- 0.85%) 191,608k 195,141k
Parse Time 1.80s (± 1.18%) 1.81s (± 0.80%) +0.02s (+ 0.93%) 1.79s 1.84s
Bind Time 0.85s (± 0.59%) 0.85s (± 0.56%) -0.00s (- 0.00%) 0.85s 0.86s
Check Time 10.27s (± 0.42%) 10.09s (± 0.72%) -0.17s (- 1.70%) 10.02s 10.19s
Emit Time 3.03s (± 1.10%) 3.04s (± 0.43%) +0.00s (+ 0.05%) 3.02s 3.05s
Total Time 15.95s (± 0.29%) 15.79s (± 0.39%) -0.16s (- 1.03%) 15.72s 15.87s
Monaco - node (v16.17.1, x64)
Memory used 345,126k (± 0.01%) 345,149k (± 0.02%) +23k (+ 0.01%) 345,068k 345,198k
Parse Time 3.10s (± 1.17%) 3.10s (± 0.53%) +0.00s (+ 0.05%) 3.08s 3.13s
Bind Time 1.11s (± 0.66%) 1.11s (± 1.28%) +0.01s (+ 0.57%) 1.09s 1.13s
Check Time 7.88s (± 0.32%) 7.91s (± 0.56%) +0.03s (+ 0.42%) 7.84s 7.95s
Emit Time 4.53s (± 0.75%) 4.53s (± 0.80%) +0.01s (+ 0.16%) 4.49s 4.60s
Total Time 16.60s (± 0.46%) 16.65s (± 0.41%) +0.05s (+ 0.29%) 16.56s 16.76s
TFS - node (v16.17.1, x64)
Memory used 299,929k (± 0.00%) 299,939k (± 0.00%) +11k (+ 0.00%) 299,922k 299,954k
Parse Time 2.43s (± 0.88%) 2.44s (± 0.84%) +0.01s (+ 0.37%) 2.40s 2.46s
Bind Time 1.27s (± 0.62%) 1.27s (± 0.68%) -0.00s (- 0.03%) 1.26s 1.28s
Check Time 7.42s (± 0.36%) 7.44s (± 0.42%) +0.02s (+ 0.28%) 7.41s 7.48s
Emit Time 4.25s (± 1.22%) 4.26s (± 0.57%) +0.01s (+ 0.14%) 4.24s 4.29s
Total Time 15.37s (± 0.41%) 15.40s (± 0.26%) +0.04s (+ 0.24%) 15.35s 15.45s
material-ui - node (v16.17.1, x64)
Memory used 475,698k (± 0.01%) 475,695k (± 0.00%) -3k (- 0.00%) 475,684k 475,711k
Parse Time 3.66s (± 0.27%) 3.66s (± 0.29%) +0.00s (+ 0.04%) 3.65s 3.68s
Bind Time 1.02s (± 0.40%) 1.02s (± 0.77%) -0.00s (- 0.03%) 1.00s 1.03s
Check Time 17.92s (± 0.80%) 17.90s (± 0.44%) -0.02s (- 0.13%) 17.82s 18.01s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 22.60s (± 0.66%) 22.58s (± 0.38%) -0.02s (- 0.10%) 22.48s 22.70s
xstate - node (v16.17.1, x64)
Memory used 543,932k (± 0.01%) 543,978k (± 0.01%) +46k (+ 0.01%) 543,924k 544,042k
Parse Time 4.57s (± 0.44%) 4.58s (± 0.44%) +0.02s (+ 0.33%) 4.55s 4.60s
Bind Time 1.79s (± 0.86%) 1.79s (± 0.30%) +0.00s (+ 0.25%) 1.79s 1.80s
Check Time 2.94s (± 0.73%) 2.93s (± 0.75%) -0.01s (- 0.31%) 2.90s 2.96s
Emit Time 0.08s (± 6.19%) 0.08s (± 6.44%) +0.00s (+ 1.97%) 0.08s 0.09s
Total Time 9.39s (± 0.30%) 9.39s (± 0.22%) +0.00s (+ 0.02%) 9.37s 9.43s
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-135-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
Benchmark Name Iterations
Current 52282 6
Baseline main 6

Developer Information:

Download Benchmark

# Conflicts:
#	src/compiler/checker.ts
@DanielRosenwasser
Copy link
Member

@typescript-bot top200

@jakebailey
Copy link
Member

Before we approve/merge this, can we make sure that cases like those listed in various replies to #48840 (and #52387) are all tested to be working with their desired behaviors?

@ahejlsberg
Copy link
Member Author

@jakebailey This PR doesn't change anything with regards to #48840 and #52387.

@ahejlsberg
Copy link
Member Author

@typescript-bot test top200

@ahejlsberg
Copy link
Member Author

ahejlsberg commented Feb 7, 2023

@DanielRosenwasser I don't think there's such a thing as test200 in typescript-bot. The results of test100 are here and, as we discussed, are acceptable.

@jakebailey
Copy link
Member

@jakebailey This PR doesn't change anything with regards to #48840 and #52387.

I guess I shouldn't have mentioned the PR as I think it drew the focus away from what I actually wanted checked. I meant that the issue thread (#48840) has some discussion as to what constitutes a top-function-type, which is changing in this PR via isTopSignature.

I'll just pack this PR and test it.

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 7, 2023

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 7, 2023

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/145789/artifacts?artifactName=tgz&fileId=6D0BA089D05A85084C1DC8BBF2F77939D48C21AA802E40F6C6C93CD6D630F5C802&fileName=/typescript-5.0.0-insiders.20230207.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]".;

@jakebailey
Copy link
Member

jakebailey commented Feb 7, 2023

Okay, the things I wanted to test appear to work fine, or at least have not changed behavior from 4.9 anyhow.

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

Would you be able to add tests that demonstrate the ordering among supertypey functions?

type A = (...args: any) => unknown;
type B = (...args: any[]) => unknown;
type C = (...args: never) => unknown;
type D = (...args: never[]) => unknown;
type E = () => unknown;

type FnTypes = A | B | C | D | E;

declare function isSomeFunc<T>(x: any): x is T;

function checkA(f: FnTypes) {
    if (isSomeFunc<A>(f)) {
        f
    }
    else {
        f
    }
    f
}

function checkB(f: FnTypes) {
    if (isSomeFunc<B>(f)) {
        f
    }
    else {
        f
    }
    f
}

function checkC(f: FnTypes) {
    if (isSomeFunc<C>(f)) {
        f
    }
    else {
        f
    }
    f
}

function checkD(f: FnTypes) {
    if (isSomeFunc<D>(f)) {
        f
    }
    else {
        f
    }
    f
}

function checkE(f: FnTypes) {
    if (isSomeFunc<E>(f)) {
        f
    }
    else {
        f
    }
    f
}

@typescript-bot 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 Feb 14, 2023
@ahejlsberg
Copy link
Member Author

@DanielRosenwasser More tests added.

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
Archived in project
Development

Successfully merging this pull request may close these issues.

Type guard affects type of variable in surprising way
4 participants