Arguments object has @@iterator

src/accessors.cc

Index: src/accessors.cc
diff --git a/src/accessors.cc b/src/accessors.cc
index c2361b3e9cda753c69ae5e12bb57ded05385cde8..d2636d2bd589c59ce788196320abf0235d3d77ac 100644
--- a/src/accessors.cc
+++ b/src/accessors.cc
@@ -156,6 +156,45 @@ bool SetPropertyOnInstanceIfInherited(
 
 
 //
+// Accessors::ArgumentsIterator
+//
+
+
+void Accessors::ArgumentsIteratorGetter(
+    v8::Local<v8::Name> name, const v8::PropertyCallbackInfo<v8::Value>& info) {
+  i::Isolate* isolate = reinterpret_cast<i::Isolate*>(info.GetIsolate());
+  DisallowHeapAllocation no_allocation;
+  HandleScope scope(isolate);
+  Object* result = isolate->native_context()->array_values_iterator();
+  info.GetReturnValue().Set(Utils::ToLocal(Handle<Object>(result, isolate)));
+}
+
+
+void Accessors::ArgumentsIteratorSetter(
+    v8::Local<v8::Name> name, v8::Local<v8::Value> val,
+    const v8::PropertyCallbackInfo<void>& info) {
+  i::Isolate* isolate = reinterpret_cast<i::Isolate*>(info.GetIsolate());
+  HandleScope scope(isolate);
+  Handle<JSObject> object = Utils::OpenHandle(*info.This());
+  Handle<Object> value = Utils::OpenHandle(*val);
+  LookupIterator it(object, Utils::OpenHandle(*name));
+  // Advance the iterator to find out more about the property.
+  bool has_property = it.HasProperty();
+  DCHECK(has_property);
+  MaybeHandle<Object> maybe = Object::SetDataProperty(&it, value);

[Toon Verwaest, 2014/08/18 13:47:25]

This doesn't seem right to me, if only because the setter could have been called
through a derived dictionary-mode object for which the iterator will stop on the
dictionary mode object; and HasProperty will be false.

What's the expected behavior exactly? Can't you just use SetProperty(&it,
value), or SetOwnPropertyIgnoreAttributes?

[arv (Not doing code reviews), 2014/08/18 14:16:56]

The intent is to replace the existing interceptor with a plain old data
property.

[wingo, 2014/08/20 15:46:57]

As Erik says the intent is that arguments[@@iterator] is just a data property. 
It has to be implemented using API accessor callbacks because we know what the
value should be for it, and we don't want to bloat the accessor object with a
different word.  But if someone does arguments[@@iterator] = 10, it should
replace the API accessors with a normal data property.

[Toon Verwaest, 2014/08/21 11:07:20]

To be clear what I meant, what about:

o = {__proto__:arguments};
// Make o dict-mode.
%OptimizeObjectForAddingMultipleProperties(o, 0);
o[@@iterator] = 10;

The setter will be called with a slow-mode o, for which (without calling
Next()), IsFound() is true while HasProperty is false.

This sucks about ExecutableAccessorInfo: it looks like a data property but
behaves like an accessor. That's why SetPropertyOnInstanceIfInherited is used as
a hack; which by itself isn't fully correct with regard to non-extensible
objects in strict-mode.

Or even worse, what about:
String.prototype.__proto__ = arguments;
"bla"[@@iterator] = 10;

+  maybe.Check();
+}
+
+
+Handle<AccessorInfo> Accessors::ArgumentsIteratorInfo(
+    Isolate* isolate, PropertyAttributes attributes) {
+  return MakeAccessor(isolate, isolate->factory()->length_string(),

[Toon Verwaest, 2014/08/21 11:07:20]

Ok, in that case this is part of what confused me. This shouldn't be
length-string, but rather @@iterator, right?

+                      &ArgumentsIteratorGetter, &ArgumentsIteratorSetter,
+                      attributes);
+}
+
+
+//
 // Accessors::ArrayLength
 //