Skip to content

perf(jq): a module both top-level included and a transitive dependency gets a duplicate link run (#2955) #3153

Description

@newhoggy

Severity: Low

Summary

Found by review of #2955 / PR #3150. ModuleLoader::hoist_order's visit
(src/bin/succinctly/jq_runner.rs:1068-1087) records a module id into the
hoisted link-run order whenever it is reached as a dependency
(as_dependency=true), even when that same id was already visited earlier as
an ordinary top-level include/import (as_dependency=false). The
top-level visit does not populate recorded (only expanded, which gates
re-descent, not the push), so a later dependency-reachable visit of the same
id still records it -- and link_dependency_runs
(src/bin/succinctly/jq_runner.rs:1333) then emits a second, full copy of
that module's defs under \0link:<id>::name link names, on top of the
module's own ordinary bare-name top-level run.

Repro

shared.jq:   def helper: 111;
consumer.jq: include "shared"; def use_helper: helper;

succinctly jq -L <dir> -nc 'include "shared"; include "consumer"; [helper, use_helper]'
gives [111,111], matching /usr/bin/jq byte-for-byte (confirmed live) --
not a wrong-answer bug. But shared's defs are compiled twice: once as an
ordinary bare-name run, once as a hoisted link run reachable only through
consumer's forwarding stub.

Why this looks structurally necessary, not accidental

consumer's own def body is floored at consumer's own run's begin marker
(#2951), so it cannot see shared's bare-name top-level run even though it
sits textually outside consumer's scope -- only a stub, which crosses
floors, can reach it. A stub always targets the qualified link name, never a
bare name, regardless of whether its target module also happens to have an
ordinary top-level run. hoist_order's own doc comment (jq_runner.rs
1058-1060) already documents this as intentional: "A module that is also a
top-level include/import is walked for its dependencies but not recorded
for itself unless something depends on it."

Scale

Bounded, not exponential: at most one extra full copy of a module's own def
bodies, for modules that are simultaneously top-level-included/imported and
a transitive dependency of another top-level module. Does not compound with
chain depth or fan-out the way #2955's original bug did.

Suggested direction

Have the module's existing ordinary top-level run also emit small
forwarding aliases (def \0link:<id>::name: name;) for any dependent that
needs the link-name spelling, instead of a full second copy of the bodies --
but the alias defs need to sit where they can actually see the ordinary run's
bare names, and link runs are wrapped outermost (see process_program), so a
naive "just add aliases inside the link run" placement cannot reach a bare
name declared further inside the chain. The ordering interacts with
link_dependency_runs' existing reference-pruning pass and needs to be
worked out carefully. A depth/size-bounded issue, not urgent.

Class: Opus

Requires re-deriving the link-run ordering/visibility invariants #2955 just
finished establishing (floor-crossing soundness, hoist_order's
outermost-first wrapping), so a fix risks reopening those without careful
re-verification against the existing 63-test module-scoping matrix
(tests/jq_cli_tests.rs).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    OpusSuitable for an Opus-class model to implementbugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions