Refactor comment_location from Array to Hash

Convert ClassModule#comment_location from an Array of [comment, location]
pairs to a Hash of { location => comment }. Ruby hashes preserve insertion
order, and replacing an existing key preserves its position, which naturally
fixes the comment reordering bug during server re-parse without needing
empty placeholder workarounds.

Key changes:
- add_comment simplifies to a single hash assignment
- clear_file_contributions gains keep_position: keyword for server re-parse
- C parser's delete_if special case is no longer needed (hash replaces)
- Same-file duplicate comments are naturally deduplicated
- Marshal format is unchanged (serialized via parse() as before)

Author: st0012

lib/rdoc/code_object/class_module.rb

@@ -30,22 +30,19 @@ class RDoc::ClassModule < RDoc::Context
   attr_accessor :constant_aliases
 
   ##
-  # An array of `[comment, location]` pairs documenting this class/module.
+  # A hash of <tt>{ location => comment }</tt> documenting this class/module.
   # Use #add_comment to add comments.
   #
+  # Ruby hashes maintain insertion order, so comments render in the order
+  # they were first added. Re-assigning a key preserves its position.
+  #
   # Before marshalling:
-  # - +comment+ is a String
   # - +location+ is an RDoc::TopLevel
+  # - +comment+ is a String
   #
   # After unmarshalling:
-  # - +comment+ is an RDoc::Markup::Document
   # - +location+ is a filename String
-  #
-  # These type changes are acceptable (for now) because:
-  # - +comment+: Both String and Document respond to #empty?, and #parse
-  #   returns Document as-is (see RDoc::Text#parse)
-  # - +location+: Only used by #parse to set Document#file, which accepts
-  #   both TopLevel (extracts relative_name) and String
+  # - +comment+ is an RDoc::Markup::Document
 
   attr_accessor :comment_location
 
@@ -63,7 +60,7 @@ class RDoc::ClassModule < RDoc::Context
   def self.from_module(class_type, mod)
     klass = class_type.new mod.name
 
-    mod.comment_location.each do |comment, location|
+    mod.comment_location.each do |location, comment|
       klass.add_comment comment, location
     end
 
@@ -125,7 +122,7 @@ def initialize(name, superclass = nil)
     @is_alias_for     = nil
     @name             = name
     @superclass       = superclass
-    @comment_location = [] # Array of [comment, location] pairs
+    @comment_location = {} # Hash of { location => comment }
 
     super()
   end
@@ -147,11 +144,7 @@ def add_comment(comment, location)
                 normalize_comment comment
               end
 
-    if location.parser == RDoc::Parser::C
-      @comment_location.delete_if { |(_, l)| l == location }
-    end
-
-    @comment_location << [comment, location]
+    @comment_location[location] = comment
 
     self.comment = original
   end
@@ -270,7 +263,7 @@ def document_self_or_methods
   def documented?
     return true if @received_nodoc
     return false if @comment_location.empty?
-    @comment_location.any? { |comment, _| not comment.empty? }
+    @comment_location.each_value.any? { |comment| not comment.empty? }
   end
 
   ##
@@ -408,13 +401,7 @@ def marshal_load(array) # :nodoc:
     @superclass = array[3]
     document    = array[4]
 
-    @comment    = RDoc::Comment.from_document document
-
-    @comment_location = if document.parts.first.is_a?(RDoc::Markup::Document)
-                          document.parts.map { |doc| [doc, doc.file] }
-                        else
-                          [[document, document.file]]
-                        end
+    load_comment_from_document(document)
 
     array[5].each do |name, rw, visibility, singleton, file|
       singleton  ||= false
@@ -492,13 +479,7 @@ def merge(class_module)
 
       document = document.merge other_document
 
-      @comment = RDoc::Comment.from_document(document)
-
-      @comment_location = if document.parts.first.is_a?(RDoc::Markup::Document)
-                            document.parts.map { |doc| [doc, doc.file] }
-                          else
-                            [[document, document.file]]
-                          end
+      load_comment_from_document(document)
     end
 
     cm = class_module
@@ -643,8 +624,8 @@ def parse(comment_location)
     case comment_location
     when String then
       super
-    when Array then
-      docs = comment_location.map do |comment, location|
+    when Hash then
+      docs = comment_location.map do |location, comment|
         doc = super comment
         doc.file = location
         doc
@@ -745,12 +726,22 @@ def search_record
   # Returns an HTML snippet of the first comment for search results.
 
   def search_snippet
-    first_comment = @comment_location.first&.first
+    first_comment = @comment_location.each_value.first
     return '' unless first_comment && !first_comment.empty?
 
     snippet(first_comment)
   end
 
+  ##
+  # Rebuilds +@comment+ from the current +@comment_location+ entries,
+  # skipping any empty placeholders.
+
+  def rebuild_comment_from_location
+    texts = @comment_location.each_value.filter_map { |c| c.to_s unless c.empty? }
+    merged = texts.join("\n---\n")
+    @comment = merged.empty? ? '' : RDoc::Comment.new(merged)
+  end
+
   ##
   # Sets the store for this class or module and its contained code objects.
 
@@ -926,6 +917,15 @@ def embed_mixins
 
   private
 
+  def load_comment_from_document(document) # :nodoc:
+    @comment = RDoc::Comment.from_document(document)
+    @comment_location = if document.parts.first.is_a?(RDoc::Markup::Document)
+                          document.parts.to_h { |doc| [doc.file, doc] }
+                        else
+                          { document.file => document }
+                        end
+  end
+
   def prepare_to_embed(code_object, singleton=false)
     code_object = code_object.dup
     code_object.mixin_from = code_object.parent

lib/rdoc/i18n/text.rb

@@ -7,7 +7,7 @@
 #   * Extracts translation messages from wrapped raw text.
 #   * Translates wrapped raw text in specified locale.
 #
-# Wrapped raw text is one of String, RDoc::Comment or Array of them.
+# Wrapped raw text is one of String, RDoc::Comment, Hash, or Array of them.
 
 class RDoc::I18n::Text
 
@@ -89,8 +89,8 @@ def each_line(raw, &block)
     case raw
     when RDoc::Comment
       raw.text.each_line(&block)
-    when Array
-      raw.each do |comment, location|
+    when Hash
+      raw.each_value do |comment|
         each_line(comment, &block)
       end
     else

lib/rdoc/server.rb

@@ -356,7 +356,7 @@ def reparse_and_refresh(changed_files, removed_files)
         changed_files.each do |f|
           begin
             relative = @rdoc.relative_path_for(f)
-            @store.clear_file_contributions(relative)
+            @store.clear_file_contributions(relative, keep_position: true)
             @rdoc.parse_file(f)
             @file_mtimes[f] = File.mtime(f) rescue nil
           rescue => e

lib/rdoc/store.rb

@@ -220,7 +220,7 @@ def remove_file(relative_name)
   # prevents duplication when the file is re-parsed while preserving
   # shared namespaces like +RDoc+ that span many files.
 
-  def clear_file_contributions(relative_name)
+  def clear_file_contributions(relative_name, keep_position: false)
     top_level = @files_hash[relative_name]
     return unless top_level
 
@@ -245,30 +245,34 @@ def clear_file_contributions(relative_name)
       cm.aliases.reject! { |a| a.file == top_level }
       cm.external_aliases.reject! { |a| a.file == top_level }
 
-      # Remove comment entries from this file and rebuild the comment
+      # Clear or remove comment entries from this file
       if cm.is_a?(RDoc::ClassModule)
-        cm.comment_location.reject! { |(_, loc)| loc == top_level }
-        texts = cm.comment_location.map { |(c, _)| c.to_s }
-        merged = texts.join("\n---\n")
-        cm.instance_variable_set(:@comment,
-          merged.empty? ? '' : RDoc::Comment.new(merged))
+        if keep_position
+          # Set to empty comment; hash preserves key position for re-parse
+          cm.comment_location[top_level] = RDoc::Comment.new('') if cm.comment_location.key?(top_level)
+        else
+          cm.comment_location.delete(top_level)
+        end
+        cm.rebuild_comment_from_location
       end
 
-      # Remove this file from the class/module's file list
-      cm.in_files.delete(top_level)
-
-      # If no files contribute to this class/module anymore, remove it
-      # from the store entirely.  This handles file deletion correctly
-      # for classes that are only defined in the deleted file, while
-      # preserving classes that span multiple files.
-      if cm.in_files.empty?
-        if cm.is_a?(RDoc::NormalModule)
-          @modules_hash.delete(cm.full_name)
-        else
-          @classes_hash.delete(cm.full_name)
+      unless keep_position
+        # Remove this file from the class/module's file list
+        cm.in_files.delete(top_level)
+
+        # If no files contribute to this class/module anymore, remove it
+        # from the store entirely.  This handles file deletion correctly
+        # for classes that are only defined in the deleted file, while
+        # preserving classes that span multiple files.
+        if cm.in_files.empty?
+          if cm.is_a?(RDoc::NormalModule)
+            @modules_hash.delete(cm.full_name)
+          else
+            @classes_hash.delete(cm.full_name)
+          end
+          cm.parent&.classes_hash&.delete(cm.name)
+          cm.parent&.modules_hash&.delete(cm.name)
         end
-        cm.parent&.classes_hash&.delete(cm.name)
-        cm.parent&.modules_hash&.delete(cm.name)
       end
     end
 

test/rdoc/code_object/class_module_test.rb

@@ -12,21 +12,20 @@ def test_add_comment
     comment_tl1 = RDoc::Comment.new('# comment 1', @top_level, :ruby)
     cm.add_comment comment_tl1, tl1
 
-    assert_equal [[comment_tl1, tl1]], cm.comment_location
+    assert_equal({ tl1 => comment_tl1 }, cm.comment_location)
     assert_equal 'comment 1', cm.comment.text
 
     comment_tl2 = RDoc::Comment.new('# comment 2', @top_level, :ruby)
     cm.add_comment comment_tl2, tl2
 
-    assert_equal [[comment_tl1, tl1], [comment_tl2, tl2]], cm.comment_location
+    assert_equal({ tl1 => comment_tl1, tl2 => comment_tl2 }, cm.comment_location)
     assert_equal "comment 1\n---\ncomment 2", cm.comment
 
     comment_tl3 = RDoc::Comment.new('# * comment 3', @top_level, :ruby)
     cm.add_comment comment_tl3, tl3
 
-    assert_equal [[comment_tl1, tl1],
-                  [comment_tl2, tl2],
-                  [comment_tl3, tl3]], cm.comment_location
+    assert_equal({ tl1 => comment_tl1, tl2 => comment_tl2,
+                   tl3 => comment_tl3 }, cm.comment_location)
     assert_equal "comment 1\n---\ncomment 2\n---\n* comment 3", cm.comment
   end
 
@@ -47,8 +46,27 @@ def test_add_comment_duplicate
     cm.add_comment comment1, tl1
     cm.add_comment comment2, tl1
 
-    assert_equal [[comment1, tl1],
-                  [comment2, tl1]], cm.comment_location
+    # Hash replaces in-place for the same location
+    assert_equal({ tl1 => comment2 }, cm.comment_location)
+  end
+
+  def test_add_comment_preserves_order_on_replace
+    tl1 = @store.add_file 'one.rb'
+    tl2 = @store.add_file 'two.rb'
+
+    cm = RDoc::ClassModule.new 'Klass'
+    cm.add_comment 'comment from one', tl1
+    cm.add_comment 'comment from two', tl2
+
+    # Simulate keep_position clearing: set tl1's comment to empty
+    cm.comment_location[tl1] = RDoc::Comment.new('')
+
+    # Re-adding a comment for tl1 replaces in-place (hash preserves key order)
+    cm.add_comment 'updated comment from one', tl1
+
+    assert_equal 2, cm.comment_location.size
+    assert_equal [tl1, tl2], cm.comment_location.keys
+    assert_equal 'updated comment from one', cm.comment_location[tl1].to_s
   end
 
   def test_add_comment_stopdoc
@@ -156,7 +174,7 @@ def test_from_module_comment
 
     klass = RDoc::ClassModule.from_module RDoc::NormalClass, klass
 
-    assert_equal [['really a class', tl]], klass.comment_location
+    assert_equal({ tl => 'really a class' }, klass.comment_location)
   end
 
   def test_marshal_dump
@@ -631,7 +649,7 @@ def test_search_snippet_after_marshal
     assert_match(/class comment/, snippet)
   end
 
-  def test_comment_location_is_array_after_marshal
+  def test_comment_location_is_hash_after_marshal
     @store.path = Dir.tmpdir
     tl = @store.add_file 'file.rb'
 
@@ -642,10 +660,10 @@ def test_comment_location_is_array_after_marshal
     loaded = Marshal.load Marshal.dump cm
     loaded.store = @store
 
-    assert_kind_of Array, loaded.comment_location
-    assert_equal 1, loaded.comment_location.length
+    assert_kind_of Hash, loaded.comment_location
+    assert_equal 1, loaded.comment_location.size
 
-    comment, location = loaded.comment_location.first
+    location, comment = loaded.comment_location.first
     assert_kind_of RDoc::Markup::Document, comment
     # After marshal, location is the filename string (from doc.file)
     assert_equal tl.relative_name, location
@@ -668,7 +686,7 @@ def test_merge
     assert c1.current_section, 'original current_section'
     assert c2.current_section, 'merged current_section'
 
-    comment, location = c2.comment_location.first
+    location, comment = c2.comment_location.first
     assert_kind_of RDoc::Markup::Document, comment
     assert_equal tl.relative_name, location
   end
@@ -793,7 +811,10 @@ def test_merge_comment
     inner2 = @RM::Document.new @RM::Paragraph.new 'klass 2'
     inner2.file = 'two.rb'
 
-    expected = @RM::Document.new inner2, inner1
+    # With hash-based comment_location, one.rb comes first (its key was
+    # inserted first), then two.rb.  Document.merge preserves this order
+    # since both self and other have the same files.
+    expected = @RM::Document.new inner1, inner2
 
     assert_equal expected, cm1.comment.parse
   end
@@ -1217,17 +1238,15 @@ def test_parse_comment_location
     cm.add_comment 'comment 1', tl1
     cm.add_comment 'comment 2', tl2
 
-    assert_kind_of Array, cm.comment_location
-    assert_equal 2, cm.comment_location.length
-    assert_equal 'comment 1', cm.comment_location[0][0]
-    assert_equal tl1, cm.comment_location[0][1]
-    assert_equal 'comment 2', cm.comment_location[1][0]
-    assert_equal tl2, cm.comment_location[1][1]
+    assert_kind_of Hash, cm.comment_location
+    assert_equal 2, cm.comment_location.size
+    assert_equal 'comment 1', cm.comment_location[tl1]
+    assert_equal 'comment 2', cm.comment_location[tl2]
 
     cm = Marshal.load Marshal.dump cm
 
-    # After marshal, comment_location should still be an array
-    assert_kind_of Array, cm.comment_location
+    # After marshal, comment_location should still be a hash
+    assert_kind_of Hash, cm.comment_location
 
     # parse() produces a Document with parts for each comment
     parsed = cm.parse(cm.comment_location)

test/rdoc/parser/c_test.rb

@@ -318,8 +318,7 @@ def test_do_classes_duplicate_class
 
     klass = util_get_class content, 'cFoo'
     assert_equal 1, klass.comment_location.size
-    first = klass.comment_location.first
-    first_comment = first[0]
+    first_comment = klass.comment_location.each_value.first
     assert_equal 'first', first_comment.text
   end