Feature #9112
closedMake module lookup more dynamic (Including modules into a module after it has already been included)
Added by PragTob (Tobias Pfeiffer) almost 11 years ago. Updated about 4 years ago.
Description
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)
Updated by drbrain (Eric Hodel) almost 11 years ago
- 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
Updated by nobu (Nobuyoshi Nakada) almost 11 years ago
It has been rejected because it is hard to implement without significant performance penalty.
Now we have subclass trees, so it might be possible.
Updated by PragTob (Tobias Pfeiffer) over 10 years ago
Thank you for the quick reply Eric and Nobu! Looking forward to how this turns out :-)
Updated by Hanmac (Hans Mackowiak) over 10 years ago
would it with the change also possible to add Module multible times into the Ancestor chain?
like
C1 < M1 < C2 < M1 < M2 < C3
?
Updated by nobu (Nobuyoshi Nakada) over 10 years ago
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.
Updated by headius (Charles Nutter) almost 10 years ago
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
Updated by fxn (Xavier Noria) almost 10 years ago
+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
Updated by tduehr (Timur Duehr) almost 10 years ago
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?
Updated by bitsweat (Jeremy Daer) almost 10 years ago
See #1586
1. allow multiple inclusion
1. propagate when a module includes a module
Both have been (basically) accepted
Updated by tduehr (Timur Duehr) almost 10 years ago
I understand 1. I'm missing something on 2. Does that mean my example code above should raise on including m2
?
Updated by nobu (Nobuyoshi Nakada) almost 10 years ago
No.
Module#include
doesn't change superclass of the receiver's metaclass.
Updated by tduehr (Timur Duehr) almost 10 years ago
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
Updated by matz (Yukihiro Matsumoto) almost 10 years ago
- Related to Feature #1586: Including a module already present in ancestors should not be ignored added
Updated by matz (Yukihiro Matsumoto) almost 10 years ago
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.
Updated by tduehr (Timur Duehr) almost 10 years ago
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?
Updated by jeremyevans0 (Jeremy Evans) about 4 years ago
- Status changed from Assigned to Closed
Support for this was implemented in 3556a834a2847e52162d1d3302d4c64390df1694 and 41582d5866ae34c57094f70f95c3d31f4a1fa4ff.