Severity: Low
Summary
Found by review of #2955 / PR #3150. At module-load time,
ModuleLoader::load_and_bind_module (src/bin/succinctly/jq_runner.rs
~946-951) computes called = called_func_names(&body) once per def on the
original, unwrapped body, purely to decide which forwarding stubs
dep_stubs_for should add. That set is then discarded once the stubs are
wrapped in.
Later, at compile time, link_dependency_runs's per-linked-module fixed point
(src/bin/succinctly/jq_runner.rs:1333, the for &id in order.iter().rev()
loop) calls called_func_names(body) again on the now-larger, stub-wrapped
body, to rediscover essentially the same reachability fact: which names each
kept def calls, split into embedded link names (found by recursing into the
newly-added stub bodies) and bare sibling names (present in the original
body).
Cost
Not a correctness issue -- purely wasted work on a cold, one-time compile
path. But it re-walks a bigger tree (original body plus all its stub
wrappers) than the walk load_and_bind_module already did on the smaller
original body, for every referenced def in every linked module.
Suggested direction
Record each def's resolved bare-sibling names and stub link-name targets at
load time (in load_and_bind_module/dep_stubs_for), and have
link_dependency_runs consume that recorded data directly instead of
re-deriving it via a second called_func_names walk. This likely needs either
a new field alongside FuncDefList's (name, params, body) tuples or a
separate run id -> per-def reachability side table threaded from the loader
through to link_dependency_runs -- both non-trivial changes to a
widely-used type/data flow, which is why this wasn't folded into #2955's PR
directly.
Class: Sonnet
Contained to jq_runner.rs's loader/linker; no resolver or evaluator
changes, no new soundness argument -- just added bookkeeping and a data-flow
change between two existing functions.
Severity: Low
Summary
Found by review of #2955 / PR #3150. At module-load time,
ModuleLoader::load_and_bind_module(src/bin/succinctly/jq_runner.rs~946-951) computes
called = called_func_names(&body)once per def on theoriginal, unwrapped body, purely to decide which forwarding stubs
dep_stubs_forshould add. That set is then discarded once the stubs arewrapped in.
Later, at compile time,
link_dependency_runs's per-linked-module fixed point(
src/bin/succinctly/jq_runner.rs:1333, thefor &id in order.iter().rev()loop) calls
called_func_names(body)again on the now-larger, stub-wrappedbody, to rediscover essentially the same reachability fact: which names each
kept def calls, split into embedded link names (found by recursing into the
newly-added stub bodies) and bare sibling names (present in the original
body).
Cost
Not a correctness issue -- purely wasted work on a cold, one-time compile
path. But it re-walks a bigger tree (original body plus all its stub
wrappers) than the walk
load_and_bind_modulealready did on the smalleroriginal body, for every referenced def in every linked module.
Suggested direction
Record each def's resolved bare-sibling names and stub link-name targets at
load time (in
load_and_bind_module/dep_stubs_for), and havelink_dependency_runsconsume that recorded data directly instead ofre-deriving it via a second
called_func_nameswalk. This likely needs eithera new field alongside
FuncDefList's(name, params, body)tuples or aseparate
run id -> per-def reachabilityside table threaded from the loaderthrough to
link_dependency_runs-- both non-trivial changes to awidely-used type/data flow, which is why this wasn't folded into #2955's PR
directly.
Class: Sonnet
Contained to
jq_runner.rs's loader/linker; no resolver or evaluatorchanges, no new soundness argument -- just added bookkeeping and a data-flow
change between two existing functions.