Implement ES6 7.2.2 IsArray

RESOLVED FIXED in mozilla38

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

(Blocks: 1 bug)

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
This is special case that allows going through a Proxy (ScriptedDirectProxy in our case) to check for arrays. Used in some places like Array.isArray, JSON.stringify etc.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1096753
Keywords: dev-doc-needed
(Assignee)

Comment 2

3 years ago
Created attachment 8539530 [details] [diff] [review]
is-array

Missing tests. The changes to JSON.stringify aren't really useful for proxies until bug 895223 is fixed.
(Assignee)

Updated

3 years ago
Attachment #8539530 - Attachment is obsolete: true

Comment 3

3 years ago
Comment on attachment 8539530 [details] [diff] [review]
is-array

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

::: js/src/jsobj.cpp
@@ +3650,5 @@
> +    if (obj->is<ProxyObject>()) {
> +        if (obj->as<ProxyObject>().handler()->isES6Proxy()) {
> +            RootedObject target(cx, obj->as<ProxyObject>().target());
> +            if (!target) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_PROXY_REVOKED);

Rev 30 has changed this to return false instead of throwing an exception.
(Assignee)

Comment 4

3 years ago
Awesome \o/. Thank you ziyunfei! This means we can actually simplify this function again, because we don't really need the context parameter and can just return true/false.

There is however still one problem I am aware of. Something like this is currently not working (actually after fixing bug 1115361): Array.isArray(new (newGlobal().Proxy([], {})). We could fix this by introducing a new ESClass_Array value that follows the semantics of IsArray.
(Assignee)

Comment 5

2 years ago
I fixed this, but we need to land some tests.
(Assignee)

Comment 6

2 years ago
Created attachment 8556106 [details] [diff] [review]
Some tests
Attachment #8556106 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 7

2 years ago
We will add some test for JSON.stringify in bug 1111604.
Comment on attachment 8556106 [details] [diff] [review]
Some tests

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

::: js/src/tests/ecma_6/Array/isArray.js
@@ +18,5 @@
> +}
> +
> +revocable.revoke();
> +assertEq(Array.isArray(revocable.proxy), false);
> +assertEq(Array.isArray(proxy), false);

Bah.  I am not really a fan of these semantics.  It seems to me that touching a revoked proxy or examining anything about it, at all, should always throw.  Just as Object.isExtensible(revocable.proxy) throws, rather than returning true or false.  What do you think about getting the spec changed on the question?

Fine to land what's here for now, tho, if/until we get that changed.

@@ +23,5 @@
> +
> +var global = newGlobal();
> +var array = global.Array();
> +assertEq(Array.isArray(array), true);
> +assertEq(Array.isArray(new Proxy(array, {})), true);

assertEq(Array.isArray(new global.Proxy(array, {})), true);
Attachment #8556106 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 9

2 years ago
Revoked proxies actually used to throw. Waldo said he would open a bug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3538586606e4
https://hg.mozilla.org/mozilla-central/rev/3538586606e4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Keywords: dev-doc-needed
Filed https://bugs.ecmascript.org/show_bug.cgi?id=3840 for the spec issue, which applies to various other predicates in the spec as well.
You need to log in before you can comment on or make changes to this bug.