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.

Sign up for GitHub

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

Jump to bottom

Case with complex when uses == instead of === #4804

Closed
MaxLap opened this issue Oct 4, 2017 · 12 comments
Closed

Case with complex when uses == instead of === #4804

MaxLap opened this issue Oct 4, 2017 · 12 comments
Labels
ir
Milestone
JRuby 9.1.14.0

Comments

@MaxLap
Copy link

MaxLap commented Oct 4, 2017 •
edited
Loading

Environment

jruby 9.1.9.0 and jruby 9.1.7.0
Using mode 1.9, didn't test with others.
Ubuntu 16.04
Linux max-u1604 4.4.0-96-generic #119-Ubuntu SMP Tue Sep 12 14:59:54 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

In irb, or in a file, put the following:

class EqEqEq
  def ===(other); puts 'test'; end
  def ==(other); puts 'test2'; end
end
eqeqeq = EqEqEq.new
case 1; when eqeqeq; end
puts 'between'
case 1; when (3;eqeqeq); end

Expected Behavior

output:

test
between
test

Since (3;eqeqeq) returns the eqeqeq, both cases should end up comparing against the eqeqeq using the === operator that case should use. So 'test' should be printed twice (with between in-between).

Actual Behavior

test
between
test2

So in the second case, == is called instead of ===.

More details

I'm working on something that generates code, and a more complex version of this happens. I managed to reduce the issue to this simple case. This work as expected in MRI.

Another different example of the issue:

case 'a'; when /a/; puts 'hi1'; end #=> prints
case 'a'; when (1;/a/); puts 'hi2'; end
case 'a'; when (1;'a'); puts 'hi3'; end #=> prints
@MaxLap MaxLap changed the title Case-when Case with complex when uses == instead of === Oct 4, 2017
@headius
Copy link
Member

headius commented Oct 4, 2017

Weird. We don't do anything special for different types of objects in case/when (other than static cases like all integers) so I'm not sure why this would be. Shouldn't be hard to fix though.

@headius
Copy link
Member

headius commented Oct 4, 2017

Ahh I hadn't noticed the failing version is a little unusual. So I suspect this is because when we can't see a direct value for the when we are just evaluating its elements differently. These cases look like they're more complex expressions to our compiler.

@headius
Copy link
Member

headius commented Oct 4, 2017

The IR in both cases appears to be identical. So it's something inside the eqq instruction, most likely.

Note in my example eqeqeq is defined as a call rather than a local variable, but it shouldn't matter to IR.

  2:  %v_5 := call_0o(self<%self>, callType: VARIABLE, name: eqeqeq, potentiallyRefined: false)
  3:  %v_4 := eqq(%v_5, fix<1>, callSite: CallSite("===",FUNCTIONAL), splattedValue: false)

@headius
Copy link
Member

headius commented Oct 4, 2017

Oops, I should have paid closer attention to your report. I just realized you reported against 9.1.9.0 and 9.1.7.0, and indeed both versions do use == instead of === for these more complex expressions.

However, it seems only JRuby master, which will become JRuby 9.2, has fixed this.

@enebo Maybe this can be backported, but I'm also not sure if we're doing another 9.1.x.

@MaxLap
Copy link
Author

MaxLap commented Oct 4, 2017 •
edited
Loading

If it's fixed on master, then that's good. For my use case, I found a workaround using && instead of ; since the first part is always truthy. So I don't need it to be fixed in 9.1.

Up to you if you still want to backport.

Thank you

Edit: Just tested, i confirm things are working fine on jruby-head. Sorry, I didn't think to check against it.

@headius
Copy link
Member

headius commented Oct 4, 2017

@MaxLap Ok no problem. In the absence of anyone else with a serious concern, I'll call this fixed in 9.2 and we'll have it out in the next couple months.

@headius headius closed this as completed Oct 4, 2017
@headius headius added this to the JRuby 9.2.0.0 milestone Oct 4, 2017
@headius headius added the ir label Oct 4, 2017
@enebo
Copy link
Member

enebo commented Oct 4, 2017

@headius yeah I think we should backport it unless it is tied to something 2.4 specific. Can I see the commit easily by hitting that helper?

@headius
Copy link
Member

headius commented Oct 4, 2017

@enebo I assume it would be something in the compiler, since the IR is different.

@headius headius mentioned this issue Oct 10, 2017
Merged
@headius
Copy link
Member

headius commented Oct 10, 2017

Reopening to backport to 9.1.14.

@headius headius reopened this Oct 10, 2017
@headius headius modified the milestones: JRuby 9.2.0.0, JRuby 9.1.14.0 Oct 10, 2017
@headius
Copy link
Member

headius commented Oct 10, 2017

@MaxLap Hey a small request: could you try to turn this into a spec for https://github.com/ruby/spec? I realized I never added a test for it, but that would be a good way for you to contribute.

I've got a backport in process so this will be fixed in the next 9.1 release.

@headius headius closed this as completed Oct 10, 2017
marcandre added a commit to ruby/spec that referenced this issue Oct 10, 2017
@marcandre
Add spec for complex expression in ().
fa47230 
See jruby/jruby#4804
@marcandre
Copy link

@headius Rubyspec updated: ruby/spec@fa47230

@MaxLap
Copy link
Author

MaxLap commented Oct 11, 2017

Thank you @marcandre

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
ir
Projects
None yet
Milestone
JRuby 9.1.14.0
Development

No branches or pull requests
4 participants
@headius @enebo @marcandre @MaxLap