Skip to content

Type containing intersection of object with optional property allows unsafe assignment #22255

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
kpdonn opened this issue Mar 1, 2018 · 6 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@kpdonn
Copy link
Contributor

kpdonn commented Mar 1, 2018

TypeScript Version: 2.8.0-dev.20180228

Search Terms:

  • optional intersection
  • partial intersection
  • unsafe assignment
  • incompatible assignment
  • invalid assignment
  • incorrect assignment
  • unsound assignment

Code

// A *self-contained* demonstration of the problem follows...

declare const a:  {x?: string} & {y: any}
const example1: {x?: number, y: any} = a // Expected error but didn't get one
const example2: {x?: number} = a // Got a TS2322 error as expected if the y parameter is excluded

declare const b:  {x?: string, y: any} 
const example3: {x?: number, y: any} = b // Now get a TS2322 error as expected

type MappedIdentity<T> = { [K in keyof T]: T[K] }
declare const c: MappedIdentity<{x?: string} & {y: any}>
const example4: {x?: number, y: any} = c // TS2322 error as expected when going through identity mapping

Expected behavior:
Error for example1.

Actual behavior:
No error for example1. The other examples are working as expected I just added them for context.

Playground Link:
https://www.typescriptlang.org/play/#src=declare%20const%20a%3A%20%20%7Bx%3F%3A%20string%7D%20%26%20%7By%3A%20any%7D%0Aconst%20example1%3A%20%7Bx%3F%3A%20number%2C%20y%3A%20any%7D%20%3D%20a%20%2F%2F%20Expected%20error%20but%20didn't%20get%20one%0Aconst%20example2%3A%20%7Bx%3F%3A%20number%7D%20%3D%20a%20%2F%2F%20Got%20a%20TS2322%20error%20as%20expected%20if%20the%20y%20parameter%20is%20excluded%0A%0Adeclare%20const%20b%3A%20%20%7Bx%3F%3A%20string%2C%20y%3A%20any%7D%20%0Aconst%20example3%3A%20%7Bx%3F%3A%20number%2C%20y%3A%20any%7D%20%3D%20b%20%2F%2F%20Now%20get%20a%20TS2322%20error%20as%20expected%0A%0Atype%20MappedIdentity%3CT%3E%20%3D%20%7B%20%5BK%20in%20keyof%20T%5D%3A%20T%5BK%5D%20%7D%0Adeclare%20const%20c%3A%20MappedIdentity%3C%7Bx%3F%3A%20string%7D%20%26%20%7By%3A%20any%7D%3E%0Aconst%20example4%3A%20%7Bx%3F%3A%20number%2C%20y%3A%20any%7D%20%3D%20c%20%2F%2F%20TS2322%20error%20as%20expected%20when%20going%20through%20identity%20mapping

Related Issues:
There are obviously a lot of issues about unsafe assignments but after looking through all the search terms above and checking the issues that have titles that sounded related, the only one that really seemed similar was #19927 which I opened a few months ago.

This case seems different to me though (specifically because of example2 above, which is the most similar to #19927 but has different behavior) which is why I'm creating the separate issue. But they're both about unsafe assignments that are allowed when there are weird combinations of optional properties and intersections so maybe there is a single root cause behind them both.

@jack-williams
Copy link
Collaborator

jack-williams commented Mar 1, 2018

Is this is similar problem?

const a: { y: string, x: number } = { y: 'hello', x: 4 };
const b: { y: string } = a; 
const c: { y: string; x?: boolean } = b;
const d: boolean | undefined = c.x; // No error. c.x is 4.

TypeScript has the following assignment compatibility rule:

{a: A; b: B; ...; y: Y } is a assignment compatible to {a: A; b: B; ...; y: Y; z?: Z }

also writen as:

M is an optional property and S has no apparent property of the same name as M.

where S is the type on the left hand side (source of the assignment).

This is not sound in general.

According to the definition of apparent members I think yours should still be rejected, but mine should not.

When one of more constituent types of I have an apparent property named N, I has an apparent property named N of an intersection type of the respective property types.

@ahejlsberg
Copy link
Member

This one is working as intended, even if it is unfortunate. It is generally the case that a type A & B is assignable to a type T if either A or B is assignable to T. In your particular example, { y: any } is assignable to { x?: number, y: any }. Therefore, for any given X, { y: any } & X is also assignable to { x?: number, y: any }. But obviously that's problematic when X contains an incompatible x property.

The core dilemma really is that we say (a) a type X is assignable to a type T even when X omits optional properties in T, and (b) a type S is assignable to X even if S has properties not present in X. To be provably safe you can really only have one of those rules--but removing either would be hugely inconvenient.

@ahejlsberg ahejlsberg added the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 1, 2018
@jack-williams
Copy link
Collaborator

@ahejlsberg

Do you think it would ever be suitable to have some stricter mode that always applies rule (a) before (b), so that it's not possible to forget properties then re-add them at different types in the same compatibility test. In the strict mode, if users still wanted the original behaviour they could factor through an intermediate type in two coercions.

@ahejlsberg
Copy link
Member

Do you think it would ever be suitable to have some stricter mode that always applies rule (a) before (b)

Not it a consistent manner. Given an intersection A & B, if A and B are both non-generic types (as in the original example) it certainly is possible to be stricter. However, when one type is generic, as in { y: any } & X, it would be odd to disallow assignment just because X might be incompatible. Indeed, if we disallowed that, then even when A or B is assignable to T, we'd have to say that A & B isn't. That obviously is not what we want. So, there's no way to do this consistently.

@kpdonn
Copy link
Contributor Author

kpdonn commented Mar 2, 2018

Thanks for the explanation I'll close this then. Even in the case where I ran into this it was easy to work around via adding the extra layer of mapped type so it was not very much of an inconvenience.

@kpdonn kpdonn closed this as completed Mar 2, 2018
@jack-williams
Copy link
Collaborator

@ahejlsberg Thanks for the explanation--I overlooked generics.

I think I'll just have to concede that my view on strictness here is not pragmatic.

it would be odd to disallow assignment just because X might be incompatible.

For instance, this would not be odd to me for the same reason that X should not be assignable to number because it might be incompatible. I appreciate that while there might be a conceptual similarity, in practice one is useful to have while the other not.

Indeed, if we disallowed that, then even when A or B is assignable to T, we'd have to say that A & B isn't.

I agree this is undesirable, but I think the rules currently prevent things might be desirable such as:

  1. if R is a subtype of T, and T is assignment compatible to S, then R is assignment compatible to S

That applies in some cases but not others:

  • {a: string} & {b: number} is assignable to {a: string, b?: string }
  • {a: string, b: number} is not assignment compatible to { a: string, b?: string }

A stricter rule would be consistent in that (1) holds for all types when the assignment from T to S does not involve adding adding optional properties, and when the assignment does add properties then the subtyping relation may not remove said properties, also applying equally to all types.

Anyway the issue is closed; I think there is enough evidence to suggest that any stricter rule would not be suitable in general.

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

3 participants