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

Object spread with Partial breaks in TS 2.4 #16509

Closed
evmar opened this issue Jun 13, 2017 · 9 comments
Closed

Object spread with Partial breaks in TS 2.4 #16509

evmar opened this issue Jun 13, 2017 · 9 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@evmar
Copy link
Contributor

evmar commented Jun 13, 2017

Test program works in TS2.3 and fails in TS2.4:

interface Fields {
  foo: number;
  bar: string;
}

declare let y: Fields;
declare let z: Partial<Fields>;

let x: Fields = {...y, ...z};
sigint:~/t$ tsc --version
Version 2.3.4
sigint:~/t$ tsc
sigint:~/t$ ./node_modules/.bin/tsc --version
Version 2.4.0
sigint:~/t$ ./node_modules/.bin/tsc 
f.ts(9,5): error TS2322: Type '{ foo: number | undefined; bar: string | undefined; }' is not assignable to type 'Fields'.
  Types of property 'foo' are incompatible.
    Type 'number | undefined' is not assignable to type 'number'.
      Type 'undefined' is not assignable to type 'number'.
@Jessidhia
Copy link

Jessidhia commented Jun 14, 2017

That might actually be catching a bug, but I think it's running into typescript limitations.

If z is missing the fields (or they're all not undefined), then the result x will still be a valid object; but if z has one of the Fields' fields and it is set to undefined, then x will be an invalid object with an invalid value. TypeScript doesn't distinguish an unset field from a field set to undefined (other than in excess property checks?).

Object.assign({a: 'a'}, {}) // => {a: 'a'} (matches {a: string})
Object.assign({a: 'a'}, {a: undefined}) // => {a: undefined} (does not match {a: string})

@sandersn sandersn added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Jun 14, 2017
@sandersn
Copy link
Member

@Kovensky is right. Typescript does not currently distinguish between missing and undefined in most cases [1]. We discussed adding missing but came to the conclusion that the additional complexity of yet-another-null-type is not currently worth it, even if we could find a way for users not to need to annotate it.

So 2.4 errs on the side of safety and prevents @Kovensky's second example from compiling at the cost of preventing the first, correct example from compiling.

[1] The excess property check is an ad-hoc check carried out in a completely different way from other assignability checking.

@evmar
Copy link
Contributor Author

evmar commented Jun 14, 2017

That's interesting. The language syntactically makes a distinction between

interface Foo { bar?: string }
interface Foo2 { bar: string|undefined }

but you're saying they are typed the same?


The best workaround I could find is to cast away from partial when you use it. Kinda disappointing though. Do you have any better suggestion?

function update(f: Foo, newFields: Partial<Foo>) {
  // no longer works:
  // return {...f, ...newFields };
  // workaround:
  return {...f, ...(newFields as Foo) };
}

@mhegazy
Copy link
Contributor

mhegazy commented Jun 14, 2017

This issue has been raised erlier in #6613 and #12793.

@mhegazy mhegazy reopened this Jun 14, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Jun 14, 2017

This looks like an unintended consequence of #15938. untill we have a way to distinguish between missing and undefined, we should error on the side of usability, and treat optional properties with undefined in their type as missing

@mhegazy mhegazy added Bug A bug in TypeScript and removed Design Limitation Constraints of the existing architecture prevent this from being fixed labels Jun 14, 2017
@mhegazy mhegazy added this to the TypeScript 2.4.1 milestone Jun 14, 2017
@sandersn
Copy link
Member

@evmar basically yes. In this case we're going to make an exception for spread since {...T, ...Partial<T>} should mean that T is a bag of defaults and Partial<T> is a bag of overrides, which nobody has been foolish enough to stick undefined in. This is technically a hole, but we're stuck with holes or over-strictness without missing.

I'll open an issue to track distinguishing between missing and undefined.

@sandersn
Copy link
Member

Here's a stub for distinguishing missing and undefined: #16524

It still needs a lot more detail.

sandersn added a commit that referenced this issue Jun 14, 2017
Fixes #16509 by making the change from #15938 less strict. This is
technically a hole, but it's not as big a hole as before #15938.
@sandersn
Copy link
Member

Fix is up at #16525

@sandersn sandersn added this to In Progress in Rolling Work Tracking Jun 14, 2017
@sandersn sandersn moved this from In Progress to Done in Rolling Work Tracking Jun 14, 2017
@sandersn sandersn added the Fixed A PR has been merged for this issue label Jun 14, 2017
evmar added a commit to angular/tsickle that referenced this issue Jun 14, 2017
You can no longer update an object using an object spread with
Partial<T>.

Seee bug:
  microsoft/TypeScript#16509

Work around it for now.
sandersn added a commit that referenced this issue Jun 14, 2017
Fixes #16509 by making the change from #15938 less strict. This is
technically a hole, but it's not as big a hole as before #15938.
@sandersn sandersn removed this from Done in Rolling Work Tracking Jun 14, 2017
@evmar
Copy link
Contributor Author

evmar commented Jun 14, 2017

Thanks for the quick fix! I'll undo my workaround.

evmar added a commit to angular/tsickle that referenced this issue Jun 27, 2017
You can no longer update an object using an object spread with
Partial<T>.

Seee bug:
  microsoft/TypeScript#16509

Work around it for now.
mbebenita added a commit to mbebenita/pathfinder that referenced this issue Oct 25, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

4 participants