Project

General

Profile

Actions

Bug #11182

closed

Refinement with alias causes strange behavior

Added by ko1 (Koichi Sasada) almost 9 years ago. Updated over 2 years ago.

Status:
Closed
Target version:
-
[ruby-core:69360]
Tags:

Description

The following script causes strange behavior.

class C
  def foo
    p "C"
  end
end

module M
  refine C do
    def foo
      p "Refiend C"
    end
  end
end

class D < C
  alias bar foo
end

using M
D.new.bar
#=> t.rb:21:in `<main>': undefined method `bar' for #<D:0x29fbf58> (NoMethodError)

It seems strange.

Maybe (1) C#foo or (2) M#C#foo should be called. But I'm not sure which is suitable.

Previous versions:

ruby 2.0.0p606 (2014-11-28 revision 48636) [i386-mswin32_110]
t.rb:9: warning: Refinements are experimental, and the behavior may change in future versions of Ruby!
"C"

ruby 2.1.5p312 (2015-03-10 revision 49912) [i386-mswin32_110]
"C"

Files

1.PNG (38.7 KB) 1.PNG ko1 (Koichi Sasada), 05/27/2015 05:15 AM
2.PNG (43.7 KB) 2.PNG ko1 (Koichi Sasada), 05/27/2015 05:15 AM
4.PNG (38.5 KB) 4.PNG ko1 (Koichi Sasada), 05/27/2015 05:34 AM
3.PNG (37.9 KB) 3.PNG ko1 (Koichi Sasada), 05/27/2015 05:34 AM
alias_affected_by_original_refinement.diff (1.34 KB) alias_affected_by_original_refinement.diff shugo (Shugo Maeda), 05/28/2015 07:54 AM
6.PNG (36.6 KB) 6.PNG ko1 (Koichi Sasada), 05/28/2015 09:12 AM
7.PNG (40.7 KB) 7.PNG ko1 (Koichi Sasada), 05/28/2015 09:12 AM

Related issues 1 (0 open1 closed)

Related to Ruby master - Bug #13817: test/unit breaks HashClosedktsj (Kazuki Tsujimoto)Actions

Updated by shugo (Shugo Maeda) almost 9 years ago

  • Status changed from Open to Assigned
  • Assignee set to shugo (Shugo Maeda)
Actions #2

Updated by shugo (Shugo Maeda) almost 9 years ago

  • Status changed from Assigned to Closed

Applied in changeset r50642.


Updated by shugo (Shugo Maeda) almost 9 years ago

  • Backport changed from 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN to 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: REQUIRED

Koichi Sasada wrote:

Maybe (1) C#foo or (2) M#C#foo should be called. But I'm not sure which is suitable.

By D.new.bar, not foo but bar is searched in refinements, so M#C#foo cannot be found.
I've fixed alias to use C#foo.

Updated by ko1 (Koichi Sasada) almost 9 years ago

By D.new.bar, not foo but bar is searched in refinements, so M#C#foo cannot be found.
I've fixed alias to use C#foo.

Why that? I assumed two scenarios.

(1) Invoke C#foo

  • D#bar -> find a method entry (alias to C#foo)
  • Invoke C#foo

(2) Invoke M#C#foo

  • D#bar -> find a method entry (alias to C#foo)
  • C#foo is refined by M so invke M#C#foo

Of course, if D#bar is refined, then the refining method should be invoked.

Updated by ko1 (Koichi Sasada) almost 9 years ago

Simplify example:

class C
  def foo
    p "C"
  end
end

module M
  refine C do
    def foo
      p "Refiend C"
    end
  end
end

class C
  alias bar foo
end

C.new.foo
C.new.bar
using M
C.new.foo
C.new.bar
C.new.foo # C
C.new.bar # C
using M
C.new.foo # refined C
C.new.bar # C

I agree that invoking only "C" is reasonable because it can be direct pointer to C#foo.
However, I think it is also reasonable to call refined C because people can assume C#bar should be same as C#foo.

Updated by shugo (Shugo Maeda) almost 9 years ago

Koichi Sasada wrote:

However, I think it is also reasonable to call refined C because people can assume C#bar should be same as C#foo.

If so, what should be printed by the last C.new.bar in the following example?

class C
  def foo
    p "C#foo"
  end
end

module M
  refine C do
    def foo
      p "Refiend C#foo"
    end

    def bar
      p "Refined C#bar"
    end
  end
end

class C
  alias bar foo
end

C.new.foo
C.new.bar
using M
C.new.foo
C.new.bar

Updated by shugo (Shugo Maeda) almost 9 years ago

  • Status changed from Closed to Open
  • Backport changed from 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: REQUIRED to 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN

Updated by ko1 (Koichi Sasada) almost 9 years ago

I can consider two models (1) and (2) the following pictures show.

(1)

(2)

I believe the implementation uses (2) model.

For (1), calling M::C#bar is reasonable.
For (2), calling M::C#bar is reasonable.

Wow, both should be M::C#bar.

Example on #5, both models should call M::C#bar. But my comment at #5 "C#bar should be same as C#foo" is wrong.

Updated by shugo (Shugo Maeda) almost 9 years ago

Koichi Sasada wrote:

I believe the implementation uses (2) model.

For (1), calling M::C#bar is reasonable.
For (2), calling M::C#bar is reasonable.

Wow, both should be M::C#bar.

Example on #5, both models should call M::C#bar. But my comment at #5 "C#bar should be same as C#foo" is wrong.

M::C#bar is not defined in Example #5, so the orignal C#bar, which is an alias of C#foo, should be called, shoudn't it?

Updated by ko1 (Koichi Sasada) almost 9 years ago

My last explanation was wrong.

I wrote pictures again.

(3)
(4)

With model (1), it seems C#foo will be called.
WIth model (2), it seems M::C#foo will be called.

How about it?

Actions #11

Updated by headius (Charles Nutter) almost 9 years ago

I will offer a quick opinion: refinements are an overlay on the method table, and therefore modifications to the method table should not reflect refinements. Put differently: the alias bar foo should call C#foo always, because there it is a modification to D's method table.

We already established in past discussions about refinements that the class's actual method table (and methods that query or manipulate it) should not reflect refinements, since that would require them to be aware of the caller's scope. This is just another case of the same thing: creating an alias looks up in the target class/module's method table and adds a new entry. It's as simple as that.

Updated by shugo (Shugo Maeda) almost 9 years ago

Koichi Sasada wrote:

With model (1), it seems C#foo will be called.
WIth model (2), it seems M::C#foo will be called.

How about it?

I agree that both models are possible and attached a patch implementing model (2).
However, even in the selector model, alias need not to be affected by refinements of
the original method as implemented in trunk, where alias creates a direct link to
the original method instead of a link to its selector.

I agree with Charles from past discussions.
At least the behavior should not be changed in 2.2.

Updated by ko1 (Koichi Sasada) almost 9 years ago

I wrote pictures again for original examples D, C and M.

(1) Like module include model (on original example)
(2) Method selector model (on original example)

In this case, I think both are acceptable (calling C#foo directly or M::C#foo). Why you say "It must be C#foo"? Or model (2) is wrong model?

We already established in past discussions about refinements that the class's actual method table (and methods that query or manipulate it) should not reflect refinements, since that would require them to be aware of the caller's scope.

It is just MRI do (selector model, I wrote).
We choose this because we don't have any penalty if nobody use refinements.
(this is based on this assumption: only a few people use refinements)

Updated by ko1 (Koichi Sasada) almost 9 years ago

Let's discuss with use cases. I don't have good example, but please assume we want to make new Hash class to support something like HashWithIndifferentAccess.

Okay, we need an extra Hash class doing something special.
class MyHash < Hash
  def initialize *args
    args.each{|(k, v)|self[k] = v}
  end

  alias iterate each
end

h = MyHash.new([:b, 1], [:a, 2])

h.each{|k, v| p [k, v]} # [:b, 1] [:a, 2]

Good.

Next time, I invented a nice refinement to iterate Hash contents by ordered.

module OrderedHashEach
  refine Hash do
    def each
      sort.each{|k, v|
        yield k, v
      }
    end
  end
end

using OrderedHashEach
{b: 1, a: 2}.each{|k, v| p [k, v]} # [:a, 2], [:b, 1]

Excellent.

And of course, we can combine MyHash and OrderedHashEach.

module OrderedHashEach
  refine Hash do
    def each
      sort.each{|k, v|
        yield k, v
      }
    end
  end
end

class MyHash < Hash
  def initialize *args
    args.each{|(k, v)|self[k] = v}
  end
end

h = MyHash.new([:b, 1], [:a, 2])

using OrderedHashEach
h.each{|k, v| p [k, v]} # [:a, 2] [:b, 1]

Great.

Wait. each is not good terminology for our project. Use iterate intead.

module OrderedHashEach
  refine Hash do
    def each
      sort.each{|k, v|
        yield k, v
      }
    end
  end
end

class MyHash < Hash
  def initialize *args
    args.each{|(k, v)|self[k] = v}
  end

  alias iterate each
end

h = MyHash.new([:b, 1], [:a, 2])

using OrderedHashEach
h.iterate{|k, v| p [k, v]} # [:b, 1] [:a, 2]

It doesn't affect :( It is unexpected result for me.

Note that I don't want to change this specification. I want to know what is ideal specification.

This time, I made a scenario that we may want to use M::C#foo.

Updated by headius (Charles Nutter) almost 9 years ago

  • Assignee changed from ko1 (Koichi Sasada) to shugo (Shugo Maeda)

Alias does not redispatch, so this is the result I'd expect. These are some of the edge cases that I don't think we can solve for everyone.

Bottom line is that method table changes are method table changes. The only way we could implement this the way you want would be to have aliased methods redispatch to the original name, which has all sorts of really nasty problems (redispatch from bottom class or current class? super reflects new name or old name?).

I don't think we can do it ko1's way without introducing many more significant problems (by changing what alias means).

Updated by shugo (Shugo Maeda) almost 9 years ago

Koichi Sasada wrote:

Wait. each is not good terminology for our project. Use iterate intead.

module OrderedHashEach
  refine Hash do
    def each
      sort.each{|k, v|
        yield k, v
      }
    end
  end
end

class MyHash < Hash
  def initialize *args
    args.each{|(k, v)|self[k] = v}
  end

  alias iterate each
end

h = MyHash.new([:b, 1], [:a, 2])

using OrderedHashEach
h.iterate{|k, v| p [k, v]} # [:b, 1] [:a, 2]

It doesn't affect :( It is unexpected result for me.

If you need to refine iterate you should define OrderedHashIterate
and use it instead.

Refinements are designed not to change behavior implicitly.
If refinements are changed to affect aliases, someone might complain
that refinements don't support local rebinding.

Updated by shugo (Shugo Maeda) over 7 years ago

  • Assignee changed from shugo (Shugo Maeda) to gotoken (Kentaro Goto)

Updated by shugo (Shugo Maeda) over 7 years ago

  • Assignee changed from gotoken (Kentaro Goto) to ko1 (Koichi Sasada)
Actions #19

Updated by wanabe (_ wanabe) over 6 years ago

  • Related to Bug #13817: test/unit breaks Hash added

Updated by shugo (Shugo Maeda) over 2 years ago

  • Status changed from Feedback to Closed

ko1 agreed to keep the current behavior, so I close this issue.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0