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

please delete, thx #60524

Closed
Mitsunee opened this issue Nov 17, 2024 · 12 comments
Closed

please delete, thx #60524

Mitsunee opened this issue Nov 17, 2024 · 12 comments

Comments

@Mitsunee
Copy link

Mitsunee commented Nov 17, 2024

Dunno why I thought using github issues in public was a valid way to report incorrect behaviour in software.

@ritschwumm
Copy link

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

jcalz commented Nov 17, 2024

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

Mitsunee commented Nov 17, 2024

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

jcalz commented Nov 17, 2024

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

"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
Copy link
Contributor

jcalz commented Nov 17, 2024

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

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

@jcalz
Copy link
Contributor

jcalz commented Nov 17, 2024

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

@MartinJohns
Copy link
Contributor

Your write-up is still appreciated for future reference.

@Mitsunee Mitsunee changed the title Object.assign (or spread) can be used to sneak undefined into required property please delete, thx Nov 18, 2024
@MartinJohns

This comment has been minimized.

@Mitsunee
Copy link
Author

Mitsunee commented Nov 18, 2024

Not very nice.

@MartinJohns Please immediatly remove the above screenshot. My personal social media are not yours to share, this ticket is closed and I have already requested deletion.

@jcalz
Copy link
Contributor

jcalz commented Nov 18, 2024

😭

I don't know quite what's going on here, but I'm sorry for my part in it. I'm going to unsubscribe from this. If you need my attention please @-mention me. Good luck.

@microsoft microsoft locked as too heated and limited conversation to collaborators Nov 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants