microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.12k stars 12.5k forks source link

Object.assign (or spread) can be used to sneak undefined into required property #60524

Open Mitsunee opened 5 hours ago

Mitsunee commented 5 hours ago

⚙ Compilation target

ES2022

⚙ Library

TypeScript 5.6.3

Missing / Incorrect Definition

I discovered that Object.assign (and also spread) allows for copying of undefined as a value:

interface Thing {
  template: string;
  color?: string;
}

type Arg = Partial<Thing>;

const base: Thing = { template: "foobar" }; // Thing
const arg: Arg = { color: "red", template: undefined }; // Partial<Thing>
const result = Object.assign({}, base, arg); // Thing & Partial<Thing>
result.template // string

console.assert(typeof result.template == "string"); // will trigger
console.log(`template is '${result.template}'`); // template is 'undefined'

Note: The above code block is available as a TypeScript Playground link below, where it can also be ran to reproduce the console.log message.

I have further done a little research such as running a similar test setup in Node 22 (current LTS as the time of writing) as well as finding an issue on Rhino that explicitly calls for it to recreate this behaviour (as it seems to be intended to work like this).

Welcome to Node.js v22.11.0.
Type ".help" for more information.
> const base = { template: "foobar" }
undefined
> const args = { color: "red", template: undefined }
undefined
> const res = { ...base, ...args }
undefined
> res
{ template: undefined, color: 'red' }
> 

The resulting type should actually contain undefined as an explicit possibility for the template property such as { template: string | undefined, color?: string }.

Links

ritschwumm commented 4 hours ago

setting exactOptionalPropertyTypes in the tsconfig.json seems to help:

Type '{ color: string; template: undefined; }' is not assignable to type 'Partial<Thing>' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.
  Types of property 'template' are incompatible.
    Type 'undefined' is not assignable to type 'string'.
jcalz commented 4 hours ago

Note: not a TS team member.

Duplicate of #11100, tracked at #10727.

Object.assign() produces an intersection which is known to have problems in the case of overwriting properties. TS needs a good "spread" operator, which it doesn't have. You could sort of write your own, but it would be a complicated mess of conditional types.

And even if that existed it would still fail to catch problems in general without exact types as per #12936, since nothing prevents you from sneaking random extra stuff in there:

const x = { template: 123 } 
const y: {} = x; // okay
const base: Thing = { template: "foobar" }; // Thing
const result = Object.assign({}, base, y); // Thing?!
result.template.toUpperCase(); // runtime explosion because it's really a number

Playground

So Object.assign()'s typing has deficiencies, but it's simple and works reasonably well in a lot of use cases. Making it more complex and possibly adding new language features to improve it... well I think that might not be considered worth the effort?

Mitsunee commented 4 hours ago

setting exactOptionalPropertyTypes in the tsconfig.json seems to help:

yep, that seems to help, sadly in my case the property will be set by the user of the library I'm trying to build (hence the name arg in the example), so I cannot make sure the user has this setting on, requiring additional type gymnastics on my end.

EDIT: I have now tested this further and enabling the setting does not change the behaviour of Object.assign at all, still exposing me to the possibility of this bug.


@jcalz the major issue I see with your example is the use of {} for y, which acts virtually identical to any

jcalz commented 4 hours ago

"acts virtually identical to any"? No, it... how about I sidestep and rewrite it as:

interface Thing {
  template: string;
  color?: string;
}

const x = { color: "abc", template: 123 }
const y: { color: string } = x;
const base: Thing = { template: "foobar" }; // Thing
const result = Object.assign({}, base, y); // Thing & {color: string}
result.template.toUpperCase(); // runtime explosion because it's really a number

Playground

Does that demonstrate the issue to you?

Mitsunee commented 3 hours ago

"acts virtually identical to any"? No, it... how about I sidestep and rewrite it as:

This does not disprove {} essentially acting like any. See this for more information: https://typescript-eslint.io/rules/no-empty-object-type

Does that demonstrate the issue to you?

This exits the realm of explicilty setting undefined on an optional property and goes more towards ways to lie to TypeScript's type analysis, which is outside the scope of this issue. If a user intentionally lies to TS like this, they are expected to know what they are doing, this goes for assingments, return types and as all the same.

jcalz commented 3 hours ago

any and {}

I was hoping to avoid this discussion by changing the example. I agree that {} disables excess property checking on object literals, but there's a huge gulf between that and any. Since this is off-topic here I'll stop, unless you think it's important to hammer out.

ways to lie

Widening is not "lying" to TypeScript's type analysis. The following is not lying, it's just widening:

interface Foo { x: string }
interface Baz extends Foo { y: string  }
function f(foo: Foo): Baz {  return Object.assign({}, { y: "abc" }, foo); }
interface Bar extends Foo {  y: number }
const bar: Bar = { x: "abc", y: 123 };
const baz = f(bar);
baz.y.toUpperCase() // error

Playground

If you want to fix that problem, you need exact types. If you think it's off-topic, let's forget about it entirely. Sorry for the digression.


Optional properties accept undefined on purpose, and the problem here is the intersection. The --exactOptionalPropertyTypes option doesn't stop the "sneaking in" issue with Object.assign(). I'm hoping you'll agree this example is the same problem, and one that could not be addressed with --exactOptionalPropertyTypes:

type Arg = { [K in keyof Thing]: Thing[K] | undefined };
/* type Arg = {
    template: string | undefined;
    color?: string | undefined;
} */

const base: Thing = { template: "foobar" }; // Thing
const arg: Arg = { color: "red", template: undefined }; // Arg
const result = Object.assign({}, base, arg); // Thing & Arg
result.template // string

Playground

Indeed we can change undefined to any other data type and have the same problem:

type Arg = { [K in keyof Thing]: Thing[K] | number };
/* type Arg = {
    template: string | number;
    color?: string | number;
} */

const base: Thing = { template: "foobar" }; // Thing
const arg: Arg = { color: "red", template: 123 }; // Arg
const result = Object.assign({}, base, arg); // Thing & Arg
result.template // string 

The problem here is the lack of a spread operator. Here's a possible spread implementation:

type MergeTwo<T extends object, U extends object> = { [
  K in keyof T | keyof U]:
  (x: K extends (keyof T & keyof U) ?
    {} extends Pick<U, K> ? { [P in keyof T as P & K]: T[P] | U[K] } : Pick<U, K> :
    K extends keyof T ? Pick<T, K> : K extends keyof U ? Pick<U, K> : never) => void
} extends Record<keyof T | keyof U, (x: infer I) => void> ? { [K in keyof I]: I[K] } : never

type Merge<T extends object[], A extends object = object> =
  T extends [infer F extends object, ...infer R extends object[]]
  ? Merge<R, object extends A ? F : MergeTwo<A, F>> : A;

interface ObjectConstructor {
  assign<T extends [object, ...object[]]>(...args: T): Merge<T>
}

which gives you string | undefined now in your example:

type Arg = Partial<Thing>;
const base: Thing = { template: "foobar" }; // Thing
const arg: Arg = { color: "red", template: undefined }; // Partial<Thing>
const result = Object.assign({}, base, arg);
/* const result: {
    template: string | undefined;
    color?: string | undefined;
} */
result.template // string | undefined

const y = Object.assign({}, base, { template: Math.random() < 0.99 ? 123 : "abc" })
/* const y: {
    template: string | number;
    color?: string | undefined;
} */

Playground

But that Merge type is awful (and possibly has terrible edge cases... ugh, index signatures are not handled well) and not something anyone wants to see in the standard TS library (I hope) so it would be nice if TS could provide a native operator, which is why this is a duplicate of #10727.

Mitsunee commented 3 hours ago

can you cease writing essays about unrelated stuff in my issue please?

jcalz commented 3 hours ago

It’s not unrelated. As for writing essays, that’s a fair point. I’ll disengage now entirely. Good luck.

MartinJohns commented 3 hours ago

Your write-up is still appreciated for future reference.