function statement and destructuring parameter name clash favours the parameter

VERIFIED FIXED

Status

()

--
minor
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

11 years ago
Consider the following test case:

function f(a) {
    function a() { }
    return a;
}

function g([a, b]) {
    function a() { }
    return a;
}

var type = typeof f(1);
if (type !== "function")
    throw "Unexpected type for simple parameter case: "+type;

var type = typeof g([1, 2]);
if (type !== "function")
    throw "Unexpected type for destructuring parameter case: "+type;

Currently when executed it throws an exception:

~/m/trunk/mozilla $ js ~/m/y.js
uncaught exception: Unexpected type for destructing parameter case: number

The reason for that is that the emitter places JSOP_DEFLOCALFUN bytecode that defines the function statement into the prologue section while destructing parameters goes to the main section.
(Assignee)

Comment 1

11 years ago
I will make a common patch for this bug and for bug 410400.
Blocks: 410400
(Assignee)

Comment 2

11 years ago
Created attachment 295251 [details] [diff] [review]
v1

To fix this bug the patch changes the code generation to place JSOP_DEFLOCALFUN bytecode to the main section of code. 

To ensure that the bytecode is executed before the rest of code the patch changes the emitter to process the top level block of a function with top-level function statements twice. The first time the emitter process the function statement nodes. The second time the rest of nodes are processed and the source notes for functions are inserted.

As alternative to this double-phase emitting I considered rearranging the nodes in Statements so the top-level function definitions would come first. But that requires to create additional nodes with source notes for the decompiler to preserve the relative position of function statements in the uneval output. I.e. simply moving function nodes to the beginning of the node list would change the decompiled source of 

function () {
    return g;
    function g() { }
}

into

function () {
    function g() { }
    return g;
}

In principle one can view that as a canonical presentation, but even if such transformation is desirable, it is for another bug.  

Since now with top-level let declarations JSOP_DEFLOCALFUN follows JSOP_ENTERBLOCK, the patch can remove the complex code to setup the scope chain temporary for that case in the interpreter.
Attachment #295251 - Flags: review?(brendan)
Comment on attachment 295251 [details] [diff] [review]
v1

We could even keep a separate list of function definitions to process, to avoid the pre-pass over the statement list. Not a big deal.

>+        fun = GET_FUNCTION_PRIVATE(cx, pn->pn_funpob->object);
>+        if (fun->u.i.script) {

A comment here at the start of the "then" clause, about why this case exists, would be helpful.

>         if (!(cg->treeContext.flags & TCF_IN_FUNCTION)) {
>             /*
>+             * Top-levels also need a prolog op to predefine their names in the
>+             * variable object, or if local, to fill their stack slots.
>+             */
>+            CG_SWITCH_TO_PROLOG(cg);
>+
>+            /*
>              * Emit JSOP_CLOSURE for eval code to do less checks when

Pre-existing comment, but please fix it to say "fewer" not "less" before "checks". Thanks.

>+        if (pn->pn_extra & PNX_FUNSTMT) {

Comment here too, just to match the TOK_FUNCTION: eary-out-on-fun-having-a-script case.

See later for renaming suggestion for PNX_FUNSTMT.

*Great* to get rid of all the complexity in the interpreter! But, what about the top-level case (not local functions, just functions and blocks)?

var f;
let b = 42;
function f() b;
print(f());

ECMA-262 10.1.3 specifies function bindings are instantiated before variable ones -- and variable ones do not replace pre-existing bindings. Your patch reverses this order. It shouldn't matter normally, as var f vs. function f is a rare naming collision. Even with such a collision, it shouldn't matter since function replaces any pre-existing binding, but could it result in js_CheckRedeclaration false reports? Not obviously. Still, this change bothers me and I thought I'd mention it.

>@@ -274,16 +274,17 @@ struct JSParseNode {
>     uint8               pn_op;
>     int8                pn_arity;
>     JSTokenPos          pn_pos;
>     ptrdiff_t           pn_offset;      /* first generated bytecode offset */
>     union {
>         struct {                        /* TOK_FUNCTION node */
>             JSParsedObjectBox *funpob;  /* function object */
>             JSParseNode *body;          /* TOK_LC list of statements */
>+            uint32      index;          /* emiter's index */

Fix "emiter" misspelling.

>+#define PNX_FUNSTMT    0x100            /* contains top-level function
>+                                           statements */

Suggest PNX_FUNCDEFS to match the srcnote and to be plural -- this flag says that the list contains function definitions (not statements -- those make JSOP_CLOSUREs). So the comment should s/statements/definitions/

/be
> eary-out-on-fun-having-a-script case.

s/eary/early/ - editing my own review-comment typos!

/be
(Assignee)

Comment 5

11 years ago
(In reply to comment #3)
> (From update of attachment 295251 [details] [diff] [review])
> We could even keep a separate list of function definitions to process, to avoid
> the pre-pass over the statement list. Not a big deal.

But that would still require to add a special nodes with source notes to preserve the positions of the function statements in the decompiled output. And I am not sure that the extra code and memory allocations would not to be too costly to justify avoidance of double-passing of nodes. Or do you mean that it is acceptable that 

function () {
    return g;
    function g() { }
}

would be decompiled as

function () {
    function g() { }
    return g;
}

?

> what about
> the top-level case (not local functions, just functions and blocks)?

This is not applicable here. The patch changes only relative placement of JSOP_DEFLOCALFUN. The bytecode is only emitted for function bodies and functions do not use JSOP_DEFVAR. 
(Assignee)

Comment 6

11 years ago
Created attachment 295354 [details] [diff] [review]
v2

The new version adds comments and renames PNX_FUNSTMT into PNX_FUNCDEFS.
Attachment #295251 - Attachment is obsolete: true
Attachment #295354 - Flags: review?(brendan)
Attachment #295251 - Flags: review?(brendan)
(Assignee)

Comment 7

11 years ago
Created attachment 295356 [details] [diff] [review]
v2b

This is v2 with better comment text.
Attachment #295354 - Attachment is obsolete: true
Attachment #295356 - Flags: review?
Attachment #295354 - Flags: review?(brendan)
(Assignee)

Comment 8

11 years ago
Created attachment 295357 [details] [diff] [review]
v1-v2b delta
(Assignee)

Updated

11 years ago
Attachment #295356 - Flags: review? → review?(brendan)
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 295251 [details] [diff] [review] [details])
> > We could even keep a separate list of function definitions to process, to avoid
> > the pre-pass over the statement list. Not a big deal.
> 
> But that would still require to add a special nodes with source notes to
> preserve the positions of the function statements in the decompiled output.

Yes, the AST would have nodes in situ for the function definitions, and a separate list for pre-processing -- Narcissus does this.

> And
> I am not sure that the extra code and memory allocations would not to be too
> costly to justify avoidance of double-passing of nodes.

I agree for now, no worries -- I was trying to voice that with "Not a big deal" ;-).

> Or do you mean that it is acceptable that 
> 
> function () {
>     return g;
>     function g() { }
> }
> 
> would be decompiled as
> 
> function () {
>     function g() { }
>     return g;
> }
> 
> ?

No, that's not good.

> > what about
> > the top-level case (not local functions, just functions and blocks)?
> 
> This is not applicable here. The patch changes only relative placement of
> JSOP_DEFLOCALFUN. The bytecode is only emitted for function bodies and
> functions do not use JSOP_DEFVAR. 

Yeah, that's what I thought. It still made my brain hurt a bit, because 10.1.3 in ES1-3 is so picky about vars after funcs.

Reviewing now.

/be
Comment on attachment 295356 [details] [diff] [review]
v2b


>+        fun = GET_FUNCTION_PRIVATE(cx, pn->pn_funpob->object);
>+        if (fun->u.i.script) {
>+            /*
>+             * This is the second pass to emit JSOP_NOP with a source note for
>+             * the already emitted function. See comments in the TOK_LC case.

Nit: "already-emitted".

>+         * For a script we emit the code during as we parse. Thus the bytecode

Delete "during".

>+         * for top-level functions should go to the prolog to predefine their

s/go to/go in/

>+         * names in the variable object before the already generated code is

"already-generated main code"

>+         * executed. This is not necessary when we emit the code for a

Could use "This extra work for top-level scripts is not necessary..."

>+         * function. It is fully parsed prior invocation of the emitter and

"prior to"?

>+         * calls to js_EmitTree for function statements can be scheduled

"function definitions" (I think; see below and try to use "function statement" *only* for the ECMAScript extension we support of a function not directly parented by top-level script or function body -- those we call "function defintions").

>         js_PushStatement(&cg->treeContext, &stmtInfo, STMT_BLOCK, top);
>+        if (pn->pn_extra & PNX_FUNCDEFS) {
>+            /*
>+             * The block contains the top-level function statements. To ensure

"This block contains top-level function definitions."

>+             * that we emit the bytecode defining them prior the rest of code
>+             * in the block we use a separated pass over functions. During the

Could just say "separate" here.

>+             * main pass later the emitter will add JSOP_NOP with source notes
>+             * for the function to preserve the original function position

"for the functions ... the original functions' positions"

>+             * when decompiling.
>+             *
>+             * Currently this is used only for functions as compile-as-we go

Comma before "as"?

>+             * mode for scripts does not allow separated emitter passes.

"separate" again.

>+        if (pn2->pn_type == TOK_FUNCTION) {
>+            /*
>+             * PNX_FUNCDEFS notifies the emitter that the block contains top-
>+             * level function statements that should be processed before the

"function definitions"

>+             * rest of nodes.
>+             *
>+             * TCF_HAS_FUNCTION_STMT is for the TOK_LC case in Statement. It
>+             * is relevant only for non-top function statements.

Could say "for function definitions not at top-level, which we call function statements."

>+             */
>+            if (AT_TOP_LEVEL(tc))
>+                pn->pn_extra |= PNX_FUNCDEFS;
>+            else
>+                tc->flags |= TCF_HAS_FUNCTION_STMT;

Here the PNX_CYBERCRUD naming convention clashes with the roomier TCF_MY_SUMMER_VACATION style ;-). Oh well, fix later or never.

r/a=me with nits picked, thanks.

/be
Attachment #295356 - Flags: review?(brendan)
Attachment #295356 - Flags: review+
Attachment #295356 - Flags: approval1.9+
(Assignee)

Comment 11

11 years ago
Created attachment 295776 [details] [diff] [review]
v3

The new version addresses the nits from comment 10 and updates XDR version. The latter is necessary due to changes in the semantic of JSOP_DEFLOCALFUN.
Attachment #295356 - Attachment is obsolete: true
Attachment #295357 - Attachment is obsolete: true
Attachment #295776 - Flags: review?(brendan)
(Assignee)

Comment 12

11 years ago
Comment on attachment 295776 [details] [diff] [review]
v3

Note that bugzilla's inter-diff correctly shows the difference between v2b and v3
(Assignee)

Comment 13

11 years ago
Created attachment 297028 [details] [diff] [review]
v4

This is v3 synchronized with the CVS trunk.
Attachment #295776 - Attachment is obsolete: true
Attachment #297028 - Flags: review?(brendan)
Attachment #295776 - Flags: review?(brendan)
Bugzilla interdiff fails now -- can you put up a real interdiff? Thanks,

/be
(Assignee)

Comment 15

11 years ago
(In reply to comment #14)
> Bugzilla interdiff fails now -- can you put up a real interdiff? Thanks,

There is no differences between v4 and v3 besides a sync with the trunk caused by changes from bug 411630 and the inter-diff between v3 an v2 works:

https://bugzilla.mozilla.org/attachment.cgi?oldid=295356&action=interdiff&newid=295776&headers=1
(Assignee)

Comment 16

11 years ago
Created attachment 297136 [details]
v3-v4 plain diff

This is a plain diff to see the differences caused by the merge with the trunk.
Comment on attachment 297028 [details] [diff] [review]
v4

>+            /*
>+             * This is the second pass to emit JSOP_NOP with a source note for

Final nit: comma after "second pass", to disambiguate second pass of two that both emit JSOP_NOP (obviously not the case but grammatically denoted) from second pass, which emits JSOP_NOP. Could use "... second pass, which emits JSOP_NOP ..." instead, the non-defining clause connected by which helps. Or change the lead-in to "This second pass is needed to emit JSOP_NOP with a source note ...".

/be
Attachment #297028 - Flags: review?(brendan)
Attachment #297028 - Flags: review+
Attachment #297028 - Flags: approval1.9+
(Assignee)

Comment 18

11 years ago
Created attachment 298042 [details] [diff] [review]
v5

This is a version ready for check in with the last nits addressed. Here is the delta from v4:

--- /home/igor/m/ff/mozilla/quilt.p14759/js/src/jsemit.c	2008-01-20 01:56:21.000000000 +0100
+++ js/src/jsemit.c	2008-01-20 01:55:23.000000000 +0100
@@ -3981,18 +3981,19 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
                 return JS_FALSE;
             break;
         }
 #endif
 
         fun = GET_FUNCTION_PRIVATE(cx, pn->pn_funpob->object);
         if (fun->u.i.script) {
             /*
-             * This is the second pass to emit JSOP_NOP with a source note for
-             * the already-emitted function. See comments in the TOK_LC case.
+             * This second pass is needed to emit JSOP_NOP with a source note
+             * for the already-emitted function. See comments in the TOK_LC
+             * case.
              */
             JS_ASSERT(pn->pn_op == JSOP_NOP);
             JS_ASSERT(cg->treeContext.flags & TCF_IN_FUNCTION);
             JS_ASSERT(pn->pn_index != (uint32) -1);
             if (!EmitFunctionDefNop(cx, cg, pn->pn_index))
                 return JS_FALSE;
             break;
         }
Attachment #297028 - Attachment is obsolete: true
Attachment #297136 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #298042 - Flags: review+
Attachment #298042 - Flags: approval1.9+
(Assignee)

Comment 19

11 years ago
I checked in the patch from comment 18 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%252Fcvsroot&date=explicit&mindate=1200825149&maxdate=1200825278&who=igor%25mir2.org
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Comment on attachment 298042 [details] [diff] [review]
v5

Index: js/src/jsparse.c
> static JSParseNode *
> Statements(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc)
> {
...
>+    JS_ASSERT(CURRENT_TOKEN(ts).type == TOK_LC);

I'm hitting this assertion in the JS shell when I attempt to evaluate any statement. I don't think it's valid when called from js_ParseScript (which, unlike js_CompileScript does not inline Statements).
(Assignee)

Updated

11 years ago
Depends on: 413241

Comment 21

11 years ago
Created attachment 304866 [details]
js1_7/regress/regress-410649.js

Igor, this fails for the destructuring parameter case. Is this bug really fixed?

Comment 22

11 years ago
Created attachment 304867 [details]
js1_7/decompilation/regress-410649.js
(Assignee)

Comment 23

11 years ago
(In reply to comment #22)
> Created an attachment (id=304867) [details]
> js1_7/decompilation/regress-410649.js
> 

This is indeed has not been fixed. I will file a regression bug about it.
(Assignee)

Updated

11 years ago
Blocks: 419662
(Assignee)

Updated

11 years ago
No longer blocks: 419662
Depends on: 419662

Comment 24

11 years ago
/cvsroot/mozilla/js/tests/public-failures.txt,v  <--  public-failures.txt
new revision: 1.44; previous revision: 1.43

/cvsroot/mozilla/js/tests/js1_7/regress/regress-410649.js,v  <--  regress-410649.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_7/decompilation/regress-410649.js,v  <--  regress-410649.js
initial revision: 1.1
Flags: in-testsuite+
Flags: in-litmus-
(Assignee)

Updated

11 years ago
Depends on: 428424

Comment 25

11 years ago
v 1.9.0 js1_7/decompilation/regress-410649.js only.

js1_7/regress/regress-410649.js -> bug 419662
Status: RESOLVED → VERIFIED

Updated

10 years ago
Depends on: 416737
You need to log in before you can comment on or make changes to this bug.