Fix #55632 issue (No type inferrence of callback arguments inside a tuple union type) #60434
Conversation
|
@microsoft-github-policy-service agree |
|
@Andarist @jcalz any feedback (even negative) would be greatly appreciated:
Thanks |
src/compiler/checker.ts
Outdated
|
|
||
| const elementsLength = node.elements.length; | ||
| const filteredType = filterType(contextualType, type => { | ||
| if (!isTupleLikeType(type)) return true; |
There was a problem hiding this comment.
While this isn't particularly practical, I think you can't use isTupleLikeType here. What about { 1: (a: number) => void } contributing to the contextual type?
function test(arg: { 1: (arg: number) => void }) {}
test([null, (arg) => {}] as const);Basically all types should be consulted for exact property matches and number index signatures.
There was a problem hiding this comment.
But ofc those tuples that can't be matched (like the tuples that are guaranteed too short for the given array literal expression) can be filtered out.
There was a problem hiding this comment.
I don't use isTupleLikeType anymore and check for types that have only integer properties (or "length"). I added a new test with your { 1: (arg: number) => void } type.
…d of .map/.filter, add new test cases)
| @@ -32029,6 +32029,52 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| ); | |||
| } | |||
|
|
|||
| function areTupleLikeProperties(properties: Symbol[]): boolean { | |||
| return properties.length > 0 && every(properties, p => p.escapedName === "length" as __String || /^\d+$/.test(p.escapedName.toString())); | |||
There was a problem hiding this comment.
this won't work in a situation like this:
function test(arg: { 1: (arg: number) => void; foo?: string }) {}
test([null, (arg) => {}] as const);There was a problem hiding this comment.
And, if I'm not mistaken this won't work either:
function test(arg: { 1: (arg: number) => void; [k: number]: (...args: never) => void }) {}
test([() => {}, (arg) => {}] as const);I'd try to avoid ignoring a type if it can't be proven to be incompatible. It should be easier to land on a more correct solution this way
There was a problem hiding this comment.
I fixed the code so it works now with:
function test(arg: { 1: (arg: number) => void; foo?: string }) {}
test([null, (arg) => {}] as const);I only discard types (prior to the discriminateTypeByDiscriminableItems call) if:
- They are true tuple types with an incompatible length,
- They are types with at least a non optional property key of the integer form which is out of range (negative or >= of the array value length).
So, now, this is ok:
declare function f2(_: { 0: number, 1: (arg: number) => void } | { 0: number, 1: (arg: boolean) => void, 2: any }): void;
f2([1, arg => { arg === 0 }] as const);Two questions:
-
I use
/^[-+]?\d+$/to match index like property keys (so{ "-1": boolean }is discarded because it has a negative index). Is it correct? -
{ 1: (arg: number) => void } | [1, (arg: boolean) => void]leads to an error with[1, (arg) => { }](Parameter 'arg' implicitly has an 'any' type), becausediscriminateTypeByDiscriminableItemsfails to discard the first type of the union. I should fix that as well, no?
There was a problem hiding this comment.
I use /^[-+]?\d+$/ to match index like property keys (so { "-1": boolean } is discarded because it has a negative index). Is it correct?
I guess it could be discarded if that property is required. It shouldn't be discarded if it's optional though. But I'm not sure if it's really worth discarding them - if somebody wants to write a union type with such a thing and hope for something to be discarded... they should rethink their choices 😅
{ 1: (arg: number) => void } | [1, (arg: boolean) => void] leads to an error with [1, (arg) => { }] (Parameter 'arg' implicitly has an 'any' type), because discriminateTypeByDiscriminableItems fails to discard the first type of the union. I should fix that as well, no?
No, this is correct. Both of those types can provide the type for (arg) => {}. So the implicit any is the correct result here.
| // declare function f5(arg: { 1: (arg: number) => void; foo?: string } | [1, (arg: boolean) => void]): void; | ||
| // f5([null, (arg) => { arg === 0 }] as const); | ||
|
|
||
| // declare function f6(arg: { 1: (arg: number) => void; [k: number]: (...args: never) => void } | [1, (arg: boolean) => void]): void; | ||
| // f6([() => {}, (arg) => { arg === 0 }] as const); |
There was a problem hiding this comment.
perhaps u meant to uncomment those?
There was a problem hiding this comment.
Yes, this is part of my problem. This test issues an error (Parameter 'arg' implicitly has an 'any' type):
declare function f5(arg: { 1: (arg: number) => void } | [1, (arg: boolean) => void]): void;
f5([1, (arg) => { arg === true }] as const);I don't known what to do about this error because both types in { 1: (arg: number) => void } | [1, (arg: boolean) => void] are possible matches for [1, (arg) => { }]. But [1, (arg: boolean) => void] seems to be a better match. So, is the compiler meant to find the best possible match or only discard incompatible types?
There was a problem hiding this comment.
I don't known what to do about this error because both types in { 1: (arg: number) => void } | [1, (arg: boolean) => void] are possible matches for [1, (arg) => { }]. But [1, (arg: boolean) => void] seems to be a better match. So, is the compiler meant to find the best possible match or only discard incompatible types?
To the best of my knowledge, you should return both and this should result in an implicit any because the (arg: boolean) => void shouldn't be treated as "a better match". I distinctly remember when I sent out a PR that was too eager about discarding those tuple like types, it got merged and then reverted :P
There was a problem hiding this comment.
Ok, this is good news ;) What about this one:
declare function f6(arg: { 1: (arg: number) => void; [k: number]: (...args: never) => void } | [1, (arg: boolean) => void]): void;
f6([() => {}, (arg) => { }] as const);It fails with a Parameter 'arg' implicitly has an 'any' type error, because () => {} isn't a possible discriminant value (isPossiblyDiscriminantValue(element) returns false). But this is a weird because () => {} shouldn't match 1.
There was a problem hiding this comment.
When in doubt, check how objects work for the equivalent case:
declare function f7(
arg:
| { foo: (arg: number) => void; [k: string]: (...args: never) => void }
| { bar: 1; foo: (arg: boolean) => void },
): void;
f7({
bar: () => {},
foo: (arg) => {}, // implicit any
});There was a problem hiding this comment.
Thanks a lot for your help and advices!
I just pushed a new commit with few additional tests. Can I consider that this PR could be accepted as is? What do I need to do next?
|
Hi @Andarist. Any chance to get this PR merged in a foreseeable future? Tell me if I need to do anything else. |
|
@Andarist a kind reminder: what's the point of asking for help if solutions aren't merged? |
|
@fwolff they're an external contributor; there's no need for that kind of commentary. @typescript-bot test it |
|
Hey @jakebailey, 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 |
|
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: frida-gum Package: ffi-napi Package: ref-struct-di |
|
@jakebailey Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Hello @jakebailey. Sorry for being impatient: @Andarist helped me a lot, I didn't want to be rude! |
| return !hasIndexPropertyOutOfRange(type, elementsLength); | ||
| }); | ||
|
|
||
| if (filteredType.flags & TypeFlags.Never) return setCachedType(key, contextualType); |
There was a problem hiding this comment.
q: why is contextualType preserved here if everything was filtered out?
There was a problem hiding this comment.
There used to be an error with other tests when I returned an empty filteredType but this is no longer the case. The new code returns filteredType now.
There was a problem hiding this comment.
This definitely needs more tests with spread elements. You should go crazy with cases like [...other, (arg) => {}] and more
| return index >= 0 || index < length; | ||
| }); | ||
|
|
||
| const elementsLength = node.elements.length; |
There was a problem hiding this comment.
This ties back to https://github.com/microsoft/TypeScript/pull/60434/files#r1957160962 - you can't use this as-is. This is the length of AST elements, not the length of actual elements in the array.
You might want to take a look into getContextualTypeForElementExpression and how it's using firstSpreadIndex and lastSpreadIndex. It feels like you should use a similar-ish logic here but I haven't thought about it deeply. You could also focus on discriminating only by leading non-spread elements first - that should be simpler.
| const isIndexPropertyLike = (property: Symbol) => /^[-+]?\d+$/.test(property.escapedName as string); | ||
| const hasIndexPropertyOutOfRange = (type: Type, length: number) => | ||
| some(getPropertiesOfType(type), p => { | ||
| if (p.flags & SymbolFlags.Optional || !isIndexPropertyLike(p)) return false; | ||
| const index = +p.escapedName; | ||
| return index < 0 || index >= length; | ||
| }); | ||
| const hasIndexPropertyInRange = (type: Type, length: number) => | ||
| some(getPropertiesOfType(type), p => { | ||
| if (!isIndexPropertyLike(p)) return false; | ||
| const index = +p.escapedName; | ||
| return index >= 0 || index < length; | ||
| }); |
There was a problem hiding this comment.
I thiiink those are mostly redundant. An algorithm matching closer discriminateContextualTypeByObjectMembers would likely suffice here. You could create a list of potential discriminators based on the leading non-spread elements and pass that to discriminateTypeByDiscriminableItems. Then you might apply one extra filter after it (or before it?) to handle tuple-specific cases based on .fixedLength and similar
|
@jakebailey I don't mind - all is good. I can echo some of that @fwolff's sentiment though. Maybe some note should be added to the CONTRIBUTING doc here that would clarify the expectations contributors might have when opening PRs targeting that label? |
Passes all existing test cases and Airtable codebase ("Identicality" checks are probably quite rare so unsurprising). Creating this in the hopes that we can use the test infra on this repo to run this against the corpus of other open source libraries.
Fixes #55632.
In summary, the PR adds a new method:
...and calls it in the existing
getApparentTypeOfContextualTypemethod:It also adds a new unit tests file
tests/cases/compiler/tupleTypeInference3.tswith the code failing in #55632 and other related tests.The PR doesn't provide anything for code completion, which could be handled by #58571.
Question: I use the
isTupleLikeTypemethod to match actual tuple types and tuple-like types liketype T = { 0: 1, 1: (a: boolean) => void }(see the new unit tests file). TheisTupleLikeTypechecks also for array-like type, but I was unable to create any such type. Can anybody tell me how to create such types and what I should do with them?