Remove duplicate signatures in intersections #32049
Conversation
|
@typescript-bot run dt |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at be4147d. You can monitor the build here. It should now contribute to this PR's status checks. |
|
DT run is clean (error is unrelated and exists with master branch as well). |
|
|
||
| function appendSignatures(signatures: Signature[] | undefined, newSignatures: readonly Signature[]) { | ||
| for (const sig of newSignatures) { | ||
| if (!signatures || every(signatures, s => !compareSignaturesIdentical(s, sig, /*partialMatch*/ false, /*ignoreThisTypes*/ false, /*ignoreReturnTypes*/ false, compareTypesIdentical))) { |
There was a problem hiding this comment.
Uhhhh, it's probably worth noting that compareSignaturesIdentical needs some TLC - it currently erases constraints to any like compareSignatures used to, which causes spurious false positives. Concretely, that means that if I have a:
type Foo = (<T extends number>(x: T) => T) & (<T extends string>(x: T) => T)I don't think we'll be happy with the result here... (I looked into this around #31023 )
|
With latest commits we now properly compare type parameters, constraints, and defaults when checking signature identity. Baseline changes nicely reflect cases that should fail but previously didn't because we just erased type parameters. |
|
@typescript-bot run dt |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 10c9fbc. You can monitor the build here. It should now contribute to this PR's status checks. |
| const s = source.typeParameters![i]; | ||
| const t = target.typeParameters[i]; | ||
| if (!(s === t || compareTypes(instantiateType(getConstraintFromTypeParameter(s), mapper) || unknownType, getConstraintFromTypeParameter(t) || unknownType) && | ||
| compareTypes(instantiateType(getDefaultFromTypeParameter(s), mapper) || unknownType, getDefaultFromTypeParameter(t) || unknownType))) { |
There was a problem hiding this comment.
Yeah, I don't think we can consider defaults when checking identity - if we do, then two conditional extends clauses that only differ in a local type parameter default (which doesn't affect relationship checking) will fail to be seen as identical.
There was a problem hiding this comment.
Eg, (A extends <T = {}>() => T ? 1 : 0) extends (A extends <T = unknown>() => T ? 1 : 0) ? true : false should resolve to true, I think, not false.
There was a problem hiding this comment.
I'm not sure I understand how that example is relevant. Best I can tell at no point will we compare signatures for identity in the example.
There was a problem hiding this comment.
Conditional extends clauses are compared using isTypeIdenticalTo when checking if one conditional extends the other in the above example. Apparently this is already in use in the wild as a way to observe type "identity" in complex type toolbelt libraries.
There was a problem hiding this comment.
Ok, I see that logic in structuredTypeRelatedTo. Still, as far as I'm concerned, those two types aren't identical and I can't think of a meaningful definition of "identical" that would make it true for them, but not for the other cases we care about. It would just seem really odd to make an exception for local type parameter defaults, but nothing else.
There was a problem hiding this comment.
I mean, we let you assign a function with a default to a function with a different default and vice-versa - it's just the typesystem equivalent of aliasing. Generally defaults aren't observable up until the point where inference fails - this exposes them into relationship checking before inference is ever invoked, success or no.
There was a problem hiding this comment.
I still don't think that merits a special exception to identity checking. The identity relation should be concerned with accurately identifying semantically identical types, and it does a better job of that now. I think we need to get this merged.
|
@weswigham I don't see a compelling reason for why defaults shouldn't be included in identity checking. After all, we're checking whether the signatures are semantically identical, meaning that you get identical results no matter which one you pick. With different defaults that isn't the case. |
|
The DT test run looks clean (the one failure appears completely unrelated). |
|
DT is clean, but RWC is strange. A bunch of errors were eliminated in |
Fixes #32038.