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() inconsistent overload: assign(target: object, ...sources: any[]): any; #59033

Open
lord-xld3 opened this issue Jun 26, 2024 · 16 comments

Comments

@lord-xld3
Copy link

lord-xld3 commented Jun 26, 2024

πŸ”Ž Search Terms

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

πŸ•— Version & Regression Information

TS 5.5.2,
nightly,

issue started in this PR: #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
Copy link
Author

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

@yaKsirhC
Copy link

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.

@lord-xld3
Copy link
Author

lord-xld3 commented Jun 26, 2024

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.

@yaKsirhC
Copy link

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.

@MartinJohns
Copy link
Contributor

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?

@lord-xld3
Copy link
Author

lord-xld3 commented Jun 26, 2024

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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

@RyanCavanaugh
Copy link
Member

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({}, ...) ?

@lord-xld3
Copy link
Author

@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
Copy link
Author

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();

@RyanCavanaugh
Copy link
Member

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.

@lord-xld3
Copy link
Author

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.

@yaKsirhC
Copy link

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.

lord-xld3 added a commit to lord-xld3/TypeScript that referenced this issue Jun 27, 2024
@lord-xld3
Copy link
Author

lord-xld3 commented Jun 29, 2024

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)

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