455981 - "Assertion failure: entry->localKind == JSLOCAL_ARG" with destructuring argument

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

$ ./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.

Comment 1

[georgi - hopefully not receiving bugspam]

this asserts on 1.9 branch also

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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 ":")

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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).

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 6

[Igor Bukanov]

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.

Comment 7

[Igor Bukanov]

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.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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.

Comment 9

[Igor Bukanov]

Attachment: v1

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 10

[Igor Bukanov]

Comment on attachment 368553 [details] [diff] [review]
v1

Asking for a review after running the shell tests without any regressions.

Comment 11

[Brendan Eich [:brendan]]

(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

Comment 12

[Igor Bukanov]

(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.

Comment 13

[Igor Bukanov]

Attachment: v2

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 14

[Igor Bukanov]

Comment on attachment 368673 [details] [diff] [review]
v2

Asking for review as the patch passes shell tests.

Comment 15

[Brendan Eich [:brendan]]

(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 16

[Brendan Eich [:brendan]]

Comment on attachment 368673 [details] [diff] [review]
v2

Yow: upvar2 merge pain for me, but I like it. Thanks,

/be

Comment 17

[Igor Bukanov]

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

Comment 18

[Brendan Eich [:brendan]]

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

/be

Comment 19

[Igor Bukanov]

(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.

Comment 20

[Bob Clary [:bc] (inactive)]

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

Comment 21

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/eea29bb421ce

Comment 22

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/66491d2ac4dd

Comment 23

[Bob Clary [:bc] (inactive)]

Attachment: 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

Comment 24

[Bob Clary [:bc] (inactive)]

fails everywhere

Comment 25

[Bob Clary [:bc] (inactive)]

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

Comment 26

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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.

Comment 27

[Bob Clary [:bc] (inactive)]

/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