Closed
Bug 185485
Opened 23 years ago
Closed 23 years ago
with (x) {function f() {}} defines global function f even if x.f exists
Categories
(Core :: JavaScript Engine, defect, P3)
Core
JavaScript Engine
Tracking
()
VERIFIED
INVALID
mozilla1.3beta
People
(Reporter: user, Assigned: brendan)
Details
(Keywords: js1.5)
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.1) Gecko/20021003
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.1) Gecko/20021003
When a function is defined under a with statement, JavaScript always defines the
function in global or function scope even if a property with the function name
exists in the with object.
Reproducible: Always
Steps to Reproduce:
Run the following script in the SM shell:
var x = { f: 0, g: 0 };
with (x) {
function f() { }
var g = function() { }
}
print(x.f);
print(x.g);
print(this.f);
Actual Results:
It prints:
0
function () {
}
function f() {
}
Expected Results:
function () {
}
function f() {
}
undefined
|
||
Comment 1•23 years ago
|
||
outdated build.
Status: UNCONFIRMED → RESOLVED
Closed: 23 years ago
Resolution: --- → INVALID
|
|
Reporter | |
Comment 2•23 years ago
|
||
Sorry to insist, but I got the bug with js from the latest CVS build via make -f
Makefile.ref
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
|
|
||
Comment 3•23 years ago
|
||
Igor is right; confirming bug in current SpiderMonkey shell:
----------------- create an object with property |f| -------------------
js> var x = {f:0};
js> x.f
0
------------------- with statement + var statement ---------------------
js> with (x) {var f = 1};
js> x.f
1
---------------- with statement + function statement -------------------
js> with (x) {function f() {}};
js> x.f
1
js> this.f
function f() {
}
Using |with + var statement|, the property |f| gets properly reset.
Using |with + function statement|, |f| does not get reset. The function
definition is made in global scope instead.
Assignee: rogerl → khanson
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
Summary: with (x) { function f() {} } define global function f even if x.f exists → with (x) {function f() {}} defines global function f even if x.f exists
|
|
||
Comment 4•23 years ago
|
||
Testcase added to JS testsuite:
mozilla/js/tests/js1_5/Scope/regress-185485.js
Currently passing in Rhino, failing in SpiderMonkey.
Note: not all sections of the test fail in SpiderMonkey.
As noted above, |var| statements are working correctly
inside the with-block. Thus, the section that tests |x.g|
above is passing.
|
|
Assignee | |
Comment 5•23 years ago
|
||
ECMA does not specify the semantics of this extension. But it's wrong for var
and function to be inconsistent. Patch in a minute.
/be
Assignee: khanson → brendan
Keywords: js1.5,
mozilla1.3
Priority: -- → P3
Target Milestone: --- → mozilla1.3beta
|
|
Assignee | |
Comment 6•23 years ago
|
||
Patch later, need sleep. This is uglier than it seemed at first.
/be
|
|
Reporter | |
Comment 7•23 years ago
|
||
MSIE follows the same behaviour as current SM. Given:
<html><body><pre><script>
var x = { f: 0, g: 0 };
with (x) {
function f() { }
var g = function() { }
}
document.writeln(x.f);
document.writeln(x.g);
document.writeln(this.f);
</script></pre></body></html>
MSIE shows
0
function() { }
function f() { }
with a similar output in Mozilla.
Changing this can break the existing web pages so it may not be a good idea at
the end. What is the behaviour in Netscape 4?
|
|
Reporter | |
Comment 8•23 years ago
|
||
In Netscape 4.7 function f modifies x.f so fixing the bug would make Mozilla js
more compatible with Netscape 4.7 but less compatible with MSIE >= 5, where
function expression statements in SM sense are always treated as function
statements even if they appear inside with.
I found this bug while fixing Rhino bug 184107 where my initial thinking was
that under with function expression statements should behave similarly to var
statements, but then SM showed its current behavior of always putting functions
to activation scope and I would like Rhino to behave as closer to SM as possible.
So will this bug be fixed or closed as wont fix?
|
|
Assignee | |
Comment 9•23 years ago
|
||
I'd rather not fix this. Cc'ing waldemar for his thoughts.
/be
|
|
||
Comment 10•23 years ago
|
||
According to ECMA, this is a syntax error because a function definition cannot
be nested inside a block.
However, if the syntax error didn't happen, then the ECMA behavior would be to
define the function in the global or function scope. That's because section
10.1.3 scans over the code looking for var and function declarations and
declares the variables and functions before any of the code (including with
statements) gets to run. Note that this section deliberately treats variables
and functions somewhat differently -- variables are created and left
uninitialized until actually executed (which is why with (foo) {var x = 5} can
create a global variable x with the value undefined and then assign 5 to foo.x
assuming it already exists) while functions are immediately initialized to the
corresponding closures.
Incidentally, the reason we didn't allow function declarations to be nested
inside blocks in ECMAScript is because it's hard/impossible to create a function
closure that captures the with statement's added environment frame before the
with statement gets to run.
Status: NEW → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → INVALID
|
|
Reporter | |
Comment 11•23 years ago
|
||
But why
with(x) {
function f() { }
}
is a syntax error according to Ecma 262 v3? For me it looks like standalong
named function expression with no side effects on any scope and the current
behaviour is a violation of Ecma v3.
is a syntax error according to Ecma 262 v3? For me it looks like standalone
named function expression with no side effects on any scope and the current
behavior is a violation of Ecma.
|
|
Assignee | |
Comment 12•23 years ago
|
||
The with statement is a botch; I regret it.
Phil, can you file a rhino bug? Thanks,
/be
|
|
||
Comment 13•23 years ago
|
||
Actually, I think the existing Rhino bug 184107 already covers the
problems discussed here. I will adjust the testcases for each bug.
Note, this isn't generating a syntax error in SpiderMonkey or Rhino.
Is that something that needs to be fixed?
IN SPIDERMONKEY:
js> var x = {};
js> with(x) {function f() {}}
js>
js> this.f
function f() {
}
---- OR: ----
js> var x = {f:0};
js> with(x) {function f() {}}
js> x.f
0
js> this.f
function f() {
}
|
|
Assignee | |
Comment 14•23 years ago
|
||
No syntax error needed -- waldemar's point was that ECMA allows syntax
extensions where the spec currently would report an error. See Chapter 16.
/be
|
|
||
Comment 15•23 years ago
|
||
Right.
Also, in response to comment 11, look at where in the grammar a
FunctionDeclaration is allowed. It's referenced in section 14 as part of a
SourceElement, which can appear either at the top level of the program or at the
top level of a FunctionBody (section 13). A Statement cannot expand into a
FunctionDeclaration, so a block or any other substatement cannot contain
standalone function declarations (function expressions are OK -- they're part of
the expression grammar).
The reason we wrote the standard this way in ECMA is precisely to avoid bugs
like this one.
|
|
Assignee | |
Comment 16•23 years ago
|
||
One good thing about "function statements" that JS customers actually use: they
enable conditional function declarations and "#if 0" tricks, such as
if (0) function big() {...}
if (DEBUGGING) function debughelper() {...}
SpiderMonkey will eliminate function big, given the above.
/be
|
|
Reporter | |
Comment 17•23 years ago
|
||
For comments 15:
According to Ecma 262 v3:
FunctionExpression :
function Identifier_opt ( FormalParameterListopt ) { FunctionBody }
...
MemberExpression :
PrimaryExpression
FunctionExpression
MemberExpression [ Expression ]
MemberExpression . Identifier
new MemberExpression Arguments
So function expression can be anywhere where member expression is allowed. Now
inside any block standalone function f() { } can not be a function declaration
but then it is a function expression with optional function name which
effectively a NOP in this case. Or did I miss something? I.e. IMO one can write
according to Ecma
if (x) {
a
}
so according to the gramma fragment
if (x) {
function () {}
function a() {}
}
should also be OK.
|
|
Reporter | |
Comment 18•23 years ago
|
||
One more question about the current SM semantics regarding a function expression
statement. As far as I can see it always defines the function in the current
activation object (ignoring scope chain as with example shows) but only when it
is evaluated, not at the parent function/script entrance. For example, given
function test_var() {
print("x="+x);
if (true) {
var x;
}
}
function test_nested_function() {
print("nested="+nested);
if (true) {
function nested() {
}
}
}
test_var();
test_nested_function();
current SM tip prints after js x.js:
x=undefined
x.js:9: ReferenceError: nested is not defined
and this is expected. Right?
Note that in MSIE >= 5.0
<html><body><pre><script>
function test_nested_function() {
document.writeln("nested="+nested);
if (false) {
function nested() { }
}
}
test_nested_function();
</script></pre></body></html>
gives
nested=function nested() { }
as it seems in MSIE the function expression statement is treated just as
function statement, which is more consistent then SM extension which is not
compatible with Netscape 4.7 either.
|
|
||
Comment 19•23 years ago
|
||
Regarding comment 17:
According to ECMA Edition 3, an ExpressionStatement cannot start with the
'function' keyword (see 12.4). Thus,
if (x) {
function () {}
function a() {}
}
has syntax errors on both functions. If you wrote
if (x) {
(function () {});
(function a() {})
}
you'd be OK (note that without the semicolon this means something else), but the
code wouldn't really do anything. It would not define 'a' in any scope.
|
|
||
Comment 20•23 years ago
|
||
Marking Verified.
I have corrected the above testcase for this bug in light
of the discussions here. It now passes in SpiderMonkey.
After the recent checkin for the Rhino analogue bug 184107,
it now passes in Rhino, too -
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•