Relate non-augmenting subtypes without resorting to structural comparison #43624
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot pack this @amcasey once the tarball is done, do you have a local non-reduced repro to verify this on? |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at f1cff54. You can monitor the build here. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at f1cff54. You can monitor the build here. Update: The results are in! |
|
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
@weswigham The actual repro is https://github.com/vanilla/vanilla. You might get to it before I do, since I'm still on DT and not yet done with meetings for today. |
|
@weswigham Here they are:Comparison Report - master..43624
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham Here they are:Comparison Report - master..43624
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Why do the interfaces have to be non-augmenting? Checks on the interface declaration itself will ensure it’s assignable to its base type, so why can’t we just relate the type argument in the subtype interface’s interface WhateverArray extends Array<string> {
extraMember: unknown;
}
let x: (string | number)[] = undefined! as WhateverArray;Wouldn’t it be sufficient here to check that |
|
Teeeechnically non-conforming "subtypes" of array can exist in programs with errors, eg: interface MyBadArray extends Array<string> {
pop(): void;
}
declare var x: MyBadArray;
declare var y: Array<string>;
x = y;
y = x;so relying on the subtype check might not be the best idea - especially if someone's What I could do is expand this to non-array subtypes, eg interface MyMap extends Map<string, string> {}
declare var x: MyMap;
declare var y: Map<string, string>;
x = y;
y = x;and instead just check if the single base type of the type with no augmentations is or is an instantiation of the base type or target of the base type. |
|
Timing for Vanilla is basically the same as #43623 (review) except that this gets things down to about 19s, rather than 22s. I think both changes are good, even if they aren't both necessary to speed up Vanilla. Edit: Stacking the two changes does not seem to provide additional benefits. |
amcasey
reviewed
Apr 12, 2021
| @@ -8,7 +8,7 @@ tests/cases/conformance/expressions/arrayLiterals/arrayLiterals3.ts(17,5): error | |||
| tests/cases/conformance/expressions/arrayLiterals/arrayLiterals3.ts(33,5): error TS2322: Type 'number[]' is not assignable to type '[number, number, number]'. | |||
| Target requires 3 element(s) but source may have fewer. | |||
| tests/cases/conformance/expressions/arrayLiterals/arrayLiterals3.ts(34,5): error TS2322: Type '(string | number)[]' is not assignable to type 'myArray'. | |||
| The types returned by 'pop()' are incompatible between these types. | |||
| Type '(string | number)[]' is not assignable to type 'Number[]'. | |||
amcasey
reviewed
Apr 12, 2021
src/compiler/checker.ts
Outdated
| @@ -18219,6 +18219,22 @@ namespace ts { | |||
| return result; | |||
| } | |||
|
|
|||
| if (isNonAugmentingArraySubtype(source) && isArrayType(target)) { | |||
There was a problem hiding this comment.
What if they're both non-augmenting array subtypes?
There was a problem hiding this comment.
We could relate them; it's just not as common in practice. This comes back to my "we could generalize this more" remark in my prior comment - this doesn't really need to be limited to just Arrays either.
amcasey
approved these changes
Apr 12, 2021
|
@amcasey I assume this PR prevents the degenerate |
|
@weswigham Yes, that would be my guess as well, but I figured there might be other pseudo-aliases or other maybe-stack wins. |
|
I think it might be worthwhile generalizing this to all non-augmenting interface types with a single base type and also rolling it into |
|
@ahejlsberg @weswigham this was one of the sorts of things that @amcasey and I had brought up when we mentioned that type aliases were actually faster than extending interfaces. You can imagine a general path that when an interface
then you simply copy the members over and set their assignability cache key. But keeping it simple with just extending one type and introducing no members is a good start too. |
|
Heya @weswigham, I've started to run the extended test suite on this PR at bfcaba4. You can monitor the build here. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at bfcaba4. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at bfcaba4. You can monitor the build here. |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at bfcaba4. You can monitor the build here. |
|
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@weswigham Here they are:Comparison Report - master..43624
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This was referenced Apr 15, 2021
Closed
ahejlsberg
requested changes
Apr 15, 2021
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at f45c2e2. You can monitor the build here. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..43624
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg this now uses flags to indicate the presence of a cache value, as requested. |
|
@ahejlsberg ping? |
ahejlsberg
approved these changes
Apr 27, 2021
|
@weswigham @ahejlsberg Just FYI: I think this PR seems to be related to this regression |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #43422 (this improves the perf of the example given to better-than-4.1-or-3.6 performance on my machine)
Much like the inspiration for #43435, this PR is about introducing a fast path for comparing trivial subtypes of
Array<T>withArray<T>itself, without resorting to a full, slow, structural comparison (which is especially useful in cases where the comparison succeeds, as it avoids an exhaustive traversal of theArraytype structure). This is useful, since non-augmenting array subtypes turn out to be pretty commonly used in the wild for describing recursive type structures in versions of TS that predate recursive type alias support.Then, that's been generalized to normalize any empty subtype to it's single base type, if applicable, sharing these savings to empty subtypes of
Mapand other types, should they be present.