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

Indexed Accesses Checking Allow Unsafe Assignments #26274

Closed
weswigham opened this issue Aug 7, 2018 · 1 comment · Fixed by #26281
Closed

Indexed Accesses Checking Allow Unsafe Assignments #26274

weswigham opened this issue Aug 7, 2018 · 1 comment · Fixed by #26281
Assignees
Labels
Bug A bug in TypeScript
Milestone

Comments

@weswigham
Copy link
Member

weswigham commented Aug 7, 2018

Code

class Component<S> {
    state!: S; // assume a default state is provided in some way
    setState<K extends keyof S>(state: Pick<S, K>) { /*react-like setState*/ }
}
    
export interface State<T> {
    a: T;
}
    
class Foo {}
    
class Comp<T extends Foo, S> extends Component<S & State<T>>
{
    foo(a: T) {
        this.setState({ a: a });
    }
}

Expected behavior:
Error on setState - a T is not inherently comparable with a S["a"] - since S is unconstrained, so too is S["a"].
For a concrete subtype which exposes the issue, consider:

class BrokenComponent extends Comp<Foo, { a: { x: number } }> {
    main() {
        const f = new Foo();
        this.foo(f); // allowed
        this.state.a.x; // According to the typesystem, this should exist, but it won't!
    }
}

Actual behavior:
No error.

@weswigham
Copy link
Member Author

weswigham commented Aug 7, 2018

The issue is caused by the errorType manifesting in comparison checking - Pick<S & State<T>, K> expands to {[K1 in K]: (S & State<T>)[K1]} which we then index with "a" (and instantiate K as "a") when checking against the object, making it {[K1 in "a"]: (S & State<T>)[K1]}["a"] which simplifies to (S & State<T>)["a"] which fails to simplify further. When the constraint of (S & State<T>)["a"] is retrieved (as indexed accesses are checked using their constraint, which we know is slightly incorrect), we return the errorType (as S has no member "a" on its constraint), which is why the assignment is allowed. It should simplify to S["a"] & State<T>["a"] which then simplifies to S["a"] & T - which can then be compared as an intersection of two type variables as expected (as an index of a type variable such as a type parameter is always consider to be a type variable itself).

This exposes two issues:

  1. Indexed accesses should always distribute over intersections, and likewise unions. In other words, (A & B)[K] === A[K] & B[K] and (A | B)[K] === A[K] | B[K]. Together, with our other flattening rules, this means it should always be Union > Intersection > Accesses. This could actually be a great simplification for us in terms of comparisons and normalization. Additionally, (A & B)["a"] we can't make any inferences on, however A["a"] & B["a"] allows us to simplify further (if any member is simplifiable) and potentially produce good inference targets - so this flattening should generally improve inference, too.
  2. The errorType should never be allowed to manifest during relationship checking or simplification. it should be the unknown type instead (which also jibes much better with the distributive relationships described above - just as with expression checking this means that in an intersection, members without the key are silently dropped, and in a union they result in not getting a type other than unknown out). This means that even though it's an error, logically, to attempt to index a type with something that does not exist, the behavior of such an operation is should always be well-defined.
    While I believe this to always be the case, we can't exactly change our behavior at the expression level here - outside of noImplicitAny, T["whatever"] needs to continue to be any to be compatible with existing usages in the wild (and changing it in noImplicitAny would just add more follow on errors, since the access is already flagged). However in the type-space, I now believe the extra strictness of the unknown result is mandatory for a consistent series of simplifications and identities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants