Project

General

Profile

Backport #8872

Case statements do not honor a refinement of the '===' method

Added by jconley88 (Jon Conley) over 5 years ago. Updated over 5 years ago.

Status:
Closed
Priority:
Normal
[ruby-core:57051]

Description

=begin
Below, I've redefined the ((|===|)) method of symbol to always return true. In ((|RefineTest#uses_refinement|)), I call ((|===|)) directly and the refined method is called. In ((|RefineTest#does_not_use_refinement|)), the ((|===|)) method is called indirectly through a case statement. If the refined ((|===|)) method was called, the result should be (('The refinement was used')), but this code currently returns (('The refinement was not used')).

module RefineSymbol
refine Symbol do
def ===(other)
true
end
end
end

using RefineSymbol

class RefineTest
def uses_refinement
:a === :b
end

def does_not_use_refinement
case :a
when :b
'The refinement was used'
else
'The refinement was not used'
end
end
end

rt = RefineTest.new
rt.uses_refinement # => true
rt.does_not_use_refinement # => expected 'The refinement was used' but got 'The refinement was not used'
=end


Related issues

Related to Backport200 - Backport #9175: Backport r43913Closed11/29/2013Actions
Has duplicate Ruby trunk - Bug #9150: Segfault in case statement execution, possibly related to refinementsClosed11/25/2013Actions

Associated revisions

Revision 66915c50
Added by charliesome (Charlie Somerville) over 5 years ago

  • vm_eval.c (vm_call0): fix prototype, the id parameter should be of
    type ID, not VALUE

  • vm_insnhelper.c (check_match): the rb_funcall family of functions
    does not care about refinements. We need to use
    rb_method_entry_with_refinements instead to call === with
    refinements. Thanks to Jon Conley for reporting this bug.
    [ruby-core:57051] [Bug #8872]

  • test/ruby/test_refinement.rb: add test

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@42869 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 42869
Added by charliesome (Charlie Somerville) over 5 years ago

  • vm_eval.c (vm_call0): fix prototype, the id parameter should be of
    type ID, not VALUE

  • vm_insnhelper.c (check_match): the rb_funcall family of functions
    does not care about refinements. We need to use
    rb_method_entry_with_refinements instead to call === with
    refinements. Thanks to Jon Conley for reporting this bug.
    [ruby-core:57051] [Bug #8872]

  • test/ruby/test_refinement.rb: add test

Revision 42869
Added by charliesome (Charlie Somerville) over 5 years ago

  • vm_eval.c (vm_call0): fix prototype, the id parameter should be of
    type ID, not VALUE

  • vm_insnhelper.c (check_match): the rb_funcall family of functions
    does not care about refinements. We need to use
    rb_method_entry_with_refinements instead to call === with
    refinements. Thanks to Jon Conley for reporting this bug.
    [ruby-core:57051] [Bug #8872]

  • test/ruby/test_refinement.rb: add test

Revision 42869
Added by charliesome (Charlie Somerville) over 5 years ago

  • vm_eval.c (vm_call0): fix prototype, the id parameter should be of
    type ID, not VALUE

  • vm_insnhelper.c (check_match): the rb_funcall family of functions
    does not care about refinements. We need to use
    rb_method_entry_with_refinements instead to call === with
    refinements. Thanks to Jon Conley for reporting this bug.
    [ruby-core:57051] [Bug #8872]

  • test/ruby/test_refinement.rb: add test

Revision 42869
Added by charliesome (Charlie Somerville) over 5 years ago

  • vm_eval.c (vm_call0): fix prototype, the id parameter should be of
    type ID, not VALUE

  • vm_insnhelper.c (check_match): the rb_funcall family of functions
    does not care about refinements. We need to use
    rb_method_entry_with_refinements instead to call === with
    refinements. Thanks to Jon Conley for reporting this bug.
    [ruby-core:57051] [Bug #8872]

  • test/ruby/test_refinement.rb: add test

Revision 42869
Added by charliesome (Charlie Somerville) over 5 years ago

  • vm_eval.c (vm_call0): fix prototype, the id parameter should be of
    type ID, not VALUE

  • vm_insnhelper.c (check_match): the rb_funcall family of functions
    does not care about refinements. We need to use
    rb_method_entry_with_refinements instead to call === with
    refinements. Thanks to Jon Conley for reporting this bug.
    [ruby-core:57051] [Bug #8872]

  • test/ruby/test_refinement.rb: add test

Revision 42869
Added by charliesome (Charlie Somerville) over 5 years ago

  • vm_eval.c (vm_call0): fix prototype, the id parameter should be of
    type ID, not VALUE

  • vm_insnhelper.c (check_match): the rb_funcall family of functions
    does not care about refinements. We need to use
    rb_method_entry_with_refinements instead to call === with
    refinements. Thanks to Jon Conley for reporting this bug.
    [ruby-core:57051] [Bug #8872]

  • test/ruby/test_refinement.rb: add test

Revision 3041c43d
Added by nagachika (Tomoyuki Chikanaga) over 5 years ago

merge revision(s) 42869: [Backport #8872]

    * vm_eval.c (vm_call0): fix prototype, the id parameter should be of
      type ID, not VALUE

    * vm_insnhelper.c (check_match): the rb_funcall family of functions
      does not care about refinements. We need to use
      rb_method_entry_with_refinements instead to call === with
      refinements. Thanks to Jon Conley for reporting this bug.
      [ruby-core:57051] [Bug #8872]

    * test/ruby/test_refinement.rb: add test

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_0_0@42923 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 42923
Added by nagachika (Tomoyuki Chikanaga) over 5 years ago

merge revision(s) 42869: [Backport #8872]

* vm_eval.c (vm_call0): fix prototype, the id parameter should be of
  type ID, not VALUE

* vm_insnhelper.c (check_match): the rb_funcall family of functions
  does not care about refinements. We need to use
  rb_method_entry_with_refinements instead to call === with
  refinements. Thanks to Jon Conley for reporting this bug.
  [ruby-core:57051] [Bug #8872]

* test/ruby/test_refinement.rb: add test

Revision 6136b992
Added by nagachika (Tomoyuki Chikanaga) over 5 years ago

merge revision(s) 43913: [Backport #8872] [Backport #9175]

    * vm_insnhelper.c (check_match): Fix SEGV with VM_CHECKMATCH_TYPE_CASE
      and class of `pattern` has `method_missing`
      [Bug #8882] [ruby-core:58606]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_0_0@44695 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 44695
Added by nagachika (Tomoyuki Chikanaga) over 5 years ago

merge revision(s) 43913: [Backport #8872] [Backport #9175]

* vm_insnhelper.c (check_match): Fix SEGV with VM_CHECKMATCH_TYPE_CASE
  and class of `pattern` has `method_missing`
  [Bug #8882] [ruby-core:58606]

History

#1

Updated by charliesome (Charlie Somerville) over 5 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r42869.
Jon, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • vm_eval.c (vm_call0): fix prototype, the id parameter should be of
    type ID, not VALUE

  • vm_insnhelper.c (check_match): the rb_funcall family of functions
    does not care about refinements. We need to use
    rb_method_entry_with_refinements instead to call === with
    refinements. Thanks to Jon Conley for reporting this bug.
    [ruby-core:57051] [Bug #8872]

  • test/ruby/test_refinement.rb: add test

Updated by charliesome (Charlie Somerville) over 5 years ago

  • Backport changed from 1.9.3: UNKNOWN, 2.0.0: UNKNOWN to 1.9.3: DONTNEED, 2.0.0: REQUIRED
#3

Updated by nagachika (Tomoyuki Chikanaga) over 5 years ago

  • Tracker changed from Bug to Backport
  • Project changed from Ruby trunk to Backport200
  • Status changed from Closed to Assigned
  • Assignee set to nagachika (Tomoyuki Chikanaga)
#4

Updated by nagachika (Tomoyuki Chikanaga) over 5 years ago

  • Status changed from Assigned to Closed

This issue was solved with changeset r42923.
Jon, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


merge revision(s) 42869: [Backport #8872]

* vm_eval.c (vm_call0): fix prototype, the id parameter should be of
  type ID, not VALUE

* vm_insnhelper.c (check_match): the rb_funcall family of functions
  does not care about refinements. We need to use
  rb_method_entry_with_refinements instead to call === with
  refinements. Thanks to Jon Conley for reporting this bug.
  [ruby-core:57051] [Bug #8872]

* test/ruby/test_refinement.rb: add test

Updated by pixeltrix (Andrew White) over 5 years ago

This has caused a regression in Rails where === isn't being sent via method_missing:

https://github.com/rails/rails/pull/13055

This results in a segmentation fault.

Updated by sorah (Sorah Fukumori) over 5 years ago

pixeltrix (Andrew White) Fixed at r43913, Thank you for your reporting

Updated by shugo (Shugo Maeda) over 5 years ago

  • Status changed from Closed to Assigned
  • Assignee changed from nagachika (Tomoyuki Chikanaga) to matz (Yukihiro Matsumoto)

As I stated in #9150, I doubt this feature conforms to the design policy of refinements.
r42869 should be reverted, shouldn't it, Matz?

Updated by shugo (Shugo Maeda) over 5 years ago

I strongly request committers not to change the behavior of Refinements without Matz's permission, except when there's an obvious bug such as SEGV in Refinements, because the current feature set of Refinements are restricted to keep compatibility with other implementations such as JRuby.

In general, refinements should be activated only in a certain lexical scope.

#9

Updated by sorah (Sorah Fukumori) over 5 years ago

shugo (Shugo Maeda) wrote:

I strongly request committers not to change the behavior of Refinements without Matz's permission, except when there's an obvious bug such as SEGV in Refinements, because the current feature set of Refinements are restricted to keep compatibility with other implementations such as JRuby.

In general, refinements should be activated only in a certain lexical scope.

Agreed.

Note that my commit (r43913) just fixes bug in the current behavior, please revert it too with r42839, if we revert r42839.

#10

Updated by matz (Yukihiro Matsumoto) over 5 years ago

The basic principle of refinements is that refinement do work in local scope.
So methods called from the scope will be refined but method called from methods called will not.

And very importantly, "===" called from case statement is controversial.
It can be viewed as a method called from case statement, or case statement itself is just a syntax sugar wrapping "===" calls. From the former point of view, refine should not affect "===", but from the latter POV, refinement should change the behavior.

Right now, we took the former POV, and OP expected the latter.
So I would like to hear from others.

Matz.

Updated by shugo (Shugo Maeda) over 5 years ago

matz (Yukihiro Matsumoto) wrote:

And very importantly, "===" called from case statement is controversial.
It can be viewed as a method called from case statement, or case statement itself is just a syntax sugar wrapping "===" calls. From the former point of view, refine should not affect "===", but from the latter POV, refinement should change the behavior.

Ruby has other features which call methods implicitly. For example, Range#to_a is called by the following code:

a = *[1..10]

Should refinements be honored in all such features?

Updated by shugo (Shugo Maeda) over 5 years ago

I've found that for expressions honor refinements.

module RefineEach
refine Array do
def each
yield "refined"
end
end
end

using RefineEach

for i in [1, 2, 3]
p i
end

for expressions seem to be more closely related to method calls, but I'm not sure how refinements should be handled in these cases....

Updated by Eregon (Benoit Daloze) over 5 years ago

shugo (Shugo Maeda) wrote:

a = *[1..10]

You probably meant

a = [*1..10]

Updated by shugo (Shugo Maeda) over 5 years ago

Eregon (Benoit Daloze) wrote:

a = *[1..10]

You probably meant

a = [*1..10]

Ah, I meant:

a = *1..10

Thanks anyway.

Updated by shugo (Shugo Maeda) over 5 years ago

Could anyone tell me use cases of this feature?

I think refinements are for object-oriented ways using dynamic dispatch, but case expressions are not for such object-oriented ways.

Updated by jballanc (Joshua Ballanco) over 5 years ago

To play devil's advocate, I think the opposing question to be asked is "how would you alter the behavior of case evaluation using refinements"?

It may be, as shugo states, that refinements are not meant to be used for altering the behavior of expressions. In this case, the answer to the question I posed is, simply, "you don't."

That said, I think it is fairly well understood in the Ruby community that "===" is related to case. I've even heard the operator described as "case equality". Playing devil's advocate again, what is the use case for refining "===" if it doesn't affect the evaluation of a case...when clause?

Updated by shugo (Shugo Maeda) over 5 years ago

jballanc (Joshua Ballanco) wrote:

To play devil's advocate, I think the opposing question to be asked is "how would you alter the behavior of case evaluation using refinements"?

It may be, as shugo states, that refinements are not meant to be used for altering the behavior of expressions. In this case, the answer to the question I posed is, simply, "you don't."

That said, I think it is fairly well understood in the Ruby community that "===" is related to case. I've even heard the operator described as "case equality". Playing devil's advocate again, what is the use case for refining "===" if it doesn't affect the evaluation of a case...when clause?

I don't come up with any use case for refining "===" regardless of whether it affects the evaluation of a case expression or not.
I'm neutral about this issue, and really want to know use cases.

Updated by nagachika (Tomoyuki Chikanaga) over 5 years ago

hi,

Is there any progress? Does anyone has usecase of refinements & case statement?

Updated by matz (Yukihiro Matsumoto) over 5 years ago

After consideration, I think for and case statement should honor refinement since they are fundamentally syntax sugar using each/=== methods.

Matz.

Updated by nagachika (Tomoyuki Chikanaga) over 5 years ago

How about 2.0.0? Can I change the behavior at 2.0.0 to same with 2.1?
If so I'll backport r43913 to fix SEGV.

#21

Updated by matz (Yukihiro Matsumoto) over 5 years ago

  • ruby -v set to 2.0.0

I don't think changing the behavior in the middle of a release is a good idea.
So I don't encourage backporting. Thanks.

Matz.

#22

Updated by matz (Yukihiro Matsumoto) over 5 years ago

  • ruby -v changed from 2.0.0 to -

Issue #8872 has been updated by Yukihiro Matsumoto.

ruby -v set to 2.0.0

I don't think changing the behavior in the middle of a release is a good idea.
So I don't encourage backporting. Thanks.

Matz.


Backport #8872: Case statements do not honor a refinement of the '===' method
https://bugs.ruby-lang.org/issues/8872#change-44355

  • Author: Jon Conley
  • Status: Assigned
  • Priority: Normal
  • Assignee: Yukihiro Matsumoto
  • Category:
  • Target version:
  • ruby -v: 2.0.0 ---------------------------------------- =begin Below, I've redefined the ((|===|)) method of symbol to always return true. In ((|RefineTest#uses_refinement|)), I call ((|===|)) directly and the refined method is called. In ((|RefineTest#does_not_use_refinement|)), the ((|===|)) method is called indirectly through a case statement. If the refined ((|===|)) method was called, the result should be (('The refinement was used')), but this code currently returns (('The refinement was not used')).

module RefineSymbol
refine Symbol do
def ===(other)
true
end
end
end

using RefineSymbol

class RefineTest
def uses_refinement
:a === :b
end

def does_not_use_refinement
  case :a
  when :b
    'The refinement was used'
  else
    'The refinement was not used'
  end
end

end

rt = RefineTest.new
rt.uses_refinement # => true
rt.does_not_use_refinement # => expected 'The refinement was used' but got 'The refinement was not used'
=end

--
http://bugs.ruby-lang.org/

Updated by nagachika (Tomoyuki Chikanaga) over 5 years ago

  • Assignee changed from matz (Yukihiro Matsumoto) to nagachika (Tomoyuki Chikanaga)

Hi,

I've told matz that this change introduced by r42923 was already released at 2.0.0p353 and matz decided to not to revert the changeset. (see https://twitter.com/yukihiro_matz/status/423500823788662784 )

I will backport r43913 to fix SEGV introduced at r42923.

Thanks.

Updated by nagachika (Tomoyuki Chikanaga) over 5 years ago

  • Status changed from Assigned to Closed

Applied in changeset r44695.


merge revision(s) 43913: [Backport #8872] [Backport #9175]

* vm_insnhelper.c (check_match): Fix SEGV with VM_CHECKMATCH_TYPE_CASE
  and class of `pattern` has `method_missing`
  [Bug #8882] [ruby-core:58606]

Also available in: Atom PDF