Feature #9112: Make module lookup more dynamic (Including modules into a module after it has already been included)

• Status: Closed
• Assignee: matz (Yukihiro Matsumoto)
• Target version: -

[PragTob (Tobias Pfeiffer)]

If a module (M) is included into a class (C) and afterwards another module (M2) is included into the first module (M) then C does not include M2 and instances do not respond to methods defined in M2. I think instances of C should respond to methods defined in M2 and C should include M2.

I created a gist detailing the problem I have: https://gist.github.com/PragTob/7472643

I think this behavior is confusing, because if I'd reopen module M and just add methods there then instances of C can call those methods. However if I include another module in M then instances of C can not call those methods.

Any opinions on if this would be a better behavior or why it isn't?

(was unsure to file it as a bug or feature)

[drbrain (Eric Hodel)]

• Category set to core
• Status changed from Open to Assigned
• Assignee set to matz (Yukihiro Matsumoto)

This would be a new feature.

I believe this was requested and rejected back in the 1.8 days, but looking for the discussion is too difficult for me.

If it was, maybe matz has changed his mind

[nobu (Nobuyoshi Nakada)]

It has been rejected because it is hard to implement without significant performance penalty.
Now we have subclass trees, so it might be possible.

[PragTob (Tobias Pfeiffer)]

Thank you for the quick reply Eric and Nobu! Looking forward to how this turns out :-)

[Hanmac (Hans Mackowiak)]

would it with the change also possible to add Module multible times into the Ancestor chain?
like
C1 < M1 < C2 < M1 < M2 < C3
?

[nobu (Nobuyoshi Nakada)]

Hanmac (Hans Mackowiak) wrote:

would it with the change also possible to add Module multible times into the Ancestor chain?

I'd tried it once, but found that some modules implicitly expect they are included only one time.
So, we'll need a way to tell a module can be included multiple times or not, perhaps.

[headius (Charles Nutter)]

nobu's point about reincluding the same module brought up a different concern.

Many modules perform very specific actions at include time. If they did not go through a reinclude process for every hierarchy, those changes might not happen. For example:

module X
  def self.included(target)
    ... make changes to target hierarchy in addition to the include
  end
end

module Y
  include X # incuded runs against Y hierarchy
end

class A
  include Y # included for both Y and X run under current behavior, but only Y would run under new behavior
end

[fxn (Xavier Noria)]

+1 over here.

I remember I discovered this the hard-way extracting some code from Rails into a gem.

I believe the current behavior is a leak of the implementation. Conceptually, in my opinion, a method or constant lookup should follow the ancestor chain at that point in runtime. That way you can add behavior to modules adding methods directly or via another module for organizational purposes.

I have always seen the linearization of ancestors as an implementation detail not belonging to the conceptual object model of Ruby. From the user perspective it does not matter if there's a pointer to super, or if there's a tree search of some sort going on.

That's my view, don't really know if it's also the view of Ruby core.

I asked Matz about this in the past and he said what was said above, that if there was no performance penalty it would be open to this change. See

https://www.ruby-forum.com/topic/1458576

[tduehr (Timur Duehr)]

I'm working on a patch for this to use a live tree for module ancestry. I had already implemented this as part of Module#prepend before coming across this discussion. I wrote a test for this behavior as a starting point but it fails under the current behavior.

    m = Module.new
    m.module_eval do
      def foo; :foo; end
    end
    class << m; self; end.class_eval do
      define_method(:included) {|mod| raise }
    end

    m2 = Module.new
    assert_raise(RuntimeError) do
      m2.module_eval { include(m) }
    end

    m3 = Module.new
    assert_raise(RuntimeError) do
      m3.module_eval { include(m2) }
    end

m.included is only called on the include for m2 not m3.

Other than wrapping methods, which would be seen by includes further down the chain in either the current or live ancestries, I can't think of an example of hierarchy changes that would fire when including m2 into m3. Do you have an example of one such action?

[bitsweat (Jeremy Daer)]

See #1586

1. allow multiple inclusion
1. propagate when a module includes a module

Both have been (basically) accepted

[tduehr (Timur Duehr)]

I understand 1. I'm missing something on 2. Does that mean my example code above should raise on including m2?

[nobu (Nobuyoshi Nakada)]

No.
Module#include doesn't change superclass of the receiver's metaclass.

[tduehr (Timur Duehr)]

Are you just looking for the following test to pass then for 2?

  def test_simple_include
    obj = Object.new
    class << obj
      class A; def foo; 1; end; end
      module B; end
      class C < A; include B; def bar; foo; end; end
      $test_simple_include1 = C.new.bar

      module X; def foo; 2; end; end
      module B; include X; end
      $test_simple_include2 = C.new.bar
      module X; def foo; 3; end; end
      $test_simple_include3 = C.new.bar

      module B; def foo; 4; end; end
      $test_simple_include4 = C.new.bar
    end

    assert_equal(1, $test_simple_include1)
    assert_equal(2, $test_simple_include2)
    assert_equal(3, $test_simple_include3)
    assert_equal(4, $test_simple_include4)
  end

[matz (Yukihiro Matsumoto)]

• Related to Feature #1586: Including a module already present in ancestors should not be ignored added

[matz (Yukihiro Matsumoto)]

The reasons behind the current behavior are as you have guessed. Besides that, it was quite difficult to implement this more dynamic inclusion efficiently (both space-wise and time-wise).

So with smarter implementation, with proper timing, I don't reject. But it will not happen in coming 2.2, at least.

Matz.

[tduehr (Timur Duehr)]

Thanks for the input. I think that's enough that I can start working on it.

Porting the implementation I have for JRuby would yield O(N) method search time with recursion up to the deepest include/prepend. I'm pretty sure the current objects will not need modification. Worst case, RClass_ext will need an additional array/linked list. Handling super calls gets a bit trickier. Since the current call location will no longer be directly in line with self's class, the next superclass up from a leaf node in the graph will need to be searched for. Fortunately, this implementation also reduces the number of T_ICLASS objects required since only one is needed every time an include is performed.

This search can also take an iterative approach trading call stack for a list on the stack. There's a way to do away with that list by linking the leaf nodes (included modules) to the next/previous node in the search but that'll add complexity to include.

My intent is to start with an iterative BFS since that should be a good tradeoff between complexity and size, and it shouldn't be too hard to extend to a threaded BFS search.

Does that sound like a good approach?

[jeremyevans0 (Jeremy Evans)]

• Status changed from Assigned to Closed

Support for this was implemented in 3556a834a2847e52162d1d3302d4c64390df1694 and 41582d5866ae34c57094f70f95c3d31f4a1fa4ff.