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

Passing block explicitly to superclass method fails #1237

Closed
wied03 opened this Issue Dec 9, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@wied03
Contributor

wied03 commented Dec 9, 2015

At one point this worked (or maybe was not fully working) in Opal 0.9 but now it complains:

class SuperClass
  def add_stub(&block)
    puts 'super method, calling block'
    block.call
  end
end

class InheritingClass < SuperClass
  def add_stub(*args, &block)
    super(*args, &block)
  end
end

InheritingClass.new.add_stub do
  puts 'block ran'
end

Runs OK in MRI. It does not happen with a standalone method nor does it happen when 1 method calls another in the same fashion, just when calling the superclass' method. This also causes arity checking to break.

MRI:

super method, calling block
block ran

Opal:

super method, calling block

/tmp/opal-nodejs-runner-20151209-19233-13hhbbd:4249
      throw exception;
            ^
call: undefined method `call' for nil
    at OpalClass.Opal.defs.self [as $new] (/tmp/opal-nodejs-runner-20151209-19233-13hhbbd:4466:15)
    at NilClass.Opal.defn.TMP_1 (/tmp/opal-nodejs-runner-20151209-19233-13hhbbd:3028:54)
    at NilClass.method_missing_stub [as $call] (/tmp/opal-nodejs-runner-20151209-19233-13hhbbd:888:35)
    at $InheritingClass.Opal.defn.TMP_1 (/tmp/opal-nodejs-runner-20151209-19233-13hhbbd:19037:20)
    at $InheritingClass.Opal.defn.TMP_2 [as $add_stub] (/tmp/opal-nodejs-runner-20151209-19233-13hhbbd:19056:72)
    at Opal.dynamic_require_severity (/tmp/opal-nodejs-runner-20151209-19233-13hhbbd:19061:66)
    at Object.<anonymous> (/tmp/opal-nodejs-runner-20151209-19233-13hhbbd:19062:3)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)

@wied03 wied03 referenced this issue Dec 9, 2015

Closed

Get working with arity checking enabled #40

7 of 12 tasks complete
@elia

This comment has been minimized.

Show comment
Hide comment
@elia

elia Dec 9, 2015

Member

the number -4 is right (confirmed on cruby) so the problem must reside in the check:

opal/lib/opal/nodes/def.rb

Lines 238 to 267 in 3d5f8d3

# Returns code used in debug mode to check arity of method call
def arity_check(args, opt, splat, kwargs, block_name, mid)
meth = mid.to_s.inspect
arity = args.size - 1
arity -= (opt.size)
arity -= 1 if splat
arity -= (kwargs.size)
arity -= 1 if block_name
arity = -arity - 1 if !opt.empty? or !kwargs.empty? or splat
# $arity will point to our received arguments count
aritycode = "var $arity = arguments.length;"
if arity < 0 # splat or opt args
min_arity = -(arity + 1)
max_arity = args.size - 1
max_arity -= 1 if block_name
checks = []
checks << "$arity < #{min_arity}" if min_arity > 0
checks << "$arity > #{max_arity}" if max_arity and not(splat)
aritycode + "if (#{checks.join(' || ')}) { Opal.ac($arity, #{arity}, this, #{meth}); }" if checks.size > 0
else
aritycode + "if ($arity !== #{arity}) { Opal.ac($arity, #{arity}, this, #{meth}); }"
end
end
end

Member

elia commented Dec 9, 2015

the number -4 is right (confirmed on cruby) so the problem must reside in the check:

opal/lib/opal/nodes/def.rb

Lines 238 to 267 in 3d5f8d3

# Returns code used in debug mode to check arity of method call
def arity_check(args, opt, splat, kwargs, block_name, mid)
meth = mid.to_s.inspect
arity = args.size - 1
arity -= (opt.size)
arity -= 1 if splat
arity -= (kwargs.size)
arity -= 1 if block_name
arity = -arity - 1 if !opt.empty? or !kwargs.empty? or splat
# $arity will point to our received arguments count
aritycode = "var $arity = arguments.length;"
if arity < 0 # splat or opt args
min_arity = -(arity + 1)
max_arity = args.size - 1
max_arity -= 1 if block_name
checks = []
checks << "$arity < #{min_arity}" if min_arity > 0
checks << "$arity > #{max_arity}" if max_arity and not(splat)
aritycode + "if (#{checks.join(' || ')}) { Opal.ac($arity, #{arity}, this, #{meth}); }" if checks.size > 0
else
aritycode + "if ($arity !== #{arity}) { Opal.ac($arity, #{arity}, this, #{meth}); }"
end
end
end

@wied03

This comment has been minimized.

Show comment
Hide comment
@wied03

wied03 Dec 9, 2015

Contributor

Looks like in the superclass case, the block isn't in TMP_1.$$p, it's the last argument in arguments, which causes arguments to be of length 5 instead of the 4 it's expecting

Contributor

wied03 commented Dec 9, 2015

Looks like in the superclass case, the block isn't in TMP_1.$$p, it's the last argument in arguments, which causes arguments to be of length 5 instead of the 4 it's expecting

@wied03

This comment has been minimized.

Show comment
Hide comment
@wied03

wied03 Dec 9, 2015

Contributor

In a way, this has nothing to do with arity checking. If I disable arity checking, then the code fails because block in the add_stub method is nil

Contributor

wied03 commented Dec 9, 2015

In a way, this has nothing to do with arity checking. If I disable arity checking, then the code fails because block in the add_stub method is nil

@elia

This comment has been minimized.

Show comment
Hide comment
@elia

elia Dec 9, 2015

Member

So it's more like the arity check message can be improved…

Member

elia commented Dec 9, 2015

So it's more like the arity check message can be improved…

@wied03

This comment has been minimized.

Show comment
Hide comment
@wied03

wied03 Dec 9, 2015

Contributor

Maybe. The bigger problem is the block thing. This reminds me of #1132.

Contributor

wied03 commented Dec 9, 2015

Maybe. The bigger problem is the block thing. This reminds me of #1132.

@wied03 wied03 changed the title from Arity Checking (Regression?) to Passing block explicitly to superclass method fails Dec 9, 2015

wied03 added a commit to wied03/opal that referenced this issue Feb 29, 2016

Fix #1237 by focusing the runtime method solely on finding the functi…
…on, then alter the compiler to cover missing functionality use the existing call node to avoid duplicating logic.

@elia elia closed this in #1367 Feb 29, 2016

elia added a commit that referenced this issue Feb 29, 2016

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