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

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

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

Comments

Projects
None yet
4 participants
@MaxLap

MaxLap commented Oct 4, 2017

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 from Case-when to Case with complex when uses == instead of === Oct 4, 2017

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 4, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 4, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 4, 2017

Member

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)
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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 4, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@MaxLap

MaxLap Oct 4, 2017

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.

MaxLap commented Oct 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 4, 2017

Member

@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.

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 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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 4, 2017

Member

@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?

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 4, 2017

Member

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

Member

headius commented Oct 4, 2017

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

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 10, 2017

Member

Reopening to backport to 9.1.14.

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 10, 2017

Member

@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.

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 Oct 10, 2017

marcandre added a commit to ruby/spec that referenced this issue Oct 10, 2017

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre commented Oct 10, 2017

@headius Rubyspec updated: ruby/spec@fa47230

@MaxLap

This comment has been minimized.

Show comment
Hide comment
@MaxLap

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