630332 - Treat FutureReservedWords as errors when encountered as function or argument name for a function containing "use strict" directive

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch and many tests

In other words,


should all be errors.

This also fixes an O(n**2) algorithm when searching for duplicate argument names.  It's filed, but I don't remember the bug number offhand.

The tests were created by going through the spec looking for uses of Identifier.  They should be exhaustive, except for omitting labeled continue/break -- in that case you'd already have seen the label with the same name which should have caused that error (and since labels can't cross function boundaries the absence of such a label always means a break/continue to nowhere, which is also a syntax error).

Comment 1

[Jeff Walden [:Waldo]]

Er,

  function let() { 'use strict'; }
  var o1 = function let() { 'use strict'; };
  function foo(let) { 'use strict'; }
  var o2 = function foo(let) { 'use strict'; };
  var o3 = { get x(let) { 'use strict'; } };

the error cases.

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #0)
> This also fixes an O(n**2) algorithm when searching for duplicate argument
> names.  It's filed, but I don't remember the bug number offhand.

Bug 622670. FIXME comment not left in code.

Is bug 532421 still valid?

/be

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 508515 [details] [diff] [review]
Patch and many tests

>+    JSDefinition *dn = ALE_DEFN(tc->decls.lookup(name));
>+    JSAutoByteString bytes;
>+    if (!js_AtomToPrintableString(cx, name, &bytes) ||
>+        !ReportStrictModeError(cx, TS(tc->parser), tc, dn, errorNumber, bytes.ptr())) {
>+        return false;
>+    }
>+    return true;

Just:

..    return js_AtomToPrintableString(cx, name, &bytes) &&
..           ReportStrictModeError(cx, TS(tc->parser), tc, dn, errorNumber, bytes.ptr());

>+ * In strict mode code, all argument names must be distinct, must not be
>+ * keywords (particularly keywords which are that only in strict mode code,
>+ * which can be retroactively applied to a function from its body),

"which are that only in strict mode code" is a defining clause, so should start with "that" -- but the parenthetical is confusing in a couple of ways:

1. "keyword" != "reserved", so "which are that only in strict mode code" would more clear if written as "that are reserved only in ...."

2. "retroactively applied to a function from its body" -- say "function head" or something like that.

>@@ -8906,13 +8917,16 @@ Parser::primaryExpr(TokenKind tt, JSBool
>                  * followed by ::. This is the only case where a keyword after
>                  * . or .. is not treated as a property name.
>                  */
>-                tt = js_CheckKeyword(pn->pn_atom->chars(), pn->pn_atom->length());
>-                if (tt == TOK_FUNCTION) {
>-                    pn->pn_arity = PN_NULLARY;
>-                    pn->pn_type = TOK_FUNCTION;
>-                } else if (tt != TOK_EOF) {
>-                    reportErrorNumber(NULL, JSREPORT_ERROR, JSMSG_KEYWORD_NOT_NS);
>-                    return NULL;
>+                const KeywordInfo *ki = FindKeyword(pn->pn_atom->charsZ(), pn->pn_atom->length());
>+                if (ki) {
>+                    tt = ki->tokentype;
>+                    if (tt == TOK_FUNCTION) {
>+                        pn->pn_arity = PN_NULLARY;
>+                        pn->pn_type = TOK_FUNCTION;
>+                    } else {
>+                        reportErrorNumber(NULL, JSREPORT_ERROR, JSMSG_KEYWORD_NOT_NS);
>+                        return NULL;
>+                    }
>                 }
>             }
>             pn = qualifiedSuffix(pn);

The if (tt == TOK_FUNCTION) test should be inverted to avoid if (A) { B; } else { C; return; } D where B and D are arbitrarily separated and differently indented:

..                    if (tt != TOK_FUNCTION) {
..                        reportErrorNumber(NULL, JSREPORT_ERROR, JSMSG_KEYWORD_NOT_NS);
..                        return NULL;
..                    }
..                    pn->pn_arity = PN_NULLARY;
..                    pn->pn_type = TOK_FUNCTION;

Great to see findDuplicateArgument go!

/be

Comment 4

[Jeff Walden [:Waldo]]

Attachment: v2

Turns out TCF_FUN_PARAM_EVAL is unused, so I removed it.  (The arguments lookalike is still used elsewhere.)

Comment 5

[Jim Blandy :jimb]

I think changing CheckStrictFormals to use the argument/parameter naming dichotomy instead of the formal/actual dichotomy is an improvement, as a/p is what ES5 uses. However, in ES5 and elsewhere, "arguments" refers to the values being passed, whereas the names in the function definition are "parameters". So if you want to rename this, it should be CheckStrictParameters.

Comment 6

[Jim Blandy :jimb]

(still reading)

+        if (FindKeyword(name->charsZ(), name->length())) {
+            /* If we're not in strict mode, the keyword was warned about when scanned. */
+            if (tc->inStrictMode() && !ReportBadArgument(cx, tc, name, JSMSG_RESERVED_ID))
+                return false;
+        }

Could we move the tc->inStrictMode test outside the FindKeyword call, to avoid looking things up if we're just going to ignore the result?

Comment 7

[Jim Blandy :jimb]

The old way we recorded the bindings automatically built a list of duplicates when it switched to the hash table representation. I gather we've lost that when we switched to using the property tree?

Comment 8

[Jim Blandy :jimb]

-    if (!CheckStrictBinding(context, &funtc, funAtom, pn))
+    if (funAtom && !CheckStrictBinding(context, &funtc, funAtom, pn))

Thanks... :(

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #7)
> The old way we recorded the bindings automatically built a list of duplicates
> when it switched to the hash table representation. I gather we've lost that
> when we switched to using the property tree?

No, the shape tree path for a given function switches to dictionary-mode unshared shapes as necessary, but my changes to use shapes rather than the old localNames scheme did not bother checking for it. Hence my joy at Waldo fixing this bug.

/be

Comment 10

[Jim Blandy :jimb]

/* If we're not in strict mode, the keyword was warned about when scanned. */

This isn't right, no matter how I read it:

If 'strict mode' refers to JSOPTION_STRICT, then it was warned about when we *are* in strict mode.

If 'strict mode' refers to ES5 strict mode code, then either a) we inherited our strictness from the surrounding code, in which case the scanner reported an error and we never reached this spot, or b) we are applying retroactive stricness, in which case the keyword was *not* warned about when scanned.

I think you meant:

/* JSOPTION_STRICT is supposed to warn about future keywords, too, but we took care of that in the scanner. */

Comment 11

[Jim Blandy :jimb]

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

Looks fine, with the issues I've mentioned above fixed.

Comment 12

[Jeff Walden [:Waldo]]

So, I am awesome and somehow managed to file a dup against myself *five days* later.  The commit message refers to the original bug, not to this one.

http://hg.mozilla.org/tracemonkey/rev/4cb688939afc

Comment 14

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/4cb688939afc