514580 - ES5 strict mode: strict mode functions may not repeat parameter names

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

From ES5 Annex C:

A strict mode function may not have two or more formal parameters that have the same name. An attempt to create such a function using a FunctionDeclaration, FunctionExpression, or Function constructor is a SyntaxError (13.1, 15.3.2).

Comment 1

[Jim Blandy :jimb]

Attachment: Forbid duplicate formal parameter names in strict mode code.

Note: this patch changes the JSOPTION_STRICT warning from a TypeError
into a SyntaxError, if JSOPTION_WERROR is also set.

Comment 2

[Jim Blandy :jimb]

Attachment: Forbid duplicate formal parameter names in strict mode code.

Revised, with tests.  Rob said he wasn't the best reviewer for this; Blake, you were recommended.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 412435 [details] [diff] [review]
Forbid duplicate formal parameter names in strict mode code.

A bunch of nits here:

>+++ b/js/src/jsfun.cpp
>+extern JSAtom *
>+js_FindDuplicateFormal(JSFunction *fun) {

Nit: First { of a function goes on its own line.

>+    unsigned nargs = fun->nargs;
>+    if (nargs <= 1) {
>+        return NULL;
>+    } else if (nargs <= MAX_ARRAY_LOCALS) {

Nit: No else-after-return. This allows you to de-indent both this block and the else block.

>+        jsuword *array = fun->u.i.names.array;
>+        unsigned i, j;

Any reason not to declare these as for-scoped?

>+        // Quadratic, but MAX_ARRAY_LOCALS is 8, so at most 28 comparisons.
>+        for (i = 0; i < nargs; i++)
>+            for (j = i + 1; j < nargs; j++)

Because both of these |for|s span multiple lines, they want braces ({}). The |if| is ok, though.

>+        JSNameIndexPair *dup = fun->u.i.names.map->lastdup;
>+        if (dup)
>+            return dup->name;
>+        else
>+            return NULL;

I think that in other places, we've written stuff like this as:

  return dup ? dup->name : NULL;

>+++ b/js/src/jsparse.cpp
>+static bool
>+CheckStrictFormals(JSContext *cx, JSTreeContext *tc, JSFunction *fun,
>+                   JSParseNode *pn) {

Ibid.

>+    if (!tc->needStrictChecks())
>+        return true;

Ultra nit: blank line here, please.

>+    JSAtom *dup = js_FindDuplicateFormal(fun);
>+    if (dup) {

My personal preference would be to invert this and early-return.

>+        JSAtomListElement *ale = tc->decls.lookup(dup);
>+        JS_ASSERT(ale);

Normally, in SpiderMonkey, we don't assert non-nullness and let the corresponding near-null crash tell us what happened. This would also allow you to nuke the otherwise one-use |ale| variable.

>+        JSDefinition *dn = ALE_DEFN(ale);
>+        if (dn->pn_op == JSOP_GETARG)
>+            // The definition's source position will be more precise.
>+            pn = dn;

Multi-line consequent wants braces.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 412435 [details] [diff] [review]
Forbid duplicate formal parameter names in strict mode code.

>+            !js_ReportStrictModeError(cx, TS(tc->compiler), tc, pn,
>+                                     JSMSG_DUPLICATE_FORMAL, name)) {

Also, the second line here is underindented by one space.

Comment 5

[Jim Blandy :jimb]

(In reply to comment #3)
> >+++ b/js/src/jsfun.cpp
> >+extern JSAtom *
> >+js_FindDuplicateFormal(JSFunction *fun) {
> 
> Nit: First { of a function goes on its own line.

D'oh.  Creeping Google Breakpad style.

> >+    unsigned nargs = fun->nargs;
> >+    if (nargs <= 1) {
> >+        return NULL;
> >+    } else if (nargs <= MAX_ARRAY_LOCALS) {
> 
> Nit: No else-after-return. This allows you to de-indent both this block and the
> else block.

I've made this change.  However, I think there's some merit to the way I had it: fun->u.i.names is a union with three alternatives; each was handled by one branch of the 'if' chain.  So the shape of the code reflected the shape of type.

> >+        jsuword *array = fun->u.i.names.array;
> >+        unsigned i, j;
> 
> Any reason not to declare these as for-scoped?

Done.

> >+        // Quadratic, but MAX_ARRAY_LOCALS is 8, so at most 28 comparisons.
> >+        for (i = 0; i < nargs; i++)
> >+            for (j = i + 1; j < nargs; j++)
> 
> Because both of these |for|s span multiple lines, they want braces ({}). The
> |if| is ok, though.

Done.

> >+        JSNameIndexPair *dup = fun->u.i.names.map->lastdup;
> >+        if (dup)
> >+            return dup->name;
> >+        else
> >+            return NULL;
> 
> I think that in other places, we've written stuff like this as:
> 
>   return dup ? dup->name : NULL;

Much better.  Done.

> >+    if (!tc->needStrictChecks())
> >+        return true;
> 
> Ultra nit: blank line here, please.

I prefer it with the blank line as well.

> >+    JSAtom *dup = js_FindDuplicateFormal(fun);
> >+    if (dup) {
> 
> My personal preference would be to invert this and early-return.

I made that change, but discovered that a later patch is going to do more tests on the function's parameters ('eval' and 'arguments') if there are no duplicates, so we need to have the duplicate error case off the main line of the function. I resolved the rejection by having the later patch put the function back the way it was. :)

> >+        JSAtomListElement *ale = tc->decls.lookup(dup);
> >+        JS_ASSERT(ale);
> 
> Normally, in SpiderMonkey, we don't assert non-nullness and let the
> corresponding near-null crash tell us what happened. This would also allow you
> to nuke the otherwise one-use |ale| variable.

Done.

> >+        JSDefinition *dn = ALE_DEFN(ale);
> >+        if (dn->pn_op == JSOP_GETARG)
> >+            // The definition's source position will be more precise.
> >+            pn = dn;
> 
> Multi-line consequent wants braces.

I moved the comment to a better place, and it became a single line.

(In reply to comment #4)
> (From update of attachment 412435 [details] [diff] [review])
> >+            !js_ReportStrictModeError(cx, TS(tc->compiler), tc, pn,
> >+                                     JSMSG_DUPLICATE_FORMAL, name)) {
> 
> Also, the second line here is underindented by one space.

I had a bunch of these sorts of mistakes, and I don't know where they came from.

Comment 6

[Brendan Eich [:brendan]]

else-after-return gets a mini-sermon in the Effective Go doc too. If only C (and C++, and Go) were an expression language. But union arms and control flow are different enuf that they don't resemble each other superficially or deeply. Once the else-after-return genie is out, it won't just be discriminated union cracking where it runs rampant.

I do like Go's switch, though.

/be

Comment 7

[Jim Blandy :jimb]

http://hg.mozilla.org/tracemonkey/rev/ab6de82f30d1

Comment 8

[Jim Blandy :jimb]

(In reply to comment #6)
> But union arms and control flow
> are different enuf that they don't resemble each other superficially or deeply.

When one is doing a formal proof, if one has a proposition of the form 'a or b', then one proves '(a or b) implies c' by showing 'a implies c' and 'b implies c'.  Then an axiom gives you '(a or b) implies c'.

When working in SML-like languages, you do the same thing to produce a c from an algebraic type with a and b alternatives; this is the code analogue of the proof maneuver, under the Curry-Howard isomorphism.

The C code in hand is superficially imperative, but what's being computed is quite functional.

So I think they do resemble each other, deeply.

Comment 9

[Jim Blandy :jimb]

When eliminating error cases, else-after-return is clearly not the right thing.

Comment 10

[Brendan Eich [:brendan]]

SML != C, what else is there to say? C (and B before it if memory serves) broke from BCPL's expression language and that was the primal sin. I'm still sad.

I see your distinction, and roc has (I think) argued for a similar exception, but it is lost on the bulk of C and C++ hackers.

Or at least, we've pushed this point into a style rule because we used to have too much random control flow (else after a jump, if(A)B;else return C;D where B;D want to be together, etc. -- only some of this was eliminating error cases).

/be

Comment 11

[Brendan Eich [:brendan]]

Trying to diagnose orangeness I seem to have caused on TraceMonkey-Unittest, but hitting a bug in the patch landed here:

+extern JSAtom *
+js_FindDuplicateFormal(JSFunction *fun) {
+    unsigned nargs = fun->nargs;
+    if (nargs <= 1) {
+        return NULL;
+    } else if (nargs <= MAX_ARRAY_LOCALS) {
+        jsuword *array = fun->u.i.names.array;
+

the fun->u.i.names union holds atoms for all locals, not just args. The function synthesizeMouse at line 199 of mochitest/tests/SimpleTest/EventUtils.js has nargs=5 but u.i.nvars=7, so u.i.names.map is the valid arm, not u.i.names.array. I'll try to fix in order to get to the underlying orange cause.

/be

Comment 12

[Brendan Eich [:brendan]]

Also, I should have looked earlier and noted that we are happy to use JSFunction methods now, not js_FindDuplicateFormal C functions. Nit on top of nit: the definition for that function has an unnecessary extern before its return type.

/be

Comment 13

[Brendan Eich [:brendan]]

This got onto m-c a while ago:

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

Is there anything more to do based on my trailing comments?

/be