-
Notifications
You must be signed in to change notification settings - Fork 2k
Bugfix 1183 #2252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix 1183 #2252
Changes from all commits
5542e00
7b66e22
a92af02
2f13fae
b03bea1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,18 @@ exports.Scope = class Scope | |
else | ||
@positions[name] = @variables.push({name, type}) - 1 | ||
|
||
# When `super somearg` is called, we need to find the name of the current | ||
# method we're in, so we know how to address the same method of the | ||
# super class. This can get complicated if super is being called | ||
# from a closure. getMethodRecurse() will walk up the scope | ||
# tree until it finds the first method object that has a name filled | ||
# in. It will return the topmost method object otherwise, which may | ||
# be null if this is being called from outside a function. | ||
getMethodRecurse: -> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we get a comment above this line with a small description or some usage intentions? |
||
if @method?.name? then @method | ||
else if @parent then @parent.getMethodRecurse() | ||
else @method # {} for a function, and null for a non-function | ||
|
||
# Look up a variable name in lexical scope, and declare it if it does not | ||
# already exist. | ||
find: (name, options) -> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxtaco -- @michaelficarra, as part of my refactoring of this commit in 34be878, I removed this change. No tests failed after I did so. Was this needed for some behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let me page this back in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Either this was gratuitous or the tests weren't comprehensive. Either way, not good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember thinking, "man, this is going to be ugly to write a regtest for." Still playing around with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I was wrong about the above, it's a pretty simple case. Here is but one instance when the current code gets it wrong:
With my fix, this snippet works and prints "A:foo 10". The new proposed fix prints "A:foo undefined" What's going on is this. Sometimes the code in the method that calls super will be compiled into a wrapped closure. If that's so, it has to trigger the check that calls the wrapper closure with
this
from the parent. With the new proposed fix, the closure is called with no arguments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasonable, although that particular test case is a wee bit golfed ;)
Want to send the patch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seriously, I'm in a mad rush. Sure, I'll send something off later tonight. What's the preferred way to force a
node.compileClosure
on line 47 innodes.coffee
? That's what my uglyswitch
was trying to do.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The preferred way to force it is to make the particular node's
isStatement
check returntrue
.