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.assign not type checking #11100

Closed
OliverJAsh opened this issue Sep 23, 2016 · 27 comments
Closed

Object.assign not type checking #11100

OliverJAsh opened this issue Sep 23, 2016 · 27 comments
Assignees
Labels
Duplicate An existing issue was already created Suggestion An idea for TypeScript
Milestone

Comments

@OliverJAsh
Copy link
Contributor

TS 2.0.3

Given:

(() => {
    type State = { foo: string };
    const state: State = { foo: 'bar' };
    const newState: State = Object.assign({}, state, { foo: 1 }) // I expect this to error
})();

I swear this used to work!

@Arnavion
Copy link
Contributor

As I explained on IRC, the result of assign is { foo: string & number }, so TS considers it okay to be assigned to { foo: string }.

@sandersn sandersn self-assigned this Sep 23, 2016
@sandersn
Copy link
Member

I am close to having a PR for spread types, so when those are in, Object.assign will change to have the correct type:

assign<T, U>(target: T, source: U): { ...T, ...U }

Right now I am working in the branch object-rest-spread-WIP if you want to follow along.

@sandersn
Copy link
Member

(also you'll just be able to use the syntax { ...state, foo: 1 } instead of calling Object.assign)

@Arnavion
Copy link
Contributor

@sandersn So with your change, the result of assign would be { foo: number } ?

@sandersn
Copy link
Member

Yes, which is not assignable to State, so it would error as desired.

@mhegazy mhegazy added the Suggestion An idea for TypeScript label Sep 23, 2016
@mhegazy mhegazy added this to the TypeScript 2.1 milestone Sep 23, 2016
@OliverJAsh
Copy link
Contributor Author

OliverJAsh commented Sep 24, 2016

Thanks, that's great @sandersn. I look forward to it!

@Arnavion How come this works?

const someState: { foo: string } & { foo: number } = { foo: 1 }

I think you explained why in IRC, but I'm not sure I completely understand. Are you saying in the above example we're trying to assign number to string & number which is an error, but if we try to assign string & number to number then it is allowed?

@Arnavion
Copy link
Contributor

DerivedClass is assignable to BaseClass because all DerivedClass are also BaseClass. Similarly string & number is assignable to number because all string & number are also number.

BaseClass is not assignable to DerivedClass because an arbitrary BaseClass is not necessarily also a DerivedClass. Similarly number is not assignable to string & number because an arbitrary number is not necessarily also a string.

@OliverJAsh
Copy link
Contributor Author

That makes sense, thank you @Arnavion:

    // number is not assignable to string & number because an arbitrary number is not necessarily also a string.
    const x = 'foo' as string & number
    const x1: number = x
    // string & number is assignable to number because all string & number are also number.
    const x2: string & number = 'foo'

@danielearwicker
Copy link

@sandersn, this looks very good! Shouldn't the new type of Object.assign be:

assign<T, U>(target: T, source: U): T & { ...U }

i.e. all type features of target are preserved, with only properties from U tacked on. This covers the very common use case of adding properties to functions.

@sandersn
Copy link
Member

@danielearwicker that's not quite right either because if the function already has a property that source overwrites then intersection doesn't give the right answer either. If we take as given that our only operators are spread and intersection, then I'd go with the stricter and more accurate one. (Of course we could introduce two kinds of spread but I don't like that idea.)

Can you point me to some source that spreads properties into functions? I'm always interested in collecting uses of the spread type — previously I thought that real-world uses were always with property bags.

@danielearwicker
Copy link

Yes, I see. I haven't been using object spread operator for this purpose (I only use TS) but I have been using Object.assign and similar functions to do the same thing.

Hiding in plain sight, jQuery's $ is probably the famous example of a function with properties. It's very handy to be able to encapsulate some capabilities in an object that is also directly callable, giving succinct access to one preferred way of using it.

I've been working on a strongly-typed composable Redux library where I make augmented functions this way. Although as you can see, I've simply defined my own assign function so I can control the type signature anyway, so I can keep using intersection and you can ignore me! :)

@mhegazy mhegazy modified the milestones: TypeScript 2.1, TypeScript 2.1.2 Oct 27, 2016
@mhegazy mhegazy modified the milestones: Future, TypeScript 2.1.2 Nov 8, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Nov 8, 2016

We are looking for a new type construct (mapped types) to address this instead of the spread and rest operators in the type space. we should have updates on this in the next few days.

@falsandtru
Copy link
Contributor

falsandtru commented Nov 8, 2016

I made #12110. This patch makes possible to declare and constrain the parameters 2nd and above.

-    assign<T, U>(target: T, source: U): T & U;
+    assign<T, U>(target: T, source: U, ...sources: U[]): T & U;
(() => {
  type State = { foo: string };
  const state: State = { foo: 'bar' };
  const newState: State = Object.assign<{}, State>({}, state, { foo: 1 }) // error
})();

@OliverJAsh
Copy link
Contributor Author

Any updates on this now 2.1 is shipped?

@mhegazy
Copy link
Contributor

mhegazy commented Dec 10, 2016

#10727 tracks adding a spread/rest type operator support. Implementing #10727 is a prerequisite for this one.

@falsandtru
Copy link
Contributor

falsandtru commented Jan 21, 2017

I made the patch #13611 used Partial types.
@sandersn I think we need to use both partial types and spread types because spread/rest types don't support rest parameters as you changed signatures in #13288.

@falsandtru
Copy link
Contributor

falsandtru commented Apr 3, 2017

Now I'm using the following signatures.

export function assign<T extends U, U extends object>(target: T, ...sources: Partial<U>[]): T;
export function assign<T extends U, U extends object>(target: object, source: T, ...sources: Partial<U>[]): T;
export function assign<T extends object>(target: T, ...sources: Partial<T>[]): T;
export function assign<T extends object>(target: object, source: T, ...sources: Partial<T>[]): T;

@dgreene1
Copy link

Subscribing for updates.

@metodribic
Copy link

What is the current status on this?

@sandersn
Copy link
Member

@metodribic waiting on a spread type, which is waiting on a difference type to make sure JSX Higher-order-component scenarios work. Now that some kind of difference types are expressible using conditional types, the scenario may work. The conversation is happening at #10727.

@airtoxin
Copy link

This type definition of Object.assign can fixed in now using utility-types's assign definition https://github.com/piotrwitek/utility-types#assign (maybe steal implementations), but we should wait for type spreading features?

@sandersn
Copy link
Member

@airtoxin would utility-type's Assign get this example right?

type O = {
  x: number
  y?: number | undefined
  m: () => number
}
declare class C {
  x: string
  y: string
  m2: () => string
}
// now Assign<C, O> should be
type Result = {
  x: number
  y: number | string
  m: () => number
}

@sandersn
Copy link
Member

Currently it produces

{
  x: number
  y?: number | undefined
  m: () => number
  m2: () => string
}

Meaning that it isn't correctly combining optional properties from the right, and isn't correctly discarding class-originating methods, which are on the prototype and will be discarded by Object.assign at runtime.

@zuzusik
Copy link

zuzusik commented Mar 30, 2018

@sandersn would your work also fix this example?

@sandersn
Copy link
Member

(Reprinting example for ease of reading)

const a: { b: number } = { b: 123 };
Object.assign(a, { c: 456 });
a.c = 456;

The problem with that example is that we don't model Object.assign's mutation of its first argument. That's not possible in Typescript today, since we'll just use the declared type of a all the way through the program. The control flow graph could allow us to model mutations to a type, but right now we only narrow types by removing constituents of a union type. And of course a doesn't have a union type.

@iflp
Copy link

iflp commented Jul 23, 2018

A workaround could be:

(() => {
    type State = { foo: string };
    const state: State = { foo: 'bar' };
    const assignedValues: State = { foo: 1 }; //complain expected
    const newState: State = Object.assign({}, state, assignedValues) 
})();

The trade-off being an extra line of code for type safety.

@RyanCavanaugh
Copy link
Member

Tracking at #10727

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 Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests