fix(monomorph): a specialized generic class reports the generic's name, not Gen$num (#7632) - #7756
Conversation
a1900f4 to
bff9987
Compare
📝 WalkthroughWalkthroughGeneric class monomorphization now registers the original class display name for specialized classes. Unit and integration tests cover constructor names, nested and inherited generics, error behavior, ChangesGeneric class display names
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-hir/src/monomorph/tests.rs (1)
984-1023: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for an origin display-name override.
This test only exercises the
original.namefallback becausemodule.class_display_nameshas no entry for class ID1.Add a second case that registers an origin override before
monomorphize_module. Assert that the specialization receives that override. This validates the required behavior for already-uniquified classes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-hir/src/monomorph/tests.rs` around lines 984 - 1023, Add a second test case alongside the existing monomorphization test that inserts a display-name override for the generic class ID before calling monomorphize_module. Assert that the generated “Gen$num” specialization’s module.class_display_names entry uses the registered override instead of falling back to the original class name, while preserving the existing origin and specialization checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry-hir/src/monomorph/tests.rs`:
- Around line 984-1023: Add a second test case alongside the existing
monomorphization test that inserts a display-name override for the generic class
ID before calling monomorphize_module. Assert that the generated “Gen$num”
specialization’s module.class_display_names entry uses the registered override
instead of falling back to the original class name, while preserving the
existing origin and specialization checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fa6efc5-3fb0-48ed-9f7b-49930b388239
📒 Files selected for processing (4)
changelog.d/7756-generic-class-display-name.mdcrates/perry-hir/src/monomorph/driver.rscrates/perry-hir/src/monomorph/tests.rstest-files/test_gap_generic_class_constructor_name_7632.ts
bff9987 to
4dfe576
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Line 318: Revert the release-version metadata changes: restore the previous
[workspace.package].version in Cargo.toml at lines 318-318 and restore the
previous Current Version value in CLAUDE.md at lines 11-11; retain only the
changelog fragment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 732aaae2-91d9-4b2c-9115-58688eeae084
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
CLAUDE.mdCargo.toml
|
|
||
| [workspace.package] | ||
| version = "0.5.1440" | ||
| version = "0.5.1441" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep release-version metadata out of feature PRs.
This PR should add the changelog fragment only. The maintainer should apply the coordinated version update during release.
Cargo.toml#L318-L318: Revert the[workspace.package].versionbump.CLAUDE.md#L11-L11: Revert theCurrent Versionupdate.
📍 Affects 2 files
Cargo.toml#L318-L318(this comment)CLAUDE.md#L11-L11
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` at line 318, Revert the release-version metadata changes: restore
the previous [workspace.package].version in Cargo.toml at lines 318-318 and
restore the previous Current Version value in CLAUDE.md at lines 11-11; retain
only the changelog fragment.
Sources: Coding guidelines, Learnings
Merging as v0.5.1441A/B'd against node on my own host: The name is fixed; everything else — Reusing Verified the unit test has a subject: commenting out The disclosure is the part I want to keep
That is worth having said out loud, and it is the right call anyway: TypeScript erases type arguments, so The observation that The checklist from the issue was measured rather than assumed, including the asymmetry that made this easy to miss: |
Fixes #7632.
The fix
Monomorphization emits a second class for
new Gen<number>()(Gen$num,monomorph::mangle::generate_specialized_name) with its own class id, and the instance is stamped with that id — so every id-keyed user-visible surface reported the mangled name. TypeScript erases type arguments; the mangling is Perry's business.monomorph/driver.rsnow registers the ORIGIN's name against the specialization's class id inModule::class_display_names— the mechanismcodegen/string_pool.rsalready prefers over the registration key when emitting named-class constants (added for #5592's uniquified class-expression bindings). It reads the origin's own display name rather than itsname, so a specialization of an already-uniquified class reports the JS name and not the internal key. Two lines of registration plus a deferred insert, because the collection loop holdsmoduleimmutably.(new Gen<number>()).constructor.nameGenGen$numGen(new GenNoExtends<number>()).constructor.nameGenNoExtendsGenNoExtends$numGenNoExtends(new Gen()).constructor.name(no type args)GenGenGenThe checklist in the issue, measured
The issue asked what else to check in the same pass. All of these were already correct and stay correct:
Gen.nameread off the constructor binding — correct before and after. (Worth noting the asymmetry that made this bug easy to miss:Gen.namewasGenwhilea.constructor.namewasGen$num.)Object.prototype.toString.call(new Gen<number>())→[object Object].class MyErr<T> extends Error {}→e.name === "Error",String(e) === "Error: boom",e instanceof Error.instanceofa Map/Set SUBCLASS is false (m instanceof MyMap); only the native base edge survives #7575instanceofhalf keeps working, including through a nestedclass Wrap<T> extends Gen<T> {}.On the issue's last checklist item — whether a diagnostic keyed on the mangled name becomes ambiguous: the HIR/codegen keys are unchanged. Only
class_display_namesis written, and it is consulted solely when emitting the JS-visible name constant.--print-hir/--tracestill showGen$num.What this does NOT fix, and why I'm flagging it
Two specializations of one generic remain distinct constructor objects:
That is the monomorphization model, not the name registry, so it is out of scope here — but it is worth stating plainly that after this change the two report the same NAME while remaining
!==, which arguably makes the identity divergence harder to notice than it was. Filing it separately.(Curiously
Object.getPrototypeOf(a) === Gen.prototypeis alreadytrue, so the prototype and constructor edges disagree with each other today. That is part of the same separate question.)Validation
test-files/test_gap_generic_class_constructor_name_7632.ts— byte-identical to node 26.5.1. Covers nested generics, two specializations of one generic, the no-type-args case, error subclasses,Object.prototype.toString, and theinstanceofa Map/Set SUBCLASS is false (m instanceof MyMap); only the native base edge survives #7575instanceofhalf.perry-hirunit testa_specialized_class_reports_the_generics_display_name. Unit as well as gap on purpose: the gap suite is tag-gated, so a regression there sits red for days (CI: PR cargo-test never executes crates/perry integration suites; main-push full run starves under merge trains (documented near-miss) #5960). Verified to fail when the registration is removed.cargo test -p perry-hir --lib: 290 passed.cargo test -p perry-codegen --lib: 822 passed.test_gap_2159_defineproperty_class_prototype, which is a listedparity_failingap_snapshot.json(standing gap, issue 2159). A full sweep was not run: this host was at 11 GB free at one point today and a disk-full mid-sweep is its own failure mode, so I scoped the run to the tests that could plausibly be affected and am saying so rather than implying broader coverage.No version bump (maintainer bumps at merge).
Summary by CodeRabbit
Bug Fixes
instanceofbehavior.Tests