Skip to content

Rename #initialize to ::new before registering it to the container - #1802

Open
tompng wants to merge 2 commits into
ruby:masterfrom
tompng:move-initialize-rename-before-add
Open

Rename #initialize to ::new before registering it to the container#1802
tompng wants to merge 2 commits into
ruby:masterfrom
tompng:move-initialize-rename-before-add

Conversation

@tompng

@tompng tompng commented Aug 30, 2026

Copy link
Copy Markdown
Member

Background

The Ruby parser renames an instance method initialize to ::new for documentation purposes. This rename happened after container.add_method, with a comment claiming the ordering is intentional: "Rename after add_method to register duplicated 'new' and 'initialize' defined in c and ruby".

The actual reason for this placement is older. In the Ripper-based streaming parser, documentation modifiers such as :notnew: were read after the method line, so at add_method time the parser simply did not know yet whether the method should be renamed:

# Having now read the method parameters and documentation modifiers, we
# now know whether we have to rename #initialize to ::new

(lib/rdoc/parser/ripper_ruby.rb, removed in #1690)

The Prism parser processes directives and modifier lines before add_method, so this constraint is gone. The post-add placement was a consequence of the streaming parser's information ordering, not a design goal — which is why it is safe to retire the post-add mutation pattern now. The comment in the current code was a port-time rationalization of an observable side effect.

What the old placement actually did

Registering the method under the #initialize key and renaming it afterwards had two effects:

  1. Context#methods_hash was left keyed by a stale name (#initialize pointing at a method now called ::new). This is one of the obstacles to making Context#find_method hash-based (see the discussion in RBS scan cache to improve overall performance #1796).
  2. Duplicate detection in Context#add_method was bypassed. A class documenting both ::new (an explicit def self.new, or a C-defined new) and #initialize ended up with two ::new entries on its page.

Change

Move the rename block before container.add_method. All information it uses (the method name, singleton, and dont_rename_initialize set by the :notnew: directive) is already available at that point.

With the rename in place before registration, the normal deduplication applies: the first registration wins, and the duplicate is reported by the existing "Duplicate method" warning (visible with --verbose).

Corpus diff

Compared per-class method lists (name, singleton, visibility, file) for whole corpora, before vs after:

Corpus Changes
ruby/ruby 4 classes lose a duplicated ::new entry: Gem::Package::TarReader, Gem::Package::TarWriter, Gem::Resolver::APISpecification, JSON::Ext::Generator::State
activesupport 8.1.3 1 class: ActiveSupport::Deprecation::DeprecatedConstantProxy
rdoc itself no change

All other entries are identical. Each changed class defines both def self.new and def initialize (or, for JSON::Ext::Generator::State, a C-defined new and a Ruby initialize — the exact "c and ruby" case the old comment referred to). On master these pages show new twice, with a duplicated id="method-c-new" anchor (invalid HTML; the index can only link to the first entry); with this change they show it once.

Cleanup

The second commit removes the dont_rename_initialize keyword argument of internal_add_method. It has never been passed a truthy value since its introduction: one call site passes an explicit false and the other relies on the false default. It is unrelated to AnyMethod#dont_rename_initialize (set by the :notnew: directive), which remains the live mechanism; removing the constant-false parameter also removes the confusion of two same-named flags in one method.

Copilot AI balanced review requested due to automatic review settings August 30, 2026 15:27
@tompng
tompng requested a deployment to fork-preview-protection August 30, 2026 15:27 — with GitHub Actions Waiting

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Moves initializenew normalization before method registration, enabling correct deduplication and hash indexing.

Changes:

  • Deduplicates explicit new and renamed initialize.
  • Removes the unused dont_rename_initialize parameter.
Suppressed comments (1)

lib/rdoc/parser/ruby.rb:761

  • The new first-registration-wins behavior and corrected hash key are the core regression this PR addresses, but the existing parser tests only cover ordinary initialize/:notnew: mapping. Add cases with def self.new both before and after def initialize, asserting a single ::new entry and no stale #initialize key (and ideally the duplicate warning), so this ordering cannot regress.
    container.add_method(meth)

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/rdoc/parser/ruby.rb Outdated
tompng and others added 2 commits August 31, 2026 00:39
The rename of #initialize to ::new happened after container.add_method,
with a comment claiming this is "to register duplicated 'new' and
'initialize' defined in c and ruby". The real reason is older: the
Ripper-based streaming parser read documentation modifiers (:notnew:)
after the method line, so whether to rename was simply unknown at
add_method time ("Having now read the method parameters and
documentation modifiers, we now know whether we have to rename
#initialize to ::new"). The Prism parser processes directives and
modifier lines before add_method, so that constraint is gone and the
post-add mutation pattern has no remaining reason to exist.

The old placement also had a real cost: the method was registered under
the '#initialize' key and renamed afterwards, which left
Context#methods_hash keyed by a stale name and bypassed duplicate
detection. A class documenting both ::new (an explicit `def self.new`,
or a C-defined new) and #initialize ended up with two ::new entries on
its page.

With the rename moved before add_method, the normal deduplication
applies: the first registration wins and the duplicate is reported by
the existing "Duplicate method" warning (visible with --verbose).
record_location is also moved before add_method so that the warning can
name the file the duplicate came from.

Corpus diff (per-class method lists, before vs after):
- ruby/ruby: 4 classes lose a duplicated ::new entry
  (Gem::Package::TarReader, Gem::Package::TarWriter,
  Gem::Resolver::APISpecification, and JSON::Ext::Generator::State --
  the last one is the C new + Ruby initialize case)
- activesupport: 1 class
  (ActiveSupport::Deprecation::DeprecatedConstantProxy)
- rdoc itself: no change
All other entries are identical.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The dont_rename_initialize keyword argument of internal_add_method has
never been passed a truthy value since it was introduced in b92986a:
one call site passes an explicit false and the other relies on the
false default. It is unrelated to AnyMethod#dont_rename_initialize
(set by the :notnew: directive), which remains the live mechanism, and
removing the constant-false parameter also removes the confusion of two
same-named flags in one method.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 30, 2026 15:39
@tompng
tompng force-pushed the move-initialize-rename-before-add branch from ccede23 to a3ef80e Compare August 30, 2026 15:39
@tompng
tompng requested a deployment to fork-preview-protection August 30, 2026 15:40 — with GitHub Actions Waiting

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread lib/rdoc/parser/ruby.rb
end

record_location(meth)
container.add_method(meth)
@st0012 st0012 added the bug label Aug 30, 2026
@skatkov

skatkov commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

It would be awesome to add a test containing both def self.new and def initialize, asserting one ::new entry.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants