Rename #initialize to ::new before registering it to the container - #1802
Open
tompng wants to merge 2 commits into
Open
Rename #initialize to ::new before registering it to the container#1802tompng wants to merge 2 commits into
#initialize to ::new before registering it to the container#1802tompng wants to merge 2 commits into
Conversation
tompng
requested a deployment
to
fork-preview-protection
August 30, 2026 15:27 — with
GitHub Actions
Waiting
Contributor
There was a problem hiding this comment.
Pull request overview
Moves initialize→new normalization before method registration, enabling correct deduplication and hash indexing.
Changes:
- Deduplicates explicit
newand renamedinitialize. - Removes the unused
dont_rename_initializeparameter.
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 withdef self.newboth before and afterdef initialize, asserting a single::newentry and no stale#initializekey (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.
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>
tompng
force-pushed
the
move-initialize-rename-before-add
branch
from
August 30, 2026 15:39
ccede23 to
a3ef80e
Compare
tompng
requested a deployment
to
fork-preview-protection
August 30, 2026 15:40 — with
GitHub Actions
Waiting
| end | ||
|
|
||
| record_location(meth) | ||
| container.add_method(meth) |
st0012
approved these changes
Aug 30, 2026
Contributor
|
It would be awesome to add a test containing both |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
The Ruby parser renames an instance method
initializeto::newfor documentation purposes. This rename happened aftercontainer.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 atadd_methodtime the parser simply did not know yet whether the method should be renamed:(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
#initializekey and renaming it afterwards had two effects:Context#methods_hashwas left keyed by a stale name (#initializepointing at a method now called::new). This is one of the obstacles to makingContext#find_methodhash-based (see the discussion in RBS scan cache to improve overall performance #1796).Context#add_methodwas bypassed. A class documenting both::new(an explicitdef self.new, or a C-definednew) and#initializeended up with two::newentries on its page.Change
Move the rename block before
container.add_method. All information it uses (the method name,singleton, anddont_rename_initializeset 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:
::newentry:Gem::Package::TarReader,Gem::Package::TarWriter,Gem::Resolver::APISpecification,JSON::Ext::Generator::StateActiveSupport::Deprecation::DeprecatedConstantProxyAll other entries are identical. Each changed class defines both
def self.newanddef initialize(or, forJSON::Ext::Generator::State, a C-definednewand a Rubyinitialize— the exact "c and ruby" case the old comment referred to). On master these pages shownewtwice, with a duplicatedid="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_initializekeyword argument ofinternal_add_method. It has never been passed a truthy value since its introduction: one call site passes an explicitfalseand the other relies on thefalsedefault. It is unrelated toAnyMethod#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.