Closed Bug 609832 Opened 14 years ago Closed 13 years ago

Function statements should be banned (for now) in ES5 strict mode

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- -
status2.0 --- wanted

People

(Reporter: brendan, Assigned: jimb)

References

(Blocks 1 open bug, )

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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
> 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
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
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
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.)
Depends on: 577325
Summary: Function statements should be block-local → Function statements should be banned (for now) in ES5 strict mode
(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
Go ahead -- I'll deal.
No longer depends on: 577325
Attachment #502061 - Flags: review?(cdleary)
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 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 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?
Attachment #502061 - Flags: review?(cdleary) → review+
(In reply to comment #12)
> why are those lines being removed from
> shell.js?

Sorry, missed comment 10.
(In reply to comment #11)
> Nit: "only" after "declared".

http://hg.mozilla.org/tracemonkey/rev/d59c08ab2171
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?
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.
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.)
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
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.
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
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
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.
(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 */
    }
  }
}
(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) { }
(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
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
You need to log in before you can comment on or make changes to this bug.