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

Conversation

tompng
Copy link
Member

@tompng tompng commented Jun 30, 2024

Fixes this bug

module Mod1
  module Sub
  end
end

module Mod2
  include Mod1
end

module Mod3
  include Mod2
  include Sub
  # 'Mod1::Sub' is included, but RDoc thinks unknown constant 'Sub' is included
end

What was wrong with constant lookup implementation

module M1
  module M2
    include A
    include B

    # Constant lookup of C
    # should search under M2, under M1, recursively B, recursively A, recursively Object
    # Was searching under M2, under B, under A, under M1
    include C

    include D
  end
end

Ruby's actual constant priority
document https://docs.ruby-lang.org/ja/latest/doc/spec=2fvariables.html#prio (Sorry, can't find the english version)

# To check constant lookup priority, comment out C=1, C=2, C=3.0, C=3.1, ... in that order
module A; C=4.0; module AA; C=4.1; end; include AA; end
module B; C=3.0; module BB; C=3.1; end; include BB; end
module X; C=:never; end
C=5.0; module O1; C=6.1; end; include O1 # TopLevel ≒ Object
class Object; C=5.1; module O2; C=6.0; end; include O2; end
module M1
  include X
  C=2
  module M2
    C=1
    include A
    include B
    p C
  end
end

@@ -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]

@st0012 st0012 added the bug label Jun 30, 2024
@colby-swandale colby-swandale self-assigned this Jul 3, 2024
@colby-swandale colby-swandale self-requested a review July 3, 2024 13:22
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants