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

Improve soundness of indexed access types #30769

Merged
merged 22 commits into from Apr 12, 2019
Merged

Conversation

ahejlsberg
Copy link
Member

With this PR we improve soundness of indexed access types in a number of ways:

  • When an indexed access T[K] occurs on the source side of a type relationship, it resolves to a union type of the properties selected by T[K], but when it occurs on the target side of a type relationship, it now resolves to an intersection type of the properties selected by T[K]. Previously, the target side would resolve to a union type as well, which is unsound.
  • Given a type variable T with a constraint C, when an indexed access T[K] occurs on the target side of a type relationship, index signatures in C are now ignored. This is because a type argument for T isn't actually required to have an index signature, it is just required to have properties with matching types.
  • A type { [key: string]: number } is no longer related to a mapped type { [P in K]: number }, where K is a type variable. This is consistent with a string index signature in the source not matching actual properties in the target.
  • Constraints of indexed access types are now more thoroughly explored. For example, given type variables T and K extends 'a' | 'b', the types { a: T, b: T }[K] and T are now considered related where previously they weren't.

Some examples:

function f1(obj: { a: number, b: string }, key: 'a' | 'b') {
    obj[key] = 1;    // Error
    obj[key] = 'x';  // Error
}

function f2(obj: { a: number, b: 0 | 1 }, key: 'a' | 'b') {
    obj[key] = 1;
    obj[key] = 2;  // Error
}

function f3<T extends { [key: string]: any }>(obj: T) {
    let foo = obj['foo'];
    let bar = obj['bar'];
    obj['foo'] = 123;  // Error
    obj['bar'] = 'x';  // Error
}

function f4<K extends string>(a: { [P in K]: number }, b: { [key: string]: number }) {
    a = b;  // Error
    b = a;
}

Previously, none of the above errors were reported.

Fixes #27895.
Fixes #30603.

@jack-williams
Copy link
Collaborator

jack-williams commented Apr 5, 2019

Has the behaviour that was added in #27490 been taken out? The following function did error, but not under the new rules.

function fun<U extends { a: T }, T, K extends 'a'>(u: U, k: K, shambles: T): U[K] {
  u[k] = shambles; // was error
  return u[k];
}

const result: number = fun<{ a: number }, unknown, 'a'>({ a: 42 }, 'a', 'a string');

I think the reason that the test-cases from #27470 didn't regress is because the restrictive instantiation has been subsequently added to conditional types, which catches the error too.

@ahejlsberg
Copy link
Member Author

Has the behaviour that was added in #27490 been taken out?

Yes, but that behavior wasn't right. For an indexed access T[K] we flat out didn't explore any constraints on the target side when both T and K were generic, which is quite inconsistent. For example, if you change the assignment in your example from u[k] = shambles to u['a'] = shambles there is no error before or now.

The real (and orthogonal) issue in your example is that we treat an assignment to T[K] as an assignment to C[K], where C is the constraint of T. I discuss the reasons in my comment here. The unsoundness caused by that rule continues to exist (and would be quite a bit harder to remove).

What has changed with this PR is that for a T[K] on the target side of a relation we now ignore index signatures in the constraint of T. That's definitely the right thing to do (because a type argument for T isn't actually required to have an index signature, it is merely required to have properties with matching types) and that's what really fixes the issue in #27470.

It's all rather involved!

@ahejlsberg
Copy link
Member Author

@weswigham Can you take a look at why this is failing on PragmaPsuedoMap?

@weswigham
Copy link
Member

Looks like we're pulling out some unexpected index signatures from the target or something? The bug should repro in the test harness, will just need to copy the relevant types into a test file.

@ahejlsberg
Copy link
Member Author

@weswigham It's actually identifying an unsafe coercion. It used to be that PragmaPseudoMap["reference"] was (unsoundly) assignable to PragmaPseudoMap[K], where K extends keyof PragmaPseudoMap, which, in combination with parameter bi-variance (because the compiler isn't yet using --strictFunctionTypes), caused the explicit type annotation on (arg: PragmaPseudoMap["reference"]) => ... to be accepted. That's no longer the case, and that's a good thing.

The easy fix is to explicitly cast the array parameter to forEach to the appropriate type.

@ahejlsberg
Copy link
Member Author

@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 6, 2019

Heya @ahejlsberg, I've started to run the extended test suite on this PR at e1fd5e5. You can monitor the build here. It should now contribute to this PR's status checks.

@ahejlsberg
Copy link
Member Author

@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 6, 2019

Heya @ahejlsberg, I've started to run the Definitely Typed test suite on this PR at e1fd5e5. You can monitor the build here. It should now contribute to this PR's status checks.

@jack-williams
Copy link
Collaborator

Thanks for the detailed write-up @ahejlsberg.

The real (and orthogonal) issue in your example is that we treat an assignment to T[K] as an assignment to C[K], where C is the constraint of T. I discuss the reasons in my comment here. The unsoundness caused by that rule continues to exist (and would be quite a bit harder to remove).

True, though did you not add behaviour to prevent pulling down C when T and K were both generic variables which is no longer in this PR? I understand that the original issue that prompted the fix is now correctly handled for other (more sensible) reasons, but discarding a sound rule seems a shame! Though, the rule is abit ad-hoc!

@ahejlsberg
Copy link
Member Author

With latest commits there are three RWC projects with new errors. The errors in two of the projects are minor and identify unsoundness that we previously didn't catch. The last project, fp-ts, which already has massive inverted pyramids of error messages, now looks even more daunting. It makes heavy use of indexed access types and when they occur in target positions we create intersection types instead of union types. There are several new errors that appear to be correct with the stricter checking, but it's hard to tell.

@ahejlsberg
Copy link
Member Author

@typescript-bot test this

@quixot1c
Copy link

  • Given a type variable T with a constraint C, when an indexed access T[K] occurs on the target side of a type relationship, index signatures in C are now ignored. This is because a type argument for T isn't actually required to have an index signature, it is just required to have properties with matching types.

@ahejlsberg If "a type argument for T isn't actually required to have an index signature", why is indexed access still allowed on the source side of a type relationship?

@ahejlsberg
Copy link
Member Author

If "a type argument for T isn't actually required to have an index signature", why is indexed access still allowed on the source side of a type relationship?

An indexed access T[K] doesn't actually require T to have an index signature. It's more helpful to think of it as a property access. As long as K is known to reference the name of a property in T (i.e. because K is a literal type like "foo"), then T[K] is permitted. However, an indexed access T[string] does require T to have an index signature.

@craigphicks
Copy link

craigphicks commented Dec 29, 2023

Could the errors in this test case be overly strict?

function f3<T extends { [key: string]: any }>(obj: T) {
    let foo = obj['foo'];
    let bar = obj['bar'];
    obj['foo'] = 123;  // Error  - (its writable and matches the parameters why error?)
    obj['bar'] = 'x';  // Error - (ditto)
}

This readonly function format exists if errors are desired:

function f5<T extends Readonly<{ [key: string]: any }>>(obj: T) {
    let foo = obj['foo'];
    let bar = obj['bar'];
    obj['foo'] = 123;  // Error
    obj['bar'] = 'x';  // Error
}
declare const obj: {
  foo: boolean;
  bar: boolean;
};
f5(obj);

Shouldn't TypeScript assume the user is reading the function parameters and trust them not to call f3 because of the any type and the fact that it is non-readonly?

Conversely, the user may have a special need for writing a writable any typed function like f3, and now they are not allowed to write it. That could be frustrating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Function can return wrong values despite type Change in generic behavior between 3.0.1 and >=3.1.1