Do not erase signature constraints when calculating variance #55864
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
|
@typescript-bot pack this |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at bc9d806. You can monitor the build here. |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at bc9d806. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the regular perf test suite on this PR at bc9d806. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the diff-based user code test suite on this PR at bc9d806. 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 Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@weswigham Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -339,6 +347,15 @@ immutable.ts(391,22): error TS2430: Interface 'Set<T>' incorrectly extends inter | |||
| export function Set<T>(): Seq.Set<T>; | |||
| export function Set<T>(collection: Iterable<T>): Seq.Set<T>; | |||
| export interface Set<T> extends Seq<never, T>, Collection.Set<T> { | |||
| ~~~ | |||
| !!! error TS2430: Interface 'Set<T>' incorrectly extends interface 'Seq<never, T>'. | |||
| !!! error TS2430: The types of 'map(...).filter(...).concat(...).toMap().toSeq().toOrderedMap().keySeq().toMap().mapKeys(...).toSeq().toOrderedMap().toSet().union' are incompatible between these types. | |||
There was a problem hiding this comment.
We go hard at finding the incompatibility, don't we. 😆
Guess there's an any somewhere in this type definition that should be a never (or vice-versa, depending on intent).
|
Hey @weswigham, the results of running the DT tests are ready. |
|
DT reported as "a lot of problems", but it's just a lot of packages depending on immutable (which has the same extends error as in the test here - so I should probably figure out where an I think this is a change we can take, once we get those big offenders fixed. Though the array subtype issue might warrant some special-casing in code, even though you can work around the issue with an annotation; arrays are forced to be covariant, whereas if we structurally inspect them (like we do their subtypes), they're measured as invariant (which is more correct! just.... harder to use). It's probably worth detecting array subtypes and short-circuiting their variance calculations, so subtypes have the same hardcoded-covariant behavior array itself does. That change can probably stand on its' own, too, since it's likely performance-positive w.r.t. variance calculation for array subtypes (unlike this change, which can only be bad for perf, since it forces us to make more comparisons with a stricter variance result). |
|
It’s interesting to me that method bivariance alone isn’t enough to keep arrays covariant (though it does help with user-defined arraylikes, as pointed out by Ryan in the array subtype covariance PR) |
weswigham
added
Breaking Change
Fixes #55671
As noted in the linked issue, this is, unfortunately, probably in-use to mask a bunch of real issues (or as a workaround for other issues) in the wild, since it was an easy to make a type that would "accept" any assignments between instances of the same alias, even though once you did a structural comparison it only accepted near-exact matches. Even in our test suite it comes up a couple times in some examples we've pulled from the wild - I expect DT and top100 to have more.