Closed Bug 1124934 Opened 5 years ago Closed 5 years ago

Change HasProperty to follow ES6 specification

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 4 obsolete files)

No description provided.
Attached patch v1 - Add a HasProperty ObjectOp (obsolete) — Splinter Review
Attachment #8553908 - Flags: review?(jorendorff)
Attached patch v1 - Implement ES6 HasProperty (obsolete) — Splinter Review
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 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.
Attachment #8553908 - Flags: review?(jorendorff) → review+
Attached patch v2 - Add a HasProperty ObjectOp (obsolete) — Splinter Review
Attachment #8553908 - Attachment is obsolete: true
Attachment #8558692 - Flags: review?(jorendorff)
Attached patch v2 - Implement ES6 HasProperty (obsolete) — Splinter Review
This still seems to have slightly worse performance, especially when the property is not found at depth 0.
Attachment #8553910 - Attachment is obsolete: true
Attachment #8558707 - Flags: review?(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...
Attachment #8558692 - Flags: review?(jorendorff) → review+
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.
Attachment #8558707 - Flags: review?(jorendorff) → review+
Oh thanks for mentioning this, I just noticed that [[HasProperty]] for typed arrays now also behaves incorrectly.
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.
Changed TypedObjects to claim ownership of all array indexes, but return false when it's oob.
Attachment #8558824 - Flags: review?(jorendorff)
Handle oob index in TypedArray. Added tests.
Attachment #8558692 - Attachment is obsolete: true
Attachment #8558707 - Attachment is obsolete: true
Attachment #8558826 - Flags: review?(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.
Attachment #8558824 - Flags: review?(jorendorff) → review+
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.
Attachment #8558826 - Flags: review?(jorendorff) → review+
Duplicate of this bug: 1084133
https://hg.mozilla.org/mozilla-central/rev/d3babbfbe771
https://hg.mozilla.org/mozilla-central/rev/053b9215d143
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.