$ ./js js> (function ({a, b, c, d, e, f, g, h, q}, q) { }) Assertion failure: entry->localKind == JSLOCAL_ARG, at jsfun.cpp:2349 Opt does fine.

this asserts on 1.9 branch also

From bug 433411: ================ (function ({a: {b: bb, c: cc, d: dd}, m: [x, n, o, p]}, x) {}) Assertion failure: entry->localKind == JSLOCAL_ARG, at jsfun.c:2296 In opt, "cc" gets replaced by "x" in the decompiled function: js> (function ({a: {b: bb, c: cc, d: dd}, m: [x, n, o, p]}, x) {}) function ({a: {b: bb, c: x, d: dd}, m: [x, n, o, p]}, x) { } Also, the testcase in comment #0, when entered into js TM tip shell, shows: $ ./js-opt-tm-intelmac js> (function ({a, b, c, d, e, f, g, h, q}, q) { }) function ({a, b, c, d, e, f, g: q, h, q}, q) { } js> (note the extra ":")

This should be a regression of bug 404734. Seems to work as expected with cvs js shell checkout at 2008-01-29 22:26 PST Asserts with cvs js shell checkout at 2008-01-29 22:28 PST http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-01-29+22%3A26%3A00&maxdate=2008-01-29+22%3A28%3A00&cvsroot=%2Fcvsroot Bonsai message: Final js1.8 feature: sugar for object destructuring (404734, r=mrbkap).

Don't think this blocks, renom with reasons if you disagree, but it's something that only we support so it isn't common on the web.

The assert from the comment 0 reflects the assumption that only argument, not variable, names can be duplicated. It allows for simpler code when recording duplicates for use during decompilation. Without this assumption HashLocalName from jsfun.cpp would need to record in JSNameIndexPair not only the index of the duplicate but also a flag indicating whether it was variable or argument that was duplicated. Now, the assumption is wrong as in function f({a, b, c, d, e, f, g, h, q}, q) { } the first usage of q is recorded internally as a variable. So when the parser discovers the second q, it will add it as an argument duplicating the variable. The consequence of this bug is that with asserts disabled js_GetLocalNameArray from jsfun.cpp will return wrong names. The function would put a duplicated destructing name at position variableIndex, not at fun->argc + variableIndex, leaving the latter element with NULL pointer. js_GetLocalNameArray is used in 3 places besides the debug-only usage in jstracer.cpp: call_enumerate, fun_xdrObject from jsfun.cpp and the decompiler. The first 2 methods are not exposed to scripts while the worst that can happen in the decompiler is returning of the wrong source. In particular, the decompiler would never see the NULL in place of duplicated variable name. The decompiler only loops through argument names and only access the variable name when some bytecode refers to it and bytecode never refers to index of duplicates. That is, the examples from the comment 0 and comment 3 is the only consequences that a script from a web page can trigger. To fix this bug there are two simple ways: 1. Add the missing flag to JSNameIndexPair to indicate whether it was argument or variable that was duplicated. This way js_GetLocalNameArray would return the proper result. 2. Ban duplicated arguments when the function contains any destructing argument pattern. My preference is the option 2.

Gary Kwong wrote in comment #4 : > This should be a regression of bug 404734. > > Seems to work as expected with cvs js shell checkout at 2008-01-29 22:26 PST > Asserts with cvs js shell checkout at 2008-01-29 22:28 PST Hm, what does it mean "work as expected" ? Prior the bug 404734 the parser should reject function({a, b, c, d, e, f, g, h, q}, q) as invalid destructing pattern. Also, even prior the bug 404734 something like (function([a, b, c, d, e, f, g, h, q], q) {}) should trigger the same assert violation.

(In reply to comment #7) > Hm, what does it mean "work as expected" ? Sorry, I meant with the testcase in comment #0, the assertion doesn't show.

The patch explicitly bans duplicated argument names whenever duplicated argument pattern is used. With the patch I have: js> function f(a, [a]) {} typein:1: TypeError: duplicate formal argument a: typein:1: function f(a, [a]) {} typein:1: ...............^ js> function f([a], a) {} typein:2: TypeError: duplicate formal argument a: typein:2: function f([a], a) {} typein:2: ................^ js> function f(a, a, [b]) {} typein:3: TypeError: duplicate formal argument a: typein:3: function f(a, a, [b]) {} typein:3: .................^ In the third case the reported position is not the first dup but rather the first occurrence of the destructuring pattern. Also the code would report a warning for the first dup, which is not shown in this shell session. It is possible to improve this, but it would require either to add a new error message (like using destructuring pattern after duplicated parameter name) or to somehow support delayed reporting of dup warnings.

Comment on attachment 368553 [details] [diff] [review] v1 Asking for a review after running the shell tests without any regressions.

(In reply to comment #9) > js> function f(a, a, [b]) {} > typein:3: TypeError: duplicate formal argument a: > typein:3: function f(a, a, [b]) {} > typein:3: .................^ > > In the third case the reported position is not the first dup but rather the > first occurrence of the destructuring pattern. Also the code would report a > warning for the first dup, which is not shown in this shell session. > > It is possible to improve this, but it would require either to add a new error > message (like using destructuring pattern after duplicated parameter name) or > to somehow support delayed reporting of dup warnings. Just remove TS(tc->parseContext) from the js_ReportCompileErrorNumber argument list and all should work as expected. /be

(In reply to comment #11) > Just remove TS(tc->parseContext) from the js_ReportCompileErrorNumber argument > list and all should work as expected. That cannot work as js_ReportCompileErrorNumber dereferences the parseContext argument when the parse node is null.

The new version uses a new error message. It makes very explicit the reason for rejection of duplicated arguments and the destructuring pattern that comes after a duplicated argument generates very reasonable error message (the last case below): @watson-32~/m/tm/js/tests> ~/build/js32.tm.dbg/js -s js> function f([a], a){} typein:1: SyntaxError: duplicate argument is mixed with destructuring pattern: typein:1: function f([a], a){} typein:1: ................^ js> function f([b], a, a){} typein:2: SyntaxError: duplicate argument is mixed with destructuring pattern: typein:2: function f([b], a, a){} typein:2: ...................^ js> function f(a, a, [b]){} typein:3: strict warning: duplicate formal argument a: typein:3: strict warning: function f(a, a, [b]){} typein:3: strict warning: ..............^ typein:3: SyntaxError: duplicate argument is mixed with destructuring pattern: typein:3: function f(a, a, [b]){} typein:3: .................^

Comment on attachment 368673 [details] [diff] [review] v2 Asking for review as the patch passes shell tests.

(In reply to comment #12) > (In reply to comment #11) > > Just remove TS(tc->parseContext) from the js_ReportCompileErrorNumber argument > > list and all should work as expected. > > That cannot work as js_ReportCompileErrorNumber dereferences the parseContext > argument when the parse node is null. I was thinking you could pass the exact pn (data->pn) to blame into BindDestructuringArg. But a new error is better still. /be

Comment on attachment 368673 [details] [diff] [review] v2 Yow: upvar2 merge pain for me, but I like it. Thanks, /be

landed to TM - http://hg.mozilla.org/tracemonkey/rev/eea29bb421ce

The test js1_8_1/regress/regress-452498-114.js needs to be updated. /be

(In reply to comment #18) > The test js1_8_1/regress/regress-452498-114.js needs to be updated. The test contains function y([{x: x, x}]){} which now generates a syntax error. Probably dropping the second x or replacing it with y or something can still leave some test coverage.

test modified: http://hg.mozilla.org/tracemonkey/rev/4d62203ea817

js1_8/regress/regress-455981-01.js js1_8/regress/regress-455981-02.js Assertion failure: entry->localKind == JSLOCAL_ARG, at ../jsfun.cpp:2431

fails everywhere

js1_8/regress/regress-455981-01.js js1_8/regress/regress-455981-02.js

(In reply to comment #23) > Created an attachment (id=378361) [details] > tests > > js1_8/regress/regress-455981-01.js > js1_8/regress/regress-455981-02.js > > Assertion failure: entry->localKind == JSLOCAL_ARG, at ../jsfun.cpp:2431 (In reply to comment #24) > fails everywhere This later assertion failure (likely) re-occurred after upvar2 landed. As this is now covered by bug 499524, I'd think that this bug should be re-resolved fixed and the attention shifted to that bug. Please reopen if anyone disagrees, with reasons why.

/cvsroot/mozilla/js/tests/js1_8/regress/regress-455981-01.js,v <-- regress-455981-01.js initial revision: 1.1 /cvsroot/mozilla/js/tests/js1_8/regress/regress-455981-02.js,v <-- regress-455981-02.js initial revision: 1.1