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

Type of key in for key in object loops #1562

Closed
zolomatok opened this issue Nov 2, 2024 · 9 comments
Closed

Type of key in for key in object loops #1562

zolomatok opened this issue Nov 2, 2024 · 9 comments
Labels
typescript TypeScript compatibility or extensions

Comments

@zolomatok
Copy link
Contributor

While doing iterations on objects, the VSC extension gives an incorrect type for the value

let o =
    hello: 24

for value in Object.values o
    console.log value // value is "string"

for key, value in o
    console.log key, value // value is "any"
Screen.Recording.2024-11-02.at.16.06.01.mov

Extension version 0.3.25

@bbrk24
Copy link
Contributor

bbrk24 commented Nov 2, 2024

While the fact that value is any in the second loop is annoying, the first loop isn't wrong -- that iterates over keys, not values.

@STRd6
Copy link
Contributor

STRd6 commented Nov 2, 2024

We could compile to const value = o[key as keyof typeof o];

@zolomatok
Copy link
Contributor Author

While the fact that value is any in the second loop is annoying, the first loop isn't wrong -- that iterates over keys, not values.

Oh, right, yeah

@edemaine
Copy link
Collaborator

edemaine commented Nov 2, 2024

This is a famous issue in the TypeScript world; see this FAQ entry and microsoft/TypeScript#12314

The argument is that, just because o is of type {hello: number}, doesn't mean that o doesn't have other keys. So you don't know exactly what you'll get out of a for..in loop or Object.keys.

Civet recently gained the ability to type key in the loop:

for key: keyof typeof o, value in o
    console.log key, value

Unfortunately, it currently compiles to

for (const key1 in o) {
  const key: keyof typeof o = key1;
  const value = o[key1];
  console.log(key, value);
}

Notably, value gets set to o[key1] (where key1 has type string) instead of o[key] (where key has the desired keyof type). We can easily fix this though.

I don't think we want to change the behavior of for (key in o) loops, for TypeScript compatibility. But maybe we can make it easier, like a single keyword that implies : keyof typeof o? (I'm not sure what... for k keyof o? for key k in o?)

And maybe we should always cast when setting value in for key, value in object loops, as @STRd6 suggested above, because it's always a type error otherwise? But I'm not sure... the error is kind of intentional.

@edemaine edemaine changed the title LSP - incorrect type information in loops Type of key in for key in object loops Nov 2, 2024
@edemaine edemaine added the typescript TypeScript compatibility or extensions label Nov 2, 2024
@STRd6
Copy link
Contributor

STRd6 commented Nov 2, 2024

Hmm.. we probably shouldn't do that compilation. After seeing this TS issue I recall that's why we didn't in the first place. It would, in general, be unsound, even for TS's standards.

@zolomatok
Copy link
Contributor Author

zolomatok commented Nov 2, 2024

@edemaine
Thanks for the thorough reply!

This is a famous issue in the TypeScript world

Regarding keys, my first example for value in Object.values o was a mistake, I was logging the indices.

It's the value in the second example I was wishing to have the type I was expecting.

Notably, value gets set to o[key1] (where key1 has type string) instead of o[key] (where key has the desired keyof type). We can easily fix this though.

Hmmm. How does that result in value having type any 🤔


I just tried this and this does work

for const [key, value] of Object.entries(o)
    console.log key, value
hover

I wonder about the difference between for key, value in o and for const [key, value] of Object.entries(o).
I guess it would a stretch to compile for key, value in o into for (const [key, value] of Object.entries(o)) ?

@edemaine
Copy link
Collaborator

edemaine commented Nov 2, 2024

Honestly that's pretty weird that Object.entries "works". Here it is in action on the TypeScript playground. I'd argue that that's a bug in Object.entries (and Object.values); it's unsound. Ah, indeed, here is the bug: microsoft/TypeScript#38520
We'll see how that issue evolves, but I wouldn't want to build anything on top of a shaky foundation like that.

To fix this, I think what you really want are "exact" object types. TypeScript object types just specify keys that must exist, but any key might exist. Flow exact object types can specify the exact set of keys that exist. This would enable finer typing of keys and values.

We've discussed exact objects for pattern matching, but adding it to the type system is probably beyond the scope of Civet; it needs to be fixed at the TypeScript level. Unfortunately, that probably means it's a long ways off, if it will ever happen. Long-open issue here: microsoft/TypeScript#12936

One more thing I learned reading these issues is that inferred types do behave well with for..in and Object.keys. Here is a rather elaborate example:

let oActual = {
    hello: 24,
    world: 42,
}
const out = f(oActual)
     //^? const out: "hello" | "world" | "no keys"
function f<T extends object>(o: T extends Record<infer A, infer B> ? T : never) {
  for (const key in o) {
    return key
  }
  return "no keys"
}

This is pretty interesting. I'm not sure exactly in what settings it works. Maybe there's some magic that Civet could do to make this automatic... but it's definitely not easy.

Meanwhile, I think you'll need to explicitly type the key, once #1564 gets released.

@zolomatok
Copy link
Contributor Author

Ah, indeed, here is the bug:

Oh woow!

but I wouldn't want to build anything on top of a shaky foundation like that.

Yes, of course, makes sense

Thank you guys for looking into this, really appreciate it 🙏

zoodogood pushed a commit to zoodogood/Civet that referenced this issue Nov 4, 2024
@STRd6
Copy link
Contributor

STRd6 commented Nov 15, 2024

Closing this as it's more of a TS issue and #1564 provides a viable workaround.

@STRd6 STRd6 closed this as completed Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript TypeScript compatibility or extensions
Projects
None yet
Development

No branches or pull requests

4 participants