Fix accessing number literal properties on objects #2293
Conversation
ghost
added
the
|
Switched to |
| @@ -3644,6 +3644,15 @@ let rec __flow cx ((l: Type.t), (u: Type.use_t)) trace = | |||
| in | |||
| rec_flow cx trace (o, u) | |||
|
|
|||
| | (NumT (reason_x, Literal (n, _)), ElemT(reason_op, (ObjT _ as o), t, rw)) -> | |||
| let x = string_of_float_trunc n in | |||
There was a problem hiding this comment.
Hi Vladimir - it looks like the runtime behavior of e.g. obj[n] is to call toString on n and use the result as a property name, so string_of_float_trunc doesn't seem right here - instead we would need some js_string_of_float which replicated the specified behavior of JS's (number).toString.
(Aside: with that in hand, it should be pretty straightforward to support numeric property names in object literals as well!)
Edit: sorry, just reread your original comment, so you know what's up at runtime. We've gone around about the relative merits of using raw, and basically decided that we need to wait until we have a credible replica of toString before going forward on this.
There was a problem hiding this comment.
Related comment: #380 (comment)
wait until we have a credible replica of toString before going forward on this.
Spec wording vaguely indicates that final result could be implementation-specific, so I'm not sure that it's worth it.
string_of_float_trunc seems to work good enough. Another option is to only allow integer literals as property keys.
There was a problem hiding this comment.
The problem with string_of_float_trunc is that it would fold distinct properties together, no?
> let obj = {}
< Object {}
> obj[1] = "hey"
< "hey"
> obj[1.1] = 999
< 999
> obj[1]
< "hey"
There was a problem hiding this comment.
No, it should work for this case. It just truncates trailing . to format integers correctly
|
@bhosmer ping |
|
I'll take this over from Basil. Thanks for being so patient. |
|
@gabelevi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
This looks good to me. I'll import, rebase it past some internal refactors, get it reviewed, and merge it. Sorry for the delay! |
|
This has turned out to be a little more complicated than anticipated. Since we support types like It's a problem if we automatically convert |
|
@gabelevi haven't thought about this. I'll try to fix it myself, if you don't mind |
|
Normally I would be hugely supportive but I'm pretty much done with the enabling refactor. Will coordinate with you regarding follow up steps once the diff lands! |
|
@samwgoldman got it! |
Summary: Before this diff, computed property access for objects was handled by ElemT exclusively. This diff generalizes GetProp, SetProp, LookupT, and related use types to handle both named properties and computed element access. This refactoring unlocks a few things: * We can properly implement MethodT for `o[p]()`, where `p` is a literal and `o[p]` resolves to a function that references `this`. * We can convert number literals to strings, if a named property matches anywhere along the prototype chain. * Many uses of symbols will require that we traverse the prototype chain. Note that none of the above are included in this diff, but rather slated for follow-up work. Note the invariant that a computed prop ref's type must be concrete (not open) and will never be a string literal. This invariant holds because we wait for the indexer element ot resolve with ElemT before generating the lookup constraints, and string literals are converted in the named property lookups. This diff also makes a small change to LookupT. Before, the reason for that use type was the reason for the property. After this change, that reason is associated with the "prop ref" instead, and the lookup's own reason should be the "op reason" -- the reason why we are doing a lookup in the first place. Ref #2293 Reviewed By: gabelevi Differential Revision: D4149126 fbshipit-source-id: 54fe4f621ebc8e95ac801e1580c158b00555bbca
|
OK, so the groundwork has landed in this revision: 1fd2a72 We now look up the prototype chain for all computed properties: Line 4007 in b857236 To make this work, the trick will be to find all the places that a Let me know if you have any questions, or if there is something else missing. |
|
Hopping in real quick to see if you're still pursuing this @vkurchatkin 😅it's been a while |
|
It looks like this has been abandoned. Feel free to reopen if you pick this back up again. |
This uses
rawvalue as string representation of number literal. This is an oversimplification, but better than nothing.The correct way is to implement this part of the spec: http://www.ecma-international.org/ecma-262/6.0/index.html#sec-tostring-applied-to-the-number-type
Fixes: #1512