Last Comment Bug 609832 - Function statements should be banned (for now) in ES5 strict mode
: Function statements should be banned (for now) in ES5 strict mode
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla2.0b8
Assigned To: Jim Blandy :jimb
:
Mentors:
http://wiki.ecmascript.org/doku.php?i...
Depends on:
Blocks: es5strict 584603
  Show dependency treegraph
 
Reported: 2010-11-04 21:55 PDT by Brendan Eich [:brendan]
Modified: 2011-01-09 15:08 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wanted


Attachments
Forbid function statements in strict mode code. (7.21 KB, patch)
2011-01-07 12:04 PST, Jim Blandy :jimb
cdleary: review+
Details | Diff | Review

Description Brendan Eich [:brendan] 2010-11-04 21:55:48 PDT
See bug 585536, the long-term fix.

Short-term, for Firefox 4, SpiderMonkey's ES5 strict mode implementation should make our conditionally bound function sub-statement extension an error. Jim, can you field this one? If not, bounce to me. Thanks,

/be
Comment 1 Dave Herman [:dherman] 2010-11-05 20:15:29 PDT
> Short-term, for Firefox 4, SpiderMonkey's ES5 strict mode implementation should
> make our conditionally bound function sub-statement extension an error.

Unclear; do you mean that all block-local function declarations are statically rejected? Or do you mean that it's a dynamic error if a local function declaration is evaluated (to prevent it from modifying the scope chain dynamically)?

Dave
Comment 2 Brendan Eich [:brendan] 2010-11-05 20:43:10 PDT
That's a good question. We could make it an early error, and that's what I had in mind. Making it a runtime error might let code migrate more easily, but is it doing anyone any favors?

/be
Comment 3 Dave Herman [:dherman] 2010-11-05 20:49:55 PDT
Either way, I just worry that forbidding something temporarily is going to create migration hazards. But since you only opt in to ES5 strict and Harmony separately, I guess it might not be a problem. (I haven't had time to think this through, for obvious reasons.)

As for static vs dynamic, I *think* I'd go with the static restriction.

Dave
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2010-11-05 21:53:50 PDT
This will conflict with bug 577325, so please hold off on it until I finish that bug.  (I have a patch passing shell tests, but browser-based tests still need some work.)
Comment 5 Brendan Eich [:brendan] 2010-12-22 18:33:51 PST
(In reply to comment #4)
> This will conflict with bug 577325, so please hold off on it until I finish
> that bug.  (I have a patch passing shell tests, but browser-based tests still
> need some work.)

Comment 4 was a while ago, and this bug needs fixing. People rebase across conflicts all the time. Any reason to hold off here?

/be
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2010-12-22 18:38:50 PST
Go ahead -- I'll deal.
Comment 9 Jim Blandy :jimb 2011-01-07 12:04:27 PST
Created attachment 502061 [details] [diff] [review]
Forbid function statements in strict mode code.
Comment 10 Jim Blandy :jimb 2011-01-07 12:10:34 PST
Oh --- by the way, the reason the patch deletes stuff from ecma_5/extensions/shell.js is that there are identical definitions in eczema_5/shell.js.
Comment 11 Brendan Eich [:brendan] 2011-01-07 12:17:59 PST
Comment on attachment 502061 [details] [diff] [review]
Forbid function statements in strict mode code.

>Bug 609832: Forbid function statements in strict mode code.
>
>diff --git a/js/src/js.msg b/js/src/js.msg
>--- a/js/src/js.msg
>+++ b/js/src/js.msg
>@@ -345,3 +345,4 @@ MSG_DEF(JSMSG_SC_RECURSION,           26
> MSG_DEF(JSMSG_CANT_WRAP_XML_OBJECT,   263, 0, JSEXN_TYPEERR, "can't wrap XML objects")
> MSG_DEF(JSMSG_BAD_CLONE_VERSION,      264, 0, JSEXN_ERR, "unsupported structured clone version")
> MSG_DEF(JSMSG_CANT_CLONE_OBJECT,      265, 0, JSEXN_TYPEERR, "can't clone object")
>+MSG_DEF(JSMSG_STRICT_FUNCTION_STATEMENT, 266, 0, JSEXN_SYNTAXERR, "in strict mode code, functions may only be declared at top level or immediately within another function")

Nit: "only" after "declared".

/be
Comment 12 Chris Leary [:cdleary] (not checking bugmail) 2011-01-07 14:31:33 PST
Comment on attachment 502061 [details] [diff] [review]
Forbid function statements in strict mode code.

Parser change and tests look good, but why are those lines being removed from shell.js?
Comment 13 Chris Leary [:cdleary] (not checking bugmail) 2011-01-07 14:32:59 PST
(In reply to comment #12)
> why are those lines being removed from
> shell.js?

Sorry, missed comment 10.
Comment 14 Chris Leary [:cdleary] (not checking bugmail) 2011-01-07 18:05:11 PST
http://hg.mozilla.org/tracemonkey/rev/b0f047a6a9da
Comment 15 Jim Blandy :jimb 2011-01-07 21:48:13 PST
(In reply to comment #11)
> Nit: "only" after "declared".

http://hg.mozilla.org/tracemonkey/rev/d59c08ab2171
Comment 17 Kris Maglione [:kmag] 2011-01-08 20:00:51 PST
Why on earth have you done this? Is there any way we can have something like strict mode that breaks things like assigning to undeclared variables while not breaking useful features?
Comment 18 Kris Maglione [:kmag] 2011-01-08 20:06:33 PST
Sorry for the noise. I was just caught off guard by this, and I'm not sure how it relates at all to strict mode.
Comment 19 Jeff Walden [:Waldo] (remove +bmo to email) 2011-01-08 22:33:24 PST
http://wiki.ecmascript.org/doku.php?id=conventions:no_non_standard_strict_decls

Remember that this "feature" is not implemented interoperably across engines or across browsers.  Strict mode code attempting to rely on it would have had different behavior across browsers.  This in turn would make it harder for scripts to opt into a future specification like ES6 which attempted to define semantics for nested function statements.  The plan now is for ES6 to have nested function statements, and this move will ease the transition to it when that time arrives.

Anyway, that's the logic.  I can understand how you might disagree with it.  But I must admit that, as this concerns a non-standard, widely-implemented yet completely non-interoperable feature, with the action being taken to eventually *improve* interoperability, you will get little sympathy from me.  (No doubt others will feel differently on the matter, of course.  :-)  I've always been relatively opinionated.)
Comment 20 Brendan Eich [:brendan] 2011-01-08 22:34:56 PST
I'm not sure Kris is objecting to what happened here. Kris, can you give an example of what you think is at stake? Also, are you targeting only SpiderMonkey (Firefox, e.g.) or all browsers?

/be
Comment 21 Kris Maglione [:kmag] 2011-01-09 02:07:56 PST
Brendan: Yes, I did misread this the first time, I'm sorry to say, but I still don't understand what it has to do with strict mode. And, yes, I'm targeting only SpiderMonkey. My cross-browser code would of course not rely on any kind of non-standard behavior, but my extension code is heavily targeted to JS1.8.
Comment 22 Brendan Eich [:brendan] 2011-01-09 11:59:25 PST
Kris: let's be precise. This bug makes only the following and similar function declarations as subordinate statements into strict mode errors:

  if (C) function f() {}
  if (D) { function f() {} }
  { function f() {} ... }
  while (E) { function f() {} ... }
  switch (E) { case C1: function f() {} ... }

Either you use such an extension, or you don't. If you don't, no problem. If you do, don't use strict mode until you've changed your code to avoid this extension.

/be
Comment 23 Brendan Eich [:brendan] 2011-01-09 12:00:22 PST
The reasons only strict mode bans this extension:

First, we can't ban it outright without warning in all modes.

Second, the future "Harmony" edition(s) of JS build on ES5 strict as a starting point. As Jeff said we need to clear the decks.

/be
Comment 24 Kris Maglione [:kmag] 2011-01-09 13:32:24 PST
I understand the scope of the change. The two things I don't understand are a) how this clears the decks, and b) why this change is acceptable in strict mode but not out of it. As far as I understand, functions defined other than at the top-level are perfectly legal in ES5 strict mode, although certain semantics such as defining them conditionally aren't, but please correct me if I'm wrong. What this change means is that perfectly legal code which runs on other browsers will throw an error in Mozilla, which I don't see the use of.

I'll also note that, while I'll admit that I have some code conditionally declaring functions this way, which is perhaps questionable, most of my broken code involves closures simply defined near the point where they're used, but not defined conditionally, used outside of the block in question, or relying on any local variables. In particular, I have several functions and the entire top-level of most of my JS modules wrapped in try-catch blocks, in the latter case because errors at the top level don't propagate and result in NS_ERROR_FILE_NOT_FOUND rather than any useful debugging information. I don't see the need to break this functionality.
Comment 25 Mike Shaver (:shaver -- probably not reading bugmail closely) 2011-01-09 14:20:00 PST
(In reply to comment #24)
> I understand the scope of the change. The two things I don't understand are a)
> how this clears the decks, and b) why this change is acceptable in strict mode
> but not out of it. As far as I understand, functions defined other than at the
> top-level are perfectly legal in ES5 strict mode, although certain semantics
> such as defining them conditionally aren't, but please correct me if I'm wrong.

yes, and it's the latter that is forbidden here.

function f() {
  "use strict";
  function g() {
    /* no problem */
  }
}

function f() {
  "use strict";
  if (cond) {
    function g() {
      /* illegal */
    }
  }
}
Comment 26 Mike Shaver (:shaver -- probably not reading bugmail closely) 2011-01-09 14:21:42 PST
(In reply to comment #24)
> I'll also note that, while I'll admit that I have some code conditionally
> declaring functions this way, which is perhaps questionable, most of my broken
> code involves closures simply defined near the point where they're used, but
> not defined conditionally, used outside of the block in question, or relying on
> any local variables.

For those cases, you can use variables and function expressions, I believe.

Before:

try {
  function f() {  .... }
  f();
} catch (e) { }

After:

try {
  const f = function f() { .... }
  f();
} catch (e) { }
Comment 27 Dave Herman [:dherman] 2011-01-09 14:51:56 PST
(In reply to comment #24)
> I understand the scope of the change. The two things I don't understand are a)
> how this clears the decks, and b) why this change is acceptable in strict mode
> but not out of it. As far as I understand, functions defined other than at the
> top-level are perfectly legal in ES5 strict mode, although certain semantics
> such as defining them conditionally aren't, but please correct me if I'm wrong.

I don't think ES5 allows functions anywhere but at program or function top-level, regardless of whether the code is in strict mode. There just isn't any such construct in the spec. Moreover, diffetent browsers implement this feature as a non-standard extension but the behavior differs from browser to browser.

Mike's comment gives a good workaround, if you want to be able to use ES5 strict: replace the nested function foo() { ... } declarations with var foo = function() { ... }. This is safer in the sense that it's completely standard, portable ES5 (or ES3 for that matter).

Dave
Comment 28 Dave Herman [:dherman] 2011-01-09 15:08:59 PST
Also, I should add that there's no subset of block-local functions that behave reasonably in SpiderMonkey. They are effectively *always* conditionally bound: any reference to such a function (e.g., a function call) will resolve differently depending on whether control flow has reached the declaration. So even if it's not directly conditional, it depends on whether the code gets run or not, dynamically. This isn't what other engines do at all. So there really isn't a simple, portable subset you can reliably use. This strict-mode error stops you from tripping over that un-portability.

Dave

Note You need to log in before you can comment on or make changes to this bug.