Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Bugfix 1183 #2252

Merged
merged 5 commits into from

3 participants

@maxtaco

This should fix #1183. Fixing the compiler error revealed a couple of bugs in the runtime (_this versus this and also passing this through 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.

src/scope.coffee
@@ -30,6 +30,11 @@ exports.Scope = class Scope
else
@positions[name] = @variables.push({name, type}) - 1
+ getMethodRecurse: ->
@michaelficarra Collaborator

Can we get a comment above this line with a small description or some usage intentions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michaelficarra
Collaborator

Nice. How do you feel about taking care of the related #1392?

@maxtaco

I took a look at #1392, I'll give it a stab if I can find some time.

@jashkenas jashkenas merged commit b03bea1 into from
@jashkenas jashkenas commented on the diff
src/nodes.coffee
@@ -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)
@jashkenas Owner

@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?

@maxtaco
maxtaco added a note

Yes, let me page this back in.

@michaelficarra Collaborator

Hmm. Either this was gratuitous or the tests weren't comprehensive. Either way, not good.

@maxtaco
maxtaco added a note

I remember thinking, "man, this is going to be ugly to write a regtest for." Still playing around with it.

@maxtaco
maxtaco added a note

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.

@jashkenas Owner

Reasonable, although that particular test case is a wee bit golfed ;)

Want to send the patch?

@maxtaco
maxtaco added a note

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.

@jashkenas Owner

The preferred way to force it is to make the particular node's isStatement check return true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
19 lib/coffee-script/nodes.js
@@ -750,7 +750,7 @@
Call.prototype.superReference = function(o) {
var accesses, method, name;
- method = o.scope.method;
+ method = o.scope.getMethodRecurse();
if (!method) {
throw SyntaxError('cannot call super outside of a function.');
}
@@ -770,6 +770,15 @@
}
};
+ Call.prototype.superThis = function(o) {
+ var _ref2, _ref3;
+ if (((_ref2 = o.scope) != null ? (_ref3 = _ref2.method) != null ? _ref3.context : void 0 : void 0) != null) {
+ return o.scope.method.context;
+ } else {
+ return "this";
+ }
+ };
+
Call.prototype.unfoldSoak = function(o) {
var call, ifn, left, list, rite, _i, _len, _ref2, _ref3;
if (this.soak) {
@@ -866,20 +875,20 @@
return _results;
})()).join(', ');
if (this.isSuper) {
- return this.superReference(o) + (".call(this" + (args && ', ' + args) + ")");
+ return this.superReference(o) + (".call(" + (this.superThis(o)) + (args && ', ' + args) + ")");
} else {
return (this.isNew ? 'new ' : '') + this.variable.compile(o, LEVEL_ACCESS) + ("(" + args + ")");
}
};
Call.prototype.compileSuper = function(args, o) {
- return "" + (this.superReference(o)) + ".call(this" + (args.length ? ', ' : '') + args + ")";
+ return "" + (this.superReference(o)) + ".call(" + (this.superThis(o)) + (args.length ? ', ' : '') + args + ")";
};
Call.prototype.compileSplat = function(o, splatArgs) {
var base, fun, idt, name, ref;
if (this.isSuper) {
- return "" + (this.superReference(o)) + ".apply(this, " + splatArgs + ")";
+ return "" + (this.superReference(o)) + ".apply(" + (this.superThis(o)) + ", " + splatArgs + ")";
}
if (this.isNew) {
idt = this.tab + TAB;
@@ -2901,7 +2910,7 @@
return node instanceof Literal && node.value === 'arguments' && !node.asKey;
},
literalThis: function(node) {
- return (node instanceof Literal && node.value === 'this' && !node.asKey) || (node instanceof Code && node.bound);
+ return (node instanceof Literal && node.value === 'this' && !node.asKey) || (node instanceof Code && node.bound) || (node instanceof Call && node.isSuper);
}
};
View
11 lib/coffee-script/scope.js
@@ -40,6 +40,17 @@
}
};
+ Scope.prototype.getMethodRecurse = function() {
+ var _ref1;
+ if (((_ref1 = this.method) != null ? _ref1.name : void 0) != null) {
+ return this.method;
+ } else if (this.parent) {
+ return this.parent.getMethodRecurse();
+ } else {
+ return this.method;
+ }
+ };
+
Scope.prototype.find = function(name, options) {
if (this.check(name, options)) {
return true;
View
16 src/nodes.coffee
@@ -496,7 +496,9 @@ exports.Call = class Call extends Base
# Grab the reference to the superclass's implementation of the current
# method.
superReference: (o) ->
- {method} = o.scope
+ # Keep walking up the scope chain until we find the original reference
+ # to a method. Stop at the first one.
+ method = o.scope.getMethodRecurse()
throw SyntaxError 'cannot call super outside of a function.' unless method
{name} = method
throw SyntaxError 'cannot call super on an anonymous function.' unless name?
@@ -508,6 +510,9 @@ exports.Call = class Call extends Base
else
"#{name}.__super__.constructor"
+ superThis : (o) ->
+ if o.scope?.method?.context? then o.scope.method.context else "this"
+
# Soaked chained invocations unfold into if/else ternary structures.
unfoldSoak: (o) ->
if @soak
@@ -566,21 +571,21 @@ exports.Call = class Call extends Base
args = @filterImplicitObjects @args
args = (arg.compile o, LEVEL_LIST for arg in args).join ', '
if @isSuper
- @superReference(o) + ".call(this#{ args and ', ' + args })"
+ @superReference(o) + ".call(#{@superThis o}#{ args and ', ' + args })"
else
(if @isNew then 'new ' else '') + @variable.compile(o, LEVEL_ACCESS) + "(#{args})"
# `super()` is converted into a call against the superclass's implementation
# of the current function.
compileSuper: (args, o) ->
- "#{@superReference(o)}.call(this#{ if args.length then ', ' else '' }#{args})"
+ "#{@superReference(o)}.call(#{@superThis o}#{ if args.length then ', ' else '' }#{args})"
# If you call a function with a splat, it's converted into a JavaScript
# `.apply()` call to allow an array of arguments to be passed.
# If it's a constructor, then things get real tricky. We have to inject an
# inner constructor in order to be able to pass the varargs.
compileSplat: (o, splatArgs) ->
- return "#{ @superReference o }.apply(this, #{splatArgs})" if @isSuper
+ return "#{ @superReference o }.apply(#{@superThis o}, #{splatArgs})" if @isSuper
if @isNew
idt = @tab + TAB
return """
@@ -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)
@jashkenas Owner

@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?

@maxtaco
maxtaco added a note

Yes, let me page this back in.

@michaelficarra Collaborator

Hmm. Either this was gratuitous or the tests weren't comprehensive. Either way, not good.

@maxtaco
maxtaco added a note

I remember thinking, "man, this is going to be ugly to write a regtest for." Still playing around with it.

@maxtaco
maxtaco added a note

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.

@jashkenas Owner

Reasonable, although that particular test case is a wee bit golfed ;)

Want to send the patch?

@maxtaco
maxtaco added a note

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.

@jashkenas Owner

The preferred way to force it is to make the particular node's isStatement check return true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
# Unfold a node's child if soak, then tuck the node under created `If`
unfoldSoak = (o, parent, name) ->
View
12 src/scope.coffee
@@ -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: ->
+ 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) ->
View
32 test/scope.coffee
@@ -41,3 +41,35 @@ test "#1973: redefining Array/Object constructors shouldn't confuse __X helpers"
obj = {arr}
for own k of obj
eq arr, obj[k]
+
+test "#1183: super + fat arrows", ->
+ dolater = (cb) -> cb()
+
+ class A
+ constructor: ->
+ @_i = 0
+ foo : (cb) ->
+ dolater =>
+ @_i += 1
+ cb()
+
+ class B extends A
+ constructor : ->
+ super
+ foo : (cb) ->
+ dolater =>
+ dolater =>
+ @_i += 2
+ super cb
+
+ b = new B()
+ b.foo => eq b._i, 3
+
+test "#1183: super + wrap", ->
+ class A
+ m : -> 10
+ class B extends A
+ constructor : -> super
+ B::m = -> r = try super()
+ eq (new B()).m(), 10
+
Something went wrong with that request. Please try again.