Improve autocomplete for union of tuples #58571
Conversation
| // Create a discriminator for each element in the list | ||
| const discriminators = map( | ||
| node.elements, | ||
| (e, ndx) => [() => getContextFreeTypeOfExpression(e), "" + ndx as __String] as const, |
There was a problem hiding this comment.
In the object version of this function there is a check for isPossiblyDiscriminantValue. Is that necessary here?
There was a problem hiding this comment.
I'd expect both functions to be very similar so I'd say - yes. if isPossiblyDiscriminantValue is needed there - it's also needed here.
src/compiler/checker.ts
Outdated
| function getMatchingUnionConstituentForArrayLiteral(unionType: UnionType, node: ArrayLiteralExpression) { | ||
| const resolvedElements = node.elements.map(el => getContextFreeTypeOfExpression(el)); | ||
| for (const type of unionType.types) { | ||
| if (!isTupleType(type)) continue; |
There was a problem hiding this comment.
q: shouldn't this exit the whole process? Or at least when !isTupleLikeType(type)?
There was a problem hiding this comment.
not sure I understand this question. if the union constituent is not a tuple, it's not valid, but I don't think that means other union constituents couldn't be, so i think we should continue looping?
There was a problem hiding this comment.
TS is structural and you can create some weird (acceptable) types like [number, "foo"] | { "1": "bar" }. This could lead to rejecting some types that should still be considered here. So the way to ignore the problem would be to just (for now?) exit this process.
isTupleLikeType point stands, I think. Types like { "0": "foo" } should contribute to autocompletes
There was a problem hiding this comment.
I was confused when I left my last comment, was thinking this function was something else. I'm realizing I don't even understand the purpose of getMatchingUnionConstituentForObjectLiteral or getMatchingUnionConstituentForArrayLiteral. Do you know what this function should be doing? I'm just going to remove it from the PR for now, can add something back if it makes sense.
| @@ -31361,6 +31375,28 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| ); | |||
| } | |||
|
|
|||
| function discriminateContextualTypeByElements(node: ArrayLiteralExpression, contextualType: UnionType) { | |||
There was a problem hiding this comment.
food for thought: length should also be treated as a potential discriminant but it probably should be ignored when requesting the contextual type for completions. You could try to use ContextFlags.Completions for that or you might have to introduce a new context flag for this purpose, smth like ContextFlags.NoTupleBounds.
There was a problem hiding this comment.
removed the length check in getMatchingUnionConstituentForArrayLiteral so not sure if this is still relevant
There was a problem hiding this comment.
it's relevant because this arg could contextually be typed as number
const tup: [(arg: number) => void] | [(arg: string) => void, number] = [
(arg) => {},
];
src/compiler/checker.ts
Outdated
| const unionElements = getElementTypes(type); | ||
| if (unionElements.length !== resolvedElements.length) continue; |
There was a problem hiding this comment.
This is not quite right. You need to experiment more with this and try to create multiple test cases mixing tuples with different fixed sizes and also the ones with variadic sizes. It would probably be the easiest to only reason about the leading fixed elements.
There was a problem hiding this comment.
i didn't see an obvious way to determine the number of leading elements so i removed that early exit
| const targetType = getTypeOfPropertyOrIndexSignatureOfType(types[i], propertyName); | ||
| const targetType = typeof propertyName === "number" | ||
| ? getContextualTypeForElementExpression(types[i], propertyName) | ||
| : getTypeOfPropertyOrIndexSignatureOfType(types[i], propertyName); |
There was a problem hiding this comment.
found that this was not working like I wanted with variadic tuple types, so for tuples im using getContextualTypeForElementExpression. Not sure if there's a better way to do this, also considered making this function an argument
|
I just wrote a fix for a related issue: it still needs improvements (see @Andarist comments here) and, while it fixes a compilation error, I have no idea if it helps solving your code completion issue. Is this PR going to be merged soon or later? Does it solves this compilation error as well? Should I continue to work on a (possibly) concurrent PR or help you in any way? |
Fixes #46457
Another attempt at this pr from two years ago 😅
This feels fairly wrong (not sure the proper way to narrow these types, I don't really understand what
getTypeArgumentsdoes), but once again since the tests pass I feel like it might be close? If anyone could give pointers I can make changes to clean this up, or feel free to modify if you know what you are doing. Thanks!The changes to
src/services/stringCompletions.tswas just me making changes while finding my way around the code, can revert them if you prefer the other formatting.