Skip to content

Change PrismRuby not to depend on hack that stores module nesting information to context.parent #1580

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.

Sign up for GitHub

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

Open
tompng wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Change PrismRuby not to depend on hack that stores module nesting information to context.parent #1580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 14 additions & 3 deletions lib/rdoc/code_object/context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,10 @@ def find_constant_named(name)
end

##
# Find a module at a higher scope
# Tries to find a module at a higher scope.
# But parent is not always a higher module nesting scope, so the result is not correct.
Copy link
Member

@st0012 st0012 Jan 25, 2026

Choose a reason for hiding this comment

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

Can we add examples here?

Looks like it's for

module Foo
  module Bar; end
  module Baz; end # we're at this scope and we try to find Bar?
end

But in some cases this might not work either?

Copy link
Member Author

@tompng tompng Jan 25, 2026

Choose a reason for hiding this comment

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

Comment added.

There are three cases I know.

  1. PrismRuby stopped updating parent chain in this PR.
  2. There's a bug updating parent chain in RDoc::Parser::Ruby, (not updated when opening module)
  3. Module nesting will change with opening class, but parent chain can only represent one global module nesting.
class A::B::C
  class D::E
    # module nesting 1
    include M
  end
end

class A::B
  class C::D::E
    # module nesting 2
    include N
  end
end
# Only the last opened module nesting will be used to resolve crossref and include/extend

# Parent chain can only represent last-opened nesting, and may be broken in some cases.
# PrismRuby parser stopped representing module nesting with parent chain at all.

def find_enclosing_module_named(name)
parent && parent.find_module_named(name)
Expand Down Expand Up @@ -860,15 +863,23 @@ def find_method_named(name)
end

##
# Find a module with +name+ using ruby's scoping rules
# Find a module with +name+ trying to using ruby's scoping rules.
# find_enclosing_module_named cannot use ruby's scoping so the result is not correct.

def find_module_named(name)
res = @modules[name] || @classes[name]
res = get_module_named(name)
return res if res
return self if self.name == name
find_enclosing_module_named name
end

# Get a module named +name+ in this context
# Don't look up for higher module nesting scopes. RDoc::Context doesn't have that information.

def get_module_named(name)
@modules[name] || @classes[name]
end

##
# Look up +symbol+, first as a module, then as a local symbol.

Expand Down
3 changes: 3 additions & 0 deletions lib/rdoc/code_object/mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ def inspect # :nodoc:
# lookup behavior.
#
# As of the beginning of October, 2011, no gem includes nonexistent modules.
#
# When mixin is created from RDoc::Parser::PrismRuby, module name is already a resolved full-path name.
#

def module
return @module if @module
Expand Down
2 changes: 2 additions & 0 deletions lib/rdoc/code_object/top_level.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ def find_module_named(name)
find_class_or_module(name)
end

alias get_module_named find_module_named

##
# Returns the relative name of this file

Expand Down
23 changes: 11 additions & 12 deletions lib/rdoc/parser/prism_ruby.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,6 @@ def with_container(container, singleton: false)
@container = container
@singleton = singleton
@in_proc_block = false
unless singleton
# Need to update module parent chain to emulate Module.nesting.
# This mechanism is inaccurate and needs to be fixed.
container.parent = old_container
end
Copy link
Member Author

@tompng tompng Jan 24, 2026

Choose a reason for hiding this comment

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

RDoc::Parser::Ruby updates parent only for NormalClass. This information is used by includes/extends name resolve and also in CrossReference resolve.
This is one of the reason that HTML generated by RDoc::Parser::PrismRuby and RDoc::Parser::Ruby has huge diffs.
And there is no way to reduce diff except re-implementing many bugs.

Copy link
Member Author

@tompng tompng Jan 24, 2026

Choose a reason for hiding this comment

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

Changing this part to

container.parent = old_container if !singleton && container.is_a?(RDoc::NormalClass)

will minimize crossref difference of generated HTML between two parsers.
(but this change doesn't make sense. It's just re-implementing a bug)

Copy link
Member

@st0012 st0012 Jan 25, 2026

Choose a reason for hiding this comment

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

Can you this some examples of the diffs caused by this? It's not entirely clear to me.

Copy link
Member Author

@tompng tompng Jan 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This is really complicated. In RDoc::Parser::Ruby parser, parent chian is updated like this.

class A; class B; class C; class D; class E; end; end; end; end; end
# Parent chain is:
# F → E → D → C → B → A → toplevel

class A::B::C
  # Method comment A::B B::C C::D D::E
  # (B::C is not linked because RDoc only search A::B::C and toplevel)
  def f; end
  module D::E
  end
end

# Parent chain is updated. This imitates module nesting
# E → C → toplevel (changed)
# B → A → toplevel, D → C (unchanged)
# `B::C` in method comment is not linked because it can't be found from C

Same situation for modules. It's strange that parent chain is not updated. Maybe bug or workaround for something.

module M; module N; module O; module P; module Q; end; end; end; end; end
# Parent chain is:
# R → Q → P → O → N → M → toplevel

module M::N::O
  # Method comment M::N N::O O::P P::Q
  # All linked because RDoc searches M::N::O, M::N, M and toplevel
  def f; end
end

Parent chain is emulating module nesting, used in crossref resolve, include/extend name resolve, superclass name resolve, etc.
Parent chain update is skipped for modules, so module parent seems to represent module owner. This inconsistency causes many bugs, but this also makes crossref work better in some cases.
PrismRuby parser updated both module and class to emulate module nesting, so crossref behavior got worse as a bugfix.

In this pull request, parent will no longer updated. Parent will exactly represent module/class owner, not module nesting.
As a result, B::C in the method comment will be linked too. It means crossref doesn't follow module nesting.
But I think it's OK, some comments already don't follow module nesting, and already linked.

class RDoc::TopLevel < RDoc::Context
  ##
  # Returns the NormalClass "Object", creating it if not found.
  # (NormalClass can't be resolved in this module nesting but we often write this kind of comment)

  def object_class
  end
end
RDoc::Parser::Ruby PrismRuby(master) PrismRuby (this PR)
Linked classes A::B C::D D::E A::B C::D D::E A::B B::C C::D D::E
Linked modules M::N N::O O::P P::Q M::N O::P P::Q M::N N::O O::P P::Q

@module_nesting.push([container, singleton])
yield container
ensure
Expand Down Expand Up @@ -482,12 +477,15 @@ def add_attributes(names, rw, line_no)
end
end

# Adds includes/extends. Module name is resolved to full before adding.

def add_includes_extends(names, rdoc_class, line_no) # :nodoc:
return if @in_proc_block
comment, directives = consecutive_comment(line_no)
handle_code_object_directives(@container, directives) if directives
names.each do |name|
ie = @container.add(rdoc_class, name, '')
resolved_name = resolve_constant_path(name)
ie = @container.add(rdoc_class, resolved_name || name, '')
ie.store = @store
ie.line = line_no
ie.comment = comment
Expand Down Expand Up @@ -590,7 +588,7 @@ def find_or_create_module_path(module_name, create_mode)
else
@module_nesting.reverse_each do |nesting, singleton|
next if singleton
mod = nesting.find_module_named(root_name)
mod = nesting.get_module_named(root_name)
break if mod
# If a constant is found and it is not a module or class, RDoc can't document about it.
# Return an anonymous module to avoid wrong document creation.
Expand All @@ -601,9 +599,9 @@ def find_or_create_module_path(module_name, create_mode)
mod ||= add_module.call(last_nesting, root_name, :module)
end
path.each do |name|
mod = mod.find_module_named(name) || add_module.call(mod, name, :module)
mod = mod.get_module_named(name) || add_module.call(mod, name, :module)
end
mod.find_module_named(name) || add_module.call(mod, name, create_mode)
mod.get_module_named(name) || add_module.call(mod, name, create_mode)
end

# Resolves constant path to a full path by searching module nesting
Expand All @@ -614,10 +612,10 @@ def resolve_constant_path(constant_path)
mod = nil
@module_nesting.reverse_each do |nesting, singleton|
next if singleton
mod = nesting.find_module_named(owner_name)
mod = nesting.get_module_named(owner_name)
break if mod
end
mod ||= @top_level.find_module_named(owner_name)
mod ||= @top_level.get_module_named(owner_name)
[mod.full_name, path].compact.join('::') if mod
end

Expand Down Expand Up @@ -657,7 +655,8 @@ def add_constant(constant_name, rhs_name, start_line, end_line)
if rhs_name =~ /^::/
@store.find_class_or_module(rhs_name)
else
@container.find_module_named(rhs_name)
full_name = resolve_constant_path(rhs_name)
@store.find_class_or_module(full_name)
end
if mod && constant.document_self
a = @container.add_module_alias(mod, rhs_name, constant, @top_level)
Expand Down
16 changes: 9 additions & 7 deletions test/rdoc/parser/prism_ruby_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1746,6 +1746,7 @@ class << self
end

def test_include_with_module_nesting
omit 'not implemented' if accept_legacy_bug?
util_parser <<~RUBY
module A
module M; end
Expand All @@ -1765,15 +1766,16 @@ class C::D::Foo
include M
end
end
# TODO: make test pass with the following code appended
# module A::B::C
# class D::Foo
# include M
# end
# end

module A::B::C
class D::Foo
include M
end
end
RUBY
klass = @store.find_class_named 'A::B::C::D::Foo'
assert_equal 'A::B::M', klass.includes.first.module.full_name
assert_equal 'A::B::M', klass.includes[0].module.full_name
assert_equal 'A::B::C::M', klass.includes[1].module.full_name
end

def test_various_argument_include
Expand Down
Loading