FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Improve soundness of indexed access types by ahejlsberg · Pull Request #30769 · microsoft/TypeScript · GitHub

Improve soundness of indexed access types - #30769

Merged
Anders Hejlsberg (ahejlsberg) merged 22 commits into
masterfrom
saferIndexedAccessTypes
Apr 12, 2019
Merged

Improve soundness of indexed access types#30769
Anders Hejlsberg (ahejlsberg) merged 22 commits into
masterfrom
saferIndexedAccessTypes

Conversation

Copy link
Copy Markdown
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 (jack-williams) commented Apr 5, 2019
edited
Loading

Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
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!

Copy link
Copy Markdown
Member Author

Wesley Wigham (@weswigham) Can you take a look at why this is failing on PragmaPsuedoMap?

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Member Author

Wesley Wigham (@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.

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) test this

TypeScript Bot (typescript-bot) commented Apr 6, 2019
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@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.

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) run dt

TypeScript Bot (typescript-bot) commented Apr 6, 2019
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@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.

Copy link
Copy Markdown
Collaborator

Thanks for the detailed write-up Anders Hejlsberg (@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!

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) test this

Copy link
Copy Markdown
  • 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.

Anders Hejlsberg (@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?

Copy link
Copy Markdown
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.

Craig P Hicks (craigphicks) commented Dec 29, 2023
edited
Loading

Copy link
Copy Markdown

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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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

Back | FazBrowse Home | New Git URL