refactor(codegen): split codegen/artifacts.rs under the file-size cap - #11093
proggeramlug wants to merge 1 commit into
Conversation
9a457ea to
bc019bb
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughCodegen now delegates class artifact generation and exported value wrapper generation to separate modules. The class helper emits methods, constructors, accessors, and static members. The wrapper helper emits import getters, function wrappers, and export aliases. ChangesArtifact emission
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to This change reorganizes code generation into smaller modules without changing what gets generated. Class methods, constructors, accessors, and exported function wrappers are emitted in the same order with the same logic as before. No merge-blocking risk was found. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
`crates/perry-codegen/src/codegen/artifacts.rs` sat at 1999 lines against the hard 2000-line cap enforced by `scripts/check_file_size.sh` (a required `lint` step), so any PR that added even two lines to it was blocked outright — #11087 concretely. Pure move, no behavior change: - `class_artifacts.rs` (new, 572 lines) — the per-class walk: instance methods and their typed/indexed/proven-`this` clones, computed members, accessors, the standalone constructor, and statics. - `export_value_wrappers.rs` (new, 530 lines) — the exported function-value surface: live getters for re-exported native named imports, the `__perry_wrap_*` closure-ABI wrappers, and the cross-module raw/renamed name aliases. - `artifacts.rs` 1999 -> 1044 lines. Both moved blocks are byte-identical to the originals (verified by diffing the extracted line ranges against `git show HEAD:`). Both siblings follow the existing `codegen/` convention (`ordinary_method_artifacts.rs`, `indexed_method_artifacts.rs`): a `pub(super) XxxCtx<'a>` struct of borrowed inputs plus a `pub(super) fn` that destructures it, declared as a plain `mod` in `codegen/mod.rs` and imported with explicit named `use`. Two edits are not moves: - `#[derive(Clone, Copy)]` on `OptsView` in `artifact_context.rs`, so the class phase can take the same by-value view without the artifact tail losing its own; every field was already a shared borrow or a scalar. - `scripts/shape_descriptor_census_baseline.json` repoints its `object_header_size_bytes(target_triple)` callsite from `artifacts.rs` to `class_artifacts.rs`. That census pins callsites by file path, so a pure move reads to it as one site removed and one added.
bc019bb to
fcf8c33
Compare
|
Landed on Cherry-picked from this PR's head Nothing needed from you. Thanks. |
crates/perry-codegen/src/codegen/artifacts.rssat at 1999 lines againstthe hard 2000-line cap enforced by
scripts/check_file_size.sh(a requiredlintstep). One line of headroom means any PR that adds even two lines to itis blocked outright — #11087 concretely. This splits it so it stops being a
tripwire.
Before / after
crates/perry-codegen/src/codegen/artifacts.rscrates/perry-codegen/src/codegen/class_artifacts.rscrates/perry-codegen/src/codegen/export_value_wrappers.rsWhat moved
artifacts.rswas a single 1958-lineemit_module_artifactsfunction, so thesplit is by phase, cut at the existing
progress.checkpoint(...)boundaries.class_artifacts.rs— the per-class walk (lines 275–760): instancemethods and their typed-f64/i32/i1/string, indexed, and proven-
thisclones;computed members; getters/setters (instance and static); the standalone
cross-module constructor including the synthesized
super(...args)forwardingctor; and static methods.
export_value_wrappers.rs— the exported function-value surface (lines764–1254): live ESM getters for re-exported native named imports (the block
train 259 just touched), the
__perry_wrap_*closure-ABI wrapper pertop-level user function, the exported-alias wrappers, the uuid (v4) fails to compile:
defaultexport name not wired up under compilePackages #837renamed-export wrappers, and the zod fails to compile under perry.compilePackages: cross-module class re-exports unresolved #836 sanitize-mismatch raw aliases.
Both siblings follow the convention already in
codegen/(
ordinary_method_artifacts.rs,indexed_method_artifacts.rs): apub(super) XxxCtx<'a>struct of borrowed inputs plus apub(super) fnthatdestructures it, declared as a plain
modincodegen/mod.rsand importedwith explicit named
use(no globs).Purity
Both moved blocks are byte-identical to the originals — verified by diffing
the extracted line ranges out of
git show HEAD:…/artifacts.rsagainst thecorresponding ranges of the two new files (
G1 IDENTICAL,G2 IDENTICAL).Comments,
?/with_contextchains and thec.cross_modulepartial-move readall came along verbatim; the new functions take their ctx as
cprecisely sothat read stays unchanged.
Two edits are not moves:
#[derive(Clone, Copy)]onOptsViewinartifact_context.rs, so the classphase can take the same by-value view without the artifact tail losing its
own. Every field of
OptsViewwas already a shared borrow or a scalar, sothis adds no semantics — same "minimum glue for the split" category as the
visibility bumps in refactor(runtime): split value/to_string.rs under the file-size cap #11088.
scripts/shape_descriptor_census_baseline.jsonrepoints one callsite keyfrom
artifacts.rstoclass_artifacts.rs. That census pins callsites byfile path, so a pure move reads to it as one site removed and one added.
The multiset is otherwise untouched, and its summary still reports
codegen_object_header_size_sites: 43.No renames, no version bump.
Verification
Each
cargocommand was re-run aftertouching the moved files, so theChecking …line proves the crate was actually re-analysed rather than servedfrom cache.
cargo check -p perry --binsis thewarningsgate's own command and adifferent feature set from
-p perry-codegen; it is what caught #11088's bug(an import left unconditional while its sole remaining caller sat behind a
#[cfg(feature = ...)]). Nothing of that shape came up here — neitherextracted block has a
cfg-gated call site.Path-keyed gates
addr_class_ratchet_baseline.txt,addr_class_allowlist.txt,raw_handle_debt_files.txtandgc_runtime_root_holders.jsonhold nocodegen/artifacts.rsentry, so there was no ceiling to relocate and no# moved-from:declaration to write — which is why the merge-base invocationof
raw_handle_debt.pyreportsnone raisedrather than a declaredrelocation. Only the census baseline was path-keyed to this file.
Beyond the five script baselines, the crate also carries source-level
path allowlists that a file split can disturb:
pshape_symbol_reachabilityand
spec_abi_symbol_reachabilityeach hold a fixed-size[&str; N]array ofsrc-relative paths. Neither is tripped here (neither moved block contains$pshapeor$spec_), and the whole class is covered by:Finally, all 35 static (no-build)
lintgates fromtest.yml'slintjobwere run against this tree and pass, including
workspace_architecture.py,check_gc_header_constants.py,gc_store_site_inventory.py,string_payload_access_inventory.py,check_test_registration.py,unrooted_local_shape.py --check, and both merge-base ratchets(
unrooted_local_shape.py/global_sink_isolation.py --asserted-no-raise-vs).Summary by CodeRabbit