Stores and compound assignments for named super properties.

Stores and compound assignments for named super properties.

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

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

[Dmitry Lomov (no reviews), 2014-09-23 11:31:38]

dslomov@chromium.org changed reviewers:
+ arv@chromium.org
- arv@chromiun.org

[Dmitry Lomov (no reviews), 2014-09-23 11:31:39]

PTAL before platform ports.
Caveats:
- does not support incerement/decrement
- does not install data properties on 'this' for non-existent property
assignment.

[Dmitry Lomov (no reviews), 2014-09-24 11:51:24]

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

[arv (Not doing code reviews), 2014-09-24 14:53:57]

https://codereview.chromium.org/593073002/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/593073002/diff/20001/src/runtime.cc#newcode2118
src/runtime.cc:2118: if (!proto->IsJSReceiver()) return
isolate->heap()->undefined_value();
Is this correct? I believe the following should work

(function() {
  super.x = 1;
}.toMethod({
  set x(v) { print(v); {
}).call(42))

and print 1

[Dmitry Lomov (no reviews), 2014-09-24 16:23:46]

https://codereview.chromium.org/593073002/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/593073002/diff/20001/src/runtime.cc#newcode2118
src/runtime.cc:2118: if (!proto->IsJSReceiver()) return
isolate->heap()->undefined_value();
On 2014/09/24 14:53:57, arv wrote:
> Is this correct? I believe the following should work
> 
> (function() {
>   super.x = 1;
> }.toMethod({
>   set x(v) { print(v); {
> }).call(42))
> 
> and print 1
I have a bit of a hard time deciphering what you are trying to say. At the very
least, 
  super.x = 1 is [[HomeObject]].__proto__.[[Set]]('x',1,this), not
[[HomeObject]].[[Set]]('x',1,this), so the setter will not execute.

[Igor Sheludko, 2014-09-25 08:06:03]

https://codereview.chromium.org/593073002/diff/20001/src/ia32/full-codegen-ia...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/593073002/diff/20001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:1845: break;
I'm afraid that the second pair of receiver-home object is not necessary in case
of non-compound assignment.

https://codereview.chromium.org/593073002/diff/20001/test/mjsunit/harmony/sup...
File test/mjsunit/harmony/super.js (right):

https://codereview.chromium.org/593073002/diff/20001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:96: Derived.prototype.testSetter = function() {
Suggestion: test the strict mode too (and same for getter tests).

https://codereview.chromium.org/593073002/diff/20001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:122: assertThrows(function() { d.mStrict(); },
ReferenceError);
Suggestion: use different instances of Derived for sloppy and strict tests and
check that after the assignment the property appears neither in Base nor in d.

[Dmitry Lomov (no reviews), 2014-09-25 08:13:32]

On 2014/09/25 08:06:03, Igor Sheludko wrote:
>
https://codereview.chromium.org/593073002/diff/20001/src/ia32/full-codegen-ia...
> File src/ia32/full-codegen-ia32.cc (right):
> 
>
https://codereview.chromium.org/593073002/diff/20001/src/ia32/full-codegen-ia...
> src/ia32/full-codegen-ia32.cc:1845: break;
> I'm afraid that the second pair of receiver-home object is not necessary in
case
> of non-compound assignment.

Thanks, good catch, actually found it myself as well. 
The proper tests should always include the use of assignment expression value,
then the bug manifests.
> 
>
https://codereview.chromium.org/593073002/diff/20001/test/mjsunit/harmony/sup...
> File test/mjsunit/harmony/super.js (right):
> 
>
https://codereview.chromium.org/593073002/diff/20001/test/mjsunit/harmony/sup...
> test/mjsunit/harmony/super.js:96: Derived.prototype.testSetter = function() {
> Suggestion: test the strict mode too (and same for getter tests).
> 
>
https://codereview.chromium.org/593073002/diff/20001/test/mjsunit/harmony/sup...
> test/mjsunit/harmony/super.js:122: assertThrows(function() { d.mStrict(); },
> ReferenceError);
> Suggestion: use different instances of Derived for sloppy and strict tests and
> check that after the assignment the property appears neither in Base nor in d.

[arv (Not doing code reviews), 2014-09-25 13:51:41]

https://codereview.chromium.org/593073002/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/593073002/diff/20001/src/runtime.cc#newcode2118
src/runtime.cc:2118: if (!proto->IsJSReceiver()) return
isolate->heap()->undefined_value();
On 2014/09/24 16:23:46, Dmitry Lomov (chromium) wrote:
> On 2014/09/24 14:53:57, arv wrote:
> > Is this correct? I believe the following should work
> > 
> > (function() {
> >   super.x = 1;
> > }.toMethod({
> >   set x(v) { print(v); {
> > }).call(42))
> > 
> > and print 1
> I have a bit of a hard time deciphering what you are trying to say. At the
very
> least, 
>   super.x = 1 is [[HomeObject]].__proto__.[[Set]]('x',1,this), not
> [[HomeObject]].[[Set]]('x',1,this), so the setter will not execute. 
> 

I was trying to give an example where this is not an object, but a primitive.

[Dmitry Lomov (no reviews), 2014-09-25 13:59:59]

https://codereview.chromium.org/593073002/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/593073002/diff/20001/src/runtime.cc#newcode2118
src/runtime.cc:2118: if (!proto->IsJSReceiver()) return
isolate->heap()->undefined_value();
On 2014/09/25 13:51:41, arv wrote:
> On 2014/09/24 16:23:46, Dmitry Lomov (chromium) wrote:
> > On 2014/09/24 14:53:57, arv wrote:
> > > Is this correct? I believe the following should work
> > > 
> > > (function() {
> > >   super.x = 1;
> > > }.toMethod({
> > >   set x(v) { print(v); {
> > > }).call(42))
> > > 
> > > and print 1
> > I have a bit of a hard time deciphering what you are trying to say. At the
> very
> > least, 
> >   super.x = 1 is [[HomeObject]].__proto__.[[Set]]('x',1,this), not
> > [[HomeObject]].[[Set]]('x',1,this), so the setter will not execute. 
> > 
> 
> I was trying to give an example where this is not an object, but a primitive.

'this' is 'receiver' here, and it is an v8::intrenal::Object, so it can be
primitive. prototype of an object (what PrototypeIterator::GetCurrent returns)
has to be either object or null per spec, so line 2118 is valid.

[Dmitry Lomov (no reviews), 2014-09-25 14:08:20]

Feedback addressed, starting platform ports

[arv (Not doing code reviews), 2014-09-25 15:39:24]

https://codereview.chromium.org/593073002/diff/40001/test/mjsunit/harmony/sup...
File test/mjsunit/harmony/super.js (right):

https://codereview.chromium.org/593073002/diff/40001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:144: }());
Can you add a test of a strict function, used as a method that contains super
that is invoked with a value as this. Like I asked about...

function f() {
  'use strict';
  assertEquals(42, this);
  assertEquals(String.prototype.toString, super.toString);
  assertThrows('super.toString()',TypeError);
}
var g = f.toMethod(new String());
g.call(42);

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

PTAL

https://codereview.chromium.org/593073002/diff/40001/test/mjsunit/harmony/sup...
File test/mjsunit/harmony/super.js (right):

https://codereview.chromium.org/593073002/diff/40001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:144: }());
On 2014/09/25 15:39:24, arv wrote:
> Can you add a test of a strict function, used as a method that contains super
> that is invoked with a value as this. Like I asked about...
> 
> function f() {
>   'use strict';
>   assertEquals(42, this);
>   assertEquals(String.prototype.toString, super.toString);
>   assertThrows('super.toString()',TypeError);
> }
> var g = f.toMethod(new String());
> g.call(42);

Done.

[Igor Sheludko, 2014-09-26 14:35:23]

lgtm

[Dmitry Lomov (no reviews), 2014-09-29 08:16:39]

Committed patchset #5 (id:80001) manually as 24268 (presubmit successful).

[arv (Not doing code reviews), 2014-09-29 14:37:27]

C++ LGTM

Tests could use some cleanup/improvements.

https://codereview.chromium.org/593073002/diff/80001/src/full-codegen.h
File src/full-codegen.h (right):

https://codereview.chromium.org/593073002/diff/80001/src/full-codegen.h#newco...
src/full-codegen.h:524: // Load a value from super.named prroperty.
typo

https://codereview.chromium.org/593073002/diff/80001/src/runtime/runtime.cc
File src/runtime/runtime.cc (right):

https://codereview.chromium.org/593073002/diff/80001/src/runtime/runtime.cc#n...
src/runtime/runtime.cc:2014: RUNTIME_FUNCTION(Runtime_StoreToSuper_Sloppy) {
Why not take strict mode as a parameter?

https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/sup...
File test/mjsunit/harmony/super.js (right):

https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:123: var getCalled = false;
Tip: These kinds of tests are always better written using a counter. That way
you also assert number of calls.

var getCalls = 0;

https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:129: getCalled = true;
getCalls++;

https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:144: assertTrue(42 == this);
If this is sloppy, should we not get a wrapper here? Maybe that is why you are
doing == which coerces?

Maybe do something like this to make it clearer?

assertEquals('object', typeof this);
assertTrue(this instanceof Number);
assertEquals(42, this.valueOf());

https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:145: getCalled = false;
getCalls = 0;

https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:148: assertTrue(getCalled);
assertEquals(1, getCalls);

https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:166: assertTrue(42 == this);
=== and/or add typeof assert.

https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:194: assertTrue(42 == this);
here as well. == is just too lenient to use for these kind of tests.

https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:196: var except = false;
assertThrows cannot be used here but the following pattern is generally better:

var ex;
try {
  ...
} catch (e) {
  ex = e;
}
assertTrue(ex instanceof TypeError);
assertEquals('...', ex.message);

https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:199: } catch(e) { except = true; }
ws after catch (like if, for, while, etc)

https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:216: "use strict";
Don't mix and match quotes. Stick to '

https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:230: var o = {}
;

[Dmitry Lomov (no reviews), 2014-09-29 14:58:11]

Thanks, will clean-up in follow-up CL.

[Dmitry Lomov (no reviews), 2014-09-29 14:58:13]

Thanks, will clean-up in follow-up CL.