Skip to content
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

Fixnum does not allow overloading the case keyword implementation using === #1486

Closed
stephenlam opened this issue Feb 5, 2014 · 4 comments
Closed
Milestone

Comments

@stephenlam
Copy link

I'm trying to redefine #===.

class Fixnum
  def === other
    true
  end
end

Here is the test case:

case 4
  when 3
     puts "YES"
  else
     puts "NO"
end

On MRI 2.0.0-p0 this works and prints "YES". I tested on Jruby 1.7.3 and 1.7.10 and they both printed "NO". I need this feature to be able to support a simpler syntax in my code (of course, the example above is overly simplified and does not reflect my end use case).

Note that calling 3 === 4 DOES return true for both Ruby and JRuby. However, the "case" statement does not use the redefined #=== method for Fixnum's. So far I have only seen this issue with Fixnum's; I know that String works without issues. You may wish to test this on other classes as well (e.g. Numeric's and Integer's).

EDIT: corrected typo. (MRI prints "YES" and Jruby prints "NO" instead of the other way around.)

@ratnikov
Copy link
Contributor

ratnikov commented Feb 8, 2014

Verified that in a test.

Seems like Fixnums are being handled differently, probably for optimizations. Right now I'm trying to pinpoint the culprit. I want to blame https://github.com/jruby/jruby/blob/jruby-1_7/core/src/main/java/org/jruby/ast/WhenOneArgNode.java#L48 (since FixNum implements IEqlNode) but commenting that out seems to not quite work.

Debugging further...

ratnikov pushed a commit to ratnikov/jruby that referenced this issue Feb 8, 2014
Unfortunately, seems like now comparing two fixnums will incur a regular
ruby object comparison overhead. But that seems unavoidable when we want
to allow to override behavior of even core classes.
@ratnikov
Copy link
Contributor

ratnikov commented Feb 8, 2014

Duh, that was indeed the culprit, it's just that my test was confusing me. Anyhow, PR sent out, but seems like as is it will cause performance regression, since when comparison will always use the "slow" ruby comparison. But then that's how multiple argument When behaves, so I think it's okay.

@headius
Copy link
Member

headius commented Feb 26, 2014

First of all... why do you want to do this?

Second, I came up with a patch that doesn't hurt the fast path as much, but this – like @ratnikov's patch – still only fixes the AST interpreter (not the AST JIT): https://gist.github.com/headius/a93ad238c0b661f62f13

I'm not convinced that fixing this is actually worthwhile.

@enebo enebo added this to the Won't Fix milestone Feb 17, 2017
@enebo
Copy link
Member

enebo commented Feb 17, 2017

We generally like to be compatible but this is one of those corners we do not want to put any effort into it unless there is a good use case. The performance benefits of not supporting this right now outweighs the impl cost of supporting an uncommon feature...

@enebo enebo closed this as completed Feb 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants