Skip to content

fix(compile,codegen): #5922/#5924/#5927 — namespace-reexport member resolution collisions#5923

Merged
proggeramlug merged 5 commits into
PerryTS:mainfrom
proggeramlug:fix/namespace-reexport-member-prefix-collision
Jul 4, 2026
Merged

fix(compile,codegen): #5922/#5924/#5927 — namespace-reexport member resolution collisions#5923
proggeramlug merged 5 commits into
PerryTS:mainfrom
proggeramlug:fix/namespace-reexport-member-prefix-collision

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Closes #5922, #5924, #5927. Partially addresses #5928.

Four related fixes found via a real-world sst/opencode source compile in one continuous investigation (companion effort to #5918/#5919).

#5922 — namespace-reexport member prefix collision

A NAMED import of a value the source module re-exports as a NAMESPACE (import { Context } from "effect" where effect's barrel does export * as Context from "./Context.js") registered every member of the target namespace into the flat import_function_prefixes map, keyed only by the member's bare name. When two namespace-reexport targets imported into the same file (e.g. Context and Option, both reached via effect's barrel) export a member with the same bare name, the second registration silently overwrote the first.

Fixed by adding namespace_member_prefixes (mirrors #680's existing per-namespace disambiguation map), consulted by expr/static_method.rs, lower_call/namespace_call.rs, and expr/property_get.rs before falling back to the flat map.

#5924 — namespace-reexport origin-name collision

Fixing #5922 alone wasn't enough — import_function_origin_names (issue #678's symbol-suffix-override map for re-export renames) has the same flat, bare-name-keyed structure. import { Effect, Layer, Context, Schema, Types } from "effect" processes five namespace-reexport targets into one file's shared maps; when an earlier-processed namespace re-exports a member under a rename, the flat map entry it leaves behind can clobber a later-processed namespace's unrenamed member of the same name (opencode's provider.ts: class Service extends Context.Service<...>()(...)).

Fixed by adding namespace_member_origin_names, unconditionally populated for every namespace member (not just renamed ones), consulted via a new import_origin_suffix_ns helper.

#5927 — plain-import-vs-namespace-member flat map collision

A third, distinct combination: a PLAIN named import's bare name and a NAMESPACE MEMBER's bare name share the same flat maps. provider.ts does import { omit } from "remeda" (plain) and import { Context } from "effect" (namespace-reexport) — effect's real Context.ts also exports a member literally named omit. Since a plain import has no other resolution path, it must always win, but namespace processing's unconditional .insert() could silently overwrite it depending on import-statement order.

Fixed by switching namespace-member flat-map writes to .entry().or_insert() (plain imports keep unconditional .insert() and always win regardless of order).

#5928 — well-known lib dedup (partial — three fixes, one remaining root cause)

Separate, deeper issue: after #5918+#5922+#5924+#5927 resolved every undefined symbol, opencode's link still failed with thousands of ld: duplicate symbol errors — programs needing several "well-known" native libraries simultaneously (opencode needs http, fastify, ioredis, net, ws all at once) hit duplicate Rust-stdlib symbols across those independently-built staticlibs. #5920/#5921 (already on main) fixed this for perry_runtime-* codegen units specifically; this PR adds three more pieces:

  1. Fixed-point dedup for shared deps (strip_bundled_shared_deps_from_well_known_lib): generalizes Spawned async tasks starve after recompile with fresh auto-optimize archives (wrapper-bundled perry-runtime copy splits event-pump state) #5920/fix(link): #5920 — dedup wrapper-bundled perry-runtime in wrappers-first links #5921's two safety rules (stdlib bundles the identical codegen unit; no other kept member depends on a symbol only the candidate provides) to every shared dependency via a fixed-point iteration (candidates can depend on each other, so a one-shot pass isn't safe — each round recomputes references against the shrinking kept-set until stable).
  2. Matching codegen-units across the "shared tokio" (perry-ext-net: outbound TCP panics — LTO dead-strips tokio CONTEXT statics #507) build group: perry-stdlib-static/perry-runtime-static override codegen-units=16 (compilePackages: eventemitter3 fails to link — undefined _js_event_emitter_* symbols #5140, preserving #[no_mangle] exports from thin-LTO internalization); the well-known ext crates had no override and used the workspace default of 1. Cargo builds them in one invocation so their crate/feature graphs are unified, but that doesn't survive a codegen-units mismatch — rustc partitions monomorphized code into that many units, so "the same" dependency (e.g. compiler_builtins) gets split differently and produces non-identical objects. Confirmed via md5: compiler_builtins codegen units are now byte-identical between libperry_ext_http.a and libperry_stdlib.a (231/231), vs. divergent sizes before (the shared core unit was 2.4 MB in the ext lib vs 551 KB in stdlib).
  3. Cold-build dedup bug: build_optimized_libs() has three return paths; two correctly set prefer_well_known_before_stdlib from whether well-known libs are in play, but the path taken when native libs must be built from scratch (not the "archives are fresh" cache hit) hardcoded it to false — silently skipping dedup entirely on any cold/cleared-cache build. Confirmed by comparing duplicate-symbol counts with and without a stale auto-optimize directory present (2152 vs 1382 for the same program). Fixed to match the other two branches.

Residual gap, root-caused but not fixed here: even with all three fixes, opencode's specific http+fastify+ioredis+net+ws combination still hits ~1382 duplicate symbols — the same count as before any of today's changes, because most of the colliding dependencies (tokio, hyper_util, rustls, ring, h2, std, core, ...) are folded into perry-stdlib-static's own linker-plugin-lto merge. An LTO-merged object is not byte-identical to a plain (non-LTO) compile of the same crate regardless of matching codegen-units — they're two structurally different compilation outputs, not the same output split differently. Rule 2 of the fixed-point dedup (above) correctly protects a whole candidate codegen-unit file the moment any symbol in it looks uniquely needed elsewhere, but a codegen unit bundles many functions, so one genuinely-unique function drags along every other (actually-duplicate) symbol in the same file.

I tried an additional, more surgical fix (exempting Rule 2's protection when a candidate is byte-identical to its matched stdlib member) — it's sound and passed the safety/regression suite, but never triggers for this combination since none of the colliding files are byte-identical for the LTO reason above, so I didn't keep it (no demonstrated value, added complexity). I also tried llvm-objcopy --localize-symbol-based symbol-level surgery in an earlier iteration; that caused a real, confirmed "there is no reactor running, must be called from the context of a Tokio 1.x runtime" regression and was reverted.

A real fix for the residual gap looks like it needs either (a) removing perry-stdlib-static's linker-plugin-lto (risky — would need to solve #5140 a different way) or (b) giving the well-known ext crates equivalent LTO treatment for just their shared deps so both sides merge the same way (uncertain effect, no cross-platform validation capacity available to me) — both are architectural calls I'd rather leave to a maintainer than guess at. Filed the analysis on #5928.

Testing

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds namespace-member-specific origin-name tracking, threads it through compile and lowering contexts, updates import registration to avoid overwriting flat mappings, adds regression fixtures for namespace re-export and import-order collisions, and introduces a new well-known-library shared-dependency stripping pass.

Changes

Namespace collision resolution

Layer / File(s) Summary
Origin-name map plumbing
crates/perry-codegen/src/codegen/opts.rs, crates/perry-codegen/src/codegen/mod.rs, crates/perry-codegen/src/codegen/closure.rs, crates/perry-codegen/src/codegen/entry.rs, crates/perry-codegen/src/codegen/function.rs, crates/perry-codegen/src/codegen/method.rs, crates/perry-codegen/src/expr/mod.rs, crates/perry/src/commands/compile/object_cache.rs, crates/perry/src/commands/compile/run_pipeline.rs
Adds namespace_member_origin_names to compile and codegen contexts, includes it in cache-key hashing, and threads it into compile options.
Import registration updates
crates/perry/src/commands/compile/run_pipeline.rs
Populates the new namespace-member origin-name map and changes flat prefix/origin inserts to preserve earlier entries.
Namespace-aware lowering
crates/perry-codegen/src/expr/v8_interop.rs, crates/perry-codegen/src/expr/property_get.rs, crates/perry-codegen/src/expr/static_method.rs, crates/perry-codegen/src/lower_call/namespace_call.rs
Adds a namespace-aware origin-suffix helper and switches namespace property, static-method, and namespace-call lowering to use namespace-scoped lookup.
Collision fixtures and regressions
test-files/fixtures/issue_5922_pkg/*, test-files/test_issue_5922_namespace_reexport_prefix_collision.ts, test-files/fixtures/issue_5924_pkg/*, test-files/test_issue_5924_namespace_reexport_origin_name_collision.ts, test-files/fixtures/issue_5927_pkg/*, test-files/test_issue_5927_order_a.ts, test-files/test_issue_5927_order_b.ts
Adds fixture barrels and modules for namespace-reexport collisions and regression tests covering prefix collisions, origin-name collisions, and import ordering.

Well-known library shared-dependency stripping

Layer / File(s) Summary
Strip helper implementation
crates/perry/src/commands/compile/strip_dedup.rs, crates/perry/src/commands/compile.rs
Adds strip_bundled_shared_deps_from_well_known_lib and the import needed to call it from compile flow.
Link-time integration
crates/perry/src/commands/compile/link/mod.rs, crates/perry/src/commands/compile/link/build_and_run.rs
Wires the new helper into well-known-library processing before duplicate-object stripping.
Review support
crates/perry/src/commands/compile/link/mod.rs
Adjusts helper imports used by the link pipeline.

Estimated code review effort: 4 (Complex) | ~75 minutes

Possibly related issues

Possibly related PRs

  • PerryTS/perry#5919: Both PRs modify crates/perry/src/commands/compile/run_pipeline.rs to prevent import_function_prefixes and related mappings from being overwritten in collision scenarios.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main fix: namespace-reexport member resolution collision handling for the listed issues.
Description check ✅ Passed The description covers summary, issue-specific changes, related issues, and testing, though it omits the exact template headings and checklist.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Ralph added 3 commits July 3, 2026 21:52
… collision

`import { Context } from "effect"` (named import of a value the source
module re-exports as a namespace via `export * as Context from
"./Context.js"`) registered every member of the target namespace into the
flat `import_function_prefixes` map, keyed only by the member's bare name.
When two such namespace-reexport targets imported into the same file
export a member with the same bare name (e.g. `Context.a` and `Option.a`,
both reached through effect's barrel), the second registration silently
overwrote the first.

perry already has the fix for this exact class of collision (PerryTS#680):
`namespace_member_prefixes`, keyed by `(namespace_local, member_name)`.
The plain `import * as X` branch already populates it; the
named-import-of-namespace-reexport branch did not. Fixed by registering
there too, and by teaching `expr/static_method.rs`'s
`namespace_imports.contains(class_name)` branch (the actual codegen
consumer for uppercase-receiver forms like `Context.a()`) to consult
`namespace_member_prefixes` before falling back to the flat map — mirroring
the pattern `namespace_call.rs` and `property_get.rs` already use for
lowercase-receiver call/read forms.

Found via a real-world source compile of `sst/opencode`, whose
`provider.ts` does `import { Effect, Layer, Context, Schema, Types } from
"effect"` — the last remaining pair of undefined symbols after PerryTS#5918/PerryTS#5919.
…ollision

Follow-up to PerryTS#5922/PerryTS#5923: fixing `import_function_prefixes` alone wasn't
enough for a real-world `sst/opencode` compile. `import_function_origin_names`
(issue PerryTS#678's symbol-suffix-override map for re-export renames) has the
same flat, bare-name-keyed structure and independently broke the same
real code.

`import { Effect, Layer, Context, Schema, Types } from "effect"` processes
five namespace-reexport targets into one file's shared maps. `Effect`
re-exports something named `Service` under a rename, so its turn inserts
`import_function_origin_names["Service"] = "<effect's internal origin
name>"`. `Context` directly exports its own `Service` (no rename needed),
so its turn inserts nothing — the earlier (wrong) entry survives. Codegen
then resolves `Context.Service(...)` (a real call in opencode's
`provider.ts`: `class Service extends Context.Service<...>()(...)`) via
the contaminated flat entry, producing an undefined
`perry_fn_..._Context_ts__<wrong-suffix>` symbol at link time.

Fixed by adding `namespace_member_origin_names`, a per-namespace-scoped
companion map mirroring PerryTS#680's `namespace_member_prefixes` — but
unconditionally populated for every namespace member (not just renamed
ones), since a sparse map would still fall through to the contaminated
flat map for unrenamed members. Consulted by every codegen site that
builds a `perry_fn_<src>__<suffix>` symbol for a namespace-member access
(`expr/static_method.rs`, `lower_call/namespace_call.rs`,
`expr/property_get.rs`) via a new `import_origin_suffix_ns` helper.
…p collision

Third companion to PerryTS#5918/PerryTS#5922/PerryTS#5924. A PLAIN named import's bare name and
a NAMESPACE MEMBER's bare name share the same flat
`import_function_prefixes`/`import_function_origin_names` maps. A plain
import has no other resolution path (a bare call has no namespace to
scope against), so it must always win — but namespace processing's
unconditional `.insert()` could silently overwrite it depending on
import-statement order.

Found via a real-world `sst/opencode` compile: `provider.ts` does
`import { omit } from "remeda"` (plain) and `import { Context } from
"effect"` (namespace-reexport), and effect's real `Context.ts` also
exports a member literally named `omit`. `Context` is imported after
`omit` in source order, so Context's registration silently overwrote
remeda's — the bare `omit(...)` call then resolved against Context.ts's
prefix combined with remeda's origin-name rename, producing an undefined
symbol request.

Fixed by using `.entry().or_insert()` instead of `.insert()` for BOTH
namespace-processing branches' flat-map writes (plain `import * as X`
and the named-import-of-namespace-reexport branch). Plain imports keep
their unconditional `.insert()`, so they always win regardless of
processing order: if a namespace claims an empty slot first, the plain
import's later unconditional insert still overwrites it; if the plain
import claims it first, the namespace's `or_insert` sees the slot taken
and skips. Namespace-member resolution is unaffected since the three
real consumers (`expr/static_method.rs`, `lower_call/namespace_call.rs`,
`expr/property_get.rs`) already prefer the namespace-scoped maps from
PerryTS#5922/PerryTS#5924 and never need the flat-map fallback for genuine namespace
members.

Verified both import orderings against Node with new regression
fixtures.
@proggeramlug proggeramlug force-pushed the fix/namespace-reexport-member-prefix-collision branch from 0fd4047 to 23be63c Compare July 4, 2026 04:55
@proggeramlug proggeramlug changed the title fix(compile,codegen): #5922 — namespace-reexport member prefix collision fix(compile,codegen): #5922/#5924/#5927 — namespace-reexport member resolution collisions Jul 4, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/perry/src/commands/compile/object_cache.rs (1)

540-554: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

LGTM on the hashing logic itself.

Suggest a symmetric cache-key test. The existing key_changes_with_namespace_member_prefixes test has no counterpart for namespace_member_origin_names, so a future refactor could silently drop this field from the cache key without any test failing.

♻️ Suggested test
#[test]
fn key_changes_with_namespace_member_origin_names() {
    let mut a = empty_opts();
    let mut b = empty_opts();
    a.namespace_member_origin_names
        .insert(("ns".into(), "Service".into()), "Service".into());
    b.namespace_member_origin_names
        .insert(("ns".into(), "Service".into()), "originalName".into());
    assert_ne!(
        compute_object_cache_key(&a, 1, "0.5.156"),
        compute_object_cache_key(&b, 1, "0.5.156")
    );
}
🤖 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/src/commands/compile/object_cache.rs` around lines 540 - 554,
Add a symmetric cache-key test for the namespace_member_origin_names input so
the hashing stays covered just like namespace_member_prefixes. Create a new test
alongside key_changes_with_namespace_member_prefixes that uses empty_opts(),
mutates namespace_member_origin_names on two otherwise identical options, and
asserts compute_object_cache_key() returns different values; this will protect
the namespace_member_origin_names handling in object_cache.rs from being dropped
in a future refactor.
🤖 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/src/commands/compile/object_cache.rs`:
- Around line 540-554: Add a symmetric cache-key test for the
namespace_member_origin_names input so the hashing stays covered just like
namespace_member_prefixes. Create a new test alongside
key_changes_with_namespace_member_prefixes that uses empty_opts(), mutates
namespace_member_origin_names on two otherwise identical options, and asserts
compute_object_cache_key() returns different values; this will protect the
namespace_member_origin_names handling in object_cache.rs from being dropped in
a future refactor.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 652f6864-bb93-4620-a011-cf6e61354108

📥 Commits

Reviewing files that changed from the base of the PR and between 0fd4047 and 23be63c.

📒 Files selected for processing (40)
  • crates/perry-codegen/src/codegen/closure.rs
  • crates/perry-codegen/src/codegen/entry.rs
  • crates/perry-codegen/src/codegen/function.rs
  • crates/perry-codegen/src/codegen/method.rs
  • crates/perry-codegen/src/codegen/mod.rs
  • crates/perry-codegen/src/codegen/opts.rs
  • crates/perry-codegen/src/expr/mod.rs
  • crates/perry-codegen/src/expr/property_get.rs
  • crates/perry-codegen/src/expr/static_method.rs
  • crates/perry-codegen/src/expr/v8_interop.rs
  • crates/perry-codegen/src/lower_call/namespace_call.rs
  • crates/perry-codegen/tests/argless_builtin_extra_args.rs
  • crates/perry-codegen/tests/class_keys_gc_root.rs
  • crates/perry-codegen/tests/constructor_recursion.rs
  • crates/perry-codegen/tests/destructure_call_location.rs
  • crates/perry-codegen/tests/large_object_barriers.rs
  • crates/perry-codegen/tests/macos_bundle_chdir_gate.rs
  • crates/perry-codegen/tests/native_proof_buffer_views.rs
  • crates/perry-codegen/tests/native_proof_regressions.rs
  • crates/perry-codegen/tests/shadow_slot_hygiene.rs
  • crates/perry-codegen/tests/static_symbol_hygiene.rs
  • crates/perry-codegen/tests/typed_feedback.rs
  • crates/perry-codegen/tests/typed_shape_descriptor.rs
  • crates/perry-codegen/tests/typed_shape_descriptors.rs
  • crates/perry/src/commands/compile/object_cache.rs
  • crates/perry/src/commands/compile/run_pipeline.rs
  • test-files/fixtures/issue_5922_pkg/barrel.ts
  • test-files/fixtures/issue_5922_pkg/context.ts
  • test-files/fixtures/issue_5922_pkg/option.ts
  • test-files/fixtures/issue_5924_pkg/barrel.ts
  • test-files/fixtures/issue_5924_pkg/ns_a.ts
  • test-files/fixtures/issue_5924_pkg/ns_a_impl.ts
  • test-files/fixtures/issue_5924_pkg/ns_b.ts
  • test-files/fixtures/issue_5927_pkg/barrel.ts
  • test-files/fixtures/issue_5927_pkg/ns_impl.ts
  • test-files/fixtures/issue_5927_pkg/plain_mod.ts
  • test-files/test_issue_5922_namespace_reexport_prefix_collision.ts
  • test-files/test_issue_5924_namespace_reexport_origin_name_collision.ts
  • test-files/test_issue_5927_order_a.ts
  • test-files/test_issue_5927_order_b.ts
✅ Files skipped from review due to trivial changes (10)
  • test-files/fixtures/issue_5924_pkg/ns_b.ts
  • test-files/fixtures/issue_5927_pkg/plain_mod.ts
  • test-files/fixtures/issue_5924_pkg/barrel.ts
  • test-files/fixtures/issue_5924_pkg/ns_a_impl.ts
  • test-files/fixtures/issue_5922_pkg/context.ts
  • test-files/fixtures/issue_5922_pkg/option.ts
  • crates/perry-codegen/tests/native_proof_buffer_views.rs
  • crates/perry-codegen/tests/argless_builtin_extra_args.rs
  • crates/perry-codegen/tests/typed_feedback.rs
  • test-files/fixtures/issue_5922_pkg/barrel.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • test-files/test_issue_5922_namespace_reexport_prefix_collision.ts

Ralph added 2 commits July 3, 2026 22:19
CI's `cargo fmt --all -- --check` flagged the import_origin_suffix_ns
addition from the PerryTS#5922/PerryTS#5924 fix — rustfmt wraps differently than the
manual edit did.
…ll-known libs

Companion to PerryTS#5920/PerryTS#5921. `strip_bundled_runtime_from_well_known_lib` only
targets `perry_runtime-*` codegen-unit members. Programs linking multiple
well-known libraries that each independently bundle a full "shared tokio"
HTTP-client stack (e.g. both `http` and `fastify` need tokio/hyper_util/h2/
rustls/reqwest/ring) still collide on every OTHER shared transitive
dependency those libraries have in common with `libperry_stdlib.a` — even
`std`/`core`/`alloc` themselves. macOS's current linker has no
`-multiply_defined suppress` / `-ld_classic` escape hatch anymore (verified
obsolete on current toolchains via direct testing), so these surface as
hard `ld: duplicate symbol` failures.

Adds `strip_bundled_shared_deps_from_well_known_lib`, applying the same two
safety rules as the runtime-specific function (stdlib bundles the identical
codegen unit; no other kept member depends on a symbol only the
duplicate-candidate provides) to every member, not just `perry_runtime-`
ones. A naive one-shot widening is NOT safe — candidates can depend on each
other (e.g. hyper_util's object referencing a symbol only tokio's object
defines, both bundled in the same well-known lib and both initially
flagged removable) — so this is a fixed-point iteration: each round
recomputes undefined-symbol references from the shrinking kept-set and
protects any still-needed candidate, repeating until stable.

Verified against `issue_5920_wrapper_bundled_runtime_async_starvation`
(passes) and a real-world `sst/opencode` compile: fully eliminates
duplicate symbols for simpler well-known libs (ioredis/net/ws went from
hundreds of duplicates to zero); substantially reduces them for the
largest, most interconnected pair (http+fastify, both needing the full
HTTP-client stack) though Rule 2 conservatively protects more members as
a single archive's internal dependency graph grows, so this specific
combination is improved but not fully zero yet.

Note: `strip_bundled_runtime_from_well_known_lib`/this function require
`llvm-objcopy`/`llvm-nm`/`llvm-ar` (via `PERRY_LLVM_OBJCOPY`/`PERRY_LLVM_NM`/
`PERRY_LLVM_AR` env vars or `PATH`) to actually run — without them they
silently no-op (each wrapped in `.unwrap_or_else` with a "(non-fatal)" log
line), producing a much later, confusing "N duplicate symbols" `ld`
failure with no indication dedup was skipped. This cost significant
debugging time this session before being traced to a missing local LLVM
toolchain install — worth a louder diagnostic in a follow-up.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
crates/perry/src/commands/compile/strip_dedup.rs (2)

995-1177: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Consider extracting the shared extract/rebuild-archive scaffolding.

strip_bundled_shared_deps_from_well_known_lib duplicates the list_members closure, temp-dir/extract, per-member iteration, and llvm-ar crs rebuild scaffolding already present (near-identically) in strip_bundled_runtime_from_well_known_lib and strip_duplicate_objects_from_lib. A shared helper taking a removal predicate/set plus a suffix for the trimmed archive name would reduce the risk of the three copies drifting out of sync on future fixes (e.g. a bug fix to extraction error handling applied to one copy but not the others).

🤖 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/src/commands/compile/strip_dedup.rs` around lines 995 - 1177,
Refactor the repeated archive-processing scaffolding out of
strip_bundled_shared_deps_from_well_known_lib so it shares the same
extraction/rebuild flow as strip_bundled_runtime_from_well_known_lib and
strip_duplicate_objects_from_lib. Introduce a common helper for listing members,
creating the temp extract dir, extracting with llvm-ar, and rebuilding with
llvm-ar crs, with the caller supplying the removal set/predicate and trimmed
archive suffix. Keep the existing fixed-point removal logic in
strip_bundled_shared_deps_from_well_known_lib, but move the duplicated
boilerplate into the shared helper to prevent the three implementations from
drifting.

1039-1177: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

No dedicated regression test for the fixed-point stripping algorithm.

The doc comment references issue_5920_wrapper_bundled_runtime_async_starvation as coverage for the sibling runtime-drop function, but this cohort doesn't show an equivalent fixture/regression test exercising the fixed-point protect/remove logic here (e.g. a synthetic case with inter-candidate dependencies, per the hyper_util/tokio example in the doc comment). Given this algorithm directly affects final link correctness, a regression test would guard against future changes silently breaking the fixed point convergence.

Want me to sketch a regression fixture (two well-known libs sharing a bundled dependency with an inter-candidate reference) to validate this path?

🤖 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/src/commands/compile/strip_dedup.rs` around lines 1039 - 1177,
Add a regression test for strip_bundled_shared_deps_from_well_known_lib that
exercises the fixed-point protect/remove loop with inter-candidate dependencies.
Create a synthetic archive fixture where two bundled members reference each
other or share a transitive dependency, similar to the hyper_util/tokio scenario
mentioned in the doc comment, and verify the algorithm keeps the required
member(s) while stripping only the safe ones. Use the
strip_bundled_shared_deps_from_well_known_lib path and assert the final trimmed
archive contents match the expected fixed-point outcome.
🤖 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/src/commands/compile/strip_dedup.rs`:
- Around line 995-1177: Refactor the repeated archive-processing scaffolding out
of strip_bundled_shared_deps_from_well_known_lib so it shares the same
extraction/rebuild flow as strip_bundled_runtime_from_well_known_lib and
strip_duplicate_objects_from_lib. Introduce a common helper for listing members,
creating the temp extract dir, extracting with llvm-ar, and rebuilding with
llvm-ar crs, with the caller supplying the removal set/predicate and trimmed
archive suffix. Keep the existing fixed-point removal logic in
strip_bundled_shared_deps_from_well_known_lib, but move the duplicated
boilerplate into the shared helper to prevent the three implementations from
drifting.
- Around line 1039-1177: Add a regression test for
strip_bundled_shared_deps_from_well_known_lib that exercises the fixed-point
protect/remove loop with inter-candidate dependencies. Create a synthetic
archive fixture where two bundled members reference each other or share a
transitive dependency, similar to the hyper_util/tokio scenario mentioned in the
doc comment, and verify the algorithm keeps the required member(s) while
stripping only the safe ones. Use the
strip_bundled_shared_deps_from_well_known_lib path and assert the final trimmed
archive contents match the expected fixed-point outcome.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6cf904a9-80e2-49c5-8b64-5a12ace791d5

📥 Commits

Reviewing files that changed from the base of the PR and between e2a0be0 and c097bc6.

📒 Files selected for processing (4)
  • crates/perry/src/commands/compile.rs
  • crates/perry/src/commands/compile/link/build_and_run.rs
  • crates/perry/src/commands/compile/link/mod.rs
  • crates/perry/src/commands/compile/strip_dedup.rs

@proggeramlug proggeramlug merged commit 18b90ea into PerryTS:main Jul 4, 2026
16 checks passed
proggeramlug pushed a commit to proggeramlug/perry that referenced this pull request Jul 4, 2026
…ollision

Follow-up to PerryTS#5922/PerryTS#5923: fixing `import_function_prefixes` alone wasn't
enough for a real-world `sst/opencode` compile. `import_function_origin_names`
(issue PerryTS#678's symbol-suffix-override map for re-export renames) has the
same flat, bare-name-keyed structure and independently broke the same
real code.

`import { Effect, Layer, Context, Schema, Types } from "effect"` processes
five namespace-reexport targets into one file's shared maps. `Effect`
re-exports something named `Service` under a rename, so its turn inserts
`import_function_origin_names["Service"] = "<effect's internal origin
name>"`. `Context` directly exports its own `Service` (no rename needed),
so its turn inserts nothing — the earlier (wrong) entry survives. Codegen
then resolves `Context.Service(...)` (a real call in opencode's
`provider.ts`: `class Service extends Context.Service<...>()(...)`) via
the contaminated flat entry, producing an undefined
`perry_fn_..._Context_ts__<wrong-suffix>` symbol at link time.

Fixed by adding `namespace_member_origin_names`, a per-namespace-scoped
companion map mirroring PerryTS#680's `namespace_member_prefixes` — but
unconditionally populated for every namespace member (not just renamed
ones), since a sparse map would still fall through to the contaminated
flat map for unrenamed members. Consulted by every codegen site that
builds a `perry_fn_<src>__<suffix>` symbol for a namespace-member access
(`expr/static_method.rs`, `lower_call/namespace_call.rs`,
`expr/property_get.rs`) via a new `import_origin_suffix_ns` helper.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

import_function_prefixes collision for namespace-reexport members (Context.a vs Option.a)

1 participant