Keyed stores to super with numeric keys.

Keyed stores to super with numeric keys.

R=verwaest@chromium.org, arv@chromium.org, ishell@chromium.org
BUG=v8:3330
LOG=N

Committed: https://code.google.com/p/v8/source/detail?r=24696

[Dmitry Lomov (no reviews), 2014-10-10 11:29:41]

PTAL. This is UNFINISHED yet, some cases are missing (interceptors and
arguments), handling of primitive values as receivers is incorrect, and calling
accessors is pretty ugly. 

Let me know if there are some machinery or checks I have missed.

[Dmitry Lomov (no reviews), 2014-10-10 13:25:37]

Ok so here is a semi-final patch (missing support for primitive receivers).

I think this is the best we can do unless we do a huge refactoring for indexed
setter access (unify all of it using a variant of LookupIterator).

[Dmitry Lomov (no reviews), 2014-10-10 15:57:51]

Everything is implemented, including primitives as a receiver

[Dmitry Lomov (no reviews), 2014-10-13 09:20:44]

dslomov@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2014-10-13 12:16:36]

https://codereview.chromium.org/649603003/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/649603003/diff/160001/src/objects.cc#newcode856
src/objects.cc:856: if (js_object->HasIndexedInterceptor()) {
If the holder is not the receiver, query the attributes rather than using the
presence of the setter.

If there's no setter, at least named properties can have data properties behind
the interceptor.

https://codereview.chromium.org/649603003/diff/160001/src/objects.cc#newcode884
src/objects.cc:884: return WriteToReadOnlyElement(isolate, receiver, index,
value, strict_mode);
You only need to throw read-only exceptions for indices within range of string
receivers. In all other cases, the operation is swallowed as the receiver is
wrapped (ToObject).

https://codereview.chromium.org/649603003/diff/160001/src/objects.cc#newcode894
src/objects.cc:894: false);
Add a test to ensure that SetElement updates the length if the receiver is an
array.

[Dmitry Lomov (no reviews), 2014-10-13 13:48:59]

Comments addressed, PTAL

https://codereview.chromium.org/649603003/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/649603003/diff/160001/src/objects.cc#newcode856
src/objects.cc:856: if (js_object->HasIndexedInterceptor()) {
On 2014/10/13 12:16:36, Toon Verwaest wrote:
> If the holder is not the receiver, query the attributes rather than using the
> presence of the setter.
> 
> If there's no setter, at least named properties can have data properties
behind
> the interceptor.

Done.

https://codereview.chromium.org/649603003/diff/160001/src/objects.cc#newcode884
src/objects.cc:884: return WriteToReadOnlyElement(isolate, receiver, index,
value, strict_mode);
On 2014/10/13 12:16:36, Toon Verwaest wrote:
> You only need to throw read-only exceptions for indices within range of string
> receivers. In all other cases, the operation is swallowed as the receiver is
> wrapped (ToObject).

Done.

https://codereview.chromium.org/649603003/diff/160001/src/objects.cc#newcode894
src/objects.cc:894: false);
On 2014/10/13 12:16:36, Toon Verwaest wrote:
> Add a test to ensure that SetElement updates the length if the receiver is an
> array.

Done.

[Dmitry Lomov (no reviews), 2014-10-13 14:48:33]

> 
>
https://codereview.chromium.org/649603003/diff/160001/src/objects.cc#newcode884
> src/objects.cc:884: return WriteToReadOnlyElement(isolate, receiver, index,
> value, strict_mode);
> On 2014/10/13 12:16:36, Toon Verwaest wrote:
> > You only need to throw read-only exceptions for indices within range of
string
> > receivers. In all other cases, the operation is swallowed as the receiver is
> > wrapped (ToObject).
> 
As discussed off-line, this is not a behavior per spec.
[[Set]] for primitive receiver return 'false', therefore the behavior should
always be to throw read-only error.
Writing a patch to fix that.

[Dmitry Lomov (no reviews), 2014-10-13 14:51:48]

PTAL

[Toon Verwaest, 2014-10-14 08:31:54]

https://codereview.chromium.org/649603003/diff/260001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/649603003/diff/260001/src/objects.cc#newcode807
src/objects.cc:807: uint32_t index) {
Replace by JSObject::GetOwnElementAccessorPair

https://codereview.chromium.org/649603003/diff/260001/src/objects.cc#newcode860
src/objects.cc:860: if (from_interceptor.has_value) {
The contract is normally that if !has_value, an exception was thrown. Return
empty MaybeHandle<Object>() in that case. You probably also want done =
from_interceptor.value != ABSENT;

[Dmitry Lomov (no reviews), 2014-10-14 09:08:37]

https://codereview.chromium.org/649603003/diff/260001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/649603003/diff/260001/src/objects.cc#newcode807
src/objects.cc:807: uint32_t index) {
On 2014/10/14 08:31:54, Toon Verwaest wrote:
> Replace by JSObject::GetOwnElementAccessorPair

Is that enough? SetElementWithCallback handles more cases than just AccessorPair

https://codereview.chromium.org/649603003/diff/260001/src/objects.cc#newcode860
src/objects.cc:860: if (from_interceptor.has_value) {
On 2014/10/14 08:31:54, Toon Verwaest wrote:
> The contract is normally that if !has_value, an exception was thrown. Return
> empty MaybeHandle<Object>() in that case. You probably also want done =
> from_interceptor.value != ABSENT;

I see. Wow, all those things could use some documenting :)

[Dmitry Lomov (no reviews), 2014-10-14 09:28:26]

PTAL

https://codereview.chromium.org/649603003/diff/260001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/649603003/diff/260001/src/objects.cc#newcode860
src/objects.cc:860: if (from_interceptor.has_value) {
On 2014/10/14 08:31:54, Toon Verwaest wrote:
> The contract is normally that if !has_value, an exception was thrown. Return
> empty MaybeHandle<Object>() in that case. You probably also want done =
> from_interceptor.value != ABSENT;

Done.

[Dmitry Lomov (no reviews), 2014-10-14 12:15:39]

PTAL, all refactorings done

[Toon Verwaest, 2014-10-15 08:31:28]

https://codereview.chromium.org/649603003/diff/390001/src/objects.cc
File src/objects.cc (left):

https://codereview.chromium.org/649603003/diff/390001/src/objects.cc#oldcode4871
src/objects.cc:4871: if (!GetOwnElementAccessorPair(object, index).is_null()) {
These cases actually only blacklist accessor pairs. The other accessors look
like data properties to JS, so have to be read.
(I don't think we ever have non-accessorpair indexed callbacks, but for
completeness ...)

https://codereview.chromium.org/649603003/diff/390001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/649603003/diff/390001/src/objects.cc#newcode856
src/objects.cc:856: if (accessor->GetStructure(receiver, js_object, index)
Come to think about it, you also only want to call the setter on the prototype
chain if it's an AccessorPair (or currently ExecutableAccessorInfo).
ExecutableAccessorInfos are changing because they are incorrect, they are
supposed to behave like data properties. I'm pretty sure we never have those
anyway as elements. So I'm not sure it's really worthwhile to replace
GetAccessorPair by GetStructure.

It also causes confusion for the others where you changed it. You aren't
supposed to blacklist non-AccessorPair callbacks.

https://codereview.chromium.org/649603003/diff/390001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/649603003/diff/390001/src/objects.h#newcode2268
src/objects.h:2268: //   interceptor, and this fucntion returns ABSENT.
typo: function

[Dmitry Lomov (no reviews), 2014-10-15 10:50:12]

Back to AccessorPairs. PTAL

[Toon Verwaest, 2014-10-17 11:22:13]

lgtm

[Dmitry Lomov (no reviews), 2014-10-17 13:19:55]

Committed patchset #12 (id:430001) manually as 24696 (presubmit successful).