Stop lazy alias resolver from overwriting real classes (#1662) (#1689)

Author: st0012

AGENTS.md

@@ -234,11 +234,25 @@ exe/
 
 ## Architecture Notes
 
-### Pluggable System
+### Parsers and Generators
 
-- **Parsers:** Ruby, C, Markdown, RD, Prism-based Ruby (experimental)
+- **Parsers:** Prism-based Ruby (default, `RDoc::Parser::PrismRuby`), legacy ripper-based Ruby (`RDoc::Parser::RipperRuby`, opt-in via `RDOC_USE_RIPPER_PARSER=1`), C, Markdown, RD
 - **Generators:** HTML/Aliki (default), HTML/Darkfish (deprecated), RI, POT (gettext), JSON, Markup
 
+Both Ruby parsers must produce equivalent code-object trees, so parser tests live in the `RDocParserPrismTestCases` module (`test/rdoc/parser/prism_ruby_test.rb`) and are included by both `RDocParserPrismRubyTest` and `RDocParserRipperRubyWithPrismRubyTestCasesTest`. The ripper variant is gated on `RDOC_USE_RIPPER_PARSER`, so `bundle exec rake` locally only runs prism; CI exercises ripper in a separate job. Add new parser tests to the mixin, and run `RDOC_USE_RIPPER_PARSER=1 bundle exec rake` locally before declaring a parser change done.
+
+### Code Object Model and Constant Aliases
+
+The code-object tree (`lib/rdoc/code_object/`) is built in two phases. Parse-time work happens in the parsers and `RDoc::Context` (`add_constant`, `add_module_alias`). Finalization happens in `Store#complete`, which calls `ClassModule#update_aliases` on each container — this is where forward-reference aliases (`Foo = Bar` parsed before `class Bar` in another file) get resolved via `Constant#resolved_alias_target`.
+
+If you add an invariant to one of these paths — for example the `Context#add_module_alias` collision guard that refuses to clobber an existing class — mirror it on the other. The two paths are not interchangeable: `add_module_alias` runs against partial store state and does extra bookkeeping (`unmatched_constant_alias`); `update_aliases` runs against the finalized store and writes the alias copies into `classes_hash` / `modules_hash`.
+
+**Known limitation: lexical scope.** `Context#find_enclosing_module_named` walks the syntactic parent chain as a stand-in for Ruby's lexical constant lookup. The prism parser doesn't represent module nesting via the parent chain at all (see the docstring on `Context#find_enclosing_module_named`), so alias resolution in deeply nested or re-opened classes can pick the wrong target. Fixing this properly requires capturing lexical scope at parse time — a feature change rather than an incremental fix.
+
+### Marshal / ri Data Compatibility
+
+`RDoc::Constant`, `RDoc::ClassModule`, and other code objects implement `marshal_dump` / `marshal_load` to persist ri data on disk, gated by a per-class `MARSHAL_VERSION` constant. The `ri` CLI (`lib/rdoc/ri/driver.rb`) and the `ri --server` servlet (`lib/rdoc/ri/servlet.rb`) read this format. Any change that alters the dumped array — adding/removing slots, reinterpreting an existing slot's meaning — needs `MARSHAL_VERSION` bumped and the loader taught to handle older payloads, otherwise locally-cached `.ri` data from an earlier rdoc version stops loading after an upgrade.
+
 ### Live Preview Server (`RDoc::Server`)
 
 The server (`lib/rdoc/server.rb`) provides `rdoc --server` for live documentation preview.

lib/rdoc/code_object/class_module.rb

@@ -847,7 +847,9 @@ def type
 
   def update_aliases
     constants.each do |const|
-      next unless cm = const.is_alias_for
+      cm = const.is_alias_for
+      cm ||= const.resolved_alias_target if const.is_a?(RDoc::Constant)
+      next unless cm
 
       # Resolve chained aliases (A = B = C) to the real class/module.
       cm = @store.find_class_or_module(cm.full_name) || cm
@@ -866,6 +868,19 @@ def update_aliases
       end
       cm_alias.full_name = nil # force update for new parent
 
+      # Don't clobber a real (non-alias) class/module already living at this
+      # name. Mirrors the BasicObject = BlankSlate guard in
+      # Context#add_module_alias. Existing alias copies (set by
+      # add_module_alias or a previous update_aliases pass) carry is_alias_for,
+      # so they're still overwritable here.
+      existing = @store.find_class_or_module(cm_alias.full_name)
+      next if existing && !existing.is_alias_for
+
+      # Persist a lazy-resolved target so Stats#report_constants and
+      # Constant#marshal_dump observe the alias relationship. Skipped
+      # aliases (above) intentionally leave the constant unmarked.
+      const.is_alias_for ||= cm
+
       cm_alias.aliases.clear
       cm_alias.is_alias_for = cm
 

lib/rdoc/code_object/constant.rb

@@ -26,6 +26,14 @@ class RDoc::Constant < RDoc::CodeObject
 
   attr_accessor :visibility
 
+  ##
+  # The constant path on the RHS when the RHS is a bare constant reference
+  # (+Foo = Bar+ or +Foo = Bar::Baz+). Captured at parse time so
+  # #resolved_alias_target doesn't have to re-derive it from the textual
+  # #value. nil for other RHS shapes.
+
+  attr_accessor :is_alias_for_path
+
   ##
   # Creates a new constant with +name+, +value+ and +comment+
 
@@ -35,8 +43,9 @@ def initialize(name, value, comment)
     @name  = name
     @value = value
 
-    @is_alias_for = nil
-    @visibility   = :public
+    @is_alias_for      = nil
+    @is_alias_for_path = nil
+    @visibility        = :public
 
     self.comment = comment
   end
@@ -83,7 +92,10 @@ def full_name
   end
 
   ##
-  # The module or class this constant is an alias for
+  # The module or class this constant is an alias for, when one was recorded
+  # explicitly (by RDoc::Context#add_module_alias, RDoc::ClassModule#update_aliases,
+  # or ri marshal load). Pure accessor; see #resolved_alias_target for the
+  # opportunistic lookup path.
 
   def is_alias_for
     case @is_alias_for
@@ -92,16 +104,22 @@ def is_alias_for
       @is_alias_for = found if found
       @is_alias_for
     else
-      @is_alias_for ||= find_alias_for
+      @is_alias_for
     end
   end
 
-  # Find alias constant for a value.
-  # This is used when the constant is parsed before the class or module defined in another file.
-  # Note that module nesting information is lost, so constant lookup is inaccurate.
-
-  def find_alias_for
-    parent.find_module_named(value) if value&.match?(/\A(::)?([A-Z][A-Za-z0-9_]*)(::[A-Z][A-Za-z0-9_]*)*\z/)
+  ##
+  # Returns the class/module this constant *would* alias if #is_alias_for_path
+  # was set by the parser and that path resolves to a known class/module, or
+  # nil. Used to support `Const = RHS` parsed before `class RHS;end` is defined
+  # in another file. Pure lookup; does not mutate state. Honors :nodoc:
+  # (returns nil if document_self is false). Note that module nesting
+  # information is lost, so constant lookup is inaccurate.
+
+  def resolved_alias_target
+    return nil unless document_self
+    return nil unless @is_alias_for_path
+    parent.find_module_named(@is_alias_for_path)
   end
 
   def inspect # :nodoc:

lib/rdoc/code_object/context.rb

@@ -544,6 +544,7 @@ def add_module_alias(from, from_name, to, file)
     new_to = from.dup
     new_to.name = to.name
     new_to.full_name = nil
+    new_to.is_alias_for = from
 
     if new_to.module? then
       @store.modules_hash[to_full_name] = new_to

lib/rdoc/parser/prism_ruby.rb

@@ -767,7 +767,7 @@ def find_or_create_constant_owner_name(constant_path)
 
   # Adds a constant
 
-  def add_constant(constant_name, rhs_name, start_line, end_line)
+  def add_constant(constant_name, rhs_name, start_line, end_line, alias_path: nil)
     comment, directives = consecutive_comment(start_line)
     handle_code_object_directives(@container, directives) if directives
     owner, name = find_or_create_constant_owner_name(constant_name)
@@ -776,19 +776,21 @@ def add_constant(constant_name, rhs_name, start_line, end_line)
     constant = RDoc::Constant.new(name, rhs_name, comment)
     constant.store = @store
     constant.line = start_line
+    constant.is_alias_for_path = alias_path
     record_location(constant)
     handle_modifier_directive(constant, start_line)
     handle_modifier_directive(constant, end_line)
     owner.add_constant(constant)
+    return unless alias_path
     mod =
-      if rhs_name =~ /^::/
-        @store.find_class_or_module(rhs_name)
+      if alias_path.start_with?('::')
+        @store.find_class_or_module(alias_path)
       else
-        full_name = resolve_constant_path(rhs_name)
+        full_name = resolve_constant_path(alias_path)
         @store.find_class_or_module(full_name)
       end
     if mod && constant.document_self
-      a = @container.add_module_alias(mod, rhs_name, constant, @top_level)
+      a = owner.add_module_alias(mod, alias_path, constant, @top_level)
       a.store = @store
       a.line = start_line
       record_location(a)
@@ -1056,23 +1058,27 @@ def visit_constant_path_write_node(node)
       path = constant_path_string(node.target)
       return unless path
 
+      alias_path = constant_path_string(node.value)
       @scanner.add_constant(
         path,
-        constant_path_string(node.value) || node.value.slice,
+        alias_path || node.value.slice,
         node.location.start_line,
-        node.location.end_line
+        node.location.end_line,
+        alias_path: alias_path
       )
       @scanner.skip_comments_until(node.location.end_line)
       # Do not traverse rhs not to document `A::B = Struct.new{def undocumentable_method; end}`
     end
 
     def visit_constant_write_node(node)
       @scanner.process_comments_until(node.location.start_line - 1)
+      alias_path = constant_path_string(node.value)
       @scanner.add_constant(
         node.name.to_s,
-        constant_path_string(node.value) || node.value.slice,
+        alias_path || node.value.slice,
         node.location.start_line,
-        node.location.end_line
+        node.location.end_line,
+        alias_path: alias_path
       )
       @scanner.skip_comments_until(node.location.end_line)
       # Do not traverse rhs not to document `A = Struct.new{def undocumentable_method; end}`

lib/rdoc/parser/ripper_ruby.rb

@@ -170,6 +170,7 @@ def create_attr(container, single, name, rw, comment) # :nodoc:
   # for "::") with the name from +constant+.
 
   def create_module_alias(container, constant, rhs_name) # :nodoc:
+    constant.is_alias_for_path = rhs_name
     mod = if rhs_name =~ /^::/ then
             @store.find_class_or_module rhs_name
           else

test/rdoc/code_object/class_module_test.rb

@@ -1459,6 +1459,56 @@ def test_update_aliases_reparent_root
     assert_equal 'Klass', store.classes_hash['Klass'].full_name
   end
 
+  def test_update_aliases_does_not_overwrite_existing_class_with_same_name
+    store = RDoc::Store.new(RDoc::Options.new)
+    top_level = store.add_file 'file.rb'
+
+    object = top_level.add_class RDoc::NormalClass, 'Object'
+    real_foo = top_level.add_class RDoc::NormalClass, 'Foo'
+    real_foo.add_method RDoc::AnyMethod.new(nil, 'real_method')
+
+    other = top_level.add_class RDoc::NormalClass, 'Other'
+    other.add_method RDoc::AnyMethod.new(nil, 'other_method')
+
+    const = RDoc::Constant.new 'Foo', 'Other', ''
+    const.is_alias_for_path = 'Other'
+    const.record_location top_level
+    object.add_constant const
+
+    object.update_aliases
+
+    assert_same real_foo, store.classes_hash['Foo']
+    assert_nil store.classes_hash['Foo'].is_alias_for
+    assert_equal ['real_method'], store.classes_hash['Foo'].method_list.map(&:name)
+    assert_same other, store.classes_hash['Other']
+    assert_nil other.is_alias_for
+    assert_equal ['other_method'], other.method_list.map(&:name)
+    assert_empty other.aliases
+  end
+
+  def test_update_aliases_skips_nodoc_constant
+    store = RDoc::Store.new(RDoc::Options.new)
+    top_level = store.add_file 'file.rb'
+
+    object = top_level.add_class RDoc::NormalClass, 'Object'
+    target = top_level.add_class RDoc::NormalClass, 'Target'
+    target.add_method RDoc::AnyMethod.new(nil, 'target_method')
+
+    const = RDoc::Constant.new 'NodocAlias', 'Target', ''
+    const.is_alias_for_path = 'Target'
+    const.record_location top_level
+    const.document_self = nil
+    object.add_constant const
+
+    object.update_aliases
+
+    assert_nil store.classes_hash['NodocAlias']
+    assert_nil store.modules_hash['NodocAlias']
+    assert_same target, store.classes_hash['Target']
+    assert_equal ['target_method'], target.method_list.map(&:name)
+    assert_empty target.aliases
+  end
+
   def test_update_includes
     a = RDoc::Include.new 'M1', nil
     b = RDoc::Include.new 'M2', nil