[codex] fix Three-style native class semantics#5225
Open
andrewtdiz wants to merge 6 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds end-to-end cross-module class accessor support: a new ChangesCross-module accessor pipeline
Cluster module EventEmitter-style native dispatch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
andrewtdiz
force-pushed
the
codex/fix-three-native-class-semantics-20260615
branch
from
609ec79 to
7ad6506
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/perry/src/commands/compile/types.rs`:
- Around line 623-627: The cross_module_class_accessors field uses Vec<String>
which loses the distinction between getter-only, setter-only, and getter+setter
accessors, causing incorrect semantics downstream in the stub/dispatch path.
Replace the Vec<String> with a data structure that explicitly tracks getter and
setter capabilities separately (such as a struct with separate getter_names and
setter_names fields or similar). Update all code that populates and consumes
this HashMap to store and use the getter/setter information from separate sets
rather than a single flattened string vector, ensuring each accessor kind is
preserved and propagated independently throughout the HIR lowering process.
🪄 Autofix (Beta)
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: 16ad032c-f8ca-4ec2-987f-cb3e03c7c906
📒 Files selected for processing (12)
crates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/type_analysis_class_fields.rscrates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/lower_module_fn.rscrates/perry-hir/src/lower_decl/class_decl.rscrates/perry/src/commands/compile/bootstrap.rscrates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/types.rstest-files/_helpers/three_like_descriptor_bases.tstest-files/test_three_like_native_class_descriptors.tstests/test_imported_inherited_accessor_datatexture.shtests/test_xmod_imported_multilevel_ctor_state.sh
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-hir/src/lower_decl/class_decl.rs (1)
1003-1034:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep static accessors out of instance accessor metadata.
Both paths can register
static get/set xas an instance accessor name. That makes constructor field inference skipthis.x = ...and makesC.prototype.x = ...look accessor-backed even when only the constructor has a static accessor. Filter to runtime instance accessors before inserting.Suggested direction
- ast::ClassMember::Method(m) - if matches!(m.kind, ast::MethodKind::Getter | ast::MethodKind::Setter) => + ast::ClassMember::Method(m) + if !m.is_static + && m.function.body.is_some() + && matches!(m.kind, ast::MethodKind::Getter | ast::MethodKind::Setter) => {- ast::ClassMember::PrivateMethod(m) - if matches!(m.kind, ast::MethodKind::Getter | ast::MethodKind::Setter) => + ast::ClassMember::PrivateMethod(m) + if !m.is_static + && m.function.body.is_some() + && matches!(m.kind, ast::MethodKind::Getter | ast::MethodKind::Setter) => {For the class-expression path, avoid deriving the registry from the unqualified
getters/settersvectors unless those vectors can distinguish static from instance accessors; otherwise collect fromclass.bodywith the same!is_static && body.is_some()filter.Also applies to: 1678-1687
🤖 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/lower_decl/class_decl.rs` around lines 1003 - 1034, Both the Method and PrivateMethod blocks are registering static getters/setters as instance accessors without filtering. Add a check to exclude static accessors before inserting into the accessor_names registry. For the Method block handling ast::ClassMember::Method, verify that the method is not static (check m.is_static or equivalent property) before calling insert_getter or insert_setter. Apply the same filtering logic at the second location mentioned (lines 1678-1687) which handles class expressions. Only instance accessors (where is_static is false) should be registered in the accessor_names to prevent constructor field inference issues and incorrect prototype assignment detection.
🤖 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 `@crates/perry-hir/src/lower/expr_assign.rs`:
- Around line 646-648: The accessor guard using lookup_class_accessor_names only
protects against mis-lowering accessor writes for direct C.prototype.x
assignments, but the aliased prototype path (where a variable holds a reference
to C.prototype before assignment) still incorrectly returns ProtoOwner::Class.
Apply the same accessor name checking logic to the code path that handles
aliased prototype assignments to ensure that when an accessor name is detected,
both direct and aliased assignments are treated consistently and not mis-lowered
as prototype monkey-patches.
---
Outside diff comments:
In `@crates/perry-hir/src/lower_decl/class_decl.rs`:
- Around line 1003-1034: Both the Method and PrivateMethod blocks are
registering static getters/setters as instance accessors without filtering. Add
a check to exclude static accessors before inserting into the accessor_names
registry. For the Method block handling ast::ClassMember::Method, verify that
the method is not static (check m.is_static or equivalent property) before
calling insert_getter or insert_setter. Apply the same filtering logic at the
second location mentioned (lines 1678-1687) which handles class expressions.
Only instance accessors (where is_static is false) should be registered in the
accessor_names to prevent constructor field inference issues and incorrect
prototype assignment detection.
🪄 Autofix (Beta)
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: 03460be8-5a48-4a6f-96cc-1144c9fc5423
📒 Files selected for processing (10)
crates/perry-hir/src/class_accessors.rscrates/perry-hir/src/lib.rscrates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/expr_assign.rscrates/perry-hir/src/lower/lower_module_fn.rscrates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower_decl/class_decl.rscrates/perry/src/commands/compile/bootstrap.rscrates/perry/src/commands/compile/types.rstests/test_imported_inherited_accessor_datatexture.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_imported_inherited_accessor_datatexture.sh
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 `@crates/perry-hir/src/lower/expr_call/native_module.rs`:
- Around line 1498-1504: In the cluster default event emitter method dispatch
block where is_cluster_default_event_emitter_method returns true, the code
constructs an Expr::NativeMethodCall that uses the non-normalized module_name
for the module field. However, the dispatch table in the codegen layer expects
the normalized module name ("cluster" instead of "node:cluster"). Change line
1503 in the Expr::NativeMethodCall construction from module:
module_name.to_string() to module: normalized_module.to_string() to ensure the
emitted module name matches the dispatch table keys, matching the pattern used
in similar dispatch logic elsewhere in the file.
🪄 Autofix (Beta)
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: 214ae6df-ecba-4ff9-a65c-05d722e6fbf2
📒 Files selected for processing (4)
crates/perry-codegen-arkts/src/tests.rscrates/perry-codegen-arkts/tests/phase2_full_app_smoke.rscrates/perry-codegen/src/lower_call/native_table/node_misc.rscrates/perry-hir/src/lower/expr_call/native_module.rs
Comment on lines
+1498
to
+1504
| if normalized_module == "cluster" | ||
| && matches!(imported_method, Some("default")) | ||
| && is_cluster_default_event_emitter_method(&method_name) | ||
| { | ||
| return Ok(Ok(Expr::NativeMethodCall { | ||
| module: module_name.to_string(), | ||
| class_name: None, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lookup_native_module definition(s) =="
rg -n -C3 '\bfn\s+lookup_native_module\b|\blookup_native_module\s*\(' crates/perry-hir/src
echo
echo "== cluster/native module registration sites =="
rg -n -C3 '"cluster"|"node:cluster"|default' crates/perry-hir/src crates/perry/src
echo
echo "== cluster rows in native dispatch table =="
rg -n -C2 'module:\s*"cluster"|module:\s*"node:cluster"' crates/perry-codegen/src/lower_call/native_table
echo
echo "== native_module_lookup normalization rules =="
rg -n -C6 'fn\s+native_module_lookup|let\s+normalized\s*=\s*match\s+module' crates/perry-codegen/src/lower_call/native_module_dispatch.rsRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Lines 1498-1510 of native_module.rs ==="
sed -n '1498,1510p' crates/perry-hir/src/lower/expr_call/native_module.rs
echo
echo "=== Lines 1490-1520 context ==="
sed -n '1490,1520p' crates/perry-hir/src/lower/expr_call/native_module.rs
echo
echo "=== Cluster table rows in node_misc.rs ==="
rg -n "cluster" crates/perry-codegen/src/lower_call/native_table/node_misc.rs | head -20
echo
echo "=== Check if normalized_module is used in the returned Expr ==="
sed -n '1498,1520p' crates/perry-hir/src/lower/expr_call/native_module.rs | grep -E "normalized_module|module_name"Repository: PerryTS/perry
Length of output: 3611
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for cluster module registration/import ==="
rg -n -B3 -A3 'cluster.*node:cluster|node:cluster.*cluster' crates/perry-hir/src crates/perry/src | head -50
echo
echo "=== Look for cluster in tests or module setup ==="
rg -n 'lookup_native_module.*cluster|cluster.*lookup_native_module' crates/perry-hir/src/lower/tests.rs
echo
echo "=== Where are native modules registered (builtin_modules) ==="
fd -type f -name '*.rs' crates/perry-hir/src/context | head -5
echo
echo "=== Search for where cluster/node:cluster gets registered ==="
rg -n 'cluster|node_module' crates/perry-hir/src/context.rs | grep -i cluster | head -20Repository: PerryTS/perry
Length of output: 910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find context.rs and see native module registration ==="
rg -n 'fn.*lookup_native_module|register.*native.*module' crates/perry-hir/src/context.rs | head -10
echo
echo "=== Look at context.rs to find builtin_modules or native_modules ==="
rg -n 'builtin_modules|native_modules|"cluster"' crates/perry-hir/src/context.rs | head -30
echo
echo "=== Check what module_name values are passed to lookup_native_module in the call site ==="
sed -n '1450,1500p' crates/perry-hir/src/lower/expr_call/native_module.rs | grep -B20 'lookup_native_module' | tail -30Repository: PerryTS/perry
Length of output: 206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find where LoweringContext is defined ==="
fd -type f -name '*.rs' crates/perry-hir/src | xargs grep -l 'struct LoweringContext'
echo
echo "=== Look for lookup_native_module implementation ==="
rg -n 'fn lookup_native_module' crates/perry-hir/src -A 10 | head -40
echo
echo "=== Check the full context around lines 1470-1480 in native_module.rs to see where module_name comes from ==="
sed -n '1470,1490p' crates/perry-hir/src/lower/expr_call/native_module.rsRepository: PerryTS/perry
Length of output: 294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find where LoweringContext is defined ==="
rg -n 'struct LoweringContext' crates/perry-hir/src | head -5
echo
echo "=== Look for lookup_native_module implementation ==="
rg -n 'fn lookup_native_module' crates/perry-hir/src -A 15 | head -60
echo
echo "=== Check full context around lines 1470-1495 in native_module.rs to see where module_name comes from ==="
sed -n '1470,1495p' crates/perry-hir/src/lower/expr_call/native_module.rsRepository: PerryTS/perry
Length of output: 3100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check native_modules field definition and registration ==="
rg -n 'native_modules.*Vec|register_native_module|native_modules.push' crates/perry-hir/src/lower/context.rs | head -30
echo
echo "=== Look for where native modules are initially populated ==="
rg -n 'native_modules.*=.*vec!|insert.*native.*module' crates/perry-hir/src/lower/context.rs | head -20
echo
echo "=== Search for where cluster specifically gets registered as native module ==="
rg -n 'cluster' crates/perry-hir/src/lower/context.rs | head -20
echo
echo "=== Check context.rs around where native_modules is populated ==="
sed -n '1,150p' crates/perry-hir/src/lower/context.rs | tail -80Repository: PerryTS/perry
Length of output: 273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Look at register_native_module function ==="
sed -n '1037,1070p' crates/perry-hir/src/lower/context.rs
echo
echo "=== Search for all register_native_module calls to see the pattern ==="
rg -n 'register_native_module' crates/perry-hir/src --context 2 | head -100
echo
echo "=== Specifically look for cluster registration ==="
rg -n 'register_native_module.*cluster|cluster.*register_native_module' crates/perry-hir/src -B2 -A2Repository: PerryTS/perry
Length of output: 7576
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Look at module_decl.rs around line 240 to see how native_module is determined ==="
sed -n '200,260p' crates/perry-hir/src/lower/module_decl.rs
echo
echo "=== Look for resolve_import_source or similar that might normalize module names ==="
rg -n 'resolve.*import|import.*source' crates/perry-hir/src/lower/module_decl.rs | head -20Repository: PerryTS/perry
Length of output: 4585
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify: does source from import statement remain as 'node:cluster' or get stripped? ==="
sed -n '230,245p' crates/perry-hir/src/lower/module_decl.rs
echo
echo "=== Check what 'source' variable is at the registration point (line 242) ==="
sed -n '160,200p' crates/perry-hir/src/lower/module_decl.rs | grep -A30 'fn.*default_import'Repository: PerryTS/perry
Length of output: 1384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find where source variable comes from in default_import branch ==="
sed -n '100,240p' crates/perry-hir/src/lower/module_decl.rs | tail -80 | head -50
echo
echo "=== Confirm: is source the raw import string 'node:cluster' or normalized? ==="
rg -n 'ImportDecl|source.*=' crates/perry-hir/src/lower/module_decl.rs | grep -B5 -A5 'default_import' | head -30Repository: PerryTS/perry
Length of output: 3465
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Look at the function handling default imports to see where source comes from ==="
sed -n '350,450p' crates/perry-hir/src/lower/module_decl.rs
echo
echo "=== Check is_native_module or similar to see if source gets normalized anywhere ==="
rg -n 'is_native_module|is_node_core_module' crates/perry-hir/src/lower/module_decl.rs -B3 -A3 | head -40Repository: PerryTS/perry
Length of output: 7953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find is_native_module definition ==="
rg -n 'fn is_native_module' crates/perry-hir/src -A 10
echo
echo "=== Search for is_native_module in context.rs or helpers ==="
rg -n 'is_native_module' crates/perry-hir/src/lower/context.rs -B2 -A10 | head -30
echo
echo "=== Check where the default import registration happens and what 'source' value is ==="
sed -n '330,380p' crates/perry-hir/src/lower/module_decl.rsRepository: PerryTS/perry
Length of output: 6298
Fix module name canonicalization in the cluster default method dispatch.
Line 1503 emits module: module_name.to_string(), but the dispatch table in crates/perry-codegen/src/lower_call/native_table/node_misc.rs is keyed as module: "cluster".
When the user writes import cluster from "node:cluster", lookup_native_module returns ("node:cluster", None), so module_name = "node:cluster". The code then checks normalized_module == "cluster" (which correctly strips the prefix), but emits module: "node:cluster" instead of the normalized form, causing dispatch table lookup failures.
Change line 1503 from:
module: module_name.to_string(),to:
module: normalized_module.to_string(),This matches the pattern used elsewhere in the file (e.g., lines 1519–1525) and ensures the emitted module name aligns with the codegen dispatch table keys.
🤖 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/lower/expr_call/native_module.rs` around lines 1498 -
1504, In the cluster default event emitter method dispatch block where
is_cluster_default_event_emitter_method returns true, the code constructs an
Expr::NativeMethodCall that uses the non-normalized module_name for the module
field. However, the dispatch table in the codegen layer expects the normalized
module name ("cluster" instead of "node:cluster"). Change line 1503 in the
Expr::NativeMethodCall construction from module: module_name.to_string() to
module: normalized_module.to_string() to ensure the emitted module name matches
the dispatch table keys, matching the pattern used in similar dispatch logic
elsewhere in the file.
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
1 participant
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.
Summary
Fix the remaining Three-style native class semantics on top of current
main. The source-visible/symlinked relative import resolution piece is already present in the current base via #5210, so this PR is rebased over it and avoids re-adding duplicate resolver tests.This PR carries:
declareclass props no longer become runtime instance fields or static slots.Root Causes
this.image = ...could bypass inherited setters.declareclass props were treated as runtime slots, shadowing constructor-created state or defaults./var; that resolver issue is now in the base branch via [codex] Fix symlinked relative import resolution #5210.Impact
PerspectiveCamerainheritedObject3Dstate when combined with the current base resolver fix.DataTextureimage/default/override/version behavior.Caveat
The remaining downstream
DataTexture.needsUpdate.sourceFlagsmoke check is a setter-onlySource.needsUpdateoracle, not a Perry runtime failure.Validation
cargo fmt --checkgit diff --checkcargo build -p perrycargo test -p perry lexical_path_testscargo test -p perry symlinked_entry_resolves_relative_imports_from_lexical_pathcargo test -p perry js_import_scan_follows_bare_dot_and_dotdot_specifierscargo build --release -p perry-runtimePERRY_BIN=/Users/andrew/.codex/worktrees/05e0/perry/target/debug/perry tests/test_symlinked_entry_imported_class.shPERRY_BIN=/Users/andrew/.codex/worktrees/05e0/perry/target/debug/perry tests/test_xmod_imported_multilevel_ctor_state.shPERRY_BIN=/Users/andrew/.codex/worktrees/05e0/perry/target/debug/perry tests/test_imported_inherited_accessor_datatexture.shPERRY_CLI=/Users/andrew/.codex/worktrees/05e0/perry/target/debug/perry PERRY_RUNTIME_DIR=/Users/andrew/.codex/worktrees/05e0/perry/target/release npm run perry:smokefrom/Users/andrew/Documents/perry-threeDownstream acceptance: imported-super, two-arg Math, public Three prototype, and public Three camera passed; public Three texture is 24/25 with only
DataTexture.needsUpdate.sourceFlagremaining.Summary by CodeRabbit
declarestatic class properties aren’t treated as runtime statics.clusterEventEmitter-style APIs (e.g.,on,addListener,once,emit,removeListener,removeAllListeners).