496134 - Function in destructuring assignment can't see up to other variables in the assignment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Ed Lee :Mardak]

function test_js() {
  let [A, B] = [function() B, 123];
  print(A);
  print(B);
  print(A());
}

That gives an output of:
function () B
123
An exception occurred: ReferenceError: B is not defined

However.. I can't reproduce this outside of the jsmodules test harness.
I have export XULRUNNER_BIN=/Applications/Shiretoko.app/Contents/MacOS/firefox

And if I run that code from Shiretoko directly, A() gets 123.

Myk: Does the jsmodules test harness do anything that might cause this?

One work around is to declare the variables beforehand:
let A, B;
[A, B] = [function() B, 123];

But then I'm not sure if this is a bug or not. Each part in the destructured assignment isn't supposed to see the other definition, so let [A, B] = [1, A] definitly wouldn't work because A isn't defined, but I'm not sure what should happen with functions.

Comment 1

[Ed Lee :Mardak]

Something even more odd...

function test_js() {
  let [A, B] = [function() [typeof A, typeof B, typeof C], 123];
  let C = {};
  print(A());
} 

output: function,number,object

function test_js() {
  let [A, B] = [function() [typeof A, typeof B], 123];
  let C = {};
  print(A());
} 

output: undefined,undefined

Comment 2

[Ed Lee :Mardak]

Oh, it seems like the "works in browser" wasn't actually correct. I was running the code in the top level. When I put it in a function, I get the same undefined issue.

So there's still the oddness where accessing something defined outside of the destructured assignment causes things in the assignment to be visible.

Comment 3

[Robert Sayre]

don't think we can block on this non-Web issue

Comment 4

[Ed Lee :Mardak]

Okay:
<script>(function() { var A = function() typeof A; alert(A()); })()</script>
result: function

Fail:
<script>(function() { var [A] = [function() typeof A]; alert(A()); })()</script>
result: undefined

Is that considered a web issue?

Comment 5

[Ed Lee :Mardak]

The data uri starts alerting undefined on build:
2009-04-18-03-mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=37d1bb0c86b5&tochange=7b39d6b3b56d

Likely Brendan Eich — upvar2, aka the big one take 2 (452498, r=mrbkap).  ?

At least not many people use destructured assignment on the web ;)

Comment 6

[Ed Lee :Mardak]

2009-04-04-03-tracemonkey

http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=f6407cd260ca&tochange=022c722d3b89

Comment 7

[Brendan Eich [:brendan]]

Attachment: fix

Brute force fix, based on rules of the analysis followed for non-destructuring var bindings. Shell test next.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: js shell testcase

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 381868 [details] [diff] [review]
fix

Please move UndominateInitializers near CheckDestructuring (and comment there) so that the two functions don't diverge in bad ways!

Comment 10

[Brendan Eich [:brendan]]

Attachment: fix, v2

I seriously tried to extend CheckDestructuring to have an "undominate" mode, but it already does too much. Better to duplicate. A "destructuring pattern visitor" helper can be done later to common the recursive [a,b,c] and {p:x,q:y,r:z} tree walking code -- later.

/be

Comment 11

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/2e6025415fb3

/be

Comment 12

[Boris Zbarsky [:bzbarsky]]

This seems to have caused bug 496682.

Comment 13

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

(In reply to comment #12)
> This seems to have caused bug 496682.

and it's causing widespread failures (crashes) on jsfunfuzz machines, making them provide virtually unusable useful information. Possibility of a backout?

Comment 14

[Robert Sayre]

yah, let's back it out

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK, other than the missing null-check that bug 496682 was initially filed on, this also causes the assert described in bug 496682 comment 4:

js> let {}={y:[],0}
Assertion failure: right->pn_arity != PN_LIST || !(right->pn_xflags &
PNX_DESTRUCT), at ../jsparse.cpp:3811

In this case, pn_arity is PN_LIST, and PNX_DESTRUCT is in fact set.

Comment 16

[Robert Sayre]

backout:

http://hg.mozilla.org/tracemonkey/rev/fb64ba1cb3ad
http://hg.mozilla.org/mozilla-central/rev/fb64ba1cb3ad

Comment 17

[Brendan Eich [:brendan]]

Sorry about that -- insufficient testsuite coverage. I will fix ASAP.

/be

Comment 18

[Brendan Eich [:brendan]]

Attachment: fix, v3

Best not to hack while packing for a move.

This patch changes the assertion that was botching into the needed error test, and avoids calling UndominateInitializers on a null right subtree. This shows the problem is a bit different from CheckDestructuring, so we do need two functions.

/be

Comment 19

[Brendan Eich [:brendan]]

Attachment: extended js shell testcase

Does not include fuzz-generated tests.

/be

Comment 20

[Brendan Eich [:brendan]]

Attachment: fix, v3a

Last patch showed unwarranted mistrust of FindPropertyValue, or just confusion over the point of UndominateInitializers. If there's no rhs for a given property initialiser in an object initialiser (yes, I'm using Ecma spelling now), then no need to extend pn->pn_pos.end.

/be

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #18)
> This patch changes the assertion that was botching into the needed error test,
> and avoids calling UndominateInitializers on a null right subtree. This shows
> the problem is a bit different from CheckDestructuring, so we do need two
> functions.

"need" is too strong -- of course CheckDestructuring could be extended, but again that seems a bridge too far. A generic visitor template could still help, maybe.

/be

Comment 22

[Brendan Eich [:brendan]]

Gary, can you apply the latest patch to a tm tip build and fuzz it while the patch awaits review? Thanks,

/be

Comment 23

[Brendan Eich [:brendan]]

Attachment: fix, v3b

Bail if right is not same type as left (both object or both array initialisers). In such a case any closure on the right can't be the initialiser of a binding on the left -- it would be a value to be destructued, so not retained as a function reference that could be called.

/be

Comment 24

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

(In reply to comment #23)
> Created an attachment (id=381973) [details]
> fix, v3b
> 
> Bail if right is not same type as left (both object or both array
> initialisers). In such a case any closure on the right can't be the initialiser
> of a binding on the left -- it would be a value to be destructued, so not
> retained as a function reference that could be called.
> 
> /be

Widespread jsfunfuzz failures with this patch. These 3 testcases (and more):

for(var{}=

for(let[]=

for(const[]=

crash TM opt js build without -j at UndominateInitializers and assert debug js build without -j at Assertion failure: right, at ../jsparse.cpp:3808

Comment 25

[Brendan Eich [:brendan]]

Attachment: fix, v3c

Dumb failure to null-defend at first call into UndominateInitializers (from Variables, when the line-at-a-time buffering fails to get a compilable unit for want of an initializer).

Gary, full speed ahead with this applied!

/be

Comment 26

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

(In reply to comment #25)
> Created an attachment (id=382042) [details]
> fix, v3c
> 
> Dumb failure to null-defend at first call into UndominateInitializers (from
> Variables, when the line-at-a-time buffering fails to get a compilable unit for
> want of an initializer).
> 
> Gary, full speed ahead with this applied!
> 
> /be

let({x}={_:0}

Assertion failure: (pnid)->pn_arity == PN_NULLARY && ((pnid)->pn_type == TOK_NUMBER || (pnid)->pn_type == TOK_STRING || (pnid)->pn_type == TOK_NAME), at ../jsparse.cpp:3569

Comment 27

[Brendan Eich [:brendan]]

Attachment: v4: fix stale assertion

Gary: that was just a bogus assertion, so if you could patch around or else run the fuzzer using optimized builds, that would help smoke out anything left.

That old assertion predates destructuring shorthands, which use PN_NAME arity. I could try to change this name node's arity to PN_NULLARY, but at some risk. Right now it's better to extend the old assertion. We should clean this up along with destructuring shorthand tree format in bug 492996. I'll comment there.

/be

Comment 28

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

(In reply to comment #27)
> Created an attachment (id=382072) [details]
> v4: fix stale assertion

This seems to be ok after a couple of hours' worth of fuzzing.

Comment 29

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/1965e3b7a406

/be

Comment 30

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1965e3b7a406

Comment 31

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a2de8442a6f3