Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix getConstraintOfIndexedAccess #17870

Merged
merged 3 commits into from
Aug 18, 2017
Merged

Conversation

sandersn
Copy link
Member

Fixes #17069
Fixes #15371

  1. T[K] now correctly produces number when K extends string, T extends Record<K, number>.
  2. T[K] no longer allows any type to be assigned to it when T extends object, K extends keyof T.

Previously both of these cases failed in getConstraintOfIndexedAccessType because both bases followed K's base constraint to string and then incorrectly produced any for types (like object) with no string index signature. In (1), this produced an error in checkBinaryLikeExpression. In (2), this failed to produce an error in checkTypeRelatedTo`.

The fix has some additional pieces.

In order to fix (1), not only does the index type parameter need to be kept around, the base constraint of T extends Record<K, number> needs to be Record<K, number> not {} so that Record<K, number>[K] produces number. So I changed computeBaseConstraint to only chase T extends Record<K, number> back to Record<K, number>, not all the way to {}.

In order to fix (2), any index type whose base constraint is string, whether it came from keyof T or not, is going to (incorrectly) produce any when indexing a type that doesn't have a string index signature -- the test case specifically tries object[string]. So I added an early exit to getConstraintOfIndexedAccessType that returns undefined in this case.

1. `T[K]` now correctly produces `number` when
   `K extends string, T extends Record<K, number>`.
2. `T[K]` no longer allows any type to be assigned to it when
   `T extends object, K extends keyof T`.

Previously both of these cases failed in
getConstraintOfIndexedAccessType because both bases followed `K`'s base
constraint to `string` and then incorrectly produced `any` for types
(like `object`) with no string index signature. In (1), this produced an
error in checkBinaryLikeExpression`. In (2), this failed to produce an
error in `checkTypeRelatedTo`.
@sandersn sandersn requested a review from ahejlsberg August 17, 2017 20:17
@sandersn
Copy link
Member Author

@ahejlsberg this undoes a change you added in the fix for #17521, but doesn't break any tests. I looked at the commit and the associated tests and the rest of the change looks like it handles types with index signatures, not mapped types. Regardless, you should take a look and then we can talk about the right way to fix this bug (#17069) at the same time as #17521's fix.

@aozgaa aozgaa merged commit 439cdca into master Aug 18, 2017
@ghost ghost deleted the fix-getConstraintOfIndexedAccess branch August 18, 2017 18:34
@aozgaa
Copy link
Contributor

aozgaa commented Aug 18, 2017

Merged this by mistake, sorry! (immediately reverted.)

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants