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

ObjectConstructor.values returns any[] in at least some cases when used in a template function #55116

Closed
craigphicks opened this issue Jul 23, 2023 · 9 comments
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@craigphicks
Copy link

craigphicks commented Jul 23, 2023

Bug Report

πŸ”Ž Search Terms

ObjectConstructor

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about ObjectConstructor

⏯ Playground Link

Playground

πŸ’» Code

type A = Record<string, number>;
type B = Record<string, boolean>;

declare interface ObjectConstructorAlt {
    values<T>(o: T):
    T extends any ?
        T extends Record<string|number, infer V> ? V[] 
            : T extends (infer V)[] ? V[]
            : any
        : never       
    ;
}
declare const ObjectAlt: ObjectConstructorAlt;

declare const a:A;
declare const b:B;
declare const ab:A|B;

Object.values(a); // number[]
Object.values(b); // boolean[]
Object.values(ab); // any[]
ObjectAlt.values(a); // number[]
ObjectAlt.values(b); // boolean[]
ObjectAlt.values(ab); // number[] | boolean[]

πŸ™ Actual behavior

For the union A|B case Object.values returns any[]

πŸ™‚ Expected behavior

For the union A|B case Object.values returns number[]|boolean[].

code actual return type expected return type
Object.values(null as any as A) number[] number[]
Object.values(null as any as B) boolean[] boolean[]
Object.values(null as any as (A|B)) any[] number[]|boolean[]
@MartinJohns
Copy link
Contributor

FYI, Object.values() returning any[] is the safe way and it should be that way, just unfortunately it was implemented wrong. See #38520. Objects are not sealed, there may be more properties present than the type declares.

@craigphicks
Copy link
Author

craigphicks commented Jul 23, 2023

@MartinJohns - Good reference, thanks. As you indirectly state, Object.values() doesn't always return any[] but sometimes it does.
Perhaps this case of using Object.values in a template functions (as shown above) is close enough to the case

declare const a: Record<string,number>;
Object.values(a); // number[]

that it should return the specific non-any value of the record?

@MartinJohns
Copy link
Contributor

MartinJohns commented Jul 23, 2023

I'm just saying that the safe approach is for Object.values() to always return any[]. Objects are not sealed.

Take this example:

const orig = { str: 'string', num: 123 };

// Works, because the types are compatible. All types required by "reduced" are present and compatible.
const reduced: { str: string } = orig;

// Works, because the information about the property "num" is lost when the object was assigned to "reduced".
const record: Record<string, string> = reduced;

// According to the type "Record<string, string>" all values should be types string, right?
// Due to the wrong / unfortunate implementation it actually is typed string[]!
const values = Object.values(record);

// But there's actually a number present!
console.log(values);

Playground link

@jcalz
Copy link
Contributor

jcalz commented Jul 23, 2023

This isn't a bug in TypeScript, is it? TS intentionally refuses to synthesize union types for inference candidates, because otherwise just about everything would succeed where people might expect a failure (canonical example is <T>(x: T, y: T) => void). The type number | boolean doesn't directly appear in the input, so it's not synthesized. I'm not seeing how it's a bug, as it should apply to many, many more things than just Object.values()... and even then it's working as intended. Maybe #44312 or the like is relevant?

If I interpret this as a feature request for Object.values() to accept unions-of-records or unions-of-arrays, I'm still skeptical that it's desirable. Right now someone who wants that type could call Object.values<number | boolean>() themselves, or widen the input to {readonly [k: string]: number | boolean}, instead of complicating the Object.values call signature for everyone everywhere. And anyone who wants Object.values to behave this way could merge that signature in to their own code base, again without affecting everyone everywhere.

@craigphicks
Copy link
Author

I'm just saying that the safe approach is for Object.values() to always return any[]. Objects are not sealed.
Take this example:

const orig = { str: 'string', num: 123 };
const reduced: { str: string } = orig;
const record: Record<string, string> = reduced;
const values = Object.values(record);

I do see your point. Although I think the "blame" could equally be assigned to allowing const reduced: { str: string } = orig without an error, or at least a warning.

There are also disadvantages of values having type any[] -

  • any may user-unintentionally propagate, effectively suppressing off type-checking, leading to further errors.
  • manually specifying the expected type with as can lead to user error.

So while I certainly see your point, I think it is a matter of tradeoffs rather than an absolute.

@MartinJohns
Copy link
Contributor

I do see your point. Although I think the "blame" could equally be assigned to allowing const reduced: { str: string } = orig without an error, or at least a warning.

That's how TypeScript operates fundamentally. It's a structurally typed language. To error or warn here would be just wrong.

But otherwise, yeah, nowadays unknown[] would be the best choice.

@craigphicks
Copy link
Author

craigphicks commented Jul 23, 2023

@jcalz - You are correct - I am not talking about changing the behavior of Typescript inference. I am only talking about changing the type definition of ObjectConstructor, which is far more narrow topic. (Does that make sense?).

Also - I must apologize - my statement of the problem was not clear, had errors, and I rewrote it. Does it make any difference to what you are saying?

If you are saying there are other cases of .d.ts type definitions for the js core functions and they all need to share the same policy wrt to union types, that sounds reasonable, but your <T>(x: T, y: T) => void) is not such an example, or is it?

@RyanCavanaugh
Copy link
Member

I don't really want to poke this hornet's nest. Object.keys is a constant source of complaint when we do it correctly, and Object.entries is a constant source of complaint when we do it incorrectly. Complicating Object.values to make it ... differently wrong? ... doesn't seem like it's worth causing a potential break somewhere else.

@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Jul 24, 2023
@craigphicks
Copy link
Author

Closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

4 participants