Typescript: Bug Report + Proposal: Object.assign()

Created on 19 Jun 2019 · 9Comments · Source: microsoft/TypeScript

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)

Working as Intended

Source

Benjamin-Dobell

Most helpful comment

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

jcalz on 19 Jun 2019

👍2

All 9 comments

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.

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

jcalz on 19 Jun 2019

👍2

Thanks, @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" (https://github.com/Microsoft/TypeScript/issues/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.

Benjamin-Dobell on 19 Jun 2019

👍1

For future reference, whilst without a doubt it's far superior to my own attempt, @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, @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.

Benjamin-Dobell on 19 Jun 2019

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.

jcalz on 19 Jun 2019

👍1

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 :wink:).

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, @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.

Benjamin-Dobell on 19 Jun 2019

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.

RyanCavanaugh on 26 Jun 2019

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 🤞

Benjamin-Dobell on 26 Jun 2019

Maybe "Best Available Compromise" is the better label

RyanCavanaugh on 26 Jun 2019

👍1

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