Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2430383004:
Remove the 'caller' property from the strict-mode arguments map (Closed)
|
Created:
4 years, 2 months ago by Henrique Ferreiro Modified:
4 years, 1 month ago CC:
Dan Ehrenberg, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionRemove the 'caller' property from the strict-mode arguments map
This was removed from ECMAScript in the September 2016 TC39 meeting, see https://github.com/tc39/ecma262/issues/670.
BUG=v8:5535
Committed: https://crrev.com/dfcd54568216243fd666d282f7cb354d1c8728d3
Cr-Commit-Position: refs/heads/master@{#40770}
Patch Set 1 #Patch Set 2 : delete an obsolete test #
Total comments: 6
Patch Set 3 : restored regress-1387.js #Patch Set 4 : fix restored test and other #
Total comments: 7
Patch Set 5 : Remove the 'caller' property from the strict-mode arguments map #
Total comments: 1
Patch Set 6 : Mark some test262 tests as FAIL #
Messages
Total messages: 36 (14 generated)
henrique.ferreiro@gmail.com changed reviewers: + caitp@igalia.com
LGTM, but I think the tests should just be altered rather than deleted https://codereview.chromium.org/2430383004/diff/20001/src/bootstrapper.cc File src/bootstrapper.cc (left): https://codereview.chromium.org/2430383004/diff/20001/src/bootstrapper.cc#old... src/bootstrapper.cc:2547: } afaik this LGTM, but lets modify the tests rather than deleting lines from them https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/es6/classe... File test/mjsunit/es6/classes.js (left): https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/es6/classe... test/mjsunit/es6/classes.js:167: arguments.caller; I don't think we need to remove this line from the base function, the base class is sloppy. Maybe something interesting would be to assign this value to a property of `this`, so that you can assert what it is https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/es6/classe... test/mjsunit/es6/classes.js:173: new D; I don't think this should have ever thrown in the first place, since the superclass' arguments object should have been sloppy arguments. Lets change this assertion to be `assertEquals(D.prototype, (new D()).__proto__)`, which I believe should be correct https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/regress/re... File test/mjsunit/regress/regress-1387.js (left): https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/regress/re... test/mjsunit/regress/regress-1387.js:38: assertEquals(get1, get2); don't delete this test. Instead, change the assertions. the `get` and `set` properties of the arguments.callee should still have the same value as each other (they are both %ThrowTypeError%), while the "caller" descriptor is just undefined, because it doesn't exist.
On 2016/10/27 13:31:02, caitp wrote: > LGTM, but I think the tests should just be altered rather than deleted > > https://codereview.chromium.org/2430383004/diff/20001/src/bootstrapper.cc > File src/bootstrapper.cc (left): > > https://codereview.chromium.org/2430383004/diff/20001/src/bootstrapper.cc#old... > src/bootstrapper.cc:2547: } > afaik this LGTM, but lets modify the tests rather than deleting lines from them > > https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/es6/classe... > File test/mjsunit/es6/classes.js (left): > > https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/es6/classe... > test/mjsunit/es6/classes.js:167: arguments.caller; > I don't think we need to remove this line from the base function, the base class > is sloppy. Maybe something interesting would be to assign this value to a > property of `this`, so that you can assert what it is > > https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/es6/classe... > test/mjsunit/es6/classes.js:173: new D; > I don't think this should have ever thrown in the first place, since the > superclass' arguments object should have been sloppy arguments. > > Lets change this assertion to be `assertEquals(D.prototype, (new > D()).__proto__)`, which I believe should be correct > > https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/regress/re... > File test/mjsunit/regress/regress-1387.js (left): > > https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/regress/re... > test/mjsunit/regress/regress-1387.js:38: assertEquals(get1, get2); > don't delete this test. Instead, change the assertions. the `get` and `set` > properties of the arguments.callee should still have the same value as each > other (they are both %ThrowTypeError%), while the "caller" descriptor is just > undefined, because it doesn't exist. Fair enough. I have modified regress-1387.js and waiting for comments on classes.js.
Description was changed from ========== Remove the 'caller' property from the strict-mode arguments map BUG=v8:5535 ========== to ========== Remove the 'caller' property from the strict-mode arguments map This was removed from ECMAScript in the September 2016 TC39 meeting, see https://github.com/tc39/ecma262/issues/670. BUG=v8:5535 ==========
https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/es6/classe... File test/mjsunit/es6/classes.js (left): https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/es6/classe... test/mjsunit/es6/classes.js:173: new D; On 2016/10/27 13:31:02, caitp wrote: > I don't think this should have ever thrown in the first place, since the > superclass' arguments object should have been sloppy arguments. > > Lets change this assertion to be `assertEquals(D.prototype, (new > D()).__proto__)`, which I believe should be correct The commit message (d5d15253) says: "Classes: Expand test to cover strict runtime behavior. This tests that the extends expression is treated as strict at runtime and not just at parse time". This seems to match the standard: "Function code is strict mode code if the associated FunctionDeclaration, [...] is contained in strict mode code".
https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/es6/classe... File test/mjsunit/es6/classes.js (left): https://codereview.chromium.org/2430383004/diff/20001/test/mjsunit/es6/classe... test/mjsunit/es6/classes.js:173: new D; On 2016/10/28 09:56:05, hferreiro wrote: > On 2016/10/27 13:31:02, caitp wrote: > > I don't think this should have ever thrown in the first place, since the > > superclass' arguments object should have been sloppy arguments. > > > > Lets change this assertion to be `assertEquals(D.prototype, (new > > D()).__proto__)`, which I believe should be correct > > The commit message (d5d15253) says: "Classes: Expand test to cover strict > runtime behavior. This tests that the extends expression is treated as strict at > runtime and not just at parse time". This seems to match the standard: "Function > code is strict mode code if the associated FunctionDeclaration, [...] is > contained in strict mode code". sure, but in this file, it occurs in sloppy mode. Otherwise the `with` statement would be a SyntaxError. This was apparently testimg that a class which extends a sloppy functionbwould give that sloppy function a strict arguments object. this is incorrect.
Fixed the deleted test and a webkit test in which I had removed some checks.
Thanks, I'm happy with the tests. LGTM. Adam or Dan, can either of you take a look?
adamk@chromium.org changed reviewers: + adamk@chromium.org
https://codereview.chromium.org/2430383004/diff/60001/test/mjsunit/es6/classe... File test/mjsunit/es6/classes.js (left): https://codereview.chromium.org/2430383004/diff/60001/test/mjsunit/es6/classe... test/mjsunit/es6/classes.js:167: arguments.caller; Seems reasonable to change this to test arguments.callee, since that still throws. Or if you want to actually test for the absence of .caller, you could add an assertThrows() for '() => e.arguments.callee'. https://codereview.chromium.org/2430383004/diff/60001/test/mjsunit/es6/classe... File test/mjsunit/es6/classes.js (right): https://codereview.chromium.org/2430383004/diff/60001/test/mjsunit/es6/classe... test/mjsunit/es6/classes.js:173: assertEquals(undefined, Object.getOwnPropertyDescriptor(e.arguments, "caller")); Nit: wrap to 80 columns. https://codereview.chromium.org/2430383004/diff/60001/test/mjsunit/es6/classe... test/mjsunit/es6/classes.js:175: assertEquals(true, Reflect.set(e.arguments, "caller", 0)); I don't understand the need for this test. You're already asserting above that there's no own "caller" property. I don't think this or the below are interesting here. https://codereview.chromium.org/2430383004/diff/60001/test/mjsunit/strict-mod... File test/mjsunit/strict-mode.js (right): https://codereview.chromium.org/2430383004/diff/60001/test/mjsunit/strict-mod... test/mjsunit/strict-mode.js:1114: assertEquals(args.caller, undefined); The proper order for asserts is assertWhatever(expected, actual) Also, get you change this to do getOwnPropertyDescriptor so it's testing the same thing as the .callee case? i.e., assertEquals(undefined, Object.getOwnPropertyDescriptor(args, "caller")); Same goes everywhere below.
https://codereview.chromium.org/2430383004/diff/60001/test/mjsunit/es6/classe... File test/mjsunit/es6/classes.js (right): https://codereview.chromium.org/2430383004/diff/60001/test/mjsunit/es6/classe... test/mjsunit/es6/classes.js:175: assertEquals(true, Reflect.set(e.arguments, "caller", 0)); On 2016/10/31 18:05:36, adamk wrote: > I don't understand the need for this test. You're already asserting above that > there's no own "caller" property. I don't think this or the below are > interesting here. I suggested adding this, because property access is complicated, and I'd like to have many paths tested (even though this is still a small subset). Similar to test262's tendency to do this.
https://codereview.chromium.org/2430383004/diff/60001/test/mjsunit/es6/classe... File test/mjsunit/es6/classes.js (right): https://codereview.chromium.org/2430383004/diff/60001/test/mjsunit/es6/classe... test/mjsunit/es6/classes.js:175: assertEquals(true, Reflect.set(e.arguments, "caller", 0)); On 2016/10/31 18:07:49, caitp wrote: > On 2016/10/31 18:05:36, adamk wrote: > > I don't understand the need for this test. You're already asserting above that > > there's no own "caller" property. I don't think this or the below are > > interesting here. > > I suggested adding this, because property access is complicated, and I'd like to > have many paths tested (even though this is still a small subset). Similar to > test262's tendency to do this. specifically, this also checks that the property is also not installed on the arguments object's prototype, and if it is, not in a configurable way. I suppose `assertFalse('caller' in arguments)` would be okay too, though (but still fewer paths taken)
https://codereview.chromium.org/2430383004/diff/60001/test/mjsunit/es6/classe... File test/mjsunit/es6/classes.js (right): https://codereview.chromium.org/2430383004/diff/60001/test/mjsunit/es6/classe... test/mjsunit/es6/classes.js:175: assertEquals(true, Reflect.set(e.arguments, "caller", 0)); On 2016/10/31 18:10:33, caitp wrote: > On 2016/10/31 18:07:49, caitp wrote: > > On 2016/10/31 18:05:36, adamk wrote: > > > I don't understand the need for this test. You're already asserting above > that > > > there's no own "caller" property. I don't think this or the below are > > > interesting here. > > > > I suggested adding this, because property access is complicated, and I'd like > to > > have many paths tested (even though this is still a small subset). Similar to > > test262's tendency to do this. > > specifically, this also checks that the property is also not installed on the > arguments object's prototype, and if it is, not in a configurable way. I suppose > `assertFalse('caller' in arguments)` would be okay too, though (but still fewer > paths taken) I just don't think it makes sense to test property access semantics in a test that's fundamentally about the absence of this property. I'm OK with assertFalse('caller' in arguments), but anything more than that feels like we're putting the test in the wrong place.
I think I have addressed all comments now.
lgtm
On 2016/11/02 19:11:04, adamk wrote: > lgtm One last thing --- I think recent test262 changes relating to this caused test262 status changes, unless the current test262 roll doesn't have the new tests yet. Dan, do you remember if test262.status needs an update?
On 2016/11/02 19:12:54, caitp wrote: > On 2016/11/02 19:11:04, adamk wrote: > > lgtm > > One last thing --- I think recent test262 changes relating to this caused > test262 status changes, unless the current test262 roll doesn't have the new > tests yet. > > Dan, do you remember if test262.status needs an update? The CQ should take care of that for us: if there's stuff in test262 that tests this, it'll fail as part of the try run (or it'll pass when we're expecting it to fail, if there are new tests).
The CQ bit was checked by henrique.ferreiro@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from caitp@igalia.com Link to the patchset: https://codereview.chromium.org/2430383004/#ps80001 (title: "Remove the 'caller' property from the strict-mode arguments map")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
On 2016/11/03 10:59:26, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, > http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) > v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, > http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...) I guess we haven't rolled in the new test262 tests, but we still need to mark the previously valid ones as failures. Take a look at test/test262/test262.status for examples, one of us can probably help you out if you have questions about it on IRC.
(in addition to the test262 status updates) https://codereview.chromium.org/2430383004/diff/80001/test/webkit/fast/js/bas... File test/webkit/fast/js/basic-strict-mode-expected.txt (right): https://codereview.chromium.org/2430383004/diff/80001/test/webkit/fast/js/bas... test/webkit/fast/js/basic-strict-mode-expected.txt:203: PASS (function f(arg){'use strict'; return Object.getOwnPropertyDescriptor(arguments, 'caller').value; })() is undefined. line 203 needs an update, gotta remove the `.value`
The CQ bit was checked by henrique.ferreiro@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by henrique.ferreiro@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from caitp@igalia.com, adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2430383004/#ps100001 (title: "Mark some test262 tests as FAIL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove the 'caller' property from the strict-mode arguments map This was removed from ECMAScript in the September 2016 TC39 meeting, see https://github.com/tc39/ecma262/issues/670. BUG=v8:5535 ========== to ========== Remove the 'caller' property from the strict-mode arguments map This was removed from ECMAScript in the September 2016 TC39 meeting, see https://github.com/tc39/ecma262/issues/670. BUG=v8:5535 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Remove the 'caller' property from the strict-mode arguments map This was removed from ECMAScript in the September 2016 TC39 meeting, see https://github.com/tc39/ecma262/issues/670. BUG=v8:5535 ========== to ========== Remove the 'caller' property from the strict-mode arguments map This was removed from ECMAScript in the September 2016 TC39 meeting, see https://github.com/tc39/ecma262/issues/670. BUG=v8:5535 Committed: https://crrev.com/dfcd54568216243fd666d282f7cb354d1c8728d3 Cr-Commit-Position: refs/heads/master@{#40770} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/dfcd54568216243fd666d282f7cb354d1c8728d3 Cr-Commit-Position: refs/heads/master@{#40770} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
