Do not widen computed properties with template literal types #54706
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
Andarist
force-pushed
the
dont-widen-computed-prop-wiht-patterns
branch
from
e7172a2
to
c2c5ae1
Compare
Andarist
force-pushed
the
dont-widen-computed-prop-wiht-patterns
branch
from
c2c5ae1
to
275410d
Compare
| const doc_46309: IDocument_46309 = { | ||
| name: "", | ||
| [`added_${tech1_46309.uuid}`]: [19700101], | ||
| }; |
There was a problem hiding this comment.
This still doesn't work. I think that it should but that's perhaps a different issue to be solved - properties like this could not widen based on the contextual type (I'm not yet sure how complex that could get with multiple index signatures in the target and so on).
The second example with [`added_${tech1_46309.uuid}` as const] is something new that works now though.
| }; | ||
|
|
||
| const obj13 = { | ||
| >obj13 : {} |
There was a problem hiding this comment.
It's worth noting here that this doesn't preserve any index signatures because getUnionIndexInfos([left, right]) in getSpreadType doesn't preserve any index signatures that are not common to all of the input types. That feels like a separate issue to be solved - if it is meant to be solved.
…rop-wiht-patterns
…rop-wiht-patterns
There was a problem hiding this comment.
I could revive that work but I'm unsure what exactly was wrong with that PR so I'm not sure what I could be prepared by me that would have any chance of being merged
Originally (it's very old), it opened a can of worms regarding things we could and could not represent as property names and index signatures. Now we have pattern literal index signatures and things are generally better (though the union case is a bit muddier still, imo, in theory we could handle it pretty well now). The real sticking point, then and now, is going to be the generic types used in these positions. If you feed a T extends string into a get${T} in a computed name position, how's that supposed to work? IMO, our index signature logic should be robust enough to handle generic index signature keys (or is very close to being robust enough to handle them), but @ahejlsberg has had philosophical issues with the concept in the past - preferring that mapped types remain "the one true way" to make "generic keys" exist (and, well, that being the case likely does simplify how we handle index signatures and property names in some places!)... the followup, thus, if that is still the case, is that maybe having a generic in a computed name position should get you an intersection with a generic Record mapped type that can defer the property creation for you. At least I think that's how this'd have to be extended to work with generics/unions, based on what I know.
In any case, the literal-y-ness part of this is almost definitely welcome. Definitely get some generic pattern literals in computed name positions into the tests for this PR, though, since we want to verify those still just give string index signatures for now, since we don't really have a better behavior for that down.
src/compiler/checker.ts
Outdated
| if (isTypeAssignableTo(computedNameType, stringNumberSymbolType)) { | ||
| if (computedNameType && !isTypeUsableAsPropertyName(computedNameType)) { | ||
| if (isPatternLiteralType(computedNameType)) { | ||
| indexKeyTypes.add(computedNameType); |
There was a problem hiding this comment.
This probably warrants the "breaking change" tag, since previously these'd make a string index signature get added, but now you get a more specific index signature. Which is, I believe, the goal of the change, it's just... maybe breaky? It's unclear.
There was a problem hiding this comment.
Also there's the whole union thing - but the union thing can be a problem for another day.
|
@weswigham if I understand correctly the only requested change here was to add a generic test case and I've just pushed out one. Could you take another look? |
…rop-wiht-patterns # Conflicts: # tests/baselines/reference/indexSignatures1.types
Andarist
force-pushed
the
dont-widen-computed-prop-wiht-patterns
branch
from
e0df494
to
1a97927
Compare
addresses @jcalz's comment here: #13948 (comment) , it's related to the primary issue there ( #13948 ) but it doesn't address "union lifting". There was once a PR that addressed this ( #21070 ) but it wasn't merged (despite being approved). I could revive that work but I'm unsure what exactly was wrong with that PR so I'm not sure what could be prepared by me that would have any chance of being merged
improves #46309