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

Only apply indexed access write simplifications to types that arise from mutation #33089

Closed
wants to merge 1 commit into from

Conversation

jack-williams
Copy link
Collaborator

@jack-williams jack-williams commented Aug 26, 2019

This is an attempt at alleviate some additional errors introduced by #30769. The write simplifications introduced there were applied whenever a type was related to an indexed access type, irrespective of whether that type denoted a real update or just a value of lookup-type.

This PR only applies the write simplifications to indexed access types that arise through assignments to element access expressions.

Fixes #31833
Fixes #32816

A related digression: there are code paths that do not get write-simplified (with and without this PR) which introduce unsoundness. Is this by design?

const foo = { x: { a: true }, y: { b: 42 } };
type Foo = typeof foo;

const bar = { x: { c: false }, y: { d: 100 } };
type Bar = typeof bar;

function example(arg: Foo | Bar, k: "x" | "y") {
    // The union Foo | Bar is not simplified
    // to an intersection under indexed access
    // even though arg[k] conceptually has the
    // type: (Foo | Bar)["x" | "y"]
    arg[k] = { a: true, b: 100 } // no error, but what if arg is Bar?
}

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are code paths that do not get write-simplified (with and without this PR) which introduce unsoundness. Is this by design?

Yeah; element accesses are generally slightly unsound.

This approach looks fine to me, but we'll need to sync it and run the extended test suites on it. Also ping @ahejlsberg since I imagine he'll be interested in it.

@jack-williams
Copy link
Collaborator Author

This approach looks fine to me, but we'll need to sync it and run the extended test suites on it. Also ping @ahejlsberg since I imagine he'll be interested in it.

@weswigham, will do! FWIW, very few people seem to have stumbled upon this issue so maybe it isn't worth the additional complexity.

@sandersn
Copy link
Member

Unfortunately, we never finished reviewing this PR. It is pretty old now, so I'm going to close it to reduce the number of open PRs.

@sandersn sandersn closed this May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
4 participants