TypeScript icon indicating copy to clipboard operation
TypeScript copied to clipboard

Object.assign() inconsistent overload: assign(target: object, ...sources: any[]): any;

Open lord-xld3 opened this issue 1 year ago • 16 comments

🔎 Search Terms

ObjectContructor, Object.assign, label:bug, assign, overload

🕗 Version & Regression Information

TS 5.5.2, nightly,

issue started in this PR: https://github.com/microsoft/TypeScript/pull/15124

⏯ Playground Link

https://www.typescriptlang.org/play/?#code/JYOwLgpgTgZghgYwgAgPICMBWEFgMID2IAzmFAK64FTIDeAUMk8nMccAOYgA8AKshAAekEABNidAL4AaNAOEQxE2pIB8ACkbNtYOFA4QwALmS9ZW7UwB0N4gXJQkxE6gDaAXWQAfOq4DSyKDIANYQAJ4EMKbuJrz+7pIeFsgAlLHIAGTI6qjyIuLZAGLkILjARN7I6AQEADYQcCCVpFCgHJUg5AC26NApAPzIIBAAbtAuKQDc9JL09AD0AFSmABYQxCjEK-a1okMEYAJdwIeNyACiUFDUVshQhg4kQxAA7oEkuqUokdkAQjX1M4+ADKZDalQAct1elAUshFvN6PVTsgALxoLA4MBWVjsLjqMjkCDSFQkmSklQpJGGKpojHYXA4ticEDqADkxDZZO5FMkVORyAQdIwDOxuJZ6gAzDzybKqQtlrwVsAJFsdnsIMdTk1LtcoCYAILMrjguDIGAlMoVMAEFhNAiY3DIUQEdb7Q5dOChZBgNbIB2iwVwWq1ODoerwxECvbokVYpl41mk82WsDlVkpSnqKZzJbIAAK1wADtA026EGdSMAQ1UUOKuBA9jaWCnSmmKgGsZHqYcULHHWLjayLW309nk7Qi8WTGy4Gy+fQIFYpwQi7nlucRsHddQTAAlCAWjZN22jYPkOCQFstU0SABScC3wIQrSLh16FfIG2QbJKxDgMAQAAtGetRssg9Aqu6dosCGBAvI2yB2A4SD+lExAvsAb7vD6fowHUtTweChDgIohzAjgDgnGEBZ1MACA0aIwD3GUYxGDYVj0AiPbmsKA4JhKwxvMUo5ENmsiTtOP5zguMDLsW66rOsmzbOQuxHCcyAvHoIBtM4phadWtTQb0LBDoh3ECu0-aigJ+LJoSED8jSKx8bZ9ZJjI0lss5hzAG58YeeoyaSr5yCYAFjJBcm-x1A0IBhcEkWDomwVeaCrQgBwYXGTZgVDmlshQj0fQ8V0yV2Z5sidCGYVNHlUUFcmJSiIeoCNvKeYiVaTwvCcrkriWUBlhIFZNKZHmIc2Zojj15gIKuYTgr6KCDaWwBuj8K1IfYjgoLN7YgJxlk0raDUpRKB1jnoHAAIwmDeWWZgtJBxVYhEcOoN23bIHKOD5Xm0HAM7oPOnXLAAkiAgFQBIoALVcXZgGEJacQKRYmOoX36AATCYnQlbCaKqMgIwEMAohwlkWPfQ9YJPcTpPk5TmR0MkwNIfTHDTLM9DAqp6m9DxGPZDdeNDNCfSM2TFNU2z2gc493MzIwJ2HEWFVBVdYli-jkuwrQL12PU70EJ9Yu-WAHBgADsgEFSRY4tMRbqLdOYu7OPmTEAA

💻 Code

let bug: any = Object.assign(
    true, 
    {p1: 'x1'},
    {p2: 'x2'},
    {p3: 'x3'},
    {p4: 'x4'},
);

console.log('[bug]', bug, bug.p1, bug.p2, bug.p3, bug.p4)

🙁 Actual behavior

Argument of type 'boolean' is not assignable to parameter of type 'object'.ts(2345).

🙂 Expected behavior

If we ignore the TS error, the following will be output to the console:

"[bug]", Boolean {true}, "x1", "x2", "x3", "x4"

Additional information about the issue

Object.assign() does accept primitives as a target. Additionally, the current overloads are inconsistent with themselves:

// es2015.core.d.ts
interface ObjectConstructor {

    assign<T extends {}, U>(target: T, source: U): T & U;
    assign<T extends {}, U, V>(target: T, source1: U, source2: V): T & U & V;
    assign<T extends {}, U, V, W>(target: T, source1: U, source2: V, source3: W): T & U & V & W;
    
    // target is 'object' instead of '{}'?
    assign(target: object, ...sources: any[]): any;
}

Actual behavior of Object.assign():

Object.assign({}, true) // returns {}
Object.assign(true, {}) // returns Boolean {true} & {}

More examples in the playground.

lord-xld3 avatar Jun 26 '24 07:06 lord-xld3

Preferred overload should return : T & O; The fallback overload should return: any;

lord-xld3 avatar Jun 26 '24 07:06 lord-xld3

You are correct in the sense that TS is limiting the target to only object type. But what would the use case be for allowing also primitives? It seems TS added the limitation because it can lead to problems and confusion. That's my take on your suggestion. If you need to remove the limitation then either use overloading as you did or @ts-ignore.

yaKsirhC avatar Jun 26 '24 08:06 yaKsirhC

I agree that it makes no sense to utilize it with primitives as a target, but if that's the case we should change the other overloads like this one:

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

The issue only appears once we start matching the final overload:

assign(target: object, ...sources: any[]): any;

which means this would work fine:

let bug: any = Object.assign(
    true, 
    {p1: 'x1'},
    {p2: 'x2'},
    {p3: 'x3'},
);

I've created another playground here

If that solution works the way I think it does, then it seems we can have our cake and eat it too.

lord-xld3 avatar Jun 26 '24 08:06 lord-xld3

I don't quite understand. If I am wrong, correct me, but I don't see how that's different from the vanilla TS parameter types. If you try to put a boolean, for example, vanilla TS would hint there is an error. But I think it runs, not sure.

yaKsirhC avatar Jun 26 '24 09:06 yaKsirhC

Reason: Object.assign() does accept primitives as a target. Although awkard, the existing TypeScript implementation does not match what happens in JavaScript.

This is a very bad reason for any change. With that reason you can allow any wild shenanigans, because it's all valid in JavaScript. See also: What does "all legal JavaScript is legal TypeScript" mean?

MartinJohns avatar Jun 26 '24 09:06 MartinJohns

The current implementation is not consistent with itself. We're using <T extends {}> in one place and essentially <T extends object> in another. Safest thing to do is replace 'target: object' with 'target: {}'.

// Works fine, matches this overload: assign<T extends {}, U, V, W>(target: T, source1: U, source2: V, source3: W): T & U & V & W;
let a = Object.assign(
    true, 
    {p1: 'x1'},
    {p2: 'x2'},
    {p3: 'x3'},
);

console.log(a, a.p1) 


// TS error, matches this overload: assign(target: object, ...sources: any[]): any;
let b = Object.assign(
    true, // ts error
    {p1: 'x1'},
    {p2: 'x2'},
    {p3: 'x3'},
    {p4: 'x4'},
);

console.log(b, b.p1) 

lord-xld3 avatar Jun 26 '24 10:06 lord-xld3

I was originally going to make a suggestion, because I wrote an overload that was really helpful for me. Currently, assign() is not providing intellisense for the target object:

ex1

Now with the added overload:

ex2

Pretty neat. But I kept digging in. If I'm going to overload a built-in I better do it carefully. After poking around with what Object.assign() actually does, I found the inconsistent overload issue.

I've found that we only need one function signature for assign():

assign<T extends {}, O extends object>(
        target: T, 
        ...sources: (O extends Function ? never : O)[] | { [K in keyof T]: T[K] }[]
    ): T & O;

While the following overload will technically work in JavaScript, it really shouldn't be used since it doesn't produce the behavior you would expect. The only case where this overload is matched instead of the prior one is when trying to assign a primitive or function to the target object, and it will not be assigned correctly. You could say this matches the actual behavior of Object.assign() in JavaScript.

assign<T extends {}, O extends any[]>(
        target: T,
        ...sources: O | { [K in keyof T]: T[K] }[]
    ): T & O;

lord-xld3 avatar Jun 26 '24 11:06 lord-xld3

If you really want to go deeper, the following code should produce the same output on all consoles. Worked on Windows/Chrome/FireFox/Edge:

let a = Object.assign(
    true,
    {k: 'v'}
)
let b= new Boolean(true);
b.k = 'v';

console.log('a:',a,`a.valueOf(): ${a.valueOf()}`)
console.log('b:',b, `b.valueOf(): ${b.valueOf()}`)
console.log('(typeof a === typeof b)?:', typeof a === typeof b)


let x = Object.assign(
    {a: 'b'},
    true,
    {c: 'd'},
)
let y = {a: 'b', c: 'd'}

// Types 'x', 'y' infer differently but are the same.
console.log('x:',x,`x.valueOf(): ${x.valueOf()}`)
console.log('y:',y, `y.valueOf(): ${y.valueOf()}`)
console.log('(typeof x === typeof y)?:', typeof x === typeof y)
console.log('Notice x or y .valueOf() is different than a or b .valueOf().')

let z = Object.assign(
    {p: 'q'},
    ()=>{console.log('somefunc')}
);

console.log(`Assigning a function to this object:`, z);
console.log(`Expect TypeError: z is not a function:`);
z();

lord-xld3 avatar Jun 26 '24 13:06 lord-xld3

@yaKsirhC @MartinJohns thanks for the feedback. I've update the issue to reflect the problem more clearly.

lord-xld3 avatar Jun 26 '24 14:06 lord-xld3

Reason: Object.assign() does accept primitives as a target, and it has its use, although awkward.

There's a lot of text/screenshots here so maybe I missed it, but what is the use of Object.assign(true, ...) over Object.assign({}, ...) ?

RyanCavanaugh avatar Jun 26 '24 21:06 RyanCavanaugh

@RyanCavanaugh Object.assign(true, {}) returns new Boolean(true). Object.assign({}, true) returns {}

// This is valid.
const cursedNumber1 = new Number(2.5)
const cursedNumber2 = new Number(3.6)

const cursedNumberObject1 = Object.assign(
    cursedNumber1,
    {someProp: 'someValue'}
)

const cursedNumberObject2 = Object.assign(
    cursedNumber2,
    {someProp: 'someValue'}
)

// These objects are comparable as numbers. I wouldn't change this behavior.
(cursedNumber1 < cursedNumber2) // true
(cursedNumber1 > cursedNumber2) // false
(cursedNumber1 == cursedNumber2) // false

// You can also access their properties.
(cursedNumber1.someProp === cursedNumber2.someProp) // true

In short, interface ObjectConstructor has 4 overloads for assign().

assign<T extends {}, U>(target: T, source: U): T & U;
assign<T extends {}, U, V>(target: T, source1: U, source2: V): T & U & V;
assign<T extends {}, U, V, W>(target: T, source1: U, source2: V, source3: W): T & U & V & W;

// This one is causing problems.
assign(target: object, ...sources: any[]): any;

'object' is not the same as '{}', so that last overload behaves differently. Which means if we match that overload...

const NumberInterfaceWithProps1 = Object.assign(
    3.14, // Argument of type 'number' is not assignable to parameter of type 'object'.ts(2345)
    {p1: 'x1'},
    {p2: 'x2'},
    {p3: 'x3'},
    {p4: 'x4'},
)

We get an error when matching the last overload, and only the last overload.

The simplest fix is replacing

assign(target: object, ...sources: any[]): any;

with

assign(target: {}, ...sources: any[]): any;

lord-xld3 avatar Jun 26 '24 23:06 lord-xld3

Here's a test I wrote up regarding Object.assign

/* These should not emit an Error. returns new instance of (Boolean | String | Number) */
Object.assign(true,{},{},{},{})
Object.assign('s',{},{},{},{})
Object.assign(3,{},{},{},{})

/* This should emit an Error: Assigning a function to an object does not make the object callable */
Object.assign({}, function(){})();

/* Properties can still be assigned to a function object */
Object.assign(function(){}, {prop: 'a'}).prop

/* EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' 
is not an allowed source of script in the following Content Security Policy directive:...
*/
Object.assign(new Function, {prop: 'a'}).prop

/* These should emit warnings: T will not be assigned */
Object.assign({}, true)
Object.assign({}, 'a')
Object.assign({}, 3)
Object.assign({}, Boolean)
Object.assign({}, String)
Object.assign({}, Number)
Object.assign({}, null)
Object.assign({}, undefined)

/* Functions with properties can be assigned to a function, 
copying the properties of the source function.
*/
let x = Object.assign(function(){console.log('source')}, {a: 'b'})
let y = Object.assign(function(){console.log('target')}, x)
y.a;
y();

lord-xld3 avatar Jun 27 '24 15:06 lord-xld3

Object.assign(true, {}) returns new Boolean(true)

OK, but why would you ever want that? All the code you're showing me here looks like code I wouldn't want to have.

RyanCavanaugh avatar Jun 27 '24 17:06 RyanCavanaugh

Object.assign(true, {}) returns new Boolean(true)

OK, but why would you ever want that? All the code you're showing me here looks like code I wouldn't want to have.

We could easily change the signature to assign<T extends object> for all the overloads to prevent that behavior. That's fine, but we should at least make it consistent across all overloads.

lord-xld3 avatar Jun 27 '24 17:06 lord-xld3

I think the case is closed. It works fine as is, and no endeavor should be made to change anything. Typescript prevents the obvious mismatch, so no confusion and bug arises afterwards.

yaKsirhC avatar Jun 27 '24 18:06 yaKsirhC

This solution makes any return of Object.assign() a valid type. I'm abusing Object.assign() in my own project and it behaves correctly. Object.assign() is the only way to safely add properties to a function.

Why would you want this?

  • Any object returned by Object.assign() has the correct type.
  • The overloads more accurately describe the behavior of Object.assign().
  • It emits errors correctly for 5 different use cases.
  • It correctly infers types for 2 different use cases.
  • Source objects enumerate properties from the target object, useful for overwriting properties of the target.

See the changes on this commit

Catches new errors:

  1. Error: Cannot assign function to an object.
Object.assign({},()=>{})
  1. Error: Attempting to assign properties from an anonymous function.
Object.assign(target, ()=>{})
  1. Error: Primitives are not assigned.
Object.assign(target, 1, "a", true)
  1. Error: Cannot merge properties of multiple functions.
Object.assign(target, func1, func2)
  1. Error: Cannot mix objects and functions.
Object.assign(target, func, {}) | (target, {}, func)

Infers types correctly

  • Infer a primitive object is not assigned to function.
let f = Object.assign(()=>{}, Object.assign(true, { primitiveObject: "value"}))
  • Infer correct target function type when copying function properties.
let w = Object.assign((...args: any)=>{target(...args)}, funcWithProperty)

lord-xld3 avatar Jun 29 '24 11:06 lord-xld3

I'm not really seeing a strong argument for changing this. Again it seems like the cases you want to be not-errors are cases that are very indicative of a likely error.

RyanCavanaugh avatar Jul 09 '24 18:07 RyanCavanaugh

In case this helps someone, I took another stab at it and got single-source assigns working perfectly as far as I can tell.

assign<T extends {}, U extends {}>(target: T,
        source: U extends (boolean | BigInt | Symbol | number)? never: U | {[K in keyof T]: T[K]}
    ): T & (U extends (object | [])? {[K in keyof U]: U[K]}: U extends string? [U] : never);

Unfortunately, multiple sources becomes very complex due to 'string' and 'arrays' doing strange things when assigned to an object. Not sure if TypeScript is powerful enough to handle it, and I don't have a use for it yet, so we're not handling it very well:

assign<T extends {}, U extends {}, V extends {}>(target: T,
        source1: U extends (boolean | BigInt | Symbol | number)? never: U | {[K in keyof T]: T[K]},
        source2: V extends (boolean | BigInt | Symbol | number)? never: V | {[K in keyof T]: T[K]}
    ): T & (U extends (object | [])? {[K in keyof U]: U[K]}: never) & 
    (V extends (object | [])? {[K in keyof V]: V[K]}: never);

No regression here for strings or arrays. In order to actually solve this we'd need to merge overlapping indices with the "last" source(s) overwriting the "first" source(s). If you want a good challenge, try to solve for this:

const A = Object.assign({}, [1,2], ["a"])
const B = A[0] // type: number
const C = A[1] // type: string

It's certainly an improvement, but I removed the catch-all, so if you're hitting that catch-all overload you'll need to cast it manually because it was never type-safe to begin with.

I'm still very passionate about this issue and will follow it until the end. I have a strong use case for it here:

export const Component = <T extends keyof HTMLElementTagNameMap, U extends {}>(
    tag: T, 
    props: U extends (boolean | BigInt | Symbol | number) ? never : U | HTMLElementTagNameMap[T]
) => Object.assign<HTMLElementTagNameMap[T], U>(document.createElement(tag), props);

export const Structure = <T extends Element, U extends Element[]>(parent: T, ...children: U) => {
    parent.append(...children);
    return parent as T & { children: readonly [...U] };
};

Since these functions are using a single-source assign, the result is a fully type-safe DOM. I can't guarantee this completely unless we modify the signature for Object.assign:

const container: HTMLElement & {
    children: readonly [HTMLDivElement & {
        id: string;
        className: string;
    } & {
        children: readonly [HTMLCanvasElement, HTMLDivElement & {
            className: string;
        }...

I made a gist/repo that explains this much better. If there's any update on this I'll be watching. The discussion could go on forever but I think we need to try it as-is, or can it due to performance (a sad excuse but very real). It will break unsafe code and make it safer.

lord-xld3 avatar Dec 25 '24 08:12 lord-xld3