-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix: static this is the constructor, not an instance (#9404); class .name/toString/inspect report source identity (#9413)
#9465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| ### Fixed | ||
|
|
||
| - **Inside a `static` body, `this` is no longer treated as an instance of the | ||
| class.** `class P { m() { return 1; } static probe() { return typeof this.m; } }` | ||
| answered `"function"`; node answers `"undefined"`. Worse than the `typeof`: | ||
| `this.m()` in a static body *succeeded*, running the instance method body with | ||
| the class ref as its receiver, where node throws a `TypeError`. | ||
|
|
||
| Two independent defects produced that one symptom, and each is reachable on | ||
| its own. | ||
|
|
||
| **1. The codegen type predicates typed static `this` as an instance.** | ||
| `receiver_class_name(Expr::This)` and `static_type_of(Expr::This)` | ||
| (`crates/perry-codegen/src/type_analysis/predicates.rs`) both answered | ||
| `Named(class_stack.last())` in a static body exactly as they do in an instance | ||
| body. `class_stack` names the owning class in a static body too — that is what | ||
| `super.x` resolves against — but a static body's `this` is the class | ||
| CONSTRUCTOR: an INT32 class ref, never a heap instance. Every consumer of | ||
| those two answers was therefore entitled to prove instance facts about the | ||
| constructor object: instance field slots, shape ids, direct method dispatch. | ||
|
|
||
| `Named(C)` is not merely imprecise here, and "the constructor object of C" | ||
| would not have been a better answer: static members are INHERITED, so `this` | ||
| in a static body of `Base` is whatever subclass the call came through | ||
| (`Sub.inherited()` sees `this === Sub`, and `Sub` may override every static | ||
| member the body touches). `None` is the only sound answer, and it is what both | ||
| predicates now return under `FnCtx::in_static_member`. | ||
|
|
||
| This is what closes the alias residual #9386 documented and left open: | ||
| `static viaLocal() { const t = this; … }` reached the computed-member route | ||
| through `guarded_declared_class_get_candidate`, which reads `local_types` — | ||
| written by `refine_type_from_init` from `static_type_of`. With that predicate | ||
| honest the wrong type never enters `local_types` | ||
| (`G.viaLocal()`: `undefined|` → `object|9`). | ||
|
|
||
| **2. The runtime's constructor-side property walk read `C.prototype`.** | ||
| Declared instance methods are mirrored onto the reflective `C.prototype` | ||
| object as own data fields. `resolve_proto_chain_field` walks that object, and | ||
| the CONSTRUCTOR-side read in `js_object_get_field_by_name` (`C.foo` on a class | ||
| ref, after own statics and the static-method chain miss) called it — so every | ||
| prototype method resolved on the class object. This needs no `this` at all: | ||
| on `dcf1ec0fbc`, `class P { m(){} }` gave `typeof P.m === "function"` and | ||
| `P.m === P.prototype.m`, via the dot, computed, and `Reflect.get` forms alike. | ||
| `js_object_has_property` already had the gate (`"m" in P` was correctly | ||
| `false`), and the `is_prototype_ref` gate in the same file plugged this hole on | ||
| the direct-vtable door for #1021/NestJS — this is that door's chain-walk twin. | ||
|
|
||
| The receiver-less `resolve_proto_chain_field` has exactly one caller and it is | ||
| that static-side read, so the exclusion is applied there rather than at the | ||
| call site. It is keyed on `class_instance_has_member` — the exact "is this a | ||
| prototype method / getter / setter of the chain" predicate — and NOT on "skip | ||
| the decl-prototype entirely". A blanket skip was tried first and is wrong: it | ||
| also removes `C.constructor`, which the decl-prototype carries as an ordinary | ||
| data field. That answer is load-bearing today for a reason outside this issue: | ||
| **perry hands a PROPERTY DECORATOR the class itself where the spec hands it | ||
| `Class.prototype`**, so NestJS-style | ||
| `Reflect.defineMetadata(k, v, target.constructor)` relies on | ||
| `C.constructor === C`. Node says `C.constructor === Function`, so perry has two | ||
| divergences that cancel, and removing either alone breaks decorator metadata — | ||
| measured: `test_decorators_nest_common_canary` and | ||
| `test_decorators_legacy_property_metadata` both went pass -> parity_fail on the | ||
| blanket version. The decorator-target defect is the one worth fixing, and it is | ||
| not this issue. | ||
|
|
||
| The `class_prototype_object` step of the same walk is never skipped: for a | ||
| subclass of a class-EXPRESSION value it holds the parent CLASS OBJECT | ||
| (#1788/#6552), which is genuinely on the constructor's static chain. | ||
|
|
||
| Fixing only (1) would have left the issue's own example broken, and would have | ||
| moved one shape — `const t = this; typeof t.computedMethod` — from | ||
| accidentally-right to wrong, because it stopped taking the computed-member | ||
| route (which answered `undefined` for the wrong reason) and joined every other | ||
| instance-member read on the leaking generic path. | ||
|
|
||
| Affected files: | ||
|
|
||
| - `crates/perry-codegen/src/type_analysis/predicates.rs` — a guarded | ||
| `Expr::This if ctx.in_static_member => None` arm ahead of each existing | ||
| `Expr::This` arm. | ||
| - `crates/perry-codegen/src/type_analysis_facts.rs` — | ||
| `CodegenTypeFacts::this_type` carries the same gate. Without it the generic | ||
| HIR inference (`infer_expr_type`) re-derived `Named(C)` for every expression | ||
| that merely *contains* `this`, routing around `static_type_of`'s refusal. | ||
| - `crates/perry-runtime/src/object/class_registry/prototype_objects.rs` — | ||
| `resolve_proto_chain_field_inner` takes `skip_decl_prototype`, set for the | ||
| constructor-side form only. | ||
|
|
||
| No fast path is lost for the operations a static body actually performs. | ||
| Static field reads through `this` (`this.sf`), static method calls through | ||
| `this` (`this.other()`), `this.prototype` and `this.name` were already on the | ||
| generic class-ref dispatch: `class_field_global_index` never matched a static | ||
| field, and `resolve_static_dispatch_cls` has no `Expr::This` arm — | ||
| deliberately, because static inheritance means `this` in a static body cannot | ||
| be resolved to the declaring class at compile time. | ||
|
|
||
| Validation: `test-files/test_static_this_is_not_an_instance_9404.ts`, | ||
| byte-compared against `node --experimental-strip-types`, covering a static | ||
| method, a static block, a static getter, `this === C`, static-to-static | ||
| dispatch through `this`, the same on a subclass where `this` is the *sub*class, | ||
| the `const t = this` alias (plain and computed member), an instance-side | ||
| control, and a static method whose name collides with a String method. | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| ### Fixed | ||
|
|
||
| - **A class's compiler-internal identity no longer escapes into `.name`, | ||
| `Function.prototype.toString`, or `util.inspect`.** Three separate leaks, all | ||
| of the same shape: a registration key or a class id that only the compiler | ||
| should ever see, handed to the program as a user-visible string. | ||
|
|
||
| 1. **`.name` reported the disambiguation key.** Two `class Made {}` in sibling | ||
| function bodies are distinct classes, so the second registers under a | ||
| uniquified key (`Made$0`) to keep the name-keyed dedup from aliasing the two | ||
| bodies onto one ClassId — see `maybe_rename_colliding_class`. That key | ||
| reached `js_register_class_name`, so `Made.name` and | ||
| `new Made().constructor.name` answered `"Made$0"`. | ||
|
|
||
| 2. **A class expression constructed in place lost its name entirely.** | ||
| `new (class extends Error {})("m").constructor.name` answered | ||
| `"__anon_class_8"` (node: `""`), and even a *named* one — | ||
| `new (class Q {})().constructor.name` — answered `"__anon_class_6"` instead | ||
| of `"Q"`. `lower_new_non_ident` lowers straight to a `New` on a synthetic | ||
| key and never recorded the spec name, while its sibling | ||
| `lower_expr/arm_class.rs` had recorded exactly that override | ||
| (`display_override`) since #5592. | ||
|
|
||
| Both are fixed by populating the existing `Module::class_display_names` | ||
| override that `codegen/string_pool.rs` already prefers over the | ||
| registration key. No new mechanism. | ||
|
|
||
| 3. **`console.log(C)` and `util.inspect(C)` printed the raw class id.** | ||
| `util.inspect(Klass)` answered `6`. A class ref shares the INT32 encoding | ||
| with a tagged small integer, and the console formatter's `is_int32()` arm | ||
| printed the payload. It now renders node's form — `[class Klass]`, | ||
| `[class Sub extends Named]`, `[class (anonymous)]`. | ||
|
|
||
| - **`String(C)` / `C.toString()` now return the class's source text.** They | ||
| returned `function Klass() { [native code] }`, which is not what node produces | ||
| for a class and not something a caller can parse. Perry already retained | ||
| function source (`Module::closure_source_text`, #4101) — the same | ||
| span-slice-at-lowering mechanism, keyed by ClassId, was simply never applied to | ||
| classes, which are the one callable kind that is not a `ClosureHeader` and so | ||
| cannot recover source from the closure registry. | ||
|
|
||
| `Module::class_source_text` is populated at lowering by slicing the module | ||
| source against `ast::Class::span` (SWC anchors it at the `class` keyword and | ||
| closes it at the body's `}`, so the slice is exactly the class's | ||
| `[[SourceText]]`), emitted by codegen as `js_register_class_source`, and read | ||
| by all three class-ref `toString` sites. A class with no registered source (a | ||
| builtin, or one perry synthesized) still gets the `[native code]` form, which | ||
| Test262's `assertToStringOrNativeFunction` accepts. Monomorphized | ||
| specializations inherit the origin's source, for the same reason #7632 makes | ||
| them inherit its name. | ||
|
|
||
| Affected files: | ||
|
|
||
| - `crates/perry-hir/src/lower_decl/class_decl.rs` — `capture_class_source` | ||
| (the class sibling of `capture_function_source`), plus the display-name | ||
| override for a renamed duplicate. | ||
| - `crates/perry-hir/src/lower/expr_new/non_ident.rs` — record the spec `.name` | ||
| of an in-place-constructed class expression. | ||
| - `crates/perry-hir/src/ir/module.rs`, | ||
| `crates/perry-hir/src/lower/{context,lowering_context,lower_module_fn}.rs`, | ||
| `crates/perry-hir/src/stable_hash/module.rs`, | ||
| `crates/perry-hir/src/monomorph/driver.rs` — the `class_source_text` map and | ||
| its flush; it participates in the stable hash because it drives codegen. | ||
| - `crates/perry-codegen/src/codegen/{string_pool,artifacts}.rs`, | ||
| `crates/perry-codegen/src/runtime_decls/strings.rs` — emit | ||
| `js_register_class_source`. | ||
| - `crates/perry-runtime/src/object/class_registry/class_meta.rs` — the source | ||
| side table, `class_ref_to_string`, `class_ref_inspect_label`. | ||
| - `crates/perry-runtime/src/value/to_string.rs`, | ||
| `crates/perry-runtime/src/object/native_call_method/common_methods.rs`, | ||
| `crates/perry-runtime/src/object/global_this/array_error.rs`, | ||
| `crates/perry-runtime/src/builtins/formatting.rs` — the four read sites. | ||
|
|
||
| Not addressed, and still divergent: `String(C.prototype.m)` for a class | ||
| METHOD returns `function () { [native code] }` (node returns the method's | ||
| source). Class methods compile to `perry_method_*` symbols rather than | ||
| closures with a registered source, so this needs the method-side equivalent of | ||
| the closure source registry, not another read of this one. Object-literal | ||
| methods already work and are kept in the fixture as the control. | ||
|
|
||
| Validation: `test-files/test_class_name_and_source_9413.ts` (ESM) and | ||
| `test-files/test_class_name_cjs_9413.cts` (CommonJS, for the | ||
| `module.exports = class {}` spellings that get no NamedEvaluation), both | ||
| byte-compared against `node --experimental-strip-types`. |
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
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
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
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
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
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the
Fixedscope for thethis.m()call form.The fixture at
test-files/test_static_this_is_not_an_instance_9404.ts, Lines [12-21], states thatthis.m()still resolves the instance vtable and returns"B.m"instead of throwingTypeError. This section presents the static-thisissue as fixed without identifying that residual behavior. If constructor-side call dispatch is out of scope, limit this note to property reads and name the call form as a remaining divergence.Based on learnings: Perry changelog fragments must describe the final shipped behavior and must not claim a residual behavior is fixed.
🤖 Prompt for AI Agents
Source: Learnings