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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

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

Comment 1

[Jim Blandy :jimb]

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.

Comment 2

[Jim Blandy :jimb]

(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.

Comment 3

[Brendan Eich [:brendan]]

Could the spec be simpler? The intent is to outlaw delete foo but allow delete bar.baz and delete quux[i].

/be

Comment 4

[Jim Blandy :jimb]

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.

Comment 5

[Jim Blandy :jimb]

Attachment: Forbid deletion of variables, arguments and functions in ES5 strict mode.

Comment 6

[Brendan Eich [:brendan]]

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

Comment 7

[Jim Blandy :jimb]

(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.

Comment 8

[Brendan Eich [:brendan]]

(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 9

[Jason Orendorff [:jorendorff]]

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.

Comment 10

[Jason Orendorff [:jorendorff]]

Er, I meant, "This is missing tests. r=me with that fixed."

Comment 11

[Brendan Eich [:brendan]]

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

Comment 12

[Jim Blandy :jimb]

(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.

Comment 13

[Brendan Eich [:brendan]]

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

Comment 14

[Jim Blandy :jimb]

(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.

Comment 15

[Brendan Eich [:brendan]]

Yes, that was the patch. Just keeping score in spare room in my head...

/be

Comment 16

[Jim Blandy :jimb]

Attachment: Forbid deletion of variables, arguments and functions in ES5 strict mode.

Now --- with tests!  (Code unchanged.)

Comment 17

[Jason Orendorff [:jorendorff]]

I can't make sense of the tests. Where are always, never, becomes, and parse_fails defined?

Comment 18

[Jason Orendorff [:jorendorff]]

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);

Comment 19

[Jason Orendorff [:jorendorff]]

Also, a test for "delete (x)" would be nice.

Comment 20

[Brendan Eich [:brendan]]

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

Comment 21

[Jeff Walden [:Waldo]]

(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.

Comment 22

[Jim Blandy :jimb]

(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 23

[Brendan Eich [:brendan]]

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

/be

Comment 24

[Brendan Eich [:brendan]]

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

/be

Comment 25

[Jim Blandy :jimb]

(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.

Comment 26

[Jim Blandy :jimb]

Attachment: Forbid deletion of variables, arguments and functions in ES5 strict mode.

Tests revised per jorendorff's and waldo's comments.  Message and braces revised per brendan's.

Comment 27

[Jim Blandy :jimb]

Attachment: Forbid deletion of variables, arguments and functions in ES5 strict mode.

Actually, I'd forgotten the test Jason requested in comment 19.  Revised.

Comment 28

[Jim Blandy :jimb]

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

Comment 29

[Robert Sayre]

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