Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1181623003:
In Array.of and Array.from, fall back to DefineOwnProperty (Closed)
|
Created:
5 years, 6 months ago by dehrenberg Modified:
5 years, 6 months ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionIn Array.of and Array.from, fall back to DefineOwnProperty
%AddElement is not intended for objects which are not arrays, and
its behavior may go away with future refactorings. This patch gets
rid of it if the receiver of from or of is not the intrinsic Array
object.
Array.of and Array.from previously papered over failures in calling
[[DefineOwnProperty]] when setting array elements. This patch
makes them lead to exceptions, and adds tests to assert that
the appropriate exceptions are thrown.
BUG=v8:4168
R=adamk
CC=rossberg,verwaest
LOG=Y
Committed: https://crrev.com/b23c328f53b4579fa9d9f5c2c344839244d0f526
Cr-Commit-Position: refs/heads/master@{#28969}
Patch Set 1 #
Total comments: 14
Patch Set 2 : fix property attributes #
Total comments: 4
Patch Set 3 : Improve tests #Patch Set 4 : fix merge conflict #Patch Set 5 : fix %AddElement call, changed in rebase #
Messages
Total messages: 30 (11 generated)
littledan@chromium.org changed reviewers: + littledan@chromium.org
verwaest@chromium.org changed reviewers: + verwaest@chromium.org
https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js#newcod... src/harmony-array.js:197: if (constructor === GlobalArray) { Is this enough? A constructor could return an array with predefined getters/setters...
arv@chromium.org changed reviewers: + arv@chromium.org
https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js#newcod... src/harmony-array.js:197: if (constructor === GlobalArray) { On 2015/06/11 08:03:14, Toon Verwaest wrote: > Is this enough? A constructor could return an array with predefined > getters/setters... I believe this is fine. constructor here is the receiver in Array.from. The array passed in here is a new instance created from Array which will never have own accessors. https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js#newcod... src/harmony-array.js:200: ObjectDefineProperty(array, i, { value: value }); non enum? non writable? non configurable? We really should expose something faster than using this high level API that uses property descriptors. How about we provide a function called CreateDataPropertyOrThrow or CreateDataProperty? We could also skip the descriptor and use the bit mask we are using internally (NONE, DONT_ENUM etc) The refactoring can be done later but the descriptor (enum, writable, configurable) needs to be fixed and tested. https://codereview.chromium.org/1181623003/diff/1/test/mjsunit/harmony/array-... File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/1181623003/diff/1/test/mjsunit/harmony/array-... test/mjsunit/harmony/array-from.js:170: })(); I see that there is already a test for accessors on Array.prototype.
https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js#newcod... src/harmony-array.js:197: if (constructor === GlobalArray) { On 2015/06/11 13:04:01, arv wrote: > On 2015/06/11 08:03:14, Toon Verwaest wrote: > > Is this enough? A constructor could return an array with predefined > > getters/setters... > > I believe this is fine. > > constructor here is the receiver in Array.from. The array passed in here is a > new instance created from Array which will never have own accessors. > function f() { var result = []; Object.defineProperty(result, "0", {value:100}); return result; } > Array.from.call(f, [200]) (d8):1: illegal access
https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js#newcod... src/harmony-array.js:197: if (constructor === GlobalArray) { On 2015/06/11 13:09:23, Toon Verwaest wrote: > On 2015/06/11 13:04:01, arv wrote: > > On 2015/06/11 08:03:14, Toon Verwaest wrote: > > > Is this enough? A constructor could return an array with predefined > > > getters/setters... > > > > I believe this is fine. > > > > constructor here is the receiver in Array.from. The array passed in here is a > > new instance created from Array which will never have own accessors. > > > function f() { var result = []; Object.defineProperty(result, "0", > {value:100}); return result; } > > Array.from.call(f, [200]) > (d8):1: illegal access Do you have Dan's patch patched in? This should go down the slow path because f !== GlobalArray.
https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js#newcod... src/harmony-array.js:197: if (constructor === GlobalArray) { On 2015/06/11 15:12:10, adamk wrote: > On 2015/06/11 13:09:23, Toon Verwaest wrote: > > On 2015/06/11 13:04:01, arv wrote: > > > On 2015/06/11 08:03:14, Toon Verwaest wrote: > > > > Is this enough? A constructor could return an array with predefined > > > > getters/setters... > > > > > > I believe this is fine. > > > > > > constructor here is the receiver in Array.from. The array passed in here is > a > > > new instance created from Array which will never have own accessors. > > > > > function f() { var result = []; Object.defineProperty(result, "0", > > {value:100}); return result; } > > > Array.from.call(f, [200]) > > (d8):1: illegal access > > Do you have Dan's patch patched in? This should go down the slow path because f > !== GlobalArray. Once we add support for @@species then we will have to use the slow path again. An assert would have been good.
https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js#newcod... src/harmony-array.js:197: if (constructor === GlobalArray) { On 2015/06/11 15:19:37, arv wrote: > On 2015/06/11 15:12:10, adamk wrote: > > On 2015/06/11 13:09:23, Toon Verwaest wrote: > > > On 2015/06/11 13:04:01, arv wrote: > > > > On 2015/06/11 08:03:14, Toon Verwaest wrote: > > > > > Is this enough? A constructor could return an array with predefined > > > > > getters/setters... > > > > > > > > I believe this is fine. > > > > > > > > constructor here is the receiver in Array.from. The array passed in here > is > > a > > > > new instance created from Array which will never have own accessors. > > > > > > > function f() { var result = []; Object.defineProperty(result, "0", > > > {value:100}); return result; } > > > > Array.from.call(f, [200]) > > > (d8):1: illegal access > > > > Do you have Dan's patch patched in? This should go down the slow path because > f > > !== GlobalArray. > > Once we add support for @@species then we will have to use the slow path again. > An assert would have been good. Not sure I follow, Array.from/of only look at the receiver, they don't check @@species.
https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js#newcod... src/harmony-array.js:197: if (constructor === GlobalArray) { On 2015/06/11 15:26:40, adamk wrote: > On 2015/06/11 15:19:37, arv wrote: > > On 2015/06/11 15:12:10, adamk wrote: > > > On 2015/06/11 13:09:23, Toon Verwaest wrote: > > > > On 2015/06/11 13:04:01, arv wrote: > > > > > On 2015/06/11 08:03:14, Toon Verwaest wrote: > > > > > > Is this enough? A constructor could return an array with predefined > > > > > > getters/setters... > > > > > > > > > > I believe this is fine. > > > > > > > > > > constructor here is the receiver in Array.from. The array passed in here > > is > > > a > > > > > new instance created from Array which will never have own accessors. > > > > > > > > > function f() { var result = []; Object.defineProperty(result, "0", > > > > {value:100}); return result; } > > > > > Array.from.call(f, [200]) > > > > (d8):1: illegal access > > > > > > Do you have Dan's patch patched in? This should go down the slow path > because > > f > > > !== GlobalArray. > > > > Once we add support for @@species then we will have to use the slow path > again. > > An assert would have been good. > > Not sure I follow, Array.from/of only look at the receiver, they don't check > @@species. You are right. nm.
https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js#newcod... src/harmony-array.js:197: if (constructor === GlobalArray) { On 2015/06/11 15:12:10, adamk wrote: > On 2015/06/11 13:09:23, Toon Verwaest wrote: > > On 2015/06/11 13:04:01, arv wrote: > > > On 2015/06/11 08:03:14, Toon Verwaest wrote: > > > > Is this enough? A constructor could return an array with predefined > > > > getters/setters... > > > > > > I believe this is fine. > > > > > > constructor here is the receiver in Array.from. The array passed in here is > a > > > new instance created from Array which will never have own accessors. > > > > > function f() { var result = []; Object.defineProperty(result, "0", > > {value:100}); return result; } > > > Array.from.call(f, [200]) > > (d8):1: illegal access > > Do you have Dan's patch patched in? This should go down the slow path because f > !== GlobalArray. Arg, ok, I see. Nevermind ;) However, shouldn't we make sure that if this is not a constructor, we also pass in GlobalArray?
The new version should fix the issue with properties defined wrong. Is there anything else that should be fixed? https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js#newcod... src/harmony-array.js:197: if (constructor === GlobalArray) { On 2015/06/11 15:35:33, Toon Verwaest wrote: > On 2015/06/11 15:12:10, adamk wrote: > > On 2015/06/11 13:09:23, Toon Verwaest wrote: > > > On 2015/06/11 13:04:01, arv wrote: > > > > On 2015/06/11 08:03:14, Toon Verwaest wrote: > > > > > Is this enough? A constructor could return an array with predefined > > > > > getters/setters... > > > > > > > > I believe this is fine. > > > > > > > > constructor here is the receiver in Array.from. The array passed in here > is > > a > > > > new instance created from Array which will never have own accessors. > > > > > > > function f() { var result = []; Object.defineProperty(result, "0", > > > {value:100}); return result; } > > > > Array.from.call(f, [200]) > > > (d8):1: illegal access > > > > Do you have Dan's patch patched in? This should go down the slow path because > f > > !== GlobalArray. > > Arg, ok, I see. Nevermind ;) > > However, shouldn't we make sure that if this is not a constructor, we also pass > in GlobalArray? I think GlobalArray will remain a constructor, so we'll go down the else path if a non-constructor is passed in. https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js#newcod... src/harmony-array.js:200: ObjectDefineProperty(array, i, { value: value }); On 2015/06/11 13:04:01, arv wrote: > non enum? non writable? non configurable? > > We really should expose something faster than using this high level API that > uses property descriptors. > > How about we provide a function called CreateDataPropertyOrThrow or > CreateDataProperty? > > We could also skip the descriptor and use the bit mask we are using internally > (NONE, DONT_ENUM etc) > > The refactoring can be done later but the descriptor (enum, writable, > configurable) needs to be fixed and tested. Fixed the descriptor and added tests. https://codereview.chromium.org/1181623003/diff/1/test/mjsunit/harmony/array-... File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/1181623003/diff/1/test/mjsunit/harmony/array-... test/mjsunit/harmony/array-from.js:170: })(); On 2015/06/11 13:04:01, arv wrote: > I see that there is already a test for accessors on Array.prototype. Not sure what you're referring to, but I added a test.
LGTM with nits https://codereview.chromium.org/1181623003/diff/1/test/mjsunit/harmony/array-... File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/1181623003/diff/1/test/mjsunit/harmony/array-... test/mjsunit/harmony/array-from.js:170: })(); On 2015/06/11 16:27:25, littledan wrote: > On 2015/06/11 13:04:01, arv wrote: > > I see that there is already a test for accessors on Array.prototype. > > Not sure what you're referring to, but I added a test. Ignore me. https://codereview.chromium.org/1181623003/diff/20001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/1181623003/diff/20001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:160: // Check that exotic was defined right I would remove this (line 160-165). We have plenty of tests making sure accessors works correctly ;-) https://codereview.chromium.org/1181623003/diff/20001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:166: // Accessor properties can't be overwritten with DefineOwnProperty This comment is wrong. The reason why is because you created the property as non configurable.
https://codereview.chromium.org/1181623003/diff/20001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/1181623003/diff/20001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:160: // Check that exotic was defined right On 2015/06/11 16:41:30, arv wrote: > I would remove this (line 160-165). We have plenty of tests making sure > accessors works correctly ;-) Done. https://codereview.chromium.org/1181623003/diff/20001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:166: // Accessor properties can't be overwritten with DefineOwnProperty On 2015/06/11 16:41:31, arv wrote: > This comment is wrong. The reason why is because you created the property as non > configurable. Done.
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org Link to the patchset: https://codereview.chromium.org/1181623003/#ps40001 (title: "Improve tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181623003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...) v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org Link to the patchset: https://codereview.chromium.org/1181623003/#ps60001 (title: "fix merge conflict")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181623003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org Link to the patchset: https://codereview.chromium.org/1181623003/#ps80001 (title: "fix %AddElement call, changed in rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181623003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b23c328f53b4579fa9d9f5c2c344839244d0f526 Cr-Commit-Position: refs/heads/master@{#28969} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
