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

Superclass mixin implementation called instead of subclass implementation #3976

Closed
manigandan-rajasekar opened this Issue Jun 22, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@manigandan-rajasekar

manigandan-rajasekar commented Jun 22, 2016

Environment

jruby 9.1.2.0 (2.3.0) 2016-05-26 7357c8f Java HotSpot(TM) 64-Bit Server VM 25.91-b14 on 1.8.0_91-b14 +jit [darwin-x86_64]

Darwin 15.5.0 Darwin Kernel Version 15.5.0:

Expected Behavior

The '<' comparison operation should return true for any inputs (contrived example) for both classes.
This is the behavior seen in CRuby.

StringSubclass Is a<b? true
StringSubclass Is b<c? true
StringSubclass Is b<a? true
NotStringSubclass Is a<b? true
NotStringSubclass Is b<c? true
NotStringSubclass Is b<a? true
class StringSubclass < String
  include Comparable

  def <=>(input)
    #puts "JRuby Never executes this code."
    return -1;
  end
end

class NotStringSubclass
  include Comparable

  def initialize(str)
    @str = str
  end

  def <=>(input)
    #puts "JRuby Executes this code since there is no superclass."
    return -1;
  end
end

puts "StringSubclass Is a<b? " + (StringSubclass.new("a")<StringSubclass.new("b")).to_s
puts "StringSubclass Is b<c? " + (StringSubclass.new("b")<StringSubclass.new("c")).to_s
puts "StringSubclass Is b<a? " + (StringSubclass.new("b")<StringSubclass.new("a")).to_s

puts "NotStringSubclass Is a<b? " + (NotStringSubclass.new("a")<NotStringSubclass.new("b")).to_s
puts "NotStringSubclass Is b<c? " + (NotStringSubclass.new("b")<NotStringSubclass.new("c")).to_s
puts "NotStringSubclass Is b<a? " + (NotStringSubclass.new("b")<NotStringSubclass.new("a")).to_s

Actual Behavior

Instead, the '<' comparison operation returns true/false based on the String class implementation of the mixin function for StringSubclass. NotStringSubclass still behaves as expected.

StringSubclass Is a<b? true
StringSubclass Is b<c? true
StringSubclass Is b<a? false
NotStringSubclass Is a<b? true
NotStringSubclass Is b<c? true
NotStringSubclass Is b<a? true

@manigandan-rajasekar manigandan-rajasekar changed the title from Overriden mixin never being called to Superclass mixin implementation called instead of subclass implementation Jun 22, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 9, 2016

Member

This is because our String#< and most of the other comparable methods are hardcoded to use the base impl always. We either overstepped on this optimization or MRI backpedaled on it. In any case, we should fix this.

Member

headius commented Jul 9, 2016

This is because our String#< and most of the other comparable methods are hardcoded to use the base impl always. We either overstepped on this optimization or MRI backpedaled on it. In any case, we should fix this.

@headius headius added this to the JRuby 9.1.3.0 milestone Jul 9, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 11, 2016

Member

I can confirm MRI does not implement the various Comparable methods directly on String, and just lets them delegate to the re-dispatching impls. We can do that in the short term, but I'd like to still have fast guarded logic in place to skip the dispatch when possible.

Member

headius commented Jul 11, 2016

I can confirm MRI does not implement the various Comparable methods directly on String, and just lets them delegate to the re-dispatching impls. We can do that in the short term, but I'd like to still have fast guarded logic in place to skip the dispatch when possible.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 26, 2016

Member

I've merged a fix along with the call site caching improvements in d2e6c5b. When that branch merges, this will be fixed.

Member

headius commented Jul 26, 2016

I've merged a fix along with the call site caching improvements in d2e6c5b. When that branch merges, this will be fixed.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 26, 2016

Member

FWIW, that branch == java_call_sites

Member

headius commented Jul 26, 2016

FWIW, that branch == java_call_sites

@headius headius closed this in d2e6c5b Aug 5, 2016

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