Project

General

Profile

Actions

Feature #9112

closed

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

Added by PragTob (Tobias Pfeiffer) about 11 years ago. Updated over 4 years ago.

Status:
Closed
Target version:
-
[ruby-core:58342]

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)


Related issues 1 (0 open1 closed)

Related to Ruby master - Feature #1586: Including a module already present in ancestors should not be ignoredRejectedmatz (Yukihiro Matsumoto)Actions

Updated by drbrain (Eric Hodel) about 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) about 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) about 11 years ago

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

Updated by Hanmac (Hans Mackowiak) about 11 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) about 11 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) about 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) about 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

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

Updated by tduehr (Timur Duehr) about 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) about 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) about 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) about 10 years ago

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

Updated by tduehr (Timur Duehr) about 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) about 10 years ago

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

Updated by matz (Yukihiro Matsumoto) about 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) about 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) over 4 years ago

  • Status changed from Assigned to Closed
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0