Bug Report + Proposal: Object.assign()

TypeScript Version: 3.4.0-dev.201xxxxx

Search Terms: Object.assign return type

Code

let test1: string = Object.assign({ a: 1 }, { a: "one" }).a
// $ExpectError
let test2: number = Object.assign({ a: 1 }, { a: "one" }).a // No error

// $ExpectError
Object.assign({ a: 1, b: 2 }, "hi").toLowerCase() // No error

// $ExpectError
Object.assign({ a: 1, b: 2 }, "hi").length // No error

Expected behavior:

Type 'string' is not assignable to type 'number'

Property 'toLowerCase' does not exist on type ...

Property 'length' does not exist on type ...

Actual behavior:

No errors raised.

Playground Link: Link

Related Issues:

Proposal

type IndexerReturnType<T extends ArrayLike<any>> = T extends ArrayLike<infer V> ? V : any;

type ArrayLikeIndexer<T> = T extends ArrayLike<any> ? { [k: number]: IndexerReturnType<T> } : never 

type PickDefined<T> = {
    [P in keyof T]: T[P] extends undefined ? never : T[P]
};

type Assign<A, B> = A extends object ? (
    B extends object ?
        Pick<A, Exclude<keyof A, keyof B>> & PickDefined<B> :
        A & ArrayLikeIndexer<B>
) : (A & B)

declare function assign<A, B>(a: A, b: B): Assign<A, B>

Note 1: PickDefined<T> is necessary with strictNullChecks enabled.
Note 2: In the definition of Assign<A, B> I would have expected the usage of keyof B to need to be replaced with keyof PickDefined<B>. It seems, that's presently not the case - so I've opted to omit it and avoid any potential slow-down. Nonetheless, I must admit that I'm hazy as to why this is permissible.

Obviously to keep things brief I've just covered the single source use-case rather than providing overloads with several source. The definition is nonetheless chainable i.e. Assign<A, Assign<B, C>> etc.

Playground Link: Link (For posterity please enable strictNullChecks)

Relevant comment (in #28553) saying why this is not already being done:

Spreading two overlapping types is pretty rare, so we decided not to create a type operator for it.

Relevant possible implementation (in #21316) of a Spread<L, R> which is similar to Assign<A, B>. I don't know if anyone has a demonstrably superior implementation of this thing given all the potential edge cases like unknown, optional, and non-own properties:

class A {
  c?: boolean;
  constructor(public a: string) {}
  sayA() {
    console.log(this.a);
  }
}
class B extends A {
  constructor(a: string, public b: string) {
    super(a);
  }
}
function hmm(a: A) {
  return assign({ b: 123, c: 456 }, a);
}
const result = hmm(new B("a", "b"));

// unknown properties
result.b; // number at compile time, but:
console.log(typeof result.b); // "string"

// optional properties
result.c; // boolean | undefined at compile time, but:
console.log(typeof result.c); // "number"

// non-own properties
result.sayA; // function at compile time, but:
console.log(typeof result.sayA); // "undefined"

Link to code

Joe Calzaretta (@jcalz)

Thanks, Joe Calzaretta (@jcalz), and great example!

Spreading two overlapping types is pretty rare, so we decided not to create a type operator for it.

It's bit of a shame as there's been a heap of improvements in soundness recently, really loving the direction!

Not that I think it's particularly compelling on its lonesome, but just for reference, I am seeing overlapping types being assigned. Specifically custom "sub-classes", default props of number[] being overwritten with string[], Date[] etc. Although, in the aforementioned case it's actually with a custom recursive assign in a JS library (chartist). I was improving the DefinitelyTyped definitions for chartist when I noticed Object.assign seemingly wasn't up to scratch.

Anyway, I do totally understand the decision though. I guess it'd only be feasible with first class support for "exact types" (#12936), and my head is hurting just thinking about how much that'd further complicate the types here.

I also noted that when chained my Assign isn't propagating the indexer; technically it needs to create a new indexer with a union of the return types in cases of:

let x: number
Object.assign({}, "abcde", [1, 2, 3])[x] // string | number

Hopefully this will become a bit more feasible in near future; I'm a sucker for soundness.

Anders Hejlsberg (@ahejlsberg)

For future reference, whilst without a doubt it's far superior to my own attempt, Anders Hejlsberg (@ahejlsberg)'s implementation unfortunately does have a couple of failures when run against my test suite. Failing test1 in particular seems non-ideal. However, just to be clear, Anders Hejlsberg (@ahejlsberg) does a much better job of dealing with prickly edge cases (i.e. those mentioned above).

EDIT: It's quite conceivable that I can't just naively apply the Spread type to my assign, and that's responsible for 2 failed tests.

You've got --strictNullChecks disabled there (well, the Playground does that by default), and therefore the inferred type of {a: "one"} is {a: string} which is also satisfied by {a: undefined}. And since that implementation of Spread<L, R> treats any possibly-undefined property as optional, you get that behavior. It is possible to detect optional properties in a way that distinguishes them from required-but-possibly-undefined properties:

type OptionalPropertyNames<T> = { [K in keyof T]-?:
  ({} extends { [P in K]: T[K] } ? K : never)
}[keyof T]

which should fix that particular issue, but I feel unclean even thinking about what happens when --strictNullChecks is disabled. Even with --strictNullChecks enabled it's still not really possible to consistently distinguish missing from undefined in TypeScript, which makes it hard to even say what the "right" thing to do is. If your test suite contains assign({a: 1}, {a: undefined}), you are probably heading down a dark path and need to turn back before it is too late.

Anders Hejlsberg (@ahejlsberg)

You've got --strictNullChecks disabled there

Ugh, spot on, thank you! 😊

... and that's my call to go to bed (don't look at my time zone 😉).

If your test suite contains assign({a: 1}, {a: undefined}), you are probably heading down a dark path and need to turn back before it is too late.

Looks like I'm too far gone. 😉

However, in all seriousness, Anders Hejlsberg (@ahejlsberg)'s implementation is fantastic and ticks all the boxes, my sincerest apologies for falsely calling out failing tests!

I'm not sure whether this issue should remain open to track the state of Object.assign, however if the TypeScript team wish to, I'd have little objections to this being closed.

There's an additional trade-off here because it's desirable for Object.assign(T, U) for two type parameters T and U to be assignable to both T and U (this comes up a lot in various code examples we've gotten reports of) - but in reality this isn't actually knowable unless you can prove that T and U will be instantiated with compatible-or-disjoint properties. Same for Object.assign(e, T); it is generally agreed that this should be assignable to typeof e in the absence of a concrete type for T. In contrast, the userland Spread<T, U> will not be assignable to T or U, nor would a hypothetical built-in type spread operator.

So anyway for concrete types, Spread is probably better, but for generics (or when at least one operand is generic) T & U is probably better, but there's no facility for overloading on generics.

it's desirable for Object.assign(T, U) for two type parameters T and U to be assignable to both T and U

That seems like a very peculiar thing to infer. I can understand why people may be expecting Object.assign(T, ...) to be assignable to T, but making assumptions about the source parameters seems particularly odd (they're almost always partials). Granted I don't at all doubt you when you say you've run into it.

EDIT: In the paragraph above "partials" was a very poor choice of word, as T is indeed assignable to Partial<T>. What I really meant was "part of", or rather a type that is explicitly an incomplete subset of another type.

e.g. A React component might spread/assign its props (U) to a child component's props (T & U). In this case child's component's props (T & U) aren't necessarily (or even likely) to be assignable to the parent component's props (U).

Given that TypeScript wants to facilitate writing correct & maintainable software, I would ideally like to see changes that cause developers to disperse with these sorts of false assumptions. However, I totally understand what's proposed is a rather large change; and I'm certainly not expecting anyone to rashly make decisions. Nonetheless, I'm a bit sad to see this labelled as 'Working as intended' given the design goals of the language.

Anyway, I'll just keep my fingers crossed there'll be a change of heart at some point 🤞

Maybe "Best Available Compromise" is the better label

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Navigation Menu

Repository navigation

Bug Report + Proposal: Object.assign() #31982

Proposal

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Bug Report + Proposal: Object.assign() #31982

Description

Proposal

Activity

jcalz commented on Jun 19, 2019

Benjamin-Dobell commented on Jun 19, 2019

Benjamin-Dobell commented on Jun 19, 2019

jcalz commented on Jun 19, 2019

Benjamin-Dobell commented on Jun 19, 2019

RyanCavanaugh commented on Jun 26, 2019

Benjamin-Dobell commented on Jun 26, 2019

RyanCavanaugh commented on Jun 26, 2019

typescript-bot commented on Jun 28, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions