Functions declared in block statements should be block-local and hoisted to top of block

RESOLVED DUPLICATE of bug 1071646

Status

()

Core
JavaScript Engine
RESOLVED DUPLICATE of bug 1071646
7 years ago
a year ago

People

(Reporter: dvander, Unassigned)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(blocking2.0 -, status2.0 wanted)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
<@brendan> it'll be block local for harmony -- maybe we should do that now
<@brendan> block local, hoisted
Need to get this done for ES5 future-proofing.

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86 → All
Target Milestone: --- → mozilla2.0
Duplicate of this bug: 593176
Idea is at this stage to make the breaking change only if "use strict" is in force. We are very late in the release cycle to try breaking the default version.

/be
This should block es5strict and fx4 or we're in trouble with the Ecma purity police (I'm a volunteer deputy).

/be
Blocks: 482298
blocking2.0: --- → ?

Updated

7 years ago
blocking2.0: ? → -
status2.0: --- → wanted
I filed bug 609832 on banning functions in blocks from es5strict for fx4, so we can take the time to get this right for the release after.

/be
Target Milestone: mozilla2.0 → ---
Removing from es5strict blocker status. Bug 609832 will suffice for ES5 strict mode conformance.

/be
No longer blocks: 482298
Summary: Function statements should be block-local → Functions declared in block statements should be block-local

Comment 7

6 years ago
What about things like this?

{ if (true) function f() { } }

That's in a block, but not directly. I'd assume this and similar things should be forbidden.

How about:

switch (x) { function f() { ... } case 1: ... }

This seems to be as meaningful as a function within any other block. However, the spec doesn't treat the body of a switch statement as a block. Another special case to remember to specify.
A function declaration not directly within a block is not even supported by some browsers, and it won't be specified in Harmony. We could probably ban it in all modes, now (I mean Firefox 5's earliest nightlies).

JS has a more structured switch a la Java, not C's statement with labels anywhere in its body (Duff's Device). So you can't write anything but case labels and one default: directly within the braced switch body (which does delimit block scope for any let bindings in the labeled statements). Of course fall through works or fails just as in C.

/be
> That's in a block, but not directly. I'd assume this and similar things should
> be forbidden.

Yes. We should be forbidding all function declarations anywhere except at program or function top-level.

> How about:
> 
> switch (x) { function f() { ... } case 1: ... }
> 
> This seems to be as meaningful as a function within any other block. However,
> the spec doesn't treat the body of a switch statement as a block. Another
> special case to remember to specify.

No need for these special cases. One simple rule: only allowed at function or program top-level.

Dave
> No need for these special cases. One simple rule: only allowed at function or
> program top-level.

Dumb Dave. I was thinking of the wrong bug. Disregard my comment, sorry.

Dave

Comment 11

6 years ago
(In reply to comment #10)
> Dumb Dave. I was thinking of the wrong bug. Disregard my comment, sorry.

No problem. The patch I submitted for bug 609832 ensures that we reject function definitions within all sorts of statements.

Updated

6 years ago
Assignee: brendan → jimb
Fixing this bug should allow block-scoped functions in strict mode code.

/be
Blocks: 694100

Updated

6 years ago
Duplicate of this bug: 705618

Updated

5 years ago
Duplicate of this bug: 727139

Comment 15

5 years ago
I am not actively working on this. De-assigning myself.
Assignee: jimb → general
Status: ASSIGNED → NEW

Updated

5 years ago
Duplicate of this bug: 788963

Comment 17

5 years ago
I am struck by this from comment 8:
> A function declaration not directly within a block is not even supported by
> some browsers, and it won't be specified in Harmony. We could probably ban
> it in all modes, now (I mean Firefox 5's earliest nightlies).

And from bug 788963:
> It just struck me that FF is the only one to implement this block-level
> behavior, seeing as JS has no such thing as block-level scope AFAIK (IF
> blocks do not create new scopes), unless it's something new in the ES5.

"Some browsers" == only FF as far as I know. 

Just simply hosting inner func declarations to the top level would be so much more polite, and would let one declare functions closer to their actual use, since that would effectively and finally become portable.

Comment 18

5 years ago
Created attachment 669086 [details]
try_catch test case

Hello Guys,

I have attached test case which represent js issue which is only actual for FF , other browsers (Ie7-10,CHROME,OPERA) doesn't meet issue there.

This test case is very simple:
try{
alert(B2());
function B2(){
	alert('DOES_NOT_WORK.JS: If this has appeared then there is no issue');
	}
}finally{}

Actually I have found bug https://bugzilla.mozilla.org/show_bug.cgi?id=593176 with almost simillar problem, which is marked as duplicate of this one, but I do not see direct connection there possibly because I am not experienced developer and do not understand many things written here.

Can someone answer if there was decided to fix such behavior or there was decided to leave it as it is ?

Thanks.
Duplicate of this bug: 790418
So this is becoming more and more of a compat problem, especially on mobile.  We really need to change things here.  I assume the spec is still totally in limbo?
Whiteboard: [js:p1]
Updated description to make it clear what's needed. bz, please correct it if I got it wrong.
Summary: Functions declared in block statements should be block-local → Functions declared in block statements should be block-local and hoisted to top of block
Whiteboard: [js:p1] → [js:p1:fx20]

Comment 22

5 years ago
Can we come to some agreement about where definitions should be hoisted to? Other browsers seem to just lift everything to the (function) body level.

Comment 23

5 years ago
Lifting to the function top body level is certainly exactly what I am looking for. 

That would preserve the existing semantics, e.g. instead of forcing users to do it manually, which is the existing FF behaviour. It seems like a relatively simple approach that just solves the problem.

Comment 24

5 years ago
This is quite a thorny issue. Hoisting to body level probably makes the most sense for web compatibility, but it makes no sense if you have "let" like SM. For example,

function f(a) {
    var i = 3;
    if (a) {
        let i = 4;
        function k() { return i; }
        return k;
    }
}
f(true)

would evaluate to 3 under hoisting to body level! Still, it's hard to stomach considering the web can't use |let|.

It seems declaring a non-body-level function should act as if the function was a |let| variable in its block. Even if we say that, there are questions open. Is redefining a function equivalent to redeclaring a let and thus banned?

function f(a) {
    var g = 42;
    if (a) {
        function g() {}
    }
    return g;
}

What does |f(true)| evaluate to?

I wish there was a spec!

Comment 25

5 years ago
If |let| is equivalent to creating an additional lambda scope, then any declarations within it proceed as before.

So f above is equivalent the following, which shows no problem with the local k:

function f(a) {
    var i = 3;
    if (a) {
        return let_i(4);
    }

    function let_i(i) {
        function k() { return i; }
        return k;
    }
}

The moral, to me at least, is that you hoist to the top of the nearest enclosing scope. Regular control-blocks are not scopes.

Comment 26

5 years ago
As for the other f example, you have the same question (and answer) with this example I would think:

function f(a) {
    var g = 42;
    if (a) {
    }
    function g() {}
    return g;
}

Also, what do other browsers do? The overall issue is a lack of compatability. Let's all solve it with the same semantics, and the web is a better place. There might not be a spec, but there is de-facto behaviour.

Comment 27

5 years ago
There's not de facto behavior wrt let.

Comment 28

5 years ago
Please make this "uplifting behaviour" (if you implement it) *non-strict-mode* only, please (and work with the other vendors to do the same). It looks like this needs to be spec-ed somehow (even if it's not ECMA-approved), so you can specify a different (non-legacy non-uplifting) behaviour for strict mode.

Comment 29

5 years ago
I am curious, what do other browsers do in strict mode? Do they hoist local functions? If they do, and they remain technically compliant, could not FF do the same?

Comment 30

5 years ago
V8 lifts to the body level except in "harmony scoping mode" (which I don't think is ever enabled on the web) in which case it scopes at the block level.

Comment 31

5 years ago
Please, lets not promote bad/confusing programming behaviour. Even if V8 hoists local functions in strict mode, *Monkey should not do the same. Strict mode is opt-in already, so IMO there is no need to expand this curious behaviour for compat.
Function "declarations" not at the top level of a function or overall program are a syntax error in strict mode in SpiderMonkey, per bug 609832.  I don't think that's going to change, regardless what happens here.  If ECMA decides to change that at some point we would probably follow, but I'm pretty sure we're not changing without their say-so.
See bug 811421 comment 8 for another example of this causing compat problems on mobile...
Assignee: general → jorendorff

Comment 34

4 years ago
(In reply to Boris Zbarsky (:bz) from comment #33)
> See bug 811421 comment 8 for another example of this causing compat problems
> on mobile...

(In reply to John Schoenick [:johns] from comment #8)
> - m.doctissimo.fr […] uses a non-standard JS syntax for functions that could
> be fairly easily fixed by the authors (bug 585536)

So this is something for TE.
V8's behavior:

1. Normally, functions in statement context are hoisted to the enclosing function.
   Later function declarations overwrite earlier ones in the same scope.

   As with standard local functions, "executing" the function "statement"
   does nothing; these functions are created and bound when the script (or innermost
   enclosing function, if any) is called.

2. With both "use strict" and --harmony-scoping, functions in statement context are
   hoisted to the enclosing block. TC39 once wanted this to be the standard. I don't
   think it's standard-track anymore, and --harmony-scoping is not a web-visible
   option, so we shouldn't emulate it.

So we will ignore (2).

The problem is that fully switching to (1) will break existing Mozilla-only code that does stuff like:

    if (monkey) {
        function yum(x) { return x + " banana"; }
    } else {
        function yum(x) { return x + " cookie"; }
    }

Comment 36

4 years ago
> The problem is that fully switching to (1) will break existing Mozilla-only
> code that does stuff like:
> 
>     if (monkey) {
>         function yum(x) { return x + " banana"; }
>     } else {
>         function yum(x) { return x + " cookie"; }
>     }

I think that Mozilla has to decide how portable it wants to be. That code will not work on any other browser (will it?). Does it even work in NodeJS?
AFAIK, the only other implementations mimicking Mozilla's function statements are ancient Safari (up to 3.0.4). See http://kangax.github.com/nfe/#function-statements

Comment 38

4 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #35)
> The problem is that fully switching to (1) will break existing Mozilla-only 
> code that does stuff like:
>     if (monkey) {
>         function yum(x) { return x + " banana"; }
>     } else {
>         function yum(x) { return x + " cookie"; }
>     }

Well that can be fixed, can't it? ;)
(In reply to Florian Bender from comment #38)
> Well that can be fixed, can't it? ;)

Um, yes.  Just as easily as sites depending on any other semantics can be changed, to not depend on function statement -- which in every engine that implement them as anything other than a syntax error, implement them as an extension.  (Function *declarations* are not the same as function *statements*.)  It's not helpful to simply say that code can be fixed, because if that were the simple answer, nobody would be using function statements at all.

Comment 40

4 years ago
Jason was talking about Mozilla-only code – which can easily be fixed before changing the engine. Thus the argument agains his first choice is not really valid. That's all what I was saying.
Mozilla-only code includes code in mozilla-central, which is fixable.  (Although there might be a lot of it.)  But it also includes code in the universe of addons controlled by a multitude of developers, not all actively updating their code, not all of whom we could even contact if we wanted to.  That's no more easily fixed than use of function statements on the web is.  Plus you have sites that have Mozilla-specific code paths that happen to rely on this -- perhaps in Mozilla-specific scripts and code.  (Big sites do have partially distinct implementations for different browser engines, so it's not out of the question.)
In V8, functions are indeed transplanted in a way that flouts lexical scoping.

    try {
        throw 0;
    } catch (x) {
        function f() { return x++; }
    }
    print(f());

The function is transplanted to toplevel, so the "x" inside the function does not refer to the "x" that is lexically in scope. So this gives a ReferenceError.

I think this is too horrible to consider implementing in SpiderMonkey. Instead I am just going to implement exactly what the summary says and hoist to the enclosing block.

Ray Blaak suggests instead hoisting to the enclosing "scope", but that makes me feel sick inside. Blocks should act like scopes, it's the only way.

Drafting an email to es-discuss to ask TC39 to resolve this.

Comment 43

4 years ago
Except that in JS, blocks are not scopes, so we can't pretend that they are. Closures are scopes, and I guess catch blocks are scopes. Anything else?

In Opera, the example above gives an error that is consistent with simply hoisting it. The call to f() doesn't even work inside of the catch block, which is interesting. It seems like the rule is really to blindly hoist it to the top (of the current closure?).

E.g. this:

    try {
        throw 0;
    } catch (x) {
        function f() { return x++; }
        alert("inside of catch: " + f());
    }
    alert("outside of catch: " + f());

gives:

Uncaught exception: ReferenceError: Undefined variable: x
Error thrown at line 8, column 8 in [file...]
    throw 0;
Error initially occurred at unknown location in f() in [file...]
called from line 11, column 8 in [file...]
    alert("inside of catch: " + f());
(In reply to Ray Blaak from comment #43)
> Except that in JS, blocks are not scopes, so we can't pretend that they are.

They are in ES6.

I think the current ES6 draft actually specifies "hoist to enclosing block". Need to read more closely.

Comment 45

4 years ago
This bug only makes sense in the context of regular JS. If blocks have lexical scopes then you cannot hoist anything at all.

With lexical scoping, these are two distinct functions, inaccessible (by name) outside of the scopes:

if (test) {
  var a = 1;
  function f() {return a}
} else {
  var a = 2; // a different var!
  function f() {return a}
}
f() // error, f not found

You have to approach
There's not a distinction to be drawn between ES6 and "regular JS". ES6 is the spec for JS.

'var' hoists to the enclosing function or top-level scope (or, in strict eval code, the eval scope, which is its own special thing). The binding is hoisted, that is. Initialization only happens when the var-statement executes. ('var' is never block-scoped, the way you have it in comment 45 -- that's not what I meant to say.)

'let' and 'const' bindings hoist to the enclosing block or top-level scope.

Function declarations will behave like 'let' and 'const'. This is what ES6 specifies, and I think implementations will converge to that. We'll go first.
olliej pointed out these:
    https://bugs.webkit.org/show_bug.cgi?id=27226#c4

He doesn't think Apple can change the behavior of block-level function-declarations in non-strict mode. My understanding of the discussion is that Apple won't implement the spec.
Just for posterity, here's the (lightly edited) IRC log.

* samth summons olliej
<jorendorff> it's funny, the bug for this https://bugzilla.mozilla.org/show_bug.cgi?id=585536 contains two or three people separately discovering that hoisting, in a language that has let, just makes no sense
<samth> and implement block scope for functions!
<dherman> jorendorff: what hoisting do you mean?
 I think it makes sense with block-hoisting
<jorendorff> var-like hoisting for functions, i mean.
<dherman> ok yeah
 agreed
<olliej> samth: noooooooo
<jorendorff> even without let!
 what it does right now in a catch-block or with statement
<olliej> samth: it’s non trivial for us currently
<jorendorff> oh it's just so wrong
<dherman> yep
<olliej> wait
<olliej> samth: do you mean outside of strict mode????
<samth> olliej: yes
<olliej> samth: because we _cannot_ change that behaviour
<jorendorff> ruh roh
<samth> olliej: wait a second ...
<olliej> samth: jorendorff: this has been discussed on numerous occasions, outside of strict mode, the behavior of nested function statements is not reconcilable
 samth: no one can change to anyone else’s behavior without breaking some set of content
<samth> olliej: this is not my recollection of what's been said in the room at tc39 meetings
<olliej> samth: no
<olliej> samth: the agreement was we could get sane behavior in strict mode and in modules 
<dherman> I think olliej is right
<olliej> samth: but outside of strict mode we’re kind of hosed
<dherman> that sounds right
<olliej> i think ES5 even says that explicitly
 i had a great set of examples somewhere
 just a sec
<dherman> yes, ES5 strict mode poisoned block-local functions
<jorendorff> i'm going to implement the spec anyway
* dherman approves
<dherman> :)
<olliej> dherman: samth jorendorff https://bugs.webkit.org/show_bug.cgi?id=27226#c4
<jorendorff> i don't think you even need the eval in the last example
 and it might even pass strict mode
<olliej> comments 4, 5 and 7
 4,5 give the ways where any engine changing non-strict behavior will break _some_ code
Whiteboard: [js:p1:fx20] → [js:p1:fx21][js:bumped:1]
Duplicate of this bug: 868671
fascinating.
Blocks: 638281
Whiteboard: [js:p1:fx21][js:bumped:1] → [js:p1][js:bumped:1]
It's time to retire this bug.

All the different JS implementations are too constrained by existing content to be able to implement breaking changes and establish a single common behavior. Once we discovered that, the urgency here evaporated.

It's unfortunate, but we've hit this a million times in other areas, where browser-specific is totally baked into the Web. Each time we die a little inside, but we've got to move on.

Now for the tiny glimmer of remaining hope: There *is* a set of use cases that do work in all implementations, and TC39 is trying to standardize just those few common use cases, in an appendix to ES6. See <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-block-level-function-declarations-web-legacy-compatibility-semantics>. I've filed a new bug, bug 1071646, to make sure we follow that spec.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
Assignee: jorendorff → nobody
No longer blocks: 638281, 694100, 584603
Whiteboard: [js:p1][js:bumped:1]
See Also: → bug 1071646
Right, the other good news: under "use strict", block-local functions will do the right thing---exactly what this bug's summary is asking for. Filed bug 1071877 for that.
See Also: → bug 1071877
See Also: → bug 1088554

Updated

2 years ago
Duplicate of this bug: 1142392

Updated

2 years ago
Duplicate of this bug: 1181251

Updated

2 years ago
Duplicate of this bug: 1217837
We're going to fix this after all.
Resolution: WONTFIX → DUPLICATE
Duplicate of bug: 1071646
Duplicate of this bug: 1229998
You need to log in before you can comment on or make changes to this bug.