Change SmartMove no-op behavior to match SimpleMove (and ES6 spec)

Change SmartMove no-op behavior to match SimpleMove (and ES6 spec)

The previous behavior, which caused Array.prototype.unshift() (with no args)
to have side-effects, no longer matches the spec (ES6 changed the no-arg behavior
in April 2014). The new SmartMove behavior is also compatible with current
versions of Firefox.

This is a baby step towards getting rid of SmartMove; it isolates the test
change in this patch, instead of lumping it in confusingly with all the
other test updates necessary for moving away from SmartMove.

R=dslomov@chromium.org

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

[adamk, 2014-10-21 21:42:25]

adamk@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2014-10-22 08:45:50]

https://codereview.chromium.org/666883009/diff/1/test/mjsunit/array-unshift.js
File test/mjsunit/array-unshift.js (right):

https://codereview.chromium.org/666883009/diff/1/test/mjsunit/array-unshift.j...
test/mjsunit/array-unshift.js:67: assertFalse(array.hasOwnProperty(0));
I am not convinced that this is actually spec-compliant behavior. The spec says
Array.prototype.unshift should always run through a moving loop, no matter how
many arguments are passed. The loop performs [[HasProperty]], [[Get]] and
[[Put]] on all present elements. The effect of that should be that "non-own data
properties" turn into "own data properties" just like this test originally
expected.

[adamk, 2014-10-22 17:15:34]

adamk@chromium.org changed reviewers:
+ arv@chromium.org, rossberg@chromium.org

[adamk, 2014-10-22 17:15:35]

Looks like ES6 changed Array.prototype.unshift() (with no arguments) to be
side-effect free on the elements of the receiver. Anyone happen to know if
that's an intended change? FWIW, Firefox seems to have the ES6 behavior.

https://codereview.chromium.org/666883009/diff/1/test/mjsunit/array-unshift.js
File test/mjsunit/array-unshift.js (right):

https://codereview.chromium.org/666883009/diff/1/test/mjsunit/array-unshift.j...
test/mjsunit/array-unshift.js:67: assertFalse(array.hasOwnProperty(0));
On 2014/10/22 at 08:45:50, Michael Starzinger wrote:
> I am not convinced that this is actually spec-compliant behavior. The spec
says Array.prototype.unshift should always run through a moving loop, no matter
how many arguments are passed. The loop performs [[HasProperty]], [[Get]] and
[[Put]] on all present elements. The effect of that should be that "non-own data
properties" turn into "own data properties" just like this test originally
expected.

This appears to be a difference between ES5 and ES6, I should have checked both!
Here's ES6:

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.uns...

In particular, step 7 is "If argCount > 0, then [run the loop]"

whereas, as you point out, ES5
(http://www.ecma-international.org/ecma-262/5.1/#sec-15.4.4.13) says nothing
about checking argCount.

Seems like something to ask an ES expert on, will start by adding rossberg and
arv to this review.

[adamk, 2014-10-22 17:19:46]

Looks like this was changed in April:

http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts#april_27_...

"Adjust Array.prototype.reverse and Array.prototype.unshift to eliminate several
unnecessary MOP calls that were exposed if the array is a proxy"

and indeed, step 7 of unshift was added in that revision. It's not clear from
this note whether the editor noticed that this wasn't just an issue for Proxies,
due to the [[Get]] and [[Put]] calls (in addition to [[HasProperty]]).

[Michael Starzinger, 2014-10-22 17:32:25]

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

[Michael Starzinger, 2014-10-22 17:32:26]

If this was an intended change to the spec then I am of course fine with landing
this change. I'll defer to Andreas and/or Dmitry in that regard.

+Andreas, +Dmitry

[adamk, 2014-10-22 18:31:19]

Updated CL description to s/nonsensical/non-spec-compliant/

[adamk, 2014-10-22 22:37:34]

Also tested on (trunk) JSC, which has the ES6 behavior.

[Dmitry Lomov (no reviews), 2014-10-23 09:11:48]

lgtm, looks legit :)

[adamk, 2014-10-23 17:46:50]

Committed patchset #1 (id:1) manually as 24854 (presubmit successful).