1052491 - Add support for symbol-keyed properties to jsinfer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Currently we treat symbol-keyed properties the same as indexed properties: they are all glommed into a single category. We'll infer more specific types if we support symbols, and I don't think it even requires any new code -- just let symbol jsids through.

Comment 1

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=d2ff062a3e75

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: bug-1052491-part-1-simplify-v1.patch

I think this must have been written before there was an invariant that string jsids are never indexes.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: bug-1052491-part-2-symbols-v1.patch

Well -- I think it's really this simple. Passes all the jit-tests and js/tests locally.

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 8486485 [details] [diff] [review]
bug-1052491-part-1-simplify-v1.patch

Review of attachment 8486485 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, I've wanted to remove this for a while.  This is some of the oldest TI code still around, and does, yeah, date to before we canonicalized jsids.

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 8486486 [details] [diff] [review]
bug-1052491-part-2-symbols-v1.patch

Review of attachment 8486486 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinferinlines.h
@@ +205,5 @@
>      JS_ASSERT(!JSID_IS_EMPTY(id));
>  
>      // All integers must map to the aggregate property for index types, including
>      // negative integers.
> +    return JSID_IS_STRING(id) || JSID_IS_SYMBOL(id) ? id : JSID_VOID;

Parens around JSID_IS_STRING(id) || JSID_IS_SYMBOL(id), to make this easier for humans to parse.  Or, change this to:

return JSID_IS_INT(id) ? JSID_VOID : id;

Also, missed this in the earlier patch, but the comment is wrong about negative integers.  The existing comment is kind of vague, so you can change it to:

// All properties which can be stored in an object's dense elements must map to the aggregate property for index types.

Comment 6

[Jason Orendorff [:jorendorff]]

Cool. (This makes it even more ridiculous to have two patches -- hg rebase deleted my second changeset. So I'll just be pushing one changeset.)

Comment 7

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd2cd2e8c258

Comment 8

[Jason Orendorff [:jorendorff]]

Of course hg rebase was wrong to delete my second changeset, and the tree burned bright orange. Pushed the fix.

Comment 9

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eec8eb2b4dd6

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd2cd2e8c258
https://hg.mozilla.org/mozilla-central/rev/eec8eb2b4dd6