158575 2016-06-09 13:09:03 -0700 ES5: Reusing function name as a parameter name shouldn't throw Syntax Error 2016-06-09 20:30:56 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 msaboff msaboff commit-queue erights keith_miller ljharb mark.lam saam webkit-bug-importer oldest_to_newest 1200940 0 msaboff 2016-06-09 13:09:03 -0700 According to the ES6 spec, <http://www.ecma-international.org/ecma-262/6.0/#sec-function-definitions-static-semantics-early-errors>, reusing the function name of a function as a parameter name should not throw a Syntax Error. This is was pointed out on the es-discuss mailing list <https://esdiscuss.org/topic/can-strict-parameters-shadow-strict-function-names>, specifically how we differ from V8 and SpiderMonkey. 1201015 1 280959 msaboff 2016-06-09 15:36:59 -0700 Created attachment 280959 Patch 1201017 2 webkit-bug-importer 2016-06-09 15:37:58 -0700 <rdar://problem/26729710> 1201019 3 280959 saam 2016-06-09 15:39:25 -0700 Comment on attachment 280959 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280959&action=review r=me with comments > Source/JavaScriptCore/parser/Parser.h:483 > + bool isValidStrictMode = (addResult.isNewEntry || !addResult.iterator->value.isParameter()) > + && m_vm->propertyNames->eval != *ident && !isArgumentsIdent; Style: one line > LayoutTests/js/script-tests/basic-strict-mode.js:89 > +shouldBe("(function a(a){'use strict'; return a+2; })(40)", "42"); > +shouldBe("var foo = function a(a){'use strict'; return a+2; }; foo(40)", "42"); Can you also add tests for method style syntax? 1201020 4 280959 mark.lam 2016-06-09 15:40:05 -0700 Comment on attachment 280959 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280959&action=review > Source/JavaScriptCore/ChangeLog:10 > + The check for a parameter with a duplicate name doesn't take into account the > + type of the prior variable. Added a check that the duplicate is also a > + parameter. Can you add the URL to the spec as a ref here? 1201032 5 msaboff 2016-06-09 16:03:49 -0700 (In reply to comment #3) > Comment on attachment 280959 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=280959&action=review > > r=me with comments > > > Source/JavaScriptCore/parser/Parser.h:483 > > + bool isValidStrictMode = (addResult.isNewEntry || !addResult.iterator->value.isParameter()) > > + && m_vm->propertyNames->eval != *ident && !isArgumentsIdent; > > Style: one line Our style guide doesn't talk about whether or not we should break long line. I believe this is more readable. The breaking is in line with the style guide. > > LayoutTests/js/script-tests/basic-strict-mode.js:89 > > +shouldBe("(function a(a){'use strict'; return a+2; })(40)", "42"); > > +shouldBe("var foo = function a(a){'use strict'; return a+2; }; foo(40)", "42"); > > Can you also add tests for method style syntax? Done. 1201033 6 msaboff 2016-06-09 16:04:05 -0700 (In reply to comment #4) > Comment on attachment 280959 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=280959&action=review > > > Source/JavaScriptCore/ChangeLog:10 > > + The check for a parameter with a duplicate name doesn't take into account the > > + type of the prior variable. Added a check that the duplicate is also a > > + parameter. > > Can you add the URL to the spec as a ref here? Done. 1201045 7 msaboff 2016-06-09 16:29:08 -0700 Committed r201892: <http://trac.webkit.org/changeset/201892> 280959 2016-06-09 15:36:59 -0700 2016-06-09 15:39:17 -0700 Patch 158575.patch text/plain 4431 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjAxODg4KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDE2LTA2LTA5ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIEVTNjogUmV1c2luZyBmdW5jdGlvbiBuYW1lIGFzIGEgcGFyYW1ldGVyIG5hbWUgc2hvdWxk bid0IHRocm93IFN5bnRheCBFcnJvcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTU4NTc1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgVGhlIGNoZWNrIGZvciBhIHBhcmFtZXRlciB3aXRoIGEgZHVwbGljYXRl IG5hbWUgZG9lc24ndCB0YWtlIGludG8gYWNjb3VudCB0aGUKKyAgICAgICAgdHlwZSBvZiB0aGUg cHJpb3IgdmFyaWFibGUuICBBZGRlZCBhIGNoZWNrIHRoYXQgdGhlIGR1cGxpY2F0ZSBpcyBhbHNv IGEKKyAgICAgICAgcGFyYW1ldGVyLgorCisgICAgICAgICogcGFyc2VyL1BhcnNlci5oOgorICAg ICAgICAoSlNDOjpTY29wZTo6ZGVjbGFyZVBhcmFtZXRlcik6CisKIDIwMTYtMDYtMDkgIENocmlz IER1bWV6ICA8Y2R1bWV6QGFwcGxlLmNvbT4KIAogICAgICAgICBVbnJldmlld2VkLCByb2xsaW5n IG91dCByMjAxODM2LCByMjAxODQ1LCBhbmQgcjIwMTg0OC4KSW5kZXg6IFNvdXJjZS9KYXZhU2Ny aXB0Q29yZS9wYXJzZXIvUGFyc2VyLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRD b3JlL3BhcnNlci9QYXJzZXIuaAkocmV2aXNpb24gMjAxODYyKQorKysgU291cmNlL0phdmFTY3Jp cHRDb3JlL3BhcnNlci9QYXJzZXIuaAkod29ya2luZyBjb3B5KQpAQCAtNDc5LDkgKzQ3OSwxMCBA QCBwdWJsaWM6CiAgICAgICAgIERlY2xhcmF0aW9uUmVzdWx0TWFzayByZXN1bHQgPSBEZWNsYXJh dGlvblJlc3VsdDo6VmFsaWQ7CiAgICAgICAgIGJvb2wgaXNBcmd1bWVudHNJZGVudCA9IGlzQXJn dW1lbnRzKG1fdm0sIGlkZW50KTsKICAgICAgICAgYXV0byBhZGRSZXN1bHQgPSBtX2RlY2xhcmVk VmFyaWFibGVzLmFkZChpZGVudC0+aW1wbCgpKTsKKyAgICAgICAgYm9vbCBpc1ZhbGlkU3RyaWN0 TW9kZSA9IChhZGRSZXN1bHQuaXNOZXdFbnRyeSB8fCAhYWRkUmVzdWx0Lml0ZXJhdG9yLT52YWx1 ZS5pc1BhcmFtZXRlcigpKQorICAgICAgICAgICAgJiYgbV92bS0+cHJvcGVydHlOYW1lcy0+ZXZh bCAhPSAqaWRlbnQgJiYgIWlzQXJndW1lbnRzSWRlbnQ7CiAgICAgICAgIGFkZFJlc3VsdC5pdGVy YXRvci0+dmFsdWUuY2xlYXJJc1ZhcigpOwogICAgICAgICBhZGRSZXN1bHQuaXRlcmF0b3ItPnZh bHVlLnNldElzUGFyYW1ldGVyKCk7Ci0gICAgICAgIGJvb2wgaXNWYWxpZFN0cmljdE1vZGUgPSBh ZGRSZXN1bHQuaXNOZXdFbnRyeSAmJiBtX3ZtLT5wcm9wZXJ0eU5hbWVzLT5ldmFsICE9ICppZGVu dCAmJiAhaXNBcmd1bWVudHNJZGVudDsKICAgICAgICAgbV9pc1ZhbGlkU3RyaWN0TW9kZSA9IG1f aXNWYWxpZFN0cmljdE1vZGUgJiYgaXNWYWxpZFN0cmljdE1vZGU7CiAgICAgICAgIG1fZGVjbGFy ZWRQYXJhbWV0ZXJzLmFkZChpZGVudC0+aW1wbCgpKTsKICAgICAgICAgaWYgKCFpc1ZhbGlkU3Ry aWN0TW9kZSkKSW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlv dXRUZXN0cy9DaGFuZ2VMb2cJKHJldmlzaW9uIDIwMTg2MykKKysrIExheW91dFRlc3RzL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDE2LTA2LTA5ICBNaWNoYWVs IFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAgICAgIEVTNjogUmV1c2luZyBmdW5j dGlvbiBuYW1lIGFzIGEgcGFyYW1ldGVyIG5hbWUgc2hvdWxkbid0IHRocm93IFN5bnRheCBFcnJv cgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU4NTc1 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkZWQg YSBuZXcgdGVzdCBjYXNlLgorCisgICAgICAgICoganMvYmFzaWMtc3RyaWN0LW1vZGUtZXhwZWN0 ZWQudHh0OgorICAgICAgICAqIGpzL3NjcmlwdC10ZXN0cy9iYXNpYy1zdHJpY3QtbW9kZS5qczoK KwogMjAxNi0wNi0wOSAgTWljaGFlbCBTYWJvZmYgIDxtc2Fib2ZmQGFwcGxlLmNvbT4KIAogICAg ICAgICBXZWJLaXRUZXN0UnVubmVyIGFuZCBEdW1wUmVuZGVyVHJlZSBkbyBub3QgaGFuZGxlIGRh bmdsaW5nIHN1cnJvZ2F0ZSBjaGFyYWN0ZXJzCkluZGV4OiBMYXlvdXRUZXN0cy9qcy9iYXNpYy1z dHJpY3QtbW9kZS1leHBlY3RlZC50eHQKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gTGF5b3V0VGVzdHMvanMvYmFz aWMtc3RyaWN0LW1vZGUtZXhwZWN0ZWQudHh0CShyZXZpc2lvbiAyMDE4NjIpCisrKyBMYXlvdXRU ZXN0cy9qcy9iYXNpYy1zdHJpY3QtbW9kZS1leHBlY3RlZC50eHQJKHdvcmtpbmcgY29weSkKQEAg LTY4LDYgKzY4LDggQEAgUEFTUyAoZnVuY3Rpb24gZihhcmcpeyd1c2Ugc3RyaWN0JzsgZi5hcgog UEFTUyAoZnVuY3Rpb24gZihhcmcpeyd1c2Ugc3RyaWN0JzsgZi5jYWxsZXI7IH0pKCkgdGhyZXcg ZXhjZXB0aW9uIFR5cGVFcnJvcjogJ2FyZ3VtZW50cycsICdjYWxsZWUnLCBhbmQgJ2NhbGxlcicg Y2Fubm90IGJlIGFjY2Vzc2VkIGluIHN0cmljdCBtb2RlLi4KIFBBU1MgKGZ1bmN0aW9uIGYoYXJn KXsndXNlIHN0cmljdCc7IGYuYXJndW1lbnRzPTU7IH0pKCkgdGhyZXcgZXhjZXB0aW9uIFR5cGVF cnJvcjogJ2FyZ3VtZW50cycsICdjYWxsZWUnLCBhbmQgJ2NhbGxlcicgY2Fubm90IGJlIGFjY2Vz c2VkIGluIHN0cmljdCBtb2RlLi4KIFBBU1MgKGZ1bmN0aW9uIGYoYXJnKXsndXNlIHN0cmljdCc7 IGYuY2FsbGVyPTU7IH0pKCkgdGhyZXcgZXhjZXB0aW9uIFR5cGVFcnJvcjogJ2FyZ3VtZW50cycs ICdjYWxsZWUnLCBhbmQgJ2NhbGxlcicgY2Fubm90IGJlIGFjY2Vzc2VkIGluIHN0cmljdCBtb2Rl Li4KK1BBU1MgKGZ1bmN0aW9uIGEoYSl7J3VzZSBzdHJpY3QnOyByZXR1cm4gYSsyOyB9KSg0MCkg aXMgNDIKK1BBU1MgdmFyIGZvbyA9IGZ1bmN0aW9uIGEoYSl7J3VzZSBzdHJpY3QnOyByZXR1cm4g YSsyOyB9OyBmb28oNDApIGlzIDQyCiBQQVNTICJjYWxsZXIiIGluIGZ1bmN0aW9uKCl7InVzZSBz dHJpY3QifSBpcyB0cnVlCiBQQVNTIChmdW5jdGlvbigpeyJ1c2Ugc3RyaWN0Ijt9KS5oYXNPd25Q cm9wZXJ0eSgiY2FsbGVyIikgaXMgdHJ1ZQogUEFTUyAiYXJndW1lbnRzIiBpbiBmdW5jdGlvbigp eyJ1c2Ugc3RyaWN0In0gaXMgdHJ1ZQpJbmRleDogTGF5b3V0VGVzdHMvanMvc2NyaXB0LXRlc3Rz L2Jhc2ljLXN0cmljdC1tb2RlLmpzCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2pzL3Njcmlw dC10ZXN0cy9iYXNpYy1zdHJpY3QtbW9kZS5qcwkocmV2aXNpb24gMjAxODYyKQorKysgTGF5b3V0 VGVzdHMvanMvc2NyaXB0LXRlc3RzL2Jhc2ljLXN0cmljdC1tb2RlLmpzCSh3b3JraW5nIGNvcHkp CkBAIC04NSw2ICs4NSw4IEBAIHNob3VsZFRocm93KCIoZnVuY3Rpb24gZihhcmcpeyd1c2Ugc3Ry aWMKIHNob3VsZFRocm93KCIoZnVuY3Rpb24gZihhcmcpeyd1c2Ugc3RyaWN0JzsgZi5jYWxsZXI7 IH0pKCkiKTsKIHNob3VsZFRocm93KCIoZnVuY3Rpb24gZihhcmcpeyd1c2Ugc3RyaWN0JzsgZi5h cmd1bWVudHM9NTsgfSkoKSIpOwogc2hvdWxkVGhyb3coIihmdW5jdGlvbiBmKGFyZyl7J3VzZSBz dHJpY3QnOyBmLmNhbGxlcj01OyB9KSgpIik7CitzaG91bGRCZSgiKGZ1bmN0aW9uIGEoYSl7J3Vz ZSBzdHJpY3QnOyByZXR1cm4gYSsyOyB9KSg0MCkiLCAiNDIiKTsKK3Nob3VsZEJlKCJ2YXIgZm9v ID0gZnVuY3Rpb24gYShhKXsndXNlIHN0cmljdCc7IHJldHVybiBhKzI7IH07IGZvbyg0MCkiLCAi NDIiKTsKIAogLy8gYXJndW1lbnRzL2NhbGxlciBwb2lzb25pbmcgc2hvdWxkIGJlIHZpc2libGUg YnV0IG5vdCB0aHJvdyB3aXRoICdpbicgJiAnaGFzT3duUHJvcGVydHknLgogc2hvdWxkQmVUcnVl KCciY2FsbGVyIiBpbiBmdW5jdGlvbigpeyJ1c2Ugc3RyaWN0In0nKTsK