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

JRuby 9.0.1.0 vs. MRI 2.2.2 define_method inconsistency #3326

Closed
PetrKaleta opened this Issue Sep 15, 2015 · 7 comments

Comments

Projects
None yet
3 participants
@PetrKaleta

PetrKaleta commented Sep 15, 2015

Hey, after upgrading from JRuby 1.7.22 to JRuby 9.0.1.0 I found, that warden is raising errors when logging user in. So I was digging a little bit and found, that there's inconsistency in behaviour between ruby versions.

Let me describe it on the following code (working subset from warden gem):

class SessionSerializer
  def serialize(obj)
    obj
  end
end

class Manager
  def self.serialize_into_session(&block)
    SessionSerializer.send :define_method, :serialize, &block
  end
end

And now how it behaves on JRuby 1.7.22 and MRI 2.2.2

Manager.serialize_into_session { |obj| obj.to_s } # => :serialize
SessionSerializer.new.serialize(1) # => 1
Manager.serialize_into_session(&:to_s) # => :serialize
SessionSerializer.new.serialize(1) # => 1

So the result is as expected, but now what about JRuby 9.0.1.0:

Manager.serialize_into_session { |obj| obj.to_s } # => :serialize
SessionSerializer.new.serialize(1) # => 1
Manager.serialize_into_session(&:to_s) # => :serialize
SessionSerializer.new.serialize(1) # ArgumentError: wrong number of arguments (1 for 0)

Hope this helps to identify this define_method inconsistency.

@PetrKaleta

This comment has been minimized.

Show comment
Hide comment
@PetrKaleta

PetrKaleta Nov 2, 2016

@enebo this is still an issue even in 9.1.5.0

PetrKaleta commented Nov 2, 2016

@enebo this is still an issue even in 9.1.5.0

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 2, 2016

Member

@PetrKaleta That is a good question. I know we fixed a number of proc passing bugs since then and I also know recently there was some activity with CI and 9.1.5.0 + warden. I would be inclined to think it has been fixed but I am not sure how to test this. I cannot run your snippets can I? Any way for you to retest?

Member

enebo commented Nov 2, 2016

@PetrKaleta That is a good question. I know we fixed a number of proc passing bugs since then and I also know recently there was some activity with CI and 9.1.5.0 + warden. I would be inclined to think it has been fixed but I am not sure how to test this. I cannot run your snippets can I? Any way for you to retest?

@PetrKaleta

This comment has been minimized.

Show comment
Hide comment
@PetrKaleta

PetrKaleta Nov 2, 2016

@enebo sure you can, its a fully working example. Means just copy & paste it, no other dependency needed

PetrKaleta commented Nov 2, 2016

@enebo sure you can, its a fully working example. Means just copy & paste it, no other dependency needed

@PetrKaleta

This comment has been minimized.

Show comment
Hide comment
@PetrKaleta

PetrKaleta Nov 2, 2016

@enebo and I don't think it's an issue related to proc passing. It looks like issue of inconsistent define_method behaviour. As you can see SessionSerializer already has serialize method, but it's redefined to serialize method withou argument. But JRuby 1.7.x and MRI keeps the original...

PetrKaleta commented Nov 2, 2016

@enebo and I don't think it's an issue related to proc passing. It looks like issue of inconsistent define_method behaviour. As you can see SessionSerializer already has serialize method, but it's redefined to serialize method withou argument. But JRuby 1.7.x and MRI keeps the original...

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 2, 2016

Member

@PetrKaleta ah I was confused by the working subset comment. I thought it still required warden and was an example. So ok I have more info. I would say your understanding is not quite right but I am unclear on what is happening. Check out this snippet:

class SessionSerializer
  def serialize(obj)
    puts "HELL YES: #{obj}"
  end
end

class Manager
  def self.serialize_into_session(&block)
    SessionSerializer.send :define_method, :serialize, &block
  end
end

def frogger
  puts "HOH"
end
public :frogger

Manager.serialize_into_session(&:frogger) # => :serialize
SessionSerializer.new.serialize(1) # ArgumentError: wrong number of arguments (1 for 0)

If I run this in MRI I get "HOH" printed out so it seems to definitely be calling the new method and not the original one. If I add a param to frogger then MRI stops working with an argument error...

Member

enebo commented Nov 2, 2016

@PetrKaleta ah I was confused by the working subset comment. I thought it still required warden and was an example. So ok I have more info. I would say your understanding is not quite right but I am unclear on what is happening. Check out this snippet:

class SessionSerializer
  def serialize(obj)
    puts "HELL YES: #{obj}"
  end
end

class Manager
  def self.serialize_into_session(&block)
    SessionSerializer.send :define_method, :serialize, &block
  end
end

def frogger
  puts "HOH"
end
public :frogger

Manager.serialize_into_session(&:frogger) # => :serialize
SessionSerializer.new.serialize(1) # ArgumentError: wrong number of arguments (1 for 0)

If I run this in MRI I get "HOH" printed out so it seems to definitely be calling the new method and not the original one. If I add a param to frogger then MRI stops working with an argument error...

@PetrKaleta

This comment has been minimized.

Show comment
Hide comment
@PetrKaleta

PetrKaleta Nov 2, 2016

@enebo ah pardon, here is the point

class SessionSerializer
  def serialize(obj)
    obj.to_s
  end
end

class Manager
  def self.serialize_into_session(&block)
    SessionSerializer.send :define_method, :serialize, &block
  end
end
SessionSerializer.new.serialize(1) # => "1"

So there's some SessionSerializer with a default method serialize that takes an object and stringify it. But user can define its own serializer by, like this:

Manager.serialize_into_session { |obj| "Custom serialization of: #{obj}" }

So now when you call the serialize method:

SessionSerializer.new.serialize(1)  # => "Custom serialization of: 1"

But JRuby 9.x.x fails on that block passing, you were right :)

PetrKaleta commented Nov 2, 2016

@enebo ah pardon, here is the point

class SessionSerializer
  def serialize(obj)
    obj.to_s
  end
end

class Manager
  def self.serialize_into_session(&block)
    SessionSerializer.send :define_method, :serialize, &block
  end
end
SessionSerializer.new.serialize(1) # => "1"

So there's some SessionSerializer with a default method serialize that takes an object and stringify it. But user can define its own serializer by, like this:

Manager.serialize_into_session { |obj| "Custom serialization of: #{obj}" }

So now when you call the serialize method:

SessionSerializer.new.serialize(1)  # => "Custom serialization of: 1"

But JRuby 9.x.x fails on that block passing, you were right :)

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 2, 2016

Member

Reduced case (bottom one):

# Ruby proc with -1 arity works
s = proc { |*a| a.first.to_s }
define_method :serialize, s
p s.arity
p serialize(0)

# Symbol.to_proc generated method does not work
s = :to_s.to_proc
define_method :serialize, s
p s.arity
p serialize(1)

So Symbol.to_proc proc claims to have -1 arity like first working example but we stumble over argument error to seemingly not having a negative arity (I would bet on 0).

Member

enebo commented Nov 2, 2016

Reduced case (bottom one):

# Ruby proc with -1 arity works
s = proc { |*a| a.first.to_s }
define_method :serialize, s
p s.arity
p serialize(0)

# Symbol.to_proc generated method does not work
s = :to_s.to_proc
define_method :serialize, s
p s.arity
p serialize(1)

So Symbol.to_proc proc claims to have -1 arity like first working example but we stumble over argument error to seemingly not having a negative arity (I would bet on 0).

@enebo enebo closed this in 643d442 Nov 2, 2016

@enebo enebo added this to the JRuby 9.1.6.0 milestone Nov 2, 2016

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