Skip to content

Type safety lost with generic return type when object spread is used #50559

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

Closed
apendua opened this issue Aug 31, 2022 · 15 comments
Closed

Type safety lost with generic return type when object spread is used #50559

apendua opened this issue Aug 31, 2022 · 15 comments

Comments

@apendua
Copy link
Contributor

apendua commented Aug 31, 2022

Bug Report

πŸ”Ž Search Terms

type, generic, constraint, return, safe, spread

πŸ•— Version & Regression Information

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

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

type MyObject = { value: number };

function modify<T extends MyObject>(a: T): T {
    return {
        ...a,
        value: 'abc', // this should be an error?
    };
}

πŸ™ Actual behavior

TS has not reported any error even though value is not of the right type. This must be somehow related to the use of the spread operator because if I refactor the same function as:

function modify<T extends MyObject>(a: T): T {
    const b = { ...a };
    b.value = 'abc'; // error here!
    return b;
}

then the error is actually detected property.

πŸ™‚ Expected behavior

TS should detect an error in both cases as there's clearly a type violation there.

@whzx5byb
Copy link

Duplicate of #50185

@apendua
Copy link
Contributor Author

apendua commented Aug 31, 2022

@whzx5byb Is it a duplicate? Does this explain why the second implementation is actually type-safe while the first one isn't?

@fatcerberus
Copy link

fatcerberus commented Aug 31, 2022

Yes, per this comment in #50185:

Lacking a specific type operator to represent spread, ... uses intersection as the next-closest thing when one of the operands is a type parameter.

Basically, the compiler inaccurately models the spread as T & { value: string } (an impossible value, but T is opaque so the compiler doesn't realize that) which is assignable to T by the rules of intersection. You would need some kind of spread type, e.g., { ...T, value: string } to fix this.

The second case is typesafe and errors correctly because { ...a } just produces another T.

@apendua
Copy link
Contributor Author

apendua commented Sep 1, 2022

@fatcerberus Thank you for a detailed explanation.

I still don't understand one thing. Since there's also a type constraint on T, it seems like TS should be able to figure out that T & { value: string } equals never in this particular case. Indeed, there's no objet with property value which is both string and number at the same time. Isn't it forbidden to have values of type never, and { ...a, value: 'abc' } clearly is of this type?

I know that in fact never extends everything (so T in particular) but it's not the return statement where the validation should fail I think. It shouldn't be allowed to produce any values of type never, by the very definition of never. The following implementation is also accepted by TS despite the fact that the type of b is never. Though TS seems to be ignoring that fact:

function modify<T extends MyObject>(a: T): T {
    const b = {
        ...a,
        value: 'abc',
    };
    return b;
}

@fatcerberus
Copy link

it seems like TS should be able to figure out that T & { value: string } equals never in this particular case.

You'd think so, but TS generally treats type parameters as opaque. There are some cases where it can fall back on the constraint (i.e. pretending that T is its constraint), but usually it just gets deferred. The compiler has to defer in this case because MyObject is not guaranteed to be assignable to T, and it'd be weird to reject return { ...a } for that reason.

That said, even if the aforementioned reduction worked, it'd still be unsound - never has no values and critically, is assignable to everything (as you yourself note). Creating impossible intersections that reduce to never isn't itself an error, so there would need to be some other mechanism to catch this mistake. You can freely give something the type string & number, for example, and TS will happily proceed to type your variable as never. Usually that gets caught later, when you try to assign something to it, but if the compiler gives an expression the type never right from the start... well, you get this issue. πŸ˜…

Thinking about this some more, you don't want to reject the expression on the basis that its type reduces to never anyway, since { ...a, value: 'abc' } does in fact produce a legal runtime value. It's just that that value isn't actually compatible with T.

This would need a whole new type of check in the compiler, I think.

@apendua
Copy link
Contributor Author

apendua commented Sep 1, 2022

@fatcerberus

Creating impossible intersections that reduce to never isn't itself an error, so there would need to be some other mechanism to catch this mistake.

Yup, I fully agree as long as we are talking about types. But a value of type never shouldn't be allowed to exist don't you think so?

Thinking about this some more, you don't want to reject the expression on the basis that its type reduces to never anyway, since { ...a, value: 'abc' } does in fact produce a legal runtime value.

I wouldn't agree that returning a value of type never is ok if the function declares its return type as object. In fact the statement "never extends anything" is just a tautology which comes from the fact that the never type contains no values at all. So yes, for each value in never... actually any condition holds πŸ˜‰ But the essence of it is that never "contains no values", so why all of the sudden are we allowing expressions (that produce values) to have type never. It's obviously an error that should be detected by a type checking system.

@fatcerberus
Copy link

fatcerberus commented Sep 1, 2022

So yes, for each value in never... actually any condition holds

Yep, the principle of explosion. It is indeed a tautology, but mathematically sound nonetheless. never being assignable to every other type is expected, because you can prove anything from a contradiction (i.e. never has no values, but here we have a value of type never).

so why all of the sudden are we allowing expressions (that produce values) to have type never.

We actually agree on this point: the spread expression produces a legal value and so should not be typed as never in the first place. Type safety is already compromised so disallowing return neverVal is just papering over the real issue, which is that T & { value: string } is not an accurate model of the spread operation, even if T were concrete. Any expression that "produces" a never should throw, by definition.

@apendua
Copy link
Contributor Author

apendua commented Sep 1, 2022

@fatcerberus

We actually agree on this point: the spread expression produces a legal value and so should not be typed as never in the first place.

You're saying this but my point is exactly about this thing not being a legal value because it has type never. It seems like our definition of "legal" value may be different. I actually think that the type is evaluated properly here (what else would it be if not never?) The problem is compiler should definitely caught this error and currently it's ignoring it.

@fatcerberus
Copy link

fatcerberus commented Sep 1, 2022

{ ...a, value: 'abc' } produces an object that is identical to a except its .value is a string instead of a number. This value should not be assignable to typeof a (the actual bug here) but it is absolutely incorrect to reject the expression.

@apendua
Copy link
Contributor Author

apendua commented Sep 1, 2022

@fatcerberus

but it is absolutely incorrect to reject the expression.

Why is it "absolutely incorrect"?

@fatcerberus
Copy link

fatcerberus commented Sep 1, 2022

In short, because it doesn't throw. You're assuming never is the correct type to assign to that expression, but it isn't, because it succeeds at runtime and produces a value. The actual bug is that TS thinks the value it does produce is compatible with T because it doesn’t currently have a more accurate way than intersection to model the type of the value.

@fatcerberus
Copy link

In other words, { ...T, value: string } is not an empty type, it's just not inhabited by any values that are legal Ts. Therefore T & { value: string } is not a valid model of the spread. A more accurate model would be Omit<T, "value"> & { value: string } but that would make the type even more opaque than it already is.

@apendua
Copy link
Contributor Author

apendua commented Sep 1, 2022

@fatcerberus Got it! Thank you for:

Omit<T, "value"> & { value: string }

Thanks to this I finally understood where the actual problem is, i.e. the type of the expression not being evaluated properly. I agree that this is essentially a duplicate of: #10727 and also #50185.

@apendua apendua closed this as completed Sep 1, 2022
@fatcerberus
Copy link

@apendua No problem, glad I was able to communicate that. Sorry if I got too technical in spots; for all its unsoundness, TypeScript was my gateway drug into type/category theory so it's very easy for me to get carried away with theoretical explanations.

@flextremedev
Copy link

I know this is closed but I wanted to know if anyone's found a better solution for this problem than:

type MyObject = { value: number };

function modify<T extends MyObject>(a: T): T {
    return {
        ...a,
        ...({value: 'abc'} as T), // this should be an error?
    };
}

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

No branches or pull requests

4 participants