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

Constrained generic type parameters accept invalid updates #60623

Closed
sliminality opened this issue Nov 27, 2024 · 4 comments
Closed

Constrained generic type parameters accept invalid updates #60623

sliminality opened this issue Nov 27, 2024 · 4 comments
Labels
Duplicate An existing issue was already created

Comments

@sliminality
Copy link

sliminality commented Nov 27, 2024

🔎 Search Terms

generic type variables unsafe modifications, generic type variables mutation, polymorphic function return type, generic type variable invalid assignment, polymorphic type variable unsound assignment

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about Generics

⏯ Playground Link

💻 Code

Given a variable x whose type is generic parameter T, we can spread invalid updates alongside x, and the result is still assignable to T:

type Image = {
    type: "image",
    src: string,
}

type Checkbox = {
    type: "checkbox",
    checked: boolean,
}

function unsafe<T extends Image | Checkbox>(x: T): T {
    return { ...x, src: 1 } // Clobbers required field!
}

const image = unsafe({ type: "image", src: "harold.gif" })

image satisfies Image // passes
image.src satisfies string // passes
console.log(image.src.charAt(0)) // Exception: image.src.charAt is not a function

Likewise, we can delete required fields in T:

type Author = 
    | { type: "member", name: string, userId: string }
    | { type: "guest", name: string, userId?: string }

function deleteUserId<T extends Author>(x: T): T {
    delete x.userId // Required for members!
    return x
}

const member = deleteUserId({
    type: "member",
    name: "Slim",
    userId: "c3d98f32-2abb-4a55-a497-db875deff5ee",
})

member satisfies Author // passes
member.userId satisfies string // passes
console.log(member.userId.charAt(0)) // Exception: member.userId is undefined

🙁 Actual behavior

TypeScript does not error when clobbering or deleting a required field in a generically-typed value, and continues to infer the type of the original parameter, which is now invalid.

🙂 Expected behavior

I'd expect any of the following:

  1. Simpler, most conservative: TypeScript prevents me from performing an update if it would invalidate any possible T.

    • Attempting to overwrite a field required in any possible T with an invalid type produces an error like Type 'number' is not assignable to type 'string'. (2322)
    • Attempting to delete a field that is required in any possible T should produce the error The operand of a 'delete' operator must be optional. (2790)
  2. Smarter, more complex: I can perform an update that is valid for some instantiations of T, but only after narrowing the type within the function body. Parametricity must also be preserved in the return type.

    • Attempting to overwrite the src field would only be permissible if x has been refined to type T extends Checkbox, where src can be treated as a superfluous property under the inexact object subtyping model. The narrowed branch must return a value assignable to T extends Checkbox.
    • Attempting to delete the userId field would only be permissible if x has been refined to type T extends { type: "guest" … }, where userId is optional. The narrowed branch must return a value assignable to T extends { type: "guest" … }.

    Note that in both cases, we can narrow safely because T is constrained by a disjoint union of objects, where the standard depth and width subtyping rules apply. That is, while T could be instantiated with a different object like

    { 
      type: "member", 
      name: "Nemo" | "Dory",
      userId: string,
      randomOtherField: boolean
    }

    this T must still contain all fields declared in the base member type, with types assignable to the corresponding fields.

There is also a third option for immutable updates, as in the first example:

  1. Allow updates, but refine the resulting type so it is not assignable to T: TypeScript could allow me to create the value { ...x, src: 1 }, but prevent me from returning this value as a T.

    Currently, TypeScript considers T & { src: number } assignable to T. But if T extends Image | Checkbox, then the resulting intersection

    { type: "image", src: string & number }
    | { type: "checkbox", checked: boolean }

    cannot satisfy the original constraint, and therefore cannot be returned by the function.

For what it's worth, Flow uses Option 3 for the case of spreading updates into a new object, and Option 1 for the case of mutating the original argument.

Additional information about the issue

No response

@jcalz
Copy link
Contributor

jcalz commented Nov 27, 2024

Note: not a TS team member.

Partially a duplicate of #50559, ultimately a likely duplicate of #9825 or similar (i.e., TS has many known holes in its type system that are present because plugging them makes things worse overall.)

@RyanCavanaugh
Copy link
Member

Duplicate #10727 - { ...x, src: 1 } is typed the way it is because that's the closest thing to a correct representation currently available. I don't think any of the proposals in this issue would work out in practice -- they fundamentally don't seal the hole being identified here, since T could be further subtyped, e.g. T could be

type MySpecialImage = {
    type: "image",
    src: "neat",
}

so it's not enough to validate that src is string

delete is rarely used and is considered "at your own risk". Since we don't have prototype modeling in the type system, there's no good way to predict what's going to happen.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Nov 27, 2024
@typescript-bot
Copy link
Collaborator

This issue has been marked as "Duplicate" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 30, 2024
@sliminality
Copy link
Author

{ ...x, src: 1 } is typed the way it is because that's the closest thing to a correct representation currently available

While I agree that a type-level spread operator would be optimal, I don't think it's necessary here. Even if we model { ...x, src: 1 } as an intersection, ideally we could conclude that

(T extends Image | Checkbox) & { src: number }

ends up with src typed as string & number, which already reduces to never when using concrete types. Subtyping T can only make src narrower, and "neat" & number is still never.

since T could be further subtyped, e.g. T could be

type MySpecialImage = {
    type: "image",
    src: "neat",
}

so it's not enough to validate that src is string

I agree it's not enough to validate that src is string. I still think it would be a lot better than nothing (i.e. would help to catch errors in practice), and strictly sounder (i.e. consistent with the hypothetical perfect implementation, rather than a one-off hack that adds tech debt). To me this fits the TypeScript ethos of incremental improvements + not letting the perfect be the enemy of the good.

Fair enough on delete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants