733820 - What is the semantics of setPropertyIsEnumerable for index properties in Arrays?

Status: RESOLVED FIXED

(Tamarin Graveyard :: Library, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Forked off of Bug 733384.

It is not clear to me whether we have a clear view of what:

  trace(a.propertyIsEnumerable(n));
  a.setPropertyIsEnumerable(n, false);
  trace(a.propertyIsEnumerable(n));

should do when 'a' is an array (potentially sparse, potentially dense) and 'n' is an index property.

Prior to the dense array changes, when arrays were a dense prefix followed by a sparse suffix, according to the code the policy appears to have been:

                    // {DontEnum} is not supported on the dense portion
                    // of an array.  Those properties are always enumerable.

The problem with this policy is that it depends on where the dense/sparse split is,
which is an implementation property of the VM, not of the language.

(An example of a saner hypothetical policy: "all index properties present in any Array object are enumerable, and 'setPropertiesIsEnumerable(n, false)' is a no-op for such properties 'n'.")

I started working on a test to explore the scenarios here.  I will post it here soon.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: test case A: tests hypothetical policy outlined in description

Tests hypothetical policy outlined in description, i.e. 

  all index properties present in any Array object are enumerable,
  and 'setPropertiesIsEnumerable(n, false)' is a no-op for such
  properties 'n'.

Note that to my knowledge, no version of TR/Flash adheres to this policy.  So this would be a break in backward compatibility (though I don't know whether we can sanely implement the original semantics atop our current implementation; this strikes me as similar to the length wrap-around cases that caused much consternation last summer, and which we decided to go ahead and allow some deviation from the past implementation-defined semantics).

Comment 2

[William Maddox]

(In reply to Felix S Klock II from comment #1)
> Created attachment 603776 [details]

>   all index properties present in any Array object are enumerable,
>   and 'setPropertiesIsEnumerable(n, false)' is a no-op for such
>   properties 'n'.

I think this is the only sane semantics.  Doing otherwise would require
that we either keep a separate account of enumerability for each element
of a dense array (e.g., a bitmap), or revert a dense array to a non-dense
representation if any index property is set non-enumerable.

Comment 3

[Edwin Smith]

Right - denseness is a pure optimization and so any sensitivity to it, in any api, is a bug.

It would be interesting to find out what happens in JS if you take a dense array and then change certian properties to be non-enumerable, if you even can (whats the api to do that? is it standard)?  Supporting any such ability to tweak the enumerability of numeric properties isn't worth the trouble.

But whatever we decide, lets write it down (as3 spec, asdocs, etc) so we deliberately test & advertize the intended behavior.

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v2: test hypothetical policy outlined in description

Refined the test, adding tests of 'length' property (another special case for the tests, though not the code), added more doc, and fixed a bug in the previous version of the test.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch C v1: implement policy outlined in description

I am now thinking we might try to get this into Cyril.  Thoughts?  QRB?

Comment 6

[Edwin Smith]

(In reply to Felix S Klock II from comment #5)
> Created attachment 604048 [details] [diff] [review]
> patch C v1: implement policy outlined in description
> 
> I am now thinking we might try to get this into Cyril.  Thoughts?  QRB?

Sooner we fix it the better IMO.

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: html code H v1: explore browser behavior for (dense?) arrays and (index?) properties

I am not certain that this is covering the right cases, but I am betting it does.

Chrome, Safari, and Firefox all appear to consistently *support* setting the {DontEnum} bit to false on any object (array or non-array) for any property (index or non-index).

(It could also be that there is a bug somewhere in my methodology.)

Dave Herman is the one who pointed me at Object.defineProperty as the way to add or *modify* the descriptor-fields associated with a property, such as the {DontEnum} bit.  See:

https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/defineProperty

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #7) 
> Chrome, Safari, and Firefox all appear to consistently *support* setting the
> {DontEnum} bit to false on any object (array or non-array) for any property
> (index or non-index).

Note that this was provided purely to answer Ed's question from comment 3.  I do not intend for it to be interpreted as a reason we should spend any time supporting this feature.  (I would prefer to adopt a sane consistent but javascript-incompatible semantics quickly than let our current insane semantics linger while we hash out what to do.)

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch J v1: modify new policy to match JS policy

(This patch is meant to be applied atop patch C.)

This implements the JS semantics as described in comment 7 and comment 8.  It tries to avoid shifting into the sparse representation unless absolutely necessary.

I do not know how much we care about the issue, if at all.  Arguably the proposed semantics from comment 0 is the closest one to our historic behavior, while the js-semantics is the most internally-consistent.

Comment 10

[Tamarin Bot]

changeset: 7292:453c0c397c84
user:      Felix Klock II <[e-mail]>
summary:   Bug 733820: implement policy where any index prop is always enumerable on any array (r=wmaddox).

http://hg.mozilla.org/tamarin-redux/rev/453c0c397c84

Comment 11

[Tamarin Bot]

changeset: 7292:453c0c397c84
user:      Felix Klock II <[e-mail]>
summary:   Bug 733820: implement policy where any index prop is always enumerable on any array (r=wmaddox).

http://hg.mozilla.org/tamarin-redux/rev/453c0c397c84

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

(We are going to go with patch C alone for now.  Patch J is certainly an option for the future, but without some justification for adopting the JS semantics, it is more risky than Patch C, since it introduces a new (admittedly rare) way for Arrays to shift from dense to sparse, which has a big negative performance impact.)