1124934 - Change HasProperty to follow ES6 specification

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S [:evilpie]]

Attachment: v1 - Add a HasProperty ObjectOp

Comment 2

[Tom S [:evilpie]]

Attachment: v1 - Implement ES6 HasProperty

This doesn't really follow how we implement Get and Set, but I think for that simple function it's probably fine.

I still need to look if this causes performance regressions.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8553908 [details] [diff] [review]
v1 - Add a HasProperty ObjectOp

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

Looks good, consider rebasing on top of bug 1127121? Or not, your call.

Comment 4

[Tom S [:evilpie]]

Attachment: v2 - Add a HasProperty ObjectOp

Comment 5

[Tom S [:evilpie]]

Attachment: v2 - Implement ES6 HasProperty

This still seems to have slightly worse performance, especially when the property is not found at depth 0.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8558692 [details] [diff] [review]
v2 - Add a HasProperty ObjectOp

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

::: js/src/builtin/TypedObject.cpp
@@ +1778,5 @@
> +      case type::Simd:
> +        return false;
> +
> +      case type::Array:
> +        return js_IdIsIndex(id, &index) || JSID_IS_ATOM(id, cx->names().length);

This is a change in behavior in the case that id is an index that's out of range for the array, right? Which behavior is correct? Figure it out and add a test...

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8558707 [details] [diff] [review]
v2 - Implement ES6 HasProperty

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

::: js/src/vm/NativeObject.cpp
@@ +1564,5 @@
>  
>      return NativeDefineProperty(cx, obj, id, value, getter, setter, attrs);
>  }
>  
> +/*** [[Has]] *************************************************************************************/

It's called [[HasProperty]] in the standard.  Lol.

@@ +1573,5 @@
> +    RootedNativeObject pobj(cx, obj);
> +    RootedShape shape(cx);
> +
> +    // This loop isn't explicit in the spec algorithm. See the comment on step
> +    // 7.a. below.

Note which draft rev and what section the step numbers refer to.

Comment 8

[Tom S [:evilpie]]

Oh thanks for mentioning this, I just noticed that [[HasProperty]] for typed arrays now also behaves incorrectly.

Comment 9

[Tom S [:evilpie]]

With the current spec 'in' on typed arrays is pointless, but there is already https://bugs.ecmascript.org/show_bug.cgi?id=3619 to correct this.

Comment 10

[Tom S [:evilpie]]

Attachment: v3 - Add a HasProperty ObjectOp

Changed TypedObjects to claim ownership of all array indexes, but return false when it's oob.

Comment 11

[Tom S [:evilpie]]

Attachment: v3 - Implement ES6 HasProperty

Handle oob index in TypedArray. Added tests.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 8558824 [details] [diff] [review]
v3 - Add a HasProperty ObjectOp

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

::: js/src/builtin/TypedObject.cpp
@@ +1782,5 @@
> +            *foundp = true;
> +            return true;
> +        }
> +        uint32_t index;
> +        // Claim ownership over all array indexes.

It would be better to say "Elements are not inherited from the prototype." Same thing in the other similar TypedObject internal methods.

There's so much duplication between this and the others... but I don't see what to do about it, except continue having a lookup method internally. Which we want to avoid I think.

@@ +1804,5 @@
> +        *foundp = false;
> +        return true;
> +    }
> +
> +    return HasProperty(cx, proto, id, foundp);

Every time I look at a recursive call like this, I wonder if it can be a tail call, and the answer is always no, because we are rooting proto here. I don't think anything can be done about it.

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 8558826 [details] [diff] [review]
v3 - Implement ES6 HasProperty

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

::: js/src/jit-test/tests/TypedObject/array-hasproperty.js
@@ +1,4 @@
> +if (!this.hasOwnProperty("TypedObject"))
> +  quit();
> +
> +var array = new (TypedObject.uint8.array(5));

Maybe there's already a test for this stuff in this directory, but we should test:
*   that elements are *not* inherited from the prototype chain
*   that other properties *are* inherited

@@ +5,5 @@
> +
> +for (var i = 0; i < array.length; i++)
> +    assertEq(i in array, true);
> +
> +for (var v of [20, 300, -10, Math.pow(2, 32), -Math.pow(2, 32)])

When writing a test like this, it's a good habit to include the exact edge cases, in this case -1 and 5.

Generally `Math.pow(2, 32) - 1` is a better test case than `Math.pow(2, 32)`.

::: js/src/tests/ecma_6/TypedArray/has-property-op.js
@@ +15,5 @@
> +
> +    for (var i = 0; i < obj.length; i++)
> +        assertEq(i in obj, true);
> +
> +    for (var v of [20, 300, -10, Math.pow(2, 32), -Math.pow(2, 32)]) {

Same comments as above.

::: js/src/vm/NativeObject.h
@@ +1389,5 @@
>  inline bool
> +js::HasProperty(JSContext *cx, HandleObject obj, HandleId id, bool *foundp)
> +{
> +    if (HasPropertyOp op = obj->getOps()->hasProperty)
> +        return op(cx, obj, id, foundp);

It seems a little precarious not to have a JS_CHECK_RECURSION in the non-native path here. The one in Proxy::has is probably sufficient, but here makes more sense, right? I dunno...

...I guess the only way to get JS_CHECK_RECURSION right, ultimately, is with static analysis.

Class js::Proxy is looking more and more redundant by the day. I like that trend.

Comment 15

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3babbfbe771
https://hg.mozilla.org/integration/mozilla-inbound/rev/053b9215d143

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d3babbfbe771
https://hg.mozilla.org/mozilla-central/rev/053b9215d143

Comment 17

[Florian Scholz (Open Web Docs)]

Not sure if there is anything to be documented here. Please re-open ddn with an explanation of this change, if there is anything for MDN docs here.