Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix module recursive lookup bug #1130

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions lib/rdoc/code_object/context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1261,4 +1261,50 @@ def upgrade_to_class mod, class_type, enclosing

autoload :Section, "#{__dir__}/context/section"

##
# Attempts to locate the module object in this context.
#
# The scoping rules of Ruby to resolve the name of an included module are:
# - first search constant directly defined in nested modules from inside to outside
# - if not found, look into the children of included modules,
# in reverse inclusion order;
# - if still not found, look into included modules of Object

def lookup_module(name, before: nil, searched: {}.compare_by_identity)
# Search nested modules first
nesting = self
while nesting
full_name = nesting.child_name(name)
mod = @store.modules_hash[full_name]
return mod if mod
nesting = nesting.parent
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that this part is not correct. should use Module.nesting instead of CodeObject#parent chain.

- while nesting
-   ...
-   nesting = nesting.parent
- end
+ unavailable_data.module_nesting.each do
+   ...
+ end

But I think RDoc::Include and RDoc::Extend does not provide enough information.
This pull request and the original implementation uses CodeObject#parent chain instead of module nesting.

Complicated example

class ::A
  class ::B
    class ::A
      class ::B::C
        # Module.nesting is [::B::C, ::A, ::B, ::A]
        include X
      end
    end
  end
end

end

# Search included modules recursively
mod = find_module(name, before: before, searched: searched)
return mod if mod

# Search Object recursively
top_level.object_class.find_module(name, searched: searched)
end

##
# Recursively search for a module in this context and its includes.

def find_module(name, before: nil, searched: {}.compare_by_identity)
return if searched.include?(self)
searched[self] = true
full_name = child_name(name)
mod = @store.modules_hash[full_name]
return mod if mod

# recursively search the includes in reverse order
includes.take_while { |i| i != before }.reverse_each do |i|
inc = i.module
next if String === inc
mod = inc.find_module(name, searched: searched)
return mod if mod
end
nil
end
end
45 changes: 6 additions & 39 deletions lib/rdoc/code_object/mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,49 +59,16 @@ def inspect # :nodoc:
# Attempts to locate the included module object. Returns the name if not
# known.
#
# The scoping rules of Ruby to resolve the name of an included module are:
# - first look into the children of the current context;
# - if not found, look into the children of included modules,
# in reverse inclusion order;
# - if still not found, go up the hierarchy of names.
#
# This method has <code>O(n!)</code> behavior when the module calling
# include is referencing nonexistent modules. Avoid calling #module until
# after all the files are parsed. This behavior is due to ruby's constant
# lookup behavior.
#
# As of the beginning of October, 2011, no gem includes nonexistent modules.
# Avoid calling #module until after all the files are parsed.
# This behavior is due to ruby's constant lookup behavior.

def module
return @module if @module
return @module = @name unless parent
return @module = @name if @name.start_with?('::')

# search the current context
return @name unless parent
full_name = parent.child_name(@name)
@module = @store.modules_hash[full_name]
return @module if @module
return @name if @name =~ /^::/

# search the includes before this one, in reverse order
searched = parent.includes.take_while { |i| i != self }.reverse
searched.each do |i|
inc = i.module
next if String === inc
full_name = inc.child_name(@name)
@module = @store.modules_hash[full_name]
return @module if @module
end

# go up the hierarchy of names
up = parent.parent
while up
full_name = up.child_name(@name)
@module = @store.modules_hash[full_name]
return @module if @module
up = up.parent
end

@name
# search the includes before this one
@module = parent.lookup_module(@name, before: self) || @name
end

##
Expand Down
28 changes: 27 additions & 1 deletion test/rdoc/test_rdoc_include.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def test_module_extended
m1_k1.add_include i1_k0_m4

assert_equal [i1_m1, i1_m2, i1_m3, i1_m4, i1_k0_m4], m1_k1.includes
assert_equal [m1_m2_k0_m4, m1_m2_m3_m4, m1_m2_m3, m1_m2, m1, @object,
assert_equal [m1_m2_k0_m4, m1_m2_m4, m1_m3, m1_m2, m1, @object,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test was wrong.
Actual Mod1::Klass1.ancestors is

[Mod1::Klass1, Mod1::Mod2::Klass0::Mod4, Mod1::Mod2::Mod4, Mod1::Mod3, Mod1::Mod2, Mod1, Object, Kernel, BasicObject]
module Mod1
  module Mod3
  end
  module Mod2
    module Mod3
      module Mod4
      end
    end
    module Mod4
    end
    class Klass0
      module Mod4
        # module Mod5
        # end
        module Mod6
        end
      end
      module Mod5; end
      include Mod4
      include Mod5
      include Mod6
      include Mod1
      include Mod2
      include Mod3
    end
  end
  class Klass1
    include Mod1
    include Mod2
    include Mod3
    include Mod4
    include Klass0::Mod4
  end
end
p Mod1::Klass1.ancestors
# Actual
#=> [Mod1::Klass1, Mod1::Mod2::Klass0::Mod4, Mod1::Mod2::Mod4,      Mod1::Mod3,       Mod1::Mod2, Mod1, Object, Kernel,    BasicObject]
# Test was expecting
#=> [(omitted),    Mod1::Mod2::Klass0::Mod4, Mod1::Mod2::Mod3,Mod4, Mod1::Mod2::Mod3, Mod1::Mod2, Mod1, Object, (omitted), BasicObject]

'BasicObject'], m1_k1.ancestors

m1_k2 = m1.add_class RDoc::NormalClass, 'Klass2'
Expand Down Expand Up @@ -96,6 +96,32 @@ def test_module_extended
assert_equal [m1_m2_m4, m1_m2, m1, @object, 'BasicObject'], m1_k3.ancestors
end

def test_include_through_include
top_level = @store.add_file 'file.rb'

mod1 = top_level.add_module RDoc::NormalModule, 'Mod1'
mod2 = top_level.add_module RDoc::NormalModule, 'Mod2'
mod3 = top_level.add_module RDoc::NormalModule, 'Mod3'
submod = mod1.add_module RDoc::NormalModule, 'Sub'
mod2.add_include RDoc::Include.new('Mod1', '')
mod3.add_include RDoc::Include.new('Mod2', '')
mod3.add_include RDoc::Include.new('Sub', '')
assert_equal [submod, mod2], mod3.ancestors
end

def test_include_through_top_level_include
top_level = @store.add_file 'file.rb'

mod1 = top_level.add_module RDoc::NormalModule, 'Mod1'
mod2 = top_level.add_module RDoc::NormalModule, 'Mod2'
mod3 = mod2.add_module RDoc::NormalModule, 'Mod3'
submod = mod1.add_module RDoc::NormalModule, 'Sub'
mod2.add_include RDoc::Include.new('Mod1', '')
top_level.add_include RDoc::Include.new('Mod2', '')
mod3.add_include RDoc::Include.new('Sub', '')
assert_equal [submod], mod3.ancestors
end

def test_store_equals
incl = RDoc::Include.new 'M', nil
incl.record_location RDoc::TopLevel.new @top_level.name
Expand Down