Bugfix 1183 #2252
Bugfix 1183 #2252
Conversation
| @@ -30,6 +30,11 @@ exports.Scope = class Scope | |||
| else | |||
| @positions[name] = @variables.push({name, type}) - 1 | |||
|
|
|||
| getMethodRecurse: -> | |||
There was a problem hiding this comment.
Can we get a comment above this line with a small description or some usage intentions?
|
Nice. How do you feel about taking care of the related #1392? |
|
I took a look at #1392, I'll give it a stab if I can find some time. |
| @@ -1924,7 +1929,8 @@ Closure = | |||
|
|
|||
| literalThis: (node) -> | |||
| (node instanceof Literal and node.value is 'this' and not node.asKey) or | |||
| (node instanceof Code and node.bound) | |||
| (node instanceof Code and node.bound) or | |||
| (node instanceof Call and node.isSuper) | |||
There was a problem hiding this comment.
@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.
Yes, let me page this back in.
There was a problem hiding this comment.
Hmm. Either this was gratuitous or the tests weren't comprehensive. Either way, not good.
There was a problem hiding this comment.
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.
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:
class A
constructor : ->
@i = 10
foo : ->
console.log "A:foo #{@i}"
class B extends A
foo : ->
x = switch x
when 'a'
something()
else
super()
b = new B()
b.foo()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.
Reasonable, although that particular test case is a wee bit golfed ;)
Want to send the patch?
There was a problem hiding this comment.
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 in nodes.coffee? That's what my ugly switch was trying to do.
There was a problem hiding this comment.
The preferred way to force it is to make the particular node's isStatement check return true.
This should fix #1183. Fixing the compiler error revealed a couple of bugs in the runtime (
_thisversusthisand also passingthisthrough closures). There might be other runtime cases that I missed, since the two cases I tried (wraps and fat-arrows) needed to be handled separately.