Closed
Bug 610350
Opened 15 years ago
Closed 15 years ago
Assigning to a named function's name in strict mode code should throw
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla2.0b9
People
(Reporter: jorendorff, Assigned: Waldo)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
5.08 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
(function f(){ "use strict"; f = 12; })();
This should throw a TypeError, since f's binding is immutable. Even in ES3 it was ReadOnly.
(Note this is a FunctionExpression, not a FunctionDeclaration. In the latter case, the assignment would succeed.)
Reporter | ||
Comment 1•15 years ago
|
||
I could see this blocking release. Getting stricter over time is really bad.
Comment 3•15 years ago
|
||
Is this a TypeError as well? `(function f(f){"use strict"})()`
Assignee | ||
Comment 4•15 years ago
|
||
I don't think so. It looks like functions get a [[Scope]] containing a single entry for the name of the function, and a new environment (chaining to [[Scope]]) is created for parameters and such. So having an argument name that's the same as the function name is kosher, if unfortunate, and appears to me to mean that the function expression can't be referred to by that name.
Assignee | ||
Updated•15 years ago
|
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•15 years ago
|
||
This undoes optimization of function-expression-name to JSOP_CALLEE when in strict mode where the name is at some point or other assigned. That makes the testcase here properly throw -- but only sometimes. Instead of the name binding to the function name binding, it instead refers to any such binding in an enclosing scope -- so the testcase here would throw a ReferenceError because there's no "f" binding to set. (In other cases it might actually mutate an outer binding!) There's clearly still work to do to implement this correctly, but this is a start -- the rest can wait for the next time I need to clear my mind by hacking something not in my pending task list.
Comment 6•15 years ago
|
||
(In reply to comment #4)
> I don't think so. It looks like functions get a [[Scope]] containing a single
> entry for the name of the function, and a new environment (chaining to
> [[Scope]]) is created for parameters and such. So having an argument name
> that's the same as the function name is kosher, if unfortunate, and appears to
> me to mean that the function expression can't be referred to by that name.
I think you're right, Jeff.
From what I can see, `(function f(){})()` creates separate declarative environment specifically to hold "f" binding — which it creates via CreateImmutableBinding. Newly created function's Scope is set to this one-binding-holding declarative environment, whose Outer Lexical Environment references currently executing one. So "f" is essentially injected in the scope chain of the function.
Later, during phase of variable and function declarations, they are bound to newly created variable environment (via CreateMutableBinding/SetMutableBinding), and so "f" formal parameter simply shadows "f" of declarative environment that's further up in the scope chain:
<currently executing env.>
^
^
[
Outer Lexical Env.,
Declarative Env. ({ f: <function f> })
]
^
^
<function f>.[[Scope]]
Then:
(function f() {"use strict"; f = 12; })(); // TypeError
(function f() {"use strict"; var f; })(); // f is undefined
(function f() {"use strict"; function f(){} })(); // f references inner function
(function f(f) {"use strict"; })(); // f is undefined
Btw, any chance Mozilla can extend es5-strict for `(function f(f){})()` to throw or at least issue warning?
Assignee | ||
Comment 7•15 years ago
|
||
If I'd noticed it a year ago, I probably would have requested it, maybe even pushed for it. At this point, I'm not sure it's all that important that it's worth Lone-Rangering it into existence.
Assignee | ||
Comment 8•15 years ago
|
||
Better idea: mark function expressions which stupidly assign to their own name as heavyweight. Existing logic (see sibling code to the modified comment in jsemit.cpp) ensures that such functions never use JSOP_CALLEE, and heavyweightness ensures that the function's name is bound in the function's call object.
Attachment #490025 -
Attachment is obsolete: true
Attachment #490250 -
Flags: review?(jimb)
Comment 9•15 years ago
|
||
Not sure what you mean by "heavyweight"... Is this about optimizing such function by avoiding creation of extra declarative environment (when one of formal parameters matches function identifier)?
Assignee | ||
Comment 10•15 years ago
|
||
"Heavyweight" is an ill-defined internal concept that means certain optimizations won't be made. The desired effect here is that an object will be created for the function name, its variables, its parameters, and so on (we can sometimes avoid creating them, but this forces one to be created and used) to be used for name lookup, ensuring that the lookup of the function name finds the correct, immutable binding.
Comment 11•15 years ago
|
||
Kangax: there's no obvious problem with a formal or local shadowing the function name. Many people give function expressions names for debuggability. They should not get a strict error just because they shadow.
Waldo: Kangax is not into SpiderMonkey internals but it is not the case that what HEAVYWEIGHT means is ill-defined. If you want docs, here's a start, but it is completely defined in the code and the design backing the code.
A heavyweight function is one that requires a Call object reifying its scope at runtime. Heavyweightness is well-defined entirely within jsparse.cpp as follows. A function is heavyweight if it:
* calls direct eval
* contains a with statement
* assigns to arguments via =, ++, --
* deletes arguments
* declares an initialized var or const named arguments
* uses the debugger statement
* contains a function sub-statement (SpiderMonkey extension)
* contains (possibly nested in intervening functions) a flat closure that uses upvars in the function in question, which cannot be flattened (copied into the flat closure because they are not initialized only with the closure post-dominating the initialization)
* contains a heavyweight function
* uses E4X's :: name-qualifier operator
* uses E4X's filtering predicate expression
* uses E4X's "default xml namespace = ..." statement
rs=me on adding as a comment, and then the trick will be maintaining the comment to match the code.
/be
Comment 12•15 years ago
|
||
Of course the list in comment 11 needs an addition for the fix in this bug's patch. Good fix!
/be
Assignee | ||
Comment 13•15 years ago
|
||
I think we're using two different yet reasonable definitions for "defined". Here's my rationale for mine. (But note that I do not offer it to attempt to argue that mine is categorically better -- merely to explain what it is and why I have adopted it. I don't think we're well-served by arguing for one of the other of us to use the definition of the other -- it's just words, after all.)
Perhaps it's true that heavyweightness is completely defined by the code. But by this standard, all concepts in the code are completely defined. If that is how to evaluate defined-ness, then it seems to me defined-ness isn't a useful measure when considering a body of code. I think we are served by richer language for describing our code. This is why I interpret defined-ness as a measure of the quality of the documentation and code for a particular concept, not whether the code implements the concept.
In any case this is afield from this bug's topic, so I took my concerns, created a patch to address them, and filed bug 612426 to cover the matter.
Comment 14•15 years ago
|
||
Compiler deoptimization triggers are often ad-hoc or defined by a list of cases. A catchy name is worth its weight, for grepability if no better reason (humor?). Shaver and I picked heavyweight with some care, knowing the definition was not a simple phrase.
/be
Comment 15•15 years ago
|
||
Comment on attachment 490250 [details] [diff] [review]
Patch and test
Did this get lost in the sideshow over HEAVYWEIGHT?
r=me anyway, we should get it in for beta9.
/be
Attachment #490250 -
Flags: review+
Assignee | ||
Comment 16•15 years ago
|
||
It didn't get lost. jimb simply doesn't get to his review queue often, and the patch was minimal and low-maintenance enough I didn't feel compelled to push it. But thanks for the review in any case...
Assignee | ||
Comment 17•15 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla2.0b9
Assignee | ||
Updated•15 years ago
|
Attachment #490250 -
Flags: review?(jimb)
Comment 18•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•