Closed Bug 630332 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b11
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: Waldo, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

(Whiteboard: fixed-in-tracemonkey, softblocker)

Attachments

(1 file, 1 obsolete file)

Attached patch Patch and many tests (obsolete) — Splinter Review
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).
Attachment #508515 - Flags: review?(jimb)
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.
(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 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
Attached patch v2Splinter Review
Turns out TCF_FUN_PARAM_EVAL is unused, so I removed it.  (The arguments lookalike is still used elsewhere.)
Attachment #508515 - Attachment is obsolete: true
Attachment #508572 - Flags: review?(jimb)
Attachment #508515 - Flags: review?(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.
(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?
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?
-    if (!CheckStrictBinding(context, &funtc, funAtom, pn))
+    if (funAtom && !CheckStrictBinding(context, &funtc, funAtom, pn))

Thanks... :(
(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
/* 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 on attachment 508572 [details] [diff] [review]
v2

Looks fine, with the issues I've mentioned above fixed.
Attachment #508572 - Flags: review?(jimb) → review+
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
blocking2.0: --- → ?
Whiteboard: fixed-in-tracemonkey
Duplicate of this bug: 629187
blocking2.0: ? → betaN+
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey, softblocker
Depends on: 630770
http://hg.mozilla.org/mozilla-central/rev/4cb688939afc
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.