Closed Bug 514572 Opened 12 years ago Closed 12 years ago

ES5 strict mode: delete operand may not be a variable, argument, or function name

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jimb, Assigned: jimb)

References

(Blocks 1 open bug)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

From ES5 Annex C:

When a delete operator occurs within strict mode code, a SyntaxError is thrown if its UnaryExpression is a direct reference to a variable, function argument, or function name(11.4.1
If I'm reading the spec correctly, it's not statically decidable whether or not a given 'delete' expression in strict mode code is a SyntaxError or not.  Calling the function 'f' below should raise a syntax error if and only if its argument is true:

function f(e) {
  let o={};
  if (e) o.x = 1;
  with(o) { (function () { "use strict"; delete x; })(); };
};

Implementing this behavior seems contrary to the intent of strict mode.
(In reply to comment #1)
> If I'm reading the spec correctly

Which, of course, I wasn't:

When an identifier's binding is found in a 'with' statement's operand, the value produced is a reference whose base is a reference to an environment record, not the 'with' operand.  Thus you get a SyntaxError either way.  This seems reasonable.
Could the spec be simpler? The intent is to outlaw delete foo but allow delete bar.baz and delete quux[i].

/be
Well, of course: the spec could simply forbid expressions of the form |delete NAME| in strict mode code.  Doesn't that meet the criteria?

"Step 2 1/2: If IsStrictReference(ref) is true, and the UnaryExpression is an identifier reference (11.1.2), throw a SyntaxError exception."

and then strike the other IsStrictReference checks.

There's already text saying what I wanted to hear in the NOTE; my worry was that the NOTE might conflict with the algorithm.
Assignee: general → jim
Depends on: 514585
Comment on attachment 404923 [details] [diff] [review]
Forbid deletion of variables, arguments and functions in ES5 strict mode.

Drive-by, hope it's ok:

>+++ b/js/src/js.msg
>@@ -314,3 +314,4 @@ MSG_DEF(JSMSG_NOT_NONNULL_OBJECT,     23
> MSG_DEF(JSMSG_STRICT_WITH,            232, 0, JSEXN_SYNTAXERR, "strict mode forbids 'with' statements")
> MSG_DEF(JSMSG_STRICT_UNDECLARED_VAR,  233, 1, JSEXN_REFERENCEERR, "assignment to undeclared variable {0} in strict mode code")
> MSG_DEF(JSMSG_DUPLICATE_PROPERTY,     234, 1, JSEXN_SYNTAXERR, "duplicated property name {0} in object literal")
>+MSG_DEF(JSMSG_STRICT_DELETE_OPERAND,  235, 0, JSEXN_SYNTAXERR, "deleting variables, arguments or functions is deprecated")

The other new strict-mode messages say "strict mode" -- worth doing so here too?

>@@ -6247,6 +6247,9 @@ UnaryExpr(JSContext *cx, JSTokenStream *
>             }
>             break;
>           case TOK_NAME:
>+            if (!js_ReportES5StrictError(cx, ts, tc, pn,
>+                                         JSMSG_STRICT_DELETE_OPERAND))

Go nuts, use >= 80 columns if < 100.

/be
(In reply to comment #6)
> The other new strict-mode messages say "strict mode" -- worth doing so here
> too?

Except for 'with', all these conditions also appear as warnings under JSOPTION_STRICT.  Will it be confusing to mention 'strict mode' when there is no "use strict", but the user has enabled JSOPTION_STRICT in some fashion?  I guess not.
(In reply to comment #7)
> (In reply to comment #6)
> > The other new strict-mode messages say "strict mode" -- worth doing so here
> > too?
> 
> Except for 'with', all these conditions also appear as warnings under
> JSOPTION_STRICT.  Will it be confusing to mention 'strict mode' when there is
> no "use strict", but the user has enabled JSOPTION_STRICT in some fashion?  I
> guess not.

Oh sorry -- I was indeed misled by the with warning. Carry on...

/be
Comment on attachment 404923 [details] [diff] [review]
Forbid deletion of variables, arguments and functions in ES5 strict mode.

The ES5 draft wording on this point is pointlessly obscure on a legendary scale.
Attachment #404923 - Flags: review?(jorendorff) → review+
Er, I meant, "This is missing tests. r=me with that fixed."
Comment on attachment 404923 [details] [diff] [review]
Forbid deletion of variables, arguments and functions in ES5 strict mode.

>Bug 514572: Forbid deletion of variables, arguments and functions in ES5 strict mode.
>
>diff --git a/js/src/js.msg b/js/src/js.msg
>--- a/js/src/js.msg
>+++ b/js/src/js.msg
>@@ -314,3 +314,4 @@ MSG_DEF(JSMSG_NOT_NONNULL_OBJECT,     23
> MSG_DEF(JSMSG_STRICT_WITH,            232, 0, JSEXN_SYNTAXERR, "strict mode forbids 'with' statements")
> MSG_DEF(JSMSG_STRICT_UNDECLARED_VAR,  233, 1, JSEXN_REFERENCEERR, "assignment to undeclared variable {0} in strict mode code")
> MSG_DEF(JSMSG_DUPLICATE_PROPERTY,     234, 1, JSEXN_SYNTAXERR, "duplicated property name {0} in object literal")
>+MSG_DEF(JSMSG_STRICT_DELETE_OPERAND,  235, 0, JSEXN_SYNTAXERR, "deleting variables, arguments or functions is deprecated")

Sorry to fuss about messages again: do we want "is deprecated" or something more specific and error-y? The other js.msg entry that talks of deprecation:

js.msg:MSG_DEF(JSMSG_DEPRECATED_USAGE,       158, 1, JSEXN_REFERENCEERR, "deprecated {0} usage")

is used for foo.arguments for function foo, __{parent,count}__, old getter and setter syntax, and sharp variables -- all strict option warnings, not strict mode makes errors.

>diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp
>--- a/js/src/jsparse.cpp
>+++ b/js/src/jsparse.cpp
>@@ -6247,6 +6247,9 @@ UnaryExpr(JSContext *cx, JSTokenStream *
>             }
>             break;
>           case TOK_NAME:
>+            if (!js_ReportES5StrictError(cx, ts, tc, pn,
>+                                         JSMSG_STRICT_DELETE_OPERAND))
>+                return NULL;

Nit attack: brace if-then where either condition or then-part (or else if present) is multiline.

/be
(In reply to comment #11)
> >+MSG_DEF(JSMSG_STRICT_DELETE_OPERAND,  235, 0, JSEXN_SYNTAXERR, "deleting variables, arguments or functions is deprecated")
> 
> Sorry to fuss about messages again: do we want "is deprecated" or something
> more specific and error-y? The other js.msg entry that talks of deprecation:
> 
> js.msg:MSG_DEF(JSMSG_DEPRECATED_USAGE,       158, 1, JSEXN_REFERENCEERR,
> "deprecated {0} usage")
> 
> is used for foo.arguments for function foo, __{parent,count}__, old getter and
> setter syntax, and sharp variables -- all strict option warnings, not strict
> mode makes errors.

It seems to me the clearest thing, message-wise, would be to provide different messages depending on whether it's JSOPTION_STRICT or strict mode that's eliciting them.  The former could simply talk about "deprecation", whereas the latter could explicitly note that strict mode was in force.  But that's a lot of message codes, whose texts really should be mostly the same.

JSErrorReport::flags will have JSREPORT_STRICT_MODE_ERROR set, so reporting functions could mention "strict mode code" in the report, as js.cpp:my_ErrorReporter checks JSREPORT_IS_STRICT and JSREPORT_IS_WARNING to provide meta-information about messages now ("strict warning: ...").

I gather you'd like different terminology to distinguish the "errors in strict mode code" subset of the "warnings with JSOPTION_STRICT" constructs.  But in honor of that subset relation, the messages shouldn't reference strict mode explicitly.  We're looking for some pithy form of "quite sternly deprecated".  But having verbal conventions to distinguish "questionable", "quite questionable" and "actually unacceptable" seems like trying to make distinctions in a way that most users will miss.  Using JSErrorReport::flags could be much more explicit.


> >diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp
> >--- a/js/src/jsparse.cpp
> >+++ b/js/src/jsparse.cpp
> >@@ -6247,6 +6247,9 @@ UnaryExpr(JSContext *cx, JSTokenStream *
> >             }
> >             break;
> >           case TOK_NAME:
> >+            if (!js_ReportES5StrictError(cx, ts, tc, pn,
> >+                                         JSMSG_STRICT_DELETE_OPERAND))
> >+                return NULL;
> 
> Nit attack: brace if-then where either condition or then-part (or else if
> present) is multiline.

I've gone through and checked all the strict-mode-related patches to ensure this rule has been followed.
I'm not looking for two sets of messages if we don't really need to encode the error vs. warning bit (which can be set to "error" by JSOPTION_WERROR). Not sure what to suggest, deprecate may be the right compromise now that you remind me (again, thanks) you are unifying where possible -- although I recall a recent patch with two different JSMSG_* cookies, one for strict mode and the other for either JSOPTION_STRICT or standard mode (you probably know what I'm thinking of).

/be
(In reply to comment #13)
> although I recall a
> recent patch with two different JSMSG_* cookies, one for strict mode and the
> other for either JSOPTION_STRICT or standard mode (you probably know what I'm
> thinking of).

You probably mean:
https://bugzilla.mozilla.org/attachment.cgi?id=404913&action=diff

Perhaps those ought to be unified as well.
Yes, that was the patch. Just keeping score in spare room in my head...

/be
Now --- with tests!  (Code unchanged.)
Attachment #404923 - Attachment is obsolete: true
Attachment #406768 - Flags: review?(jorendorff)
I can't make sense of the tests. Where are always, never, becomes, and parse_fails defined?
Sorry, found it. My comments on this test are basically the same as bug 521868 comment 3.

Like Waldo, I'm allergic to AddTestCase.

  assertThrowsInStrictMode("delete x;", SyntaxError);
  assertThrowsInBothModes("delete x.y;", ReferenceError);  // not SyntaxError

  assertThrowsInStrictMode("function f() { delete x; }", SyntaxError);
  assertThrowsInBothModes('function f() { "use strict"; delete x; }',
                          SyntaxError);
Also, a test for "delete (x)" would be nice.
Comment on attachment 406768 [details] [diff] [review]
Forbid deletion of variables, arguments and functions in ES5 strict mode.

>Bug 514572: Forbid deletion of variables, arguments and functions in ES5 strict mode. r=jorendorff
>
>diff --git a/js/src/js.msg b/js/src/js.msg
>--- a/js/src/js.msg
>+++ b/js/src/js.msg
>@@ -313,3 +313,4 @@ MSG_DEF(JSMSG_XDR_CLOSURE_WRAPPER,    23
> MSG_DEF(JSMSG_NOT_NONNULL_OBJECT,     231, 0, JSEXN_TYPEERR, "value is not a non-null object")
> MSG_DEF(JSMSG_STRICT_CODE_WITH,       232, 0, JSEXN_SYNTAXERR, "strict mode code may not contain 'with' statements")
> MSG_DEF(JSMSG_DUPLICATE_PROPERTY,     233, 1, JSEXN_SYNTAXERR, "property name {0} appears more than once in object literal")
>+MSG_DEF(JSMSG_DEPRECATED_DELETE_OPERAND, 234, 0, JSEXN_SYNTAXERR, "deleting variables, arguments or functions is deprecated")

Fussing a bit harder: if we ban all delete x expressions where x is an identifier (IdentifierName in ES5's grammar, IIRC) then the programmer could have written with (obj_with_x) delete x -- is the diagnostic too narrow in this case? Probably the more common case is delete global_undeclared_var_x, but "variables" may work there.

The alternative I'm getting at is "deleting unqualified names is deprecated" or "deleting identifier expressions is deprecated".

>diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp
>--- a/js/src/jsparse.cpp
>+++ b/js/src/jsparse.cpp
>@@ -6256,6 +6256,9 @@ UnaryExpr(JSContext *cx, JSTokenStream *
>             }
>             break;
>           case TOK_NAME:
>+            if (!js_ReportStrictModeError(cx, ts, tc, pn, JSMSG_DEPRECATED_DELETE_OPERAND)) {
>+                return NULL;
>+            }

D'oh -- no need to brace if you use the tw=99 new line length order to fit the condition on one line.

/be
(In reply to comment #18)
> Like Waldo, I'm allergic to AddTestCase.
> 
>   assertThrowsInStrictMode("delete x;", SyntaxError);
>   assertThrowsInBothModes("delete x.y;", ReferenceError);  // not SyntaxError
> 
>   assertThrowsInStrictMode("function f() { delete x; }", SyntaxError);
>   assertThrowsInBothModes('function f() { "use strict"; delete x; }',
>                           SyntaxError);

++ to infinity and beyond!  The other names may be composable, but I prefer the explicitness of these names -- never/becomes/etc. just don't do it for me.
(In reply to comment #20)
> Fussing a bit harder: if we ban all delete x expressions where x is an
> identifier (IdentifierName in ES5's grammar, IIRC) then the programmer could
> have written with (obj_with_x) delete x -- is the diagnostic too narrow in this
> case? Probably the more common case is delete global_undeclared_var_x, but
> "variables" may work there.

Just to be clear that we're talking about the message, not the behavior: 'with (obj_with_x) delete x;' is forbidden in strict mode, even following the spec as written: the reference produced by evaluating 'x' has an environment record base, which is what 11.4.1 forbids.

> The alternative I'm getting at is "deleting unqualified names is deprecated" or
> "deleting identifier expressions is deprecated".

I guess I generally think of 'with' as introducing variable bindings.  An identifier whose value is found by searching the scope chain is a variable, by definition.  But I don't mind the "identifier expression" wording.

> >diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp
> >--- a/js/src/jsparse.cpp
> >+++ b/js/src/jsparse.cpp
> >@@ -6256,6 +6256,9 @@ UnaryExpr(JSContext *cx, JSTokenStream *
> >             }
> >             break;
> >           case TOK_NAME:
> >+            if (!js_ReportStrictModeError(cx, ts, tc, pn, JSMSG_DEPRECATED_DELETE_OPERAND)) {
> >+                return NULL;
> >+            }
> 
> D'oh -- no need to brace if you use the tw=99 new line length order to fit the
> condition on one line.

argh
Comment 20 first part was all about the error message -- I should probably have written "fussing a bit harder about the message (again)".

/be
"variable" is ok, my concern was folks expecting it to mean var-declared binding.

/be
(In reply to comment #21)
> ++ to infinity and beyond!  The other names may be composable, but I prefer the
> explicitness of these names -- never/becomes/etc. just don't do it for me.

Let me readily agree that the names are hopelessly vague.  In a context where people can be expected to be familiar with the background, terseness can be a virtue, but test cases are the opposite: people get dragged into reading them by regressions with no context whatsoever.

But I do think some kind of composition, with very explicit names, is a win here.  I'll sketch something out tomorrow morning.  I've been really fuzzy-headed today; apologies to everyone.
Attachment #406768 - Flags: review?(jorendorff)
Tests revised per jorendorff's and waldo's comments.  Message and braces revised per brendan's.
Attachment #406768 - Attachment is obsolete: true
Actually, I'd forgotten the test Jason requested in comment 19.  Revised.
Attachment #412252 - Attachment is obsolete: true
http://hg.mozilla.org/tracemonkey/rev/b23078c3e55a
Status: NEW → ASSIGNED
Whiteboard: fixed-in-tracemonkey
Flags: in-testsuite+
http://hg.mozilla.org/mozilla-central/rev/b23078c3e55a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 541455
No longer depends on: 541455
You need to log in before you can comment on or make changes to this bug.