GeezerVison is showing github.com — loosen number index check, fixes #15768 by KiaraGrouwstra · Pull Request #17767 · microsoft/TypeScript
Conversation
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@tycho01,
Thanks for having already signed the Contribution License Agreement. Your agreement has not been validated yet. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
const typeOrConstraint = (tp: Type) => maybeTypeOfKind(tp, TypeFlags.TypeVariable) ? getBaseConstraintOfType(tp) || tp : tp;
|
|
|
if (isTypeAssignableToKind(typeOrConstraint(indexType), TypeFlags.NumberLike) &&
|
|
|
getIndexInfoOfType(typeOrConstraint(objectType), IndexKind.Number)) {
|
|
|
return type;
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to just:
if (getIndexInfoOfType(getApparentType(objectType), IndexKind.Number) && isTypeAssignableToKind(indexType, TypeFlags.NumberLike)) {
return type;
}
Otherwise PR looks good. Thanks!
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
KiaraGrouwstra reacted with thumbs up emoji
KiaraGrouwstra reacted with hooray emoji
All reactions
-
👍
1 reaction
-
🎉
1 reaction
|
Fixed. That's definitely even cleaner! 😅
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I might be late to the party but this doesn't seem right. When the index is out of range, the result is a union of all constituents. While that's how expression level tuples work, it's not correct and we shouldn't spread misleading behaviour to the type level.
type X<I extends number> = ['a'][I];
type Test = X<1>// 'a' :/
|
aluanhaddad reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Marin Marinov (@gcnew): see also type X = ['a'][1]; // 'a'. I guess it'd be hard to remove its number index while retaining it for the expression level though, presuming you wouldn't just remove it for both altogether.
A related concern I have is tuple type -> array type assignability mentioned at #17785, which could unblock inferring tuples rather than always widening them.
On another note, I do still get a few Type <generic> cannot be used to index type <array> errors; I'll see if I can make a minimum repro.
|
gcnew and aluanhaddad reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@tycho01 Yes, you are right. Unfortunately it's been broken and you've made it more consistent. In this case 👍.
|
aluanhaddad reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Marin Marinov (@gcnew): I gave the issue you raised a bit more thought.
When the index is out of range, the result is a union of all constituents. While that's how expression level tuples work, it's not correct and we shouldn't spread misleading behaviour to the type level.
I think the root question here is what the index types are for. There appear to be three use-cases:
['a'][1]: expected result might be either an error or equivalent to the expression variant, undefined
['a'][number], intending to get the fallback value, see above. terrible example as ['a'][-1] would cut it consistently, but then again they resolve by the same mechanism.
['a'][number], intending to get a union of all good values. workaround exists.
['a'][number], intending to get a union of all possible return values, both good and fallback (see above).
I do suppose all of this only goes to show you're right that current behavior for tuples may not be ideal.
For object types it's actually perfect; it errors so property access is safe, while getting all good values or a fallback is trivial as well.
For tuple types on the other hand, due to the current behavior getting a fallback value now requires... explicit checks.
I'm not surprised about the current behavior in the sense it just got this index behavior from Array, though it could technically overwrite to return a more narrow type, as I suggested for length.
Breaking changes would probably all get flags if they get in at all, and I do wonder about the potential impact (both positive and negative) of such a change -- or what the desired behavior should be at all.
Power users will find ways how to do anything anyway, while it seems tough to optimize for casual users at all, except maybe by never changing behavior -- it's anyone's guess which of the above potential use-cases of indices they might expect to be able to use them for anyway.
It isn't obvious what they 'should' be for to me either, except homogeneous Object and Array needed them and tuples just kinda happened to be arrays in JS.
Not that I oppose changes, hopefully (under flags) we can add in all the more ideal behavior.
tl;dr elaborate? what would you prefer and why?
Edit: to explain my confusion, the alternatives each have issues:
undefined reflects JS behavior for ['a'][1], yet clashes the sub-type requirement of Array<union>
never sub-types it but doesn't reflect behavior of ['a'][1] and is dangerous (in RHS it's an any)
- erroring on e.g.
['a'][1] though not on ['a'][number] (leaving the latter with its current behavior, or even never) seems... viable, I guess?
Do those last ideas match what you'd been hoping for there? I'm pretty neutral here, just curious.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I think it would have been best if tuples behaved like object with their indices as keys. All the normal object rules should be in effect. For example, the indexing type variable should be a keyof of the tuple, or the tuple should be intersected with an indexer to provide a default value.
// ['syntax', true] => { 0: 'syntax', 1: true }
type Tup = ['syntax', true]
type T1<I> = Tup[I] // error
type T2<I extends number> = Tup[I] // error
type T2<I extends keyof Tup> = Tup[I] // OK
type T2<I extends number> = (Tup & { [k: number]: undefined })[I] // OK
On a side note, I don't think tuples should be a subclass of Array. In the general sense, they are not arrays, just values packed together. That's further exacerbated by the fact that tuples are heterogenous in the majority of cases.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
I don't think tuples should be a subclass of Array. In the general sense, they are not arrays, just values packed together.
I guess, yeah.
Complication from recent experience: in my fixed length PR I originally had tuples extend a new Tuple interface extending ReadonlyArray instead of Array.
That just made things not compile anymore, yielding some assignability errors regarding lack of push method and the like until I had it extend Array again instead.
Unfortunately I squashes out those commits though, and I guess a couple methods already aren't compatible; there may be a way I guess.
Tuple should benefit from the decoupling too -- the indexOf variant it's inheriting off Array is utterly useless on [1,2,3] (can't search for a number as that isn't within T for Array<1|2|3>).
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 24, 2017
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|