From 0e87eb3017cf9865b6ef6083675c38e7cc33ad36 Mon Sep 17 00:00:00 2001 From: John Ky Date: Fri, 18 Sep 2026 18:30:16 +1000 Subject: [PATCH 1/5] fix(jq): link each dependency module once and forward to it through stubs (#2955, #3058) Binding a module's dependencies by copying their bodies into every def that reached them compounded down a chain: each level's bodies already carried the level below, so a chain whose defs each call F defs of the level below held F^L copies of the bottom one. The issue's 14-level fan-out-2 chain peaked at 681 MB against jq's 2 MB. A module some other module depends on is now emitted once, outermost in `process_program`, as an ordinary run whose begin marker carries a hidden alias (`ModuleRun::link_alias`) and whose defs are named `ModuleRun::link_name` (`link:::`). Inside the run a bare sibling call misses those names, floors, and is retried under the alias by the code #2989 added for imported modules, so the evaluator binds it unchanged. Each consuming def's body is wrapped in one forwarding stub per dependency (name, arity) it calls directly (`dep_stubs_for`), with the two exclusions the copying loader had; the transitive closure and #2962's rename (`rename_dep_calls`, `renamed_dep`) are gone, since a dependency's free names resolve in its own run. The one resolver change: `scan_scope` steps over an open begin marker when the name looked up is a link name, so a stub inside any run reaches the run outside. Link runs are wrapped in dependency post-order with each module's directives visited last-declared first (`hoist_order`), which is the order jq 1.7.1 reports cross-module compile errors in; and a dependency body now exists once, so an error in it is reported once (#3058). Only defs reached by name from the main filter -- through the top-level runs, retrying an imported module's bare sibling calls under its alias, then the linked modules dependents-first -- are linked, which keeps a wide chain the filter uses one def of at one def per level. Measured (release, this machine, output identical to jq): 8x6x3 142 MB -> 34 MB, 12x4x2 162 MB -> 36 MB, 14x4x2 681 MB -> 112 MB and 0.38 s -> 0.03 s; the recursion ceiling through a linked dependency is unchanged at 19494. The processed program is now linear in chain depth (169 / 241 / 313 nodes at 6 / 8 / 10 levels, against 1253 / 5093 / 20453). --- src/bin/succinctly/jq_runner.rs | 785 +++++++++++++++++++++----------- src/jq/resolve.rs | 175 +++++-- 2 files changed, 661 insertions(+), 299 deletions(-) diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index ab409b10a..d168b887e 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -53,13 +53,14 @@ pub struct EvalContext { /// parameter alias. type FuncDefList = Vec<(String, Vec, Expr)>; -/// One module's dependency defs, grouped by the module each group came from +/// One module's dependencies, grouped by the module each group came from /// (#2951), in declaration order, with the alias an `import` group was -/// brought in under (`None` for an `include`). Each group is wrapped in its -/// own flooring run (#2962): a compile error inside it names the right file, -/// and nothing of the def it is wrapped into -- its name, its parameters, its -/// module's siblings, the importing module's alias -- is visible inside it. -type DepRuns = Vec<(u32, Option, FuncDefList)>; +/// brought in under (`None` for an `include`). Only each dependency's +/// signature (name, params) travels here: the bodies stay in the cache and +/// are linked once, as a run of their own, by [`ModuleLoader::process_program`] +/// (#2955); a consuming def reaches them through forwarding stubs built from +/// these signatures by [`dep_stubs_for`]. +type DepGroups = Vec<(u32, Option, Vec<(String, Vec)>)>; /// The run id reserved for `~/.jq`'s own defs (#2951) -- assigned in /// [`ModuleLoader::new`] before any module can claim one. @@ -95,6 +96,13 @@ pub struct ModuleLoader { run_origins: BTreeMap, /// Next unused run id. `0` is reserved for `~/.jq`. next_run_id: u32, + /// `run id -> the run ids of that module's own dependencies`, in + /// declaration order (#2955). Read by [`Self::hoist_order`] to place each + /// linked module outside everything that depends on it. + deps_of: BTreeMap>, + /// `run id -> loaded_modules key` for every module some other module + /// depends on (#2955): the modules that get a link run of their own. + link_keys: BTreeMap, } /// A [`ModuleLoader`] failure, structured enough to report in jq's own @@ -445,7 +453,9 @@ fn wrap_defs(mut expr: Expr, defs: FuncDefList) -> Expr { /// This is the whole loader side of the module-scope boundary. See /// [`jq::ModuleRun`] for why the boundary is encoded as two extra defs whose /// names begin with a NUL byte, and [`jq::ModuleRun::parse`]'s callers in -/// `resolve.rs` for the one rule that reads them. +/// `resolve.rs` for the one rule that reads them. A module linked once as +/// another module's dependency is the same shape with a hidden alias +/// (#2955); see [`ModuleLoader::process_program`]. /// /// An empty run is not bracketed: a boundary with nothing inside it can only /// hide names from the code below, never reveal any, and `~/.jq` is usually @@ -470,13 +480,11 @@ fn wrap_run(expr: Expr, defs: FuncDefList, id: u32, alias: Option<&str>) -> Expr /// call site inside it can still be carrying #2036's un-resolved /// `shadow_fallback`, and such a node holds an empty `args` by construction -- /// its arity reads 0 whatever it really is. Keying on the name alone -/// over-keeps a little (a dependency `g/1` survives when only `g/0` is +/// over-keeps a little (a stub for `g/1` is emitted when only `g/0` is /// called) where keying on arity could silently *drop* a dependency a call /// genuinely needs, turning a program that compiles into a compile error. -/// The clash test in [`visible_deps_for`] and [`rename_dep_calls`] still key -/// on (name, arity), where they have to: there the exact pair is the -/// semantics, and `rename_dep_calls` reads a shadowable call's real arity -/// through [`jq::call_arity`]. +/// The clash test in [`dep_stubs_for`] still keys on (name, arity), where it +/// has to: there the exact pair is the semantics. /// /// `any_subexpr` with a predicate that never answers `true` is a full /// traversal -- the "must not rely on visiting every node" caveat in its doc @@ -537,23 +545,50 @@ fn called_func_names(expr: &Expr) -> BTreeSet { names } -/// The dependencies one of a module's own defs can reach, in [`wrap_defs`] -/// order (#2865): everything its body transitively calls, with any -/// dependency that would capture one of the def's own bindings renamed out of -/// its way (#2962). -/// -/// `deps` is one origin module's group, wrapped as run `run` (with `alias` -/// when it is an `import`). `body` is the def's own body as written, before -/// any group is wrapped around it: the groups are floored runs, so no group -/// can call into another, and only the body sees them all. +/// The forwarding stubs one of a module's own defs is wrapped in for one of +/// its dependency groups, in [`wrap_defs`] order (#2955): one per dependency +/// (name, arity) the body calls directly, each a def of that name and arity +/// whose body calls the dependency where it was linked, by its +/// [`jq::ModuleRun::link_name`], forwarding every parameter as written. +/// +/// `group` is one origin module's signatures, linked as run `run` (with +/// `alias` when it is an `import`, so its stubs are `alias::name`, exactly +/// as the run's own defs would be named at the top level). `called` is every +/// name the def's own body calls, as written. +/// +/// ### Why stubs, not copies +/// +/// Before #2955 the dependency *bodies* were copied into every def that +/// reached them, each copy carrying its own module's dependencies in turn, so +/// a chain of modules compounded as the fan-out to the power of the depth. +/// A stub is a fixed size, and the body it forwards to exists once. Nothing in +/// a stub is lexable: no free name a consumer could capture (#2962's whole +/// family), no `$variable`, no `$__loc__`, so it needs no run of its own. +/// +/// ### Direct calls only +/// +/// A dependency's own free names -- its module's siblings, and the modules +/// *it* includes -- are its own run's business: they resolve there, under the +/// run's alias retry and its stubs, so the transitive closure the copying +/// loader had to compute is gone. What remains is jq's `block_bind_referenced` +/// rule at the call site: a def's body is wrapped only in what it names, and +/// dropping an unreferenced dependency is unobservable, since nothing resolves +/// to it and jq agrees an unreferenced dependency whose own body calls an +/// undefined function is an error in neither tool. +/// +/// **Names only, not (name, arity)**: `called` comes from +/// [`called_func_names`], so a called name gets a stub at every arity the +/// group defines it at. The resolver picks the right one; an unused stub +/// costs one node and can capture nothing. /// /// ### Dependencies only -- a module's own siblings are deliberately absent /// -/// A module's defs are emitted as siblings in the top-level chain, exactly as -/// a filter's own defs are, so they already see each other there with jq's -/// lexical rule. Nesting a copy of one inside another's body instead puts it -/// under scopes it was never written in, and anything free in it is then -/// captured by them -- not just module-level names, but **builtins**: +/// A module's defs are emitted as siblings in the chain they are exported +/// into, exactly as a filter's own defs are, so they already see each other +/// there with jq's lexical rule. Nesting a copy of one inside another's body +/// instead puts it under scopes it was never written in, and anything free +/// in it is then captured by them -- not just module-level names, but +/// **builtins**: /// /// ```text /// def a: length; jq: [1,2] | h(9) is 2 @@ -563,25 +598,9 @@ fn called_func_names(expr: &Expr) -> BTreeSet { /// def h(b): a; nested: 99, the error silently swallowed /// ``` /// -/// Leaving siblings in the flat chain is what keeps them bound where they -/// were written. -/// -/// ### The transitive closure -/// -/// A dependency arrives already carrying its *own* module's dependencies, but -/// its references to that module's **siblings** are still free -- they were -/// emitted into a chain this scope does not have. So the closure widens -/// through each kept dependency's body and pulls those siblings in too; they -/// are exports of the same module, so they are in `deps` to be found. With -/// `inner.jq` = `def g: 42; def k: g;`, `outer.jq` = `include "inner"; def g: -/// k;`, jq answers `42`: `k`'s `g` is `inner`'s, and it is reached only -/// through `k`. In an `import` group the siblings carry the alias, and a -/// dependency's bare call reaches them through `resolve.rs`'s alias retry, so -/// a bare name also wants its `alias::name`. +/// ### Two names never get a stub /// -/// ### Clashes: renamed, never excluded -/// -/// The group is wrapped *inside* the def, so its entries shadow two of the +/// The stubs are wrapped *inside* the def, so a stub would shadow two of the /// def's own bindings for the body: /// /// - **The def's own (name, arity).** jq binds a def's own recursive call to @@ -591,176 +610,78 @@ fn called_func_names(expr: &Expr) -> BTreeSet { /// namespace (`Param::Dollar`'s `$g` binds `g` too) and wins: `def f(g): g; /// def q: f(7);` answers `7` in jq even with a dependency `g` in scope. /// -/// The body's own calls to such a name are therefore never the dependency's, -/// and do not keep it. Another dependency's calls to it *are*, and jq answers -/// them with the dependency: with `inner.jq` = `def c: 7; def g: c;` and -/// `mid.jq` = `include "inner"; def c: if . == 0 then g else (. - 1 | c) -/// end;`, `0 | c` is `7`. Excluding the dependency strands that call (a -/// compile error once dependency runs floor), and keeping it under its own -/// name captures the body's. So a clashing dependency that another one needs -/// is kept under [`jq::ModuleRun::renamed_dep`], and the calls that reach it -/// are renamed with it -- see [`rename_dep_calls`]. -/// -/// ### The referenced filter -/// -/// jq's own `block_bind_referenced` rule, and here a sizing requirement -/// rather than a micro-optimisation: wrapping unconditionally compounds down -/// a chain, because each level's bodies already carry the level below. A -/// synthetic chain of 40-def modules cost 359 MB peak RSS at three levels -/// (jq: 2.5 MB) and would have been tens of gigabytes at four. It is a -/// mitigation, not a cure -- see `docs/compliance/jq/limitations.md` and -/// #2955. Dropping an unreferenced dependency is unobservable: nothing -/// resolves to it, and jq agrees an unreferenced dependency whose own body -/// calls an undefined function is an error in neither tool. -fn visible_deps_for( - deps: &FuncDefList, +/// The body's own calls to such a name are therefore never the dependency's. +/// Another dependency's calls to it are answered inside that dependency's +/// own run, where the clash does not exist -- with `inner.jq` = `def c: 7; +/// def g: c;` and `mid.jq` = `include "inner"; def c: if . == 0 then g else +/// (. - 1 | c) end;`, `0 | c` is `7`, and `g`'s `c` is `inner`'s because `g` +/// was linked next to it. The copying loader had to *rename* such a +/// dependency to get that row right (#2962); linking makes the rename moot. +fn dep_stubs_for( + group: &[(String, Vec)], run: u32, alias: Option<&str>, name: &str, params: &[Param], - body: &Expr, + called: &BTreeSet, ) -> FuncDefList { let param_names: BTreeSet<&str> = params.iter().map(Param::name).collect(); let arity = params.len(); - let clashes = |dep_name: &str, dep_params: &[Param]| { - (dep_name == name && dep_params.len() == arity) - || (dep_params.is_empty() && param_names.contains(dep_name)) - }; - - let by_body = called_func_names(body); - let mut by_deps = BTreeSet::new(); - let mut keep = vec![false; deps.len()]; - - // Fixed point: a kept dependency's body can name a sibling of its own - // module, which is itself a dependency here and may sit anywhere in the - // list. Each entry is admitted at most once, so this terminates. - loop { - let mut grew = false; - for (i, (dep_name, dep_params, dep_body)) in deps.iter().enumerate() { - let wanted = by_deps.contains(dep_name) - || (by_body.contains(dep_name) && !clashes(dep_name, dep_params)); - if keep[i] || !wanted { - continue; - } - keep[i] = true; - let before = by_deps.len(); - for called in called_func_names(dep_body) { - if let Some(alias) = alias { - if !called.contains("::") { - by_deps.insert(format!("{alias}::{called}")); - } - } - by_deps.insert(called); - } - grew |= by_deps.len() != before; - } - if !grew { - break; - } - } - - // Each kept entry, and alongside it its (name, arity) as written, which the - // renaming below compares against: a renamed entry's own name no longer - // says. - let mut kept: FuncDefList = Vec::new(); - let mut written: Vec<(&str, usize)> = Vec::new(); - let mut renames = Vec::new(); - for (i, (dep_name, dep_params, dep_body)) in deps.iter().enumerate() { - if !keep[i] { + let mut seen: BTreeSet<(String, usize)> = BTreeSet::new(); + let mut stubs: FuncDefList = Vec::new(); + for (dep_name, dep_params) in group { + let stub_name = match alias { + Some(alias) => format!("{alias}::{dep_name}"), + None => dep_name.clone(), + }; + let clashes = (dep_name == name && dep_params.len() == arity) + || (dep_params.is_empty() && param_names.contains(dep_name.as_str())); + if clashes || !called.contains(&stub_name) { continue; } - let bound_as = if clashes(dep_name, dep_params) { - let renamed = jq::ModuleRun::renamed_dep(run, i, dep_name); - renames.push((kept.len(), renamed.clone())); - renamed - } else { - dep_name.clone() - }; - written.push((dep_name, dep_params.len())); - kept.push((bound_as, dep_params.clone(), dep_body.clone())); - } - - // A call reaches the renamed entry from the entry's own body (recursion) - // and from every later entry, until a later entry of the same (name, - // arity) shadows it -- `wrap_defs` nests later entries inside earlier - // ones. An earlier entry cannot see it at all. - for (at, renamed) in renames { - let (old, old_arity) = written[at]; - for (j, (_, entry_params, entry_body)) in kept.iter_mut().enumerate().skip(at) { - if j > at && written[j] == (old, old_arity) { - break; - } - if old_arity == 0 && entry_params.iter().any(|p| p.name() == old) { - continue; - } - *entry_body = rename_dep_calls(entry_body, old, old_arity, &renamed, 0); + // Two same-(name, arity) entries in one module are one export -- the + // later one, innermost in its own run -- so one stub serves both. + if !seen.insert((stub_name.clone(), dep_params.len())) { + continue; } + stubs.push(forwarding_stub( + stub_name, + dep_params, + jq::ModuleRun::link_name(run, dep_name), + )); } - - kept + stubs } -/// `expr` with every call that resolves to `old/arity` renamed to `new` -/// (#2962), for a dependency [`visible_deps_for`] renamed out of a def's way. -/// -/// Scope-aware, so a call bound to something else keeps its name: -/// -/// - a nested def of the same (name, arity) binds its own body and everything -/// after it; -/// - a nested def's parameter of that name binds its body, at arity 0; -/// - a dependency body is already bound, and carries its own dependencies as -/// runs: a def *inside* a run is floored and cannot see `old` at all, so -/// its body is left alone, but it is still wrapped around what follows the -/// run and so still shadows there. `run_depth` counts the runs open along -/// the current def chain. -fn rename_dep_calls(expr: &Expr, old: &str, arity: usize, new: &str, run_depth: usize) -> Expr { - let mut recurse = |e: &Expr| rename_dep_calls(e, old, arity, new, 0); - match expr { - Expr::FuncDef { - name, - params, - body, - then, - .. - } => { - let then_depth = match jq::ModuleRun::parse(name) { - Some(jq::RunMarker::Begin { .. }) => run_depth + 1, - Some(jq::RunMarker::End { .. }) => run_depth.saturating_sub(1), - None => run_depth, - }; - let same = name == old && params.len() == arity; - let param_binds = arity == 0 && params.iter().any(|p| p.name() == old); - let body = if run_depth > 0 || same || param_binds { - (**body).clone() - } else { - recurse(body) - }; - let then = if same { - (**then).clone() - } else { - rename_dep_calls(then, old, arity, new, then_depth) - }; - Expr::FuncDef { - name: name.clone(), - params: params.clone(), - body: Box::new(body), - then: Box::new(then), - bound: FuncDefBound::default(), - } - } - Expr::FuncCall { - name, - args, - builtin_fallback, - } if name == old && jq::call_arity(args, builtin_fallback.as_deref()) == arity => { - let mut renamed = succinctly::jq::walk::map_subexprs(expr, &mut recurse); - if let Expr::FuncCall { name, .. } = &mut renamed { - *name = new.to_string(); - } - renamed - } - _ => succinctly::jq::walk::map_subexprs(expr, &mut recurse), - } +/// `def (p1; ...; pn): (p1; ...; pn);` -- a def that forwards +/// every argument to `target` as a closure (#2955). +/// +/// The parameters are the dependency's own names, spelt bare even where the +/// dependency spells them `$p`: a bare parameter forwards the caller's +/// argument expression unevaluated, and the dependency does its own `$p` +/// binding on arrival, exactly as it would for a direct call. A parameter +/// named like the target cannot clash with it -- the +/// target is a NUL-prefixed link name -- and one named like the stub itself +/// binds only at arity 0, while the forwarded call is at the stub's arity. +fn forwarding_stub(name: String, params: &[Param], target: String) -> (String, Vec, Expr) { + let forwarded: Vec = params + .iter() + .map(|p| Param::Bare(p.name().to_string())) + .collect(); + let args = forwarded + .iter() + .map(|p| Expr::FuncCall { + name: p.name().to_string(), + args: Vec::new(), + builtin_fallback: None, + }) + .collect(); + let body = Expr::FuncCall { + name: target, + args, + builtin_fallback: None, + }; + (name, forwarded, body) } /// `path` resolved through the filesystem, falling back to `path` itself. @@ -832,6 +753,8 @@ impl ModuleLoader { run_ids: BTreeMap::new(), run_origins, next_run_id: AUTO_LOAD_RUN_ID + 1, + deps_of: BTreeMap::new(), + link_keys: BTreeMap::new(), } } @@ -843,10 +766,7 @@ impl ModuleLoader { /// a diagnostic key, never a correctness one -- `scan_scope` pairs a /// begin with an end by nesting, not by id. fn run_id_for(&mut self, module_path: &str) -> u32 { - let key = resolve_module_in(&self.search_path, module_path).map_or_else( - || module_path.to_string(), - |p| canonical_or_self(&p).to_string_lossy().into_owned(), - ); + let key = self.run_key(module_path); if let Some(&id) = self.run_ids.get(&key) { return id; } @@ -857,6 +777,15 @@ impl ModuleLoader { id } + /// The key [`Self::run_id_for`] interns `module_path` under: its + /// canonical file, or the path as written when it cannot be resolved. + fn run_key(&self, module_path: &str) -> String { + resolve_module_in(&self.search_path, module_path).map_or_else( + || module_path.to_string(), + |p| canonical_or_self(&p).to_string_lossy().into_owned(), + ) + } + /// The canonical file a run id names, for compile-error attribution. #[must_use] pub fn run_origin(&self, id: u32) -> Option<&str> { @@ -872,12 +801,13 @@ impl ModuleLoader { /// (#2395). [`Self::load_module`] is this plus that clone, for callers /// that need an owned copy. /// - /// The defs handed back are the module's **own** defs only, each with its - /// module's dependencies already wrapped around its body (#2865) -- so a - /// transitively included name is visible to the module that included it - /// and to nobody else. See [`Self::load_and_bind_module`] for why that - /// shape, rather than splicing the dependencies into the exported chain, - /// is what real jq does. + /// The defs handed back are the module's **own** defs only, each with + /// forwarding stubs for its module's dependencies already wrapped around + /// its body (#2865, #2955) -- so a transitively included name is visible + /// to the module that included it and to nobody else. See + /// [`Self::load_and_bind_module`] for why that shape, rather than + /// splicing the dependencies into the exported chain, is what real jq + /// does. fn ensure_module_loaded(&mut self, module_path: &str) -> Result<&FuncDefList, ModuleLoadError> { // `contains_key` -> load -> `insert` -> re-`get`, rather than the // `entry` spelling this used to have (#2865). `entry` holds a mutable @@ -898,8 +828,9 @@ impl ModuleLoader { .expect("just inserted above, or already present")) } - /// Read, parse and bind one module: its own defs, each wrapped in whatever - /// its own `include`/`import` directives bring into scope (#2865). + /// Read, parse and bind one module: its own defs, each wrapped in + /// forwarding stubs for whatever its own `include`/`import` directives + /// bring into scope (#2865, #2955). /// /// ### Why wrap each *body* rather than splice into the exported chain /// @@ -921,15 +852,19 @@ impl ModuleLoader { /// /// ### What each body is wrapped in /// - /// [`visible_deps_for`] decides that per def: the module's dependencies - /// that the body transitively calls, each origin's group wrapped as its - /// own flooring run, with a dependency that would shadow the def's own - /// name or a parameter renamed out of the way (#2962). The module's *own* defs are - /// deliberately not wrapped in -- they are emitted as siblings in the - /// top-level chain, where jq's lexical rule already relates them, and - /// nesting a copy of one inside another's body would put it under scopes - /// it was never written in. That function's doc comment carries the oracle - /// row behind each rule. + /// [`dep_stubs_for`] decides that per def and per origin group: one + /// forwarding stub per dependency (name, arity) the body calls directly, + /// except the def's own name and its parameters (#2962). The dependency + /// bodies themselves are not here at all: each dependency module is + /// linked once, as a run of its own, by [`Self::process_program`], and + /// the stubs call into it by an unlexable name. That is what keeps a + /// chain of modules linear in size (#2955), and it is also what binds a + /// dependency's own free names where jq binds them -- inside its own + /// module -- rather than inside whichever def happened to copy it. The + /// module's *own* defs are deliberately not wrapped in either -- they + /// are emitted as siblings in the chain they are exported into, where + /// jq's lexical rule already relates them. That function's doc comment + /// carries the oracle row behind each rule. /// /// ### Search-path resolution /// @@ -977,46 +912,39 @@ impl ModuleLoader { // *this* module's path, regressing #2774. let own = extract_and_stamp_func_defs(&program.expr, file_path); + let own_id = self.run_id_for(module_path); self.loading.push((canonical, module_path.to_string())); - let deps = self.module_dep_defs(&program); + let deps = self.module_dep_defs(&program, own_id); self.loading.pop(); let deps = deps?; - // Each def wrapped in the dependencies it reaches. The module's own - // defs are left to the top-level chain -- see [`visible_deps_for`] for - // why nesting them here is unsound. + // Each def wrapped in stubs for the dependencies it names. The + // module's own defs are left to the chain it is exported into -- see + // [`dep_stubs_for`] for why nesting them here is unsound. Ok(own .into_iter() .map(|(name, params, body)| { - // Each origin module's contribution is wrapped in its own - // run, which floors it (#2962): a dependency's body sees its - // own module's names and nothing of the def it is wrapped - // into -- not its name, its parameters, or another origin's - // group. The run also names the dependency's own file for a - // compile error in it, and keeps this module's import alias - // from reaching it. - // // Groups keep declaration order, and each is wrapped in turn // so the last-declared ends up innermost, exactly as the flat - // `wrap_defs` did before the grouping. - let visible: Vec = deps - .iter() - .map(|(origin, alias, group)| { - visible_deps_for(group, *origin, alias.as_deref(), &name, ¶ms, &body) - }) - .collect(); + // `wrap_defs` did before the grouping: `include "pa"; + // include "pb";` with both defining `foo` resolves `foo` to + // `pb`'s, as at the top level. + let called = called_func_names(&body); let mut wrapped = body; - for ((origin, alias, _), visible) in deps.iter().zip(visible).rev() { - wrapped = wrap_run(wrapped, visible, *origin, alias.as_deref()); + for (origin, alias, group) in deps.iter().rev() { + let stubs = + dep_stubs_for(group, *origin, alias.as_deref(), &name, ¶ms, &called); + wrapped = wrap_defs(wrapped, stubs); } (name, params, wrapped) }) .collect()) } - /// Every def one module's own `include`/`import` directives bring into - /// that module's scope, in [`wrap_defs`] order (last entry innermost, so - /// last-declared wins). + /// The signature of every def one module's own `include`/`import` + /// directives bring into that module's scope, in [`wrap_defs`] order + /// (last entry innermost, so last-declared wins), recording on the way + /// that `own_id` depends on each of them (#2955). /// /// Declaration order is what produces that: a later `include` is appended /// later, so it lands nearer the end and therefore nearer the body -- @@ -1030,19 +958,25 @@ impl ModuleLoader { /// `hj/0 is not defined` even with `def hj: 1234;` in `~/.jq`). They still /// leak in today through the top-level chain -- a separate, pre-existing /// gap this fix neither widens nor closes. - fn module_dep_defs(&mut self, program: &Program) -> Result { - let mut defs: DepRuns = Vec::new(); + fn module_dep_defs( + &mut self, + program: &Program, + own_id: u32, + ) -> Result { + let mut defs: DepGroups = Vec::new(); for include in &program.includes { let id = self.run_id_for(&include.path); - defs.push((id, None, self.load_module(&include.path)?)); + let sigs = self.dependency_signatures(&include.path, id, own_id)?; + defs.push((id, None, sigs)); } - // Namespaced exactly as `process_program` does it, and left as - // `ns::name` for the single `rewrite_namespaced_calls` pass at the end - // of `process_program` to pick up: these bodies are spliced into the - // tree before that pass runs, so a `NamespacedCall` inside a module - // body is rewritten along with every other one. + // A module `import`ed by a module keeps its bare signatures here; + // [`dep_stubs_for`] spells the stubs `ns::name`, as `process_program` + // spells a top-level import's defs, and leaves them for the single + // `rewrite_namespaced_calls` pass at the end of `process_program`, + // which rewrites the `NamespacedCall`s inside module bodies along + // with every other one. // // A *data* import (`import "f" as $d;`, which binds a `$`-variable to // the file's parsed JSON rather than a namespace of defs) contributes @@ -1068,21 +1002,99 @@ impl ModuleLoader { } continue; } - let namespace = &import.alias; let id = self.run_id_for(&import.path); - defs.push(( - id, - Some(namespace.clone()), - self.load_module(&import.path)? - .into_iter() - .map(|(name, params, body)| (format!("{namespace}::{name}"), params, body)) - .collect(), - )); + let sigs = self.dependency_signatures(&import.path, id, own_id)?; + defs.push((id, Some(import.alias.clone()), sigs)); } Ok(defs) } + /// Load `module_path` as a dependency of the module with run id `own_id` + /// and borrow out its defs' signatures (#2955): the names and parameters + /// the stubs are built from. The bodies stay in the cache, to be linked + /// once by [`Self::process_program`], which is what this also records: + /// that `id` needs a link run, and that `own_id` depends on it. + fn dependency_signatures( + &mut self, + module_path: &str, + id: u32, + own_id: u32, + ) -> Result)>, ModuleLoadError> { + let sigs = self + .ensure_module_loaded(module_path)? + .iter() + .map(|(name, params, _)| (name.clone(), params.clone())) + .collect(); + self.link_keys.insert(id, module_path.to_string()); + self.deps_of.entry(own_id).or_default().push(id); + Ok(sigs) + } + + /// The modules that need a link run (#2955), outermost first: a module + /// that some other module depends on, placed outside everything that + /// depends on it, each once, a diamond included. + /// + /// The walk visits each module's directives in **reverse** declaration + /// order and records a module after its own dependencies, so the runs are + /// wrapped -- and their bodies compiled, and any compile errors in them + /// reported -- in the order jq 1.7.1 reports them: a dependency's errors + /// before its includer's, the last-declared dependency's first, and a + /// chain's deepest module first (captured live for all three shapes). 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: its top-level run already carries its defs. + fn hoist_order(&self, program: &Program) -> Vec { + fn visit( + loader: &ModuleLoader, + id: u32, + as_dependency: bool, + expanded: &mut BTreeSet, + recorded: &mut BTreeSet, + order: &mut Vec, + ) { + if expanded.insert(id) { + if let Some(deps) = loader.deps_of.get(&id) { + for &dep in deps.iter().rev() { + visit(loader, dep, true, expanded, recorded, order); + } + } + } + if as_dependency && recorded.insert(id) { + order.push(id); + } + } + + // Top-level directives, last declared first, exactly as their runs + // nest (`process_program` wraps the last-declared include innermost). + let mut top: Vec<(usize, &str)> = program + .includes + .iter() + .map(|i| (i.decl_index, i.path.as_str())) + .chain( + program + .imports + .iter() + .filter(|i| !i.data) + .map(|i| (i.decl_index, i.path.as_str())), + ) + .collect(); + top.sort_by_key(|(decl, _)| core::cmp::Reverse(*decl)); + + let mut expanded = BTreeSet::new(); + let mut recorded = BTreeSet::new(); + let mut order = Vec::new(); + for (_, path) in top { + // Already interned by the load that recorded its dependencies; + // an unresolvable path is not in `deps_of` and contributes nothing. + let Some(&id) = self.run_ids.get(&self.run_key(path)) else { + continue; + }; + visit(self, id, false, &mut expanded, &mut recorded, &mut order); + } + order + } + /// Load a module and return an owned copy of its function definitions /// (name, params, body). pub fn load_module(&mut self, module_path: &str) -> Result { @@ -1259,11 +1271,172 @@ impl ModuleLoader { return Err(e); } + expr = self.link_dependency_runs(program, expr); + // Transform NamespacedCall expressions to regular FuncCall expressions expr = rewrite_namespaced_calls(expr); Ok(expr) } + + /// Wrap `expr` in one link run per module some module depends on + /// (#2955): each such module's defs, once, under their + /// [`jq::ModuleRun::link_name`]s, in a run whose alias is the module's + /// [`jq::ModuleRun::link_alias`], outermost of everything -- outside the + /// imports, `~/.jq` and includes already wrapped around `expr` -- and in + /// [`Self::hoist_order`], so every module sits outside the modules that + /// depend on it. + /// + /// A consuming def reaches a linked def through the forwarding stub + /// [`dep_stubs_for`] wrapped into its body; a linked def's own bare calls + /// to its siblings miss the link names, floor at the run's begin marker, + /// and are retried under the alias by the resolver, exactly as a bare + /// sibling call inside an `import`ed module is (#2989). The names are + /// unlexable, so the run exports nothing: `include "mid"; g` where only + /// `mid`'s dependency defines `g` is still `g/0 is not defined`. + /// + /// ### Only what is referenced is linked + /// + /// jq's `block_bind_referenced` rule, applied per module: a def is + /// emitted only if some body that is itself reached names it -- a stub + /// in a module that depends on this one, or a kept sibling of its own. + /// "Reached" is by name from the main filter outward: the filter's own + /// calls, then the top-level runs' defs those name and everything *they* + /// name, and only then each linked module, dependents first (the reverse + /// of the wrapping order), so it is a single pass. Dropping the rest is + /// unobservable, since nothing resolves to a def nobody reached names -- + /// the resolver never checks an unreached body either (#2740). What it + /// buys is that a large utility module used for one function costs one + /// def in the chain, not all of them, and a wide module chain of which + /// the filter uses one def costs one def per level: every chain def is + /// installed over the whole program below it at evaluation, so the + /// chain's length is the cost that matters (seeding from *every* + /// top-level def instead measured 22 MB against 12 MB for a six-level + /// forty-def chain the filter walks one def of). + /// + /// Every same-name entry is kept together, in declaration order, so the + /// innermost-first rule among them is the module's own (`def c: 7; def + /// c: 8; def g: c;` exports `g` as 8, and `def c: 7; def g: c; def c: 8; + /// def k: [g, c];` as `[7, 8]`, both as jq answers). + fn link_dependency_runs(&self, program: &Program, mut expr: Expr) -> Expr { + let order = self.hoist_order(program); + if order.is_empty() { + return expr; + } + + // Every name reached so far, as the reaching call spells it: bare + // for an `include`d or `~/.jq` def, `alias::name` for an `import`ed + // one, and a link name once a stub is reached. Seeded from the main + // filter, then closed over the top-level runs' defs -- all of which + // are emitted regardless; this only decides what they pull in. + // + // An `import`ed def calls its siblings bare, and the resolver retries + // that call as `alias::name` (#2989); the closure has to retry it the + // same way, or a sibling reached only that way looks unreached and + // what *it* depends on is never linked (a compile error naming the + // missing link, in a program jq runs). + let mut wanted: BTreeSet = called_func_names(&program.expr); + let mut top: Vec<(String, &Expr, Option<&str>, bool)> = Vec::new(); + for include in &program.includes { + if let Some(defs) = self.loaded_modules.get(&include.path) { + top.extend(defs.iter().map(|(n, _, b)| (n.clone(), b, None, false))); + } + } + for import in program.imports.iter().filter(|i| !i.data) { + if let Some(defs) = self.loaded_modules.get(&import.path) { + let ns = import.alias.as_str(); + top.extend( + defs.iter() + .map(|(n, _, b)| (format!("{ns}::{n}"), b, Some(ns), false)), + ); + } + } + top.extend( + self.auto_loaded_defs + .iter() + .map(|(n, _, b)| (n.clone(), b, None, false)), + ); + loop { + let mut grew = false; + for (name, body, alias, kept) in &mut top { + if *kept || !wanted.contains(name.as_str()) { + continue; + } + *kept = true; + let before = wanted.len(); + for called in called_func_names(body) { + if let Some(alias) = alias { + if !called.contains("::") { + wanted.insert(format!("{alias}::{called}")); + } + } + wanted.insert(called); + } + grew |= wanted.len() != before; + } + if !grew { + break; + } + } + + for &id in order.iter().rev() { + let Some(defs) = self + .link_keys + .get(&id) + .and_then(|k| self.loaded_modules.get(k)) + else { + continue; + }; + + // Seeds: this module's defs some stub already names. Then the + // fixed point over its own bare sibling calls. + let mut kept_names: BTreeSet<&str> = defs + .iter() + .map(|(name, _, _)| name.as_str()) + .filter(|name| wanted.contains(&jq::ModuleRun::link_name(id, name))) + .collect(); + let mut keep = vec![false; defs.len()]; + loop { + let mut grew = false; + for (i, (name, _, body)) in defs.iter().enumerate() { + if keep[i] || !kept_names.contains(name.as_str()) { + continue; + } + keep[i] = true; + for called in called_func_names(body) { + if jq::ModuleRun::is_link_name(&called) { + wanted.insert(called); + } else if let Some(sibling) = defs + .iter() + .map(|(n, _, _)| n.as_str()) + .find(|n| *n == called) + { + grew |= kept_names.insert(sibling); + } + } + } + if !grew { + break; + } + } + + let linked: FuncDefList = defs + .iter() + .zip(&keep) + .filter(|(_, keep)| **keep) + .map(|((name, params, body), _)| { + ( + jq::ModuleRun::link_name(id, name), + params.clone(), + body.clone(), + ) + }) + .collect(); + let alias = jq::ModuleRun::link_alias(id); + expr = wrap_run(expr, linked, id, Some(&alias)); + } + expr + } } /// Rewrite every computed-key `Expr` inside a `reduce`/`foreach`/`as {...}` @@ -8122,6 +8295,92 @@ mod tests { } } + /// #2955: the processed program grows *linearly* with the depth of a + /// module chain whose defs each call more than one def from the level + /// below. + /// + /// The issue's own generator: `L` levels of `D` defs, each calling `F` + /// defs of the level below, resolved at the top. Binding by copying the + /// dependency ASTs into every caller held `F^L` copies of the bottom + /// level; linking each dependency module once and forwarding to it holds + /// each body once. A wall-clock or RSS bound would flake on a loaded CI + /// box, so this counts `Expr` nodes instead and asserts the per-level + /// delta is constant -- a `~/.jq` on the developer's machine adds the + /// same constant to all three counts and cancels out. + /// + /// Against the copying loader this read 1253 / 5093 / 20453 nodes at + /// L = 6 / 8 / 10 (deltas 3840 and 15360: x4 per two levels, as `F^L` + /// predicts, and it failed here); with linking the deltas are equal. + mod link_size_guard_2955 { + use super::*; + + fn generate(dir: &std::path::Path, levels: usize, defs: usize, fan_out: usize) { + let mut m0 = String::new(); + for i in 0..defs { + m0.push_str(&format!("def f0_{i}: {i};\n")); + } + std::fs::write(dir.join("m0.jq"), m0).expect("write m0"); + for lvl in 1..levels { + let mut src = format!("include \"m{}\";\n", lvl - 1); + for i in 0..defs { + let calls: Vec = (0..fan_out) + .map(|k| format!("f{}_{}", lvl - 1, (i + k) % defs)) + .collect(); + src.push_str(&format!("def f{lvl}_{i}: {};\n", calls.join(" + "))); + } + std::fs::write(dir.join(format!("m{lvl}.jq")), src).expect("write module"); + } + } + + fn node_count(levels: usize) -> usize { + let dir = tempfile::tempdir().expect("tempdir"); + generate(dir.path(), levels, 4, 2); + let mut loader = ModuleLoader::new(&[dir.path().to_path_buf()]); + let filter = format!("include \"m{}\"; f{}_0", levels - 1, levels - 1); + let program = jq::parse_program(&filter).expect("parse"); + // The CLI loads through `unqualified_def_names` first, then + // `process_program`; mirror that so the memo is exercised the + // same way. + loader.unqualified_def_names(&program).expect("names"); + let expr = loader.process_program(&program).expect("process"); + let mut n = 0usize; + succinctly::jq::walk::any_subexpr(&expr, &mut |_| { + n += 1; + false + }); + n + } + + #[test] + fn fan_out_chain_grows_linearly_with_depth() { + let (n6, n8, n10) = (node_count(6), node_count(8), node_count(10)); + eprintln!("nodes at L = 6 / 8 / 10: {n6} / {n8} / {n10}"); + assert_eq!( + n8 - n6, + n10 - n8, + "per-level growth must be constant: {n6} / {n8} / {n10}" + ); + } + + /// The shadow-candidate seed (#2395) reads a module's own defs from + /// the cache, never a link run, so no hidden spelling can reach the + /// parser. + #[test] + fn unqualified_def_names_never_carry_a_link_spelling() { + let dir = tempfile::tempdir().expect("tempdir"); + generate(dir.path(), 4, 3, 2); + let mut loader = ModuleLoader::new(&[dir.path().to_path_buf()]); + let program = jq::parse_program("include \"m3\"; f3_0").expect("parse"); + let names = loader.unqualified_def_names(&program).expect("names"); + assert!(names.contains("f3_0")); + assert!(!names.contains("f2_0"), "a dependency is not re-exported"); + assert!( + names.iter().all(|n| !n.starts_with('\u{0}')), + "hidden spelling leaked: {names:?}" + ); + } + } + /// #1525: `seq_no_rs_byte_warning` direct unit coverage. Every expected /// value here was live-verified against the pinned jq 1.7.1 binary /// (see `tests/jq_cli_tests.rs`'s CLI-level `_1525` tests for the diff --git a/src/jq/resolve.rs b/src/jq/resolve.rs index 7f412504e..40a39ad6d 100644 --- a/src/jq/resolve.rs +++ b/src/jq/resolve.rs @@ -499,8 +499,8 @@ type Scope = Vec<(String, usize)>; /// # The rule /// /// The loader brackets every *run* of defs it wraps (each `include`, the -/// `~/.jq` block, each `import`, and each per-origin group of dependencies -/// wrapped into a body) between a begin and an end marker: +/// `~/.jq` block, each `import`, and each module linked once as some other +/// module's dependency, #2955) between a begin and an end marker: /// /// ```text /// def run:begin:[:]: .; ...the run's defs... def run:end:: .; @@ -515,6 +515,22 @@ type Scope = Vec<(String, usize)>; /// *around* the current point) and a begin marker with no matching end is the /// **floor** -- everything below it belongs to some other module and is /// invisible from here. +/// +/// ### Link runs (#2955) +/// +/// A module that another module depends on is emitted **once**, outermost, +/// as a run whose alias is [`Self::link_alias`] and whose defs carry the +/// qualified spelling [`Self::link_name`] -- `link:::` -- so +/// the run exports nothing a user can spell. Inside it, a bare sibling call +/// misses those names, floors at the run's own begin marker, and is retried +/// under the alias exactly as a bare call inside an `import`ed module is +/// (#2989); the retry renames the call in place, so the evaluator binds it +/// with no change of its own. A consumer reaches the run through a +/// forwarding stub the loader wraps into its body, `def g: link:::g;`, +/// and that lookup is the one kind that **crosses floors**: the stub sits +/// inside some run of its own, and its target is outside every run. Nothing +/// lexable can spell a link name, so only a stub or a retried sibling call +/// ever makes such a lookup. pub struct ModuleRun; /// One parsed marker name. @@ -533,10 +549,11 @@ pub enum RunMarker<'a> { const RUN_BEGIN: &str = "\u{0}run:begin:"; /// Sibling of [`RUN_BEGIN`]. const RUN_END: &str = "\u{0}run:end:"; -/// The prefix of a dependency renamed out of a def's way (#2962) -- see -/// [`ModuleRun::renamed_dep`]. Not a marker: [`ModuleRun::parse`] reads it as -/// an ordinary def, which is what it is. -const DEP_RENAME: &str = "\u{0}dep:"; +/// The prefix of a linked module's alias and of every def inside its run +/// (#2955) -- see [`ModuleRun::link_alias`] and [`ModuleRun::link_name`]. Not +/// a marker: [`ModuleRun::parse`] reads a link name as an ordinary def, which +/// is what it is. +const LINK: &str = "\u{0}link:"; impl ModuleRun { /// The name of the def that opens run `id`, carrying `alias` when the @@ -579,29 +596,39 @@ impl ModuleRun { .map(|id| RunMarker::End { id }) } - /// The name a dependency is renamed to when it would otherwise collide - /// with the def it is wrapped into (#2962): entry `index` of run `id`'s - /// dependency group, originally called `name`. - /// - /// A dependency bound into a def's body sits *inside* that def's scope, - /// so a dependency sharing the def's (name, arity), or named after one of - /// its parameters, would shadow the def's own binding for the body. jq - /// binds a module's block in its own scope and never has the clash. The - /// NUL prefix keeps the new name out of reach of anything a user can - /// write, exactly as it does for the markers. + /// The alias of the run that links module `id` once (#2955): what its + /// begin marker carries, so a bare sibling call inside the run is retried + /// as [`Self::link_name`] by the same code that retries a bare call inside + /// an `import`ed module (#2989). The NUL prefix keeps it out of reach of + /// anything a user can write, exactly as it does for the markers. + #[must_use] + pub fn link_alias(id: u32) -> String { + alloc::format!("{LINK}{id}") + } + + /// The name def `name` is emitted under inside module `id`'s link run, + /// and the name a consumer's forwarding stub calls: `::`, + /// the same shape an `import "m" as a;` gives `a::name`. #[must_use] - pub fn renamed_dep(id: u32, index: usize, name: &str) -> String { - alloc::format!("{DEP_RENAME}{id}:{index}:{name}") + pub fn link_name(id: u32, name: &str) -> String { + alloc::format!("{LINK}{id}::{name}") } - /// `name` as a user wrote it: [`Self::renamed_dep`] undone, and any other + /// Whether `name` is a [`Self::link_name`] -- the one lookup that is + /// allowed to cross a scope floor (see `scan_scope`). + #[must_use] + pub fn is_link_name(name: &str) -> bool { + name.starts_with(LINK) + } + + /// `name` as a user wrote it: [`Self::link_name`] undone, and any other /// name returned unchanged. For messages that name a def, so an internal /// spelling never reaches the terminal. #[must_use] pub fn display_name(name: &str) -> &str { - name.strip_prefix(DEP_RENAME) - .and_then(|rest| rest.splitn(3, ':').nth(2)) - .unwrap_or(name) + name.strip_prefix(LINK) + .and_then(|rest| rest.split_once("::")) + .map_or(name, |(_, written)| written) } } @@ -627,6 +654,15 @@ pub(crate) struct ScanResult { /// open run, which callers need for the `import` retry (#2989) and for /// attributing a compile error to the module it came from. /// +/// `crosses_floors` is the one exception (#2955): a lookup for a +/// [`ModuleRun::link_name`] steps over an open begin marker instead of +/// stopping at it. The name it wants lives in a link run wrapped outside +/// every module run, and the stub or retried sibling call asking for it sits +/// inside one; nothing lexable spells such a name, so no user-written call +/// can ride this exception across a boundary. Callers compute it from the +/// name they look up rather than deciding it themselves, so the two lookups +/// that can meet a link name ([`in_scope`], [`reach_in_scope`]) cannot drift. +/// /// Both walks in this file share it, rather than each spelling the marker /// bookkeeping out: the file's own header already flags the two as the pair /// that must stay in lock-step, and "which names are visible here" is exactly @@ -635,6 +671,7 @@ fn scan_scope<'a, E, T>( scope: &'a [E], name_of: impl Fn(&'a E) -> &'a str, mut probe: impl FnMut(&'a E) -> Option, + crosses_floors: bool, ) -> ScanResult { // Counts end markers seen but not yet matched by an opener. A run whose // end we have already passed is *closed*: it is wrapped around this @@ -646,7 +683,7 @@ fn scan_scope<'a, E, T>( Some(RunMarker::Begin { id, alias }) => { if closed > 0 { closed -= 1; - } else { + } else if !crosses_floors { // An open run: nothing below it is visible from inside // this module body, and it names who wrote the code here. return ScanResult { @@ -736,6 +773,7 @@ fn reach_in_scope(scope: &ReachScope, name: &str, arity: usize) -> ScanResult ScanResult<()> { scope, |(n, _)| n.as_str(), |(n, a)| (*a == arity && n == name).then_some(()), + ModuleRun::is_link_name(name), ) } @@ -1392,7 +1431,7 @@ fn in_scope(scope: &Scope, name: &str, arity: usize) -> ScanResult<()> { /// [`in_var_scope`] and [`in_label_scope`] both wrap, so a change to how /// name-stack lookups work has exactly one definition to update. fn in_name_scope(scope: &[String], name: &str) -> ScanResult<()> { - scan_scope(scope, String::as_str, |n| (n == name).then_some(())) + scan_scope(scope, String::as_str, |n| (n == name).then_some(()), false) } /// [`in_name_scope`] for `$name` against `var_scope`: a `$`-parameter of the @@ -1419,7 +1458,7 @@ fn in_label_scope(label_scope: &LabelScope, name: &str) -> ScanResult<()> { /// the same [`scan_scope`] walk with a probe that never matches, so it /// always runs to the floor (or the top) instead of stopping early. fn enclosing_run(label_scope: &LabelScope) -> Option { - scan_scope(label_scope, String::as_str, |_| None::<()>) + scan_scope(label_scope, String::as_str, |_| None::<()>, false) .run .map(|(id, _)| id) } @@ -3145,8 +3184,10 @@ mod tests { use super::*; /// Build a `Scope` from a compact spelling: `"begin:1"` / `"end:1"` / - /// `"begin:1@a"` are markers, anything else is a def of that name at - /// arity 0. + /// `"begin:1@a"` are markers, `"link:1"` opens module 1's link run + /// (a begin marker carrying its link alias, #2955), `"1::g"` is the + /// def `g` as emitted inside that run, and anything else is a def of + /// that name at arity 0. fn scope_of(entries: &[&str]) -> Scope { entries .iter() @@ -3160,6 +3201,11 @@ mod tests { } } else if let Some(id) = e.strip_prefix("end:") { ModuleRun::end_marker(id.parse().expect("id")) + } else if let Some(id) = e.strip_prefix("link:") { + let id = id.parse().expect("id"); + ModuleRun::begin_marker(id, Some(&ModuleRun::link_alias(id))) + } else if let Some((id, name)) = e.split_once("::") { + ModuleRun::link_name(id.parse().expect("id"), name) } else { (*e).to_string() }; @@ -3261,19 +3307,76 @@ mod tests { } } - /// #2962: a renamed dependency is an ordinary def to the scan, and - /// its display name is the one the user wrote -- including a name - /// that itself contains `:` (an `alias::name`). + /// #2955: a link name is an ordinary def to the scan, its display + /// name is the one the user wrote -- including a name that itself + /// contains `::` -- and a begin marker carrying a link alias parses + /// back to that alias (the `import` retry reads it from there). #[test] - fn renamed_deps_are_ordinary_defs_and_display_as_written() { - let renamed = ModuleRun::renamed_dep(4, 2, "c"); - assert_eq!(ModuleRun::parse(&renamed), None); - assert_eq!(ModuleRun::display_name(&renamed), "c"); - let renamed = ModuleRun::renamed_dep(4, 2, "ns::c"); - assert_eq!(ModuleRun::display_name(&renamed), "ns::c"); + fn link_names_are_ordinary_defs_and_display_as_written() { + let linked = ModuleRun::link_name(4, "c"); + assert_eq!(ModuleRun::parse(&linked), None); + assert!(ModuleRun::is_link_name(&linked)); + assert_eq!(ModuleRun::display_name(&linked), "c"); + assert_eq!( + ModuleRun::display_name(&ModuleRun::link_name(4, "ns::c")), + "ns::c" + ); for ordinary in ["c", "ns::c", ""] { + assert!(!ModuleRun::is_link_name(ordinary)); assert_eq!(ModuleRun::display_name(ordinary), ordinary); } + assert_eq!( + format!("{}::c", ModuleRun::link_alias(4)), + linked, + "the retry spells `alias::name`, so the two must compose" + ); + let alias = ModuleRun::link_alias(4); + assert_eq!( + ModuleRun::parse(&ModuleRun::begin_marker(4, Some(&alias))), + Some(RunMarker::Begin { + id: 4, + alias: Some(alias.as_str()) + }) + ); + } + + /// #2955: a consumer's forwarding stub sits inside its own module's + /// run and calls a def in a link run wrapped outside it. That one + /// lookup crosses the floor; a bare lookup from the same point still + /// does not, and the link run's defs are unreachable by their bare + /// names from anywhere. + #[test] + fn a_link_name_lookup_crosses_the_floor_and_nothing_else_does() { + // `link:2 { 2::g } end:2 { begin:1 { h, } }` + let chain = ["link:2", "2::g", "end:2", "begin:1", "h"]; + let target = ModuleRun::link_name(2, "g"); + assert!( + visible(&chain, &target), + "the stub's target, across run 1's floor" + ); + assert!(!visible(&chain, "g"), "never by its bare name"); + assert!(visible(&chain, "h")); + + // From the main filter, below every end marker: still not `g`. + let chain = ["link:2", "2::g", "end:2", "begin:1", "h", "end:1"]; + assert!(!visible(&chain, "g"), "a link run re-exports nothing"); + assert!(visible(&chain, "h")); + + // Inside the link run itself, a bare sibling call misses and + // reports the link alias, which is what the retry needs. + let scan = in_scope(&scope_of(&["link:2", "2::g", "2::k"]), "g", 0); + assert!(scan.hit.is_none()); + assert_eq!(scan.run, Some((2, Some(ModuleRun::link_alias(2))))); + assert!( + visible(&["link:2", "2::g", "2::k"], &target), + "...and the retry hits" + ); + + // A dependency of a dependency: the inner link run's stub crosses + // its own run's floor into the closed outer link run. + let chain = ["link:3", "3::z", "end:3", "link:2", "2::g"]; + assert!(visible(&chain, &ModuleRun::link_name(3, "z"))); + assert!(!visible(&chain, "z")); } } From 837698a2a709da5b5f975b68897d7ea64029b027 Mon Sep 17 00:00:00 2001 From: John Ky Date: Fri, 18 Sep 2026 18:30:34 +1000 Subject: [PATCH 2/5] test(jq): pin linked-module scoping, jq's cross-module error order and a chain size guard (#2955) - `link_size_guard_2955` (jq_runner.rs): counts the processed program's nodes for the issue's fan-out-2 chain at 6 / 8 / 10 levels and asserts a constant per-level delta; it failed against the copying loader (1253 / 5093 / 20453) and cannot pass by accident. A sibling test pins that `unqualified_def_names` never returns a link spelling. - `test_fan_out_module_chain_stays_linear_2955`: the issue's 8x6x3 and 12x4x2 shapes end to end, with jq's answers. - `test_linked_dependency_keeps_jq_scoping_2955`: ten rows captured from jq 1.7.1 -- a nested import's bare sibling call, an imported module's sibling reached bare that has its own dependency (the referenced closure must retry the bare call under the alias; found by review), one dependency under two includers, `$param` and closure params and `path()` through a stub, a parameter named like its def, two same-name defs, siblings plus a deeper dependency, recursion inside and through a dependency. - `test_dependency_errors_report_in_jq_order_2955`: a dependency's errors before its includer's, last-declared first, deepest first. - `test_dependency_error_is_reported_once_3058` replaces the pin of the once-per-copy count with jq's single report, for a variable, a call, and the include-plus-dependency shape. - `test_depth_guard_names_a_linked_def_as_written_2955`. - The `_2962` matrix's doc comment no longer describes a rename; its 30 rows are unchanged. --- tests/jq_cli_tests.rs | 376 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 347 insertions(+), 29 deletions(-) diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 097fbefd0..db45e0461 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -27482,8 +27482,11 @@ fn test_fold_alternation_pattern_writes_match_jq_2979() -> Result<()> { } /// #2962: a module's dependencies are bound in their own scope, as jq binds -/// a module's block, even though succinctly wraps them inside each def body -/// that reaches them. +/// a module's block. (Since #2955 that is literally so: a dependency module +/// is linked once, as a run of its own, and a def reaches it through a +/// forwarding stub -- the rows below were captured when the loader still +/// copied dependency bodies into each reaching def, and they must not +/// change.) /// /// Every row is captured whole from jq 1.7.1 -- stdout, stderr (with the /// module directory as ``) and exit code -- over the fixture files the @@ -27496,9 +27499,9 @@ fn test_fold_alternation_pattern_writes_match_jq_2979() -> Result<()> { /// - **A dependency sharing the def's (name, arity), or a parameter's name, /// was excluded**, stranding the other dependencies that call it (`R1`, /// `R8`, `R13`, `R24`, `R27`, `R28`, `R29`, `R31`, `R32`), or captured the -/// def's own binding (`R3`, `R25`). Such a dependency is now renamed, with -/// the calls that reach it. -/// - **The rename is scope-aware**: a nested def of the same name (`R21`), a +/// def's own binding (`R3`, `R25`). Such a name gets no stub, and the +/// other dependencies' calls to it are answered inside their own run. +/// - **Binding is scope-aware**: a nested def of the same name (`R21`), a /// parameter (`R22`, `R26`, `R30`), a dependency's own dependency (`R23`, /// and past its run's end marker `R33`), a later same-name entry (`R27`, /// `R28`) and declaration order (`R24`) each keep a call bound where jq @@ -27873,6 +27876,57 @@ fn test_transitive_include_chain_does_not_blow_up_2865() -> Result<()> { Ok(()) } +/// #2955: a module chain whose defs each call **more than one** def from +/// the level below stays linear in size. +/// +/// The `_2865` guard above has fan-out 1, which its referenced-closure +/// filter flattened; with fan-out `F` the copies compounded as `F^L` +/// regardless (the issue's 14 x 4 x 2 chain: 680 MB peak RSS against jq's +/// 2.5 MB, in release). Linking each dependency module once makes the +/// processed program linear -- the unit test `link_size_guard_2955` in +/// `jq_runner.rs` counts the nodes -- and this is the end-to-end row: the +/// issue's own two shapes, completing with jq's answers (both captured +/// live). Modest on purpose: evaluation itself still visits `F^L` calls, +/// as jq does, so a wall-clock bound would flake on a loaded CI box. +#[test] +fn test_fan_out_module_chain_stays_linear_2955() -> Result<()> { + fn chain(levels: usize, defs: usize, fan_out: usize) -> Vec<(String, String)> { + let mut modules = Vec::new(); + let mut base = String::new(); + for i in 0..defs { + base.push_str(&format!( + "def f0_{i}: {i}; +" + )); + } + modules.push(("m0".to_string(), base)); + for level in 1..levels { + let mut contents = format!("include \"m{}\";\n", level - 1); + for i in 0..defs { + let calls: Vec = (0..fan_out) + .map(|k| format!("f{}_{}", level - 1, (i + k) % defs)) + .collect(); + contents.push_str(&format!("def f{level}_{i}: {};\n", calls.join(" + "))); + } + modules.push((format!("m{level}"), contents)); + } + modules + } + + for (levels, defs, fan_out, want) in [(8, 6, 3, "5211"), (12, 4, 2, "3072")] { + let modules = chain(levels, defs, fan_out); + let borrowed: Vec<(&str, &str)> = modules + .iter() + .map(|(name, contents)| (name.as_str(), contents.as_str())) + .collect(); + let filter = format!(r#"include "m{}"; f{}_0"#, levels - 1, levels - 1); + let (stdout, stderr, code) = run_jq_with_modules(&borrowed, &["-nc", &filter])?; + assert_eq!(code, 0, "{levels}x{defs}x{fan_out}: stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), want, "{levels}x{defs}x{fan_out}"); + } + Ok(()) +} + /// #1376: `succinctly jq` now supports arity overloading, matching real /// jq -- `def f(x): ...` and `def f(x;y): ...` are distinct functions /// (`f/1` and `f/2`), and both stay callable after the second definition. @@ -46531,37 +46585,301 @@ fn test_unbound_variable_from_included_module_names_its_own_line_2962() -> Resul Ok(()) } -/// #2962 / #3058: a dependency copied into two reached defs reports its -/// unbound variable once per copy. jq reports it once (`jq: 1 compile -/// error`), so the count is #3058's divergence, shared with calls. What this -/// pins is the second report's form: the site table has no second -/// occurrence, and the reporter names the file alone rather than searching -/// the text for a coincidental `$nosuch` -- here, the one in a string. +/// #3058 (closed by #2955): a compile error inside a dependency reached +/// from two defs is reported **once**, as jq reports it -- the dependency is +/// linked once and its body checked once, where the copying loader checked +/// one copy per reaching def and printed `jq: 2 compile errors`, the second +/// without a line (its site table had no second occurrence). Three shapes, +/// each captured whole from jq 1.7.1: an unbound variable, an unresolved +/// call, and the module reached both as a top-level `include` and as a +/// dependency (one report, because the top-level copy's body is never +/// reached and so never checked, #2740). The residual -- the main filter +/// *also* calling the top-level copy directly -- still reports twice; it is +/// recorded in `docs/compliance/jq/limitations.md`. +#[test] +fn test_dependency_error_is_reported_once_3058() -> Result<()> { + let rows: &[ModuleRow] = &[ + ( + "variable", + &[ + ("dep", "def g: $nosuch;\ndef s: \"$nosuch\";\n"), + ("mid", "include \"dep\"; def a: g; def b: g;\n"), + ], + r#"include "mid"; a, b"#, + "jq: error: $nosuch is not defined at /dep.jq, line 1:\ndef g: $nosuch; \njq: 1 compile error\n", + ), + ( + "call", + &[ + ("depc", "def bad: nosuch;\n"), + ("midc", "include \"depc\"; def h: bad; def k: bad;\n"), + ], + r#"include "midc"; [h, k]"#, + "jq: error: nosuch/0 is not defined at /depc.jq, line 1:\ndef bad: nosuch; \njq: 1 compile error\n", + ), + ( + "include-and-dependency", + &[ + ("depc", "def bad: nosuch;\n"), + ("midc", "include \"depc\"; def h: bad; def k: bad;\n"), + ], + r#"include "depc"; include "midc"; [h, k]"#, + "jq: error: nosuch/0 is not defined at /depc.jq, line 1:\ndef bad: nosuch; \njq: 1 compile error\n", + ), + ]; + run_module_rows(rows, &[]) +} + +/// One `(id, modules, filter, expected)` row of a module matrix: the +/// `(name, contents)` pairs to write as `.jq`, the filter to run over +/// them, and the one stream the row asserts on. +type ModuleRow<'a> = (&'a str, &'a [(&'a str, &'a str)], &'a str, &'a str); + +/// Run each `(id, modules, filter, want_stderr)` row of a compile-error +/// matrix: write the modules, run `succinctly jq -L -nc +/// `, and assert empty stdout, exit 3 and `want_stderr` byte for +/// byte with `` standing for the canonical module directory. +fn run_module_rows(rows: &[ModuleRow], extra: &[&str]) -> Result<()> { + for (id, modules, filter, want_stderr) in rows { + let temp_dir = tempfile::tempdir()?; + for (name, contents) in *modules { + std::fs::write(temp_dir.path().join(format!("{name}.jq")), contents)?; + } + let dir = std::fs::canonicalize(temp_dir.path())?; + let (output, code) = spawn_with_signal_retry( + || { + let mut command = Command::new(succinctly_bin()); + command + .args(["jq", "-L"]) + .arg(temp_dir.path()) + .args(["-nc"]) + .args(extra) + .arg(filter); + command + }, + None, + )?; + let stdout = String::from_utf8(output.stdout)?; + let stderr = String::from_utf8(output.stderr)?; + let want_stderr = want_stderr.replace("", &dir.to_string_lossy()); + assert_eq!( + (stdout.as_str(), stderr.as_str(), code), + ("", want_stderr.as_str(), 3), + "{id}: {filter}" + ); + } + Ok(()) +} + +/// #2955: compile errors across a module chain come out in jq 1.7.1's order +/// -- a dependency's before its includer's, the last-declared dependency's +/// first, and a chain's deepest module first. All three shapes captured +/// whole from the pinned binary. +/// +/// That order is not chosen by the resolver: it is the order the linked +/// runs are wrapped in (`ModuleLoader::hoist_order`), outermost first, and +/// `resolve::check` walks the chain from the outside in. The copying loader +/// printed the includer's own error first, then its dependencies in +/// declaration order (`A G K` for the first row, against jq's `K G A`). +#[test] +fn test_dependency_errors_report_in_jq_order_2955() -> Result<()> { + let rows: &[ModuleRow] = &[ + ( + "two dependencies, last declared first", + &[ + ("ig", "def g: nosuchG;\n"), + ("ik", "def k: nosuchK;\n"), + ("mid3", "include \"ig\"; include \"ik\"; def a: nosuchA; def h: [k, g];\n"), + ], + r#"include "mid3"; [a, h]"#, + "jq: error: nosuchK/0 is not defined at /ik.jq, line 1:\ndef k: nosuchK; \njq: error: nosuchG/0 is not defined at /ig.jq, line 1:\ndef g: nosuchG; \njq: error: nosuchA/0 is not defined at /mid3.jq, line 1:\ninclude \"ig\"; include \"ik\"; def a: nosuchA; def h: [k, g]; \njq: 3 compile errors\n", + ), + ( + "the same two, declared the other way round", + &[ + ("ig", "def g: nosuchG;\n"), + ("ik", "def k: nosuchK;\n"), + ("mid3r", "include \"ik\"; include \"ig\"; def a: nosuchA; def h: [k, g];\n"), + ], + r#"include "mid3r"; [a, h]"#, + "jq: error: nosuchG/0 is not defined at /ig.jq, line 1:\ndef g: nosuchG; \njq: error: nosuchK/0 is not defined at /ik.jq, line 1:\ndef k: nosuchK; \njq: error: nosuchA/0 is not defined at /mid3r.jq, line 1:\ninclude \"ik\"; include \"ig\"; def a: nosuchA; def h: [k, g]; \njq: 3 compile errors\n", + ), + ( + "a chain, deepest first", + &[ + ("l0", "def z: nosuchZ;\n"), + ("l1", "include \"l0\"; def y: [z, nosuchY];\n"), + ("l2", "include \"l1\"; def x: [y, nosuchX];\n"), + ], + r#"include "l2"; x"#, + "jq: error: nosuchZ/0 is not defined at /l0.jq, line 1:\ndef z: nosuchZ; \njq: error: nosuchY/0 is not defined at /l1.jq, line 1:\ninclude \"l0\"; def y: [z, nosuchY]; \njq: error: nosuchX/0 is not defined at /l2.jq, line 1:\ninclude \"l1\"; def x: [y, nosuchX]; \njq: 3 compile errors\n", + ), + ]; + run_module_rows(rows, &[]) +} + +/// #2955: a dependency module is linked **once** and reached through +/// forwarding stubs, and every scoping row jq answers still comes out the +/// same. Each row captured live from jq 1.7.1 (`stdout`, exit 0). +/// +/// - a dependency `import`ed inside a module, whose def calls a sibling by +/// its bare name (#2989's residual one level down: the copying loader +/// dropped the sibling and reported `g/0 is not defined`); +/// - a top-level `import`ed module whose def is reached only through a +/// sibling's bare call, and itself depends on a third module (the +/// referenced closure must retry the bare call under the alias, as the +/// resolver does; found by review of this change); +/// - one dependency reached through two different including modules (one +/// link run, both stubs); +/// - a `$param` and a closure param forwarded through a stub; +/// - `path(...)` through a stub; +/// - a dependency def with a parameter named like itself, called through a +/// stub of the same shape; +/// - two same-name defs in one dependency: the later one is what its +/// sibling and its consumer both see; +/// - a dependency's siblings calling each other and a deeper dependency; +/// - a self-recursive dependency def called through a stub, at a depth a +/// copied body also reached; +/// - a consumer's own recursive def calling a dependency whose body names +/// the consumer's own name (the #2962 family: the dependency's `c` is its +/// own sibling, never the consumer's). +/// +/// Non-re-export is pinned with a compile error in +/// `test_transitive_include_does_not_leak_to_the_includer_2865` and the +/// `_2962` matrix above; nothing here re-tests it. +#[test] +fn test_linked_dependency_keeps_jq_scoping_2955() -> Result<()> { + let rows: &[ModuleRow] = &[ + ( + "nested import, bare sibling call", + &[ + ("inner3", "def g: 42; def k: [g];\n"), + ("impk", "import \"inner3\" as i; def h: i::k;\n"), + ], + r#"include "impk"; h"#, + "[42]\n", + ), + ( + "imported module's sibling reached bare, with its own dependency", + &[ + ("dep", "def d: 1;\n"), + ("m", "include \"dep\"; def g: d; def k: g;\n"), + ], + r#"import "m" as ns; ns::k"#, + "1\n", + ), + ( + "one dependency, two includers", + &[ + ("dep", "def d: 1;\n"), + ("a", "include \"dep\"; def fa: d + 1;\n"), + ("b", "include \"dep\"; def fb: d + 2;\n"), + ], + r#"include "a"; include "b"; [fa, fb]"#, + "[2,3]\n", + ), + ( + "$param and closure param through a stub", + &[ + ("dep", "def g($x; f): [$x, f];\n"), + ("m", "include \"dep\"; def h: g(1; 3,4);\n"), + ], + r#"include "m"; [h]"#, + "[[1,3,4]]\n", + ), + ( + "path() through a stub", + &[ + ("dep", "def p: .a.b;\n"), + ("m", "include \"dep\"; def q: path(p);\n"), + ], + r#"include "m"; {a:{b:1}} | q"#, + "[\"a\",\"b\"]\n", + ), + ( + "parameter named like the def", + &[ + ("dep", "def g(g): g;\n"), + ("m", "include \"dep\"; def h: g(7);\n"), + ], + r#"include "m"; h"#, + "7\n", + ), + ( + "same name twice in the dependency", + &[ + ("dep", "def c: 7; def c: 8; def g: c;\n"), + ("m", "include \"dep\"; def h: [g, c];\n"), + ], + r#"include "m"; h"#, + "[8,8]\n", + ), + ( + "siblings and a deeper dependency", + &[ + ("l0", "def z: 3;\n"), + ("l1", "include \"l0\"; def y: z + 1; def y2: y * 2;\n"), + ("l2", "include \"l1\"; def x: [y, y2];\n"), + ], + r#"include "l2"; x"#, + "[4,8]\n", + ), + ( + "recursion inside the dependency", + &[ + ( + "dep", + "def down(n): if n == 0 then \"bottom\" else down(n - 1) end;\n", + ), + ("m", "include \"dep\"; def h: down(3000);\n"), + ], + r#"include "m"; h"#, + "\"bottom\"\n", + ), + ( + "consumer recursion through a dependency that names the consumer's name", + &[ + ("dep", "def c: 7; def g: c;\n"), + ( + "mid", + "include \"dep\"; def c: if . == 0 then g else (. - 1 | c) end;\n", + ), + ], + r#"include "mid"; 3 | c"#, + "7\n", + ), + ]; + for (id, modules, filter, want_stdout) in rows { + let (stdout, stderr, code) = run_jq_with_modules(modules, &["-nc", filter])?; + assert_eq!( + (stdout.as_str(), stderr.as_str(), code), + (*want_stdout, "", 0), + "{id}: {filter}" + ); + } + Ok(()) +} + +/// #2955: the recursion-depth guard names a linked def as the user wrote +/// it, never by its link spelling (`ModuleRun::display_name`). Real jq has +/// no such guard (it recurses until the process dies), so the message is +/// succinctly's own; what is pinned is the name in it. #[test] -fn test_unbound_variable_in_a_twice_copied_dependency_3058() -> Result<()> { +fn test_depth_guard_names_a_linked_def_as_written_2955() -> Result<()> { let (stdout, stderr, code) = run_jq_with_modules( &[ - ("dep", "def g: $nosuch;\ndef s: \"$nosuch\";\n"), - ("mid", "include \"dep\"; def a: g; def b: g;\n"), + ("dep", "def loop: 1 + loop;\n"), + ("m", "include \"dep\"; def h: loop;\n"), ], - &["-nc", r#"include "mid"; a, b"#], + &["-nc", r#"include "m"; h"#], )?; - assert_eq!(code, 3, "stderr: {stderr:?}"); + assert_eq!(code, 5, "stderr: {stderr:?}"); assert_eq!(stdout, ""); - let lines: Vec<&str> = stderr.lines().collect(); - assert_eq!(lines.len(), 4, "stderr: {stderr:?}"); - assert!( - lines[0].starts_with("jq: error: $nosuch is not defined at ") - && lines[0].ends_with("dep.jq, line 1:"), - "stderr: {stderr:?}" - ); - assert_eq!(lines[1], "def g: $nosuch; "); - assert!( - lines[2].starts_with("jq: error: $nosuch is not defined at ") - && lines[2].ends_with("dep.jq"), - "the second copy names the file only, never line 2's string: {stderr:?}" + assert_eq!( + stderr, + "jq: error (at ): loop/0 exceeded maximum recursion depth\n" ); - assert_eq!(lines[3], "jq: 2 compile errors"); + assert!(!stderr.contains('\u{0}'), "a link name leaked: {stderr:?}"); Ok(()) } From 38b67ca8d4f6bfbc1f9cea0276544193d4ba0953 Mon Sep 17 00:00:00 2001 From: John Ky Date: Fri, 18 Sep 2026 18:30:34 +1000 Subject: [PATCH 3/5] docs(jq): record #2955's linking amendment, its measured costs, and the #3058 residual ADR-0023 gains an "Amendment (#2955, implemented)" section -- one link run per dependency module with a hidden alias, forwarding stubs, the floor-crossing lookup, jq's error order from placement, the referenced closure, and the soundness argument re-derived from decision 5 -- and marks the #2962 rename superseded and the "symbolic binding" alternative taken. `Expr::Shared` splicing is recorded as rejected. limitations.md closes "Deeply chained modules compound in memory" with the before/after table, closes #3058 with its include-plus-dependency residual, rewrites the #2962 section's mechanism, fixes its row count to 30, and records what remains: the evaluator's own per-call bound-body retention (the same 56 defs in one file cost 58 MB, `fib(20)` 234 MB, `fib(21)` overflows the stack) and the wide-chain startup cost of a long top-level chain (a 20x50 chain with every top-level def used: 31 ms -> 137 ms on an M4 Pro; one def used: 21 ms -> 9 ms). jq-language.md's module paragraph describes the new binding. --- docs/adrs/adr-0023.md | 95 ++++++++++++++++++- docs/compliance/jq/limitations.md | 146 +++++++++++++++++++----------- docs/reference/jq-language.md | 10 +- 3 files changed, 191 insertions(+), 60 deletions(-) diff --git a/docs/adrs/adr-0023.md b/docs/adrs/adr-0023.md index 966952fd1..b1e52ac7c 100644 --- a/docs/adrs/adr-0023.md +++ b/docs/adrs/adr-0023.md @@ -127,6 +127,84 @@ and the rename only moves an entry that the floor has already made private to it of one def's way. `ModuleRun::display_name` maps the internal name back wherever a def's name reaches a message (the recursion-depth error), so it never surfaces. +*Points 2 and 3 are superseded by the #2955 amendment below: dependency bodies are no +longer wrapped into the defs that reach them, so there is no clash to rename. Point 1 stands.* + +### Amendment (#2955, implemented): a dependency module is linked once, and forwarded to + +Binding a module's dependencies by **copying** their bodies into every def that reached them +compounded down a chain: each level's bodies already carried the level below, so a chain +whose defs each call `F` defs of the level below held `F^L` copies of the bottom one. The +issue's 14-level, fan-out-2 chain peaked at 681 MB against jq's 2 MB (release, correct +output throughout). The "symbolic binding at load time" listed under the alternatives below +is now taken, in a form that changes neither the marker representation nor the evaluator: + +1. **One link run per dependency module.** A module some other module depends on is emitted + **once**, as an ordinary run (`wrap_run`) placed outermost in `process_program` -- outside + the imports, `~/.jq` and includes -- whose begin marker carries a hidden alias + (`ModuleRun::link_alias`, `link:`) and whose defs are named + `ModuleRun::link_name` (`link:::`). Inside the run a bare sibling call + misses those names, floors at the run's own marker, and is retried under the alias by the + very code decision 3 added for `import`ed modules; the retry renames the call in place, so + the evaluator binds it unchanged. From outside, nothing lexable spells a link name, so the + run re-exports nothing. Error attribution reads the innermost open run as before, so a body + error names the dependency's own file -- and, since the body now exists once, it is + reported once (#3058, closed by this). +2. **Forwarding stubs in place of copies.** `load_and_bind_module` wraps each of a module's + own def bodies in one stub per dependency (name, arity) the body calls directly -- + `def g(a; b): link:::g(a; b);` -- with the two exclusions the copying loader had + (the def's own (name, arity), and a parameter's name at arity 0). A stub holds no lexable + free name, no `$variable` and no `$__loc__`, so it needs no run and can capture nothing; + the transitive closure and the rename above are therefore gone. Arguments forward as + closures, and the dependency does its own `$param` binding on arrival. +3. **The one resolver change.** `scan_scope` steps over an open begin marker -- instead of + stopping at it as the floor -- exactly when the name being looked up is a link name. A stub + sits inside some run of its own and its target is outside every run. The two lookups that + can meet a link name (`in_scope`, `reach_in_scope`) compute the flag from the name; variable, + label and `enclosing_run` scans never cross. +4. **Placement decides compile-error order.** Link runs are wrapped in dependency post-order + with each module's directives visited last-declared first (`ModuleLoader::hoist_order`), so + `resolve::check` meets a dependency's bodies before its includer's, the last-declared + dependency's first, and a chain's deepest first -- which is jq 1.7.1's order (captured live + for all three shapes; the copying loader printed the includer's own errors first). +5. **Only what is referenced is linked.** jq's `block_bind_referenced` rule at the module + level: a linked module emits a def only if a body reached by name from the main filter -- + through the top-level runs, then the linked modules dependents-first -- names it. Every + chain def is installed over the whole program below it at evaluation, so chain length is + the cost that matters; seeding from every top-level def instead cost 22 MB against 12 MB on + a six-level, forty-def chain the filter walks one def of. + +**Soundness, re-derived from decision 5.** A link run contains no bare-named def, so a bare +name the resolver accepts anywhere resolved to a def inner to every link run, which the +evaluator's innermost-first install also picks first. A link name exists only in its own run, +and same-name entries there are innermost-first in both passes. There is no input on which +the two pick different defs. The 30 rows of `test_dependencies_are_bound_in_their_own_scope_2962` +are unchanged. + +**Measured** (this machine, release, `/usr/bin/time -l`, output identical to jq on every row): +the 8 x 6 x 3 chain 142 MB -> 34 MB, 12 x 4 x 2 162 MB -> 36 MB, 14 x 4 x 2 681 MB -> 112 MB +and 0.38 s -> 0.03 s; the flat 6 x 40 x 1 chain 12 MB -> 9 MB, and a 1000-def utility module +used for one function through two modules 9 MB -> 9 MB. The processed program's node count is now linear in chain depth +(`link_size_guard_2955`, which read 1253 / 5093 / 20453 nodes at 6 / 8 / 10 levels before). +What remains above jq's 2 MB is the evaluator's own: a `DefCall` node caches its bound body, +and a call tree of `F^L` calls leaves `F^L` cached copies behind, module or no module -- the +same 56 defs in one file cost 58 MB, and `fib(20)` 234 MB. That is a separate, pre-existing +evaluator issue, #3148. + +**Hot path** (Apple M4 Pro, idle, interleaved, 25 reps, median): `[range(1e5) | f]` where `f` +calls one dependency through a stub moved +1.6% while the same defs written inline in one +program, which this change cannot touch, moved +3.3% -- neutral within the noise floor. + +**The one shape that pays** (same box and method): a 20-module x 50-def chain of which the +filter uses one def starts in 9 ms against 20 ms before; the same chain with all 50 top-level +defs used, so that all 950 dependency defs are referenced and linked, starts in 136 ms against +32 ms. Every def in the top-level chain is installed over the +whole program below it when it is bound (`bind_def` rebuilds its `then`), so a chain of `M` +defs costs `O(M x N)` at startup; the copying loader kept those 950 bodies nested inside the +defs that used them, where each install was local. That quadratic is the evaluator's, and +pre-existing -- a single top-level `include` of 3000 defs costs 1 GB and 0.5 s today -- and it +is what the referenced closure above bounds by usage. It is filed with the retention issue, #3148. + ## Alternatives rejected - **A field on `Expr::FuncDef`.** Adding any field costs 8 bytes on *every* `Expr` — the @@ -146,7 +224,15 @@ name reaches a message (the recursion-depth error), so it never surfaces. than inlining an AST). This is the eventual answer to #2955's memory compounding and would make the boundary structural rather than marked. It is a far larger change to the loader and evaluator both, and is deliberately left as future work: nothing in this ADR - blocks it, and the markers disappear with the chain if it ever lands. + blocks it, and the markers disappear with the chain if it ever lands. *Taken by the #2955 + amendment above, in a form that keeps the markers and leaves the evaluator untouched: the + handle is a hidden name, and the binding is the alias retry the resolver already had.* +- **Splicing dependency bodies behind `Expr::Shared`** (#2955's own first suggestion). + Unsound today on two counts: `resolve::check` un-shares a multi-owner `Rc` through + `Rc::make_mut` (#2971), so the memory comes straight back at compile time, and a shared + body's free names would resolve against whichever scope each splice site sits in, which is + #2962's capture family again. Linking once and forwarding shares nothing and resolves each + body in exactly one scope. ## Consequences @@ -180,6 +266,11 @@ name reaches a message (the recursion-depth error), so it never surfaces. - [#2962](https://github.com/rust-works/succinctly/issues/2962) — dependency capture; closed by the amendment above - [#2950](https://github.com/rust-works/succinctly/issues/2950) — shadow-candidate seeding -- [#2955](https://github.com/rust-works/succinctly/issues/2955) — memory compounding +- [#2955](https://github.com/rust-works/succinctly/issues/2955) — memory compounding; + closed by the amendment above +- [#3058](https://github.com/rust-works/succinctly/issues/3058) — a dependency's compile + error reported once per copy; closed by the same amendment +- [#3148](https://github.com/rust-works/succinctly/issues/3148) — the evaluator's per-call + bound-body retention and chain install cost, which is what remains - [ADR-0018](adr-0018.md) — reference fidelity; this is conformance to its per-mode rule, not an amendment to it diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index ff39af7bd..04e5d98b0 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -7353,7 +7353,7 @@ four shapes (two-module cycle, self-include, aliased spelling, `import`-side cyc ### Seven module-scoping rules that *are* matched, and read as bugs (#2865) Not divergences — recorded here because the next person to touch `ModuleLoader` will -otherwise read them as ones, and because the exclusions in `visible_deps_for` have no +otherwise read them as ones, and because the exclusions in `dep_stubs_for` have no other explanation. All captured live against jq 1.7.1, with `inner.jq` = `def g: 42;`: 1. **A dependency outranks the module's own same-name sibling.** With the module written @@ -7380,50 +7380,69 @@ other explanation. All captured live against jq 1.7.1, with `inner.jq` = `def g: 7. **...including when the name is defined nowhere.** `def a: b; def h(b): a;` is `b/0 is not defined`, exit 3, not something `h`'s parameter can satisfy. -Rule 4 is why an exported def's body is *not* wrapped in a dependency matching its own -(name, arity), and rule 5 is why it is not wrapped in one matching any of its parameters — -both scoped to a name the body calls *directly*, since a dependency reached only through -another one is bound where that one was written and is not the def's own to shadow. -Rules 6 and 7 are why a module's own defs are not wrapped into each other at all: they are -emitted as siblings in the top-level chain, exactly as a filter's own defs are, and jq's -lexical rule relates them there. Nesting a copy of one inside another's body puts it under -scopes it was never written in, and rule 6's third row shows sealing cannot repair that — -a call to a builtin is free in every pre-bound form of the copy. -Rules 1-3 are why the wrap goes around each exported def's **body** rather than being -spliced into the module's exported chain. - -One consequence worth stating, since it is the reason an exported body carries its -module's own earlier siblings as well as its dependencies: a def handed to another module -has to be **self-contained**. With `inner.jq` = `def g: 42; def k: g;` and `outer.jq` = +Rule 4 is why an exported def's body gets *no* forwarding stub for a dependency matching +its own (name, arity), and rule 5 is why it gets none for one matching any of its +parameters — both scoped to a name the body calls *directly*, since a dependency reached +only through another one is bound where that one was written (inside its own linked run, +since #2955) and is not the def's own to shadow. Rules 6 and 7 are why a module's own defs +are never nested inside each other: they are emitted as siblings in the chain they are +exported into — a top-level run, or the module's linked run — exactly as a filter's own +defs are, and jq's lexical rule relates them there. Nesting a copy of one inside another's +body puts it under scopes it was never written in, and rule 6's third row shows sealing +cannot repair that — a call to a builtin is free in every pre-bound form of the copy. +Rules 1-3 are why the stubs go around each exported def's **body** rather than the +dependency being spliced into the module's exported chain. + +One consequence worth stating, since it is the reason a dependency's body is resolved +inside its own module's run and nowhere else: a def handed to another module has to keep +its **own** bindings. With `inner.jq` = `def g: 42; def k: g;` and `outer.jq` = `include "inner"; def g: k;`, jq answers `42` — `k`'s `g` is `inner`'s. Leaving `k`'s `g` -to resolve outward into wherever `k` gets spliced would instead find `outer`'s own `g`, +to resolve outward into wherever `k` gets called would instead find `outer`'s own `g`, which is `k`. -### Deeply chained modules compound in memory (#2955) - -Binding copies the AST, so a chain of modules whose defs each call **more than one** def -from the level below grows exponentially with depth. jq binds symbolically and shares its -blocks, so it does not. Measured at #2865's own head (Apple M-series, release): - -| chain | succinctly peak RSS | jq peak RSS | -|----------------------------------|---------------------|-------------| -| 6 levels x 40 defs, 1 call each | 10 MB | 2.5 MB | -| 8 levels x 6 defs, 3 calls each | 91 MB | 2.6 MB | -| 14 levels x 4 defs, 2 calls each | 361 MB | 2.6 MB | - -The referenced-closure filter (jq's own `block_bind_referenced` rule, in -`visible_deps_for`) flattens the one-call-each shape completely — without it the -6 x 40 row was 359 MB rather than 10 MB — but it cannot flatten a genuinely wide closure, -because that closure is itself exponential. Every row above produces the **correct** -answer; this is a scalability limit of AST inlining, not a wrong result. It needs a -*chain of modules* to appear, and a wide one: only dependencies are wrapped into a body, -so a module with no `include`/`import` is bound exactly as cheaply as before #2865 -however many of its own defs call each other (a 24-def Fibonacci module is 9 MB, against -`main`'s 9 MB), and a chain whose defs call one def apiece stays flat (21 levels of a -two-def module is 9 MB; a 7-level 40-def chain is 12 MB). Tracked as -[#2955](https://github.com/rust-works/succinctly/issues/2955), whose most promising fix is -splicing bound bodies by handle (the `Rc`-shaded opaque sub-expression #1371 already -introduced) instead of by clone. +### Deeply chained modules compounded in memory — closed (#2955) + +Binding a module's dependencies by **copying** their bodies into every def that reached +them compounded down a chain: each level's bodies already carried the level below, so a +chain whose defs each call `F` defs from the level below held `F^L` copies of the bottom +one. jq binds symbolically and shares its blocks. Closed by linking each dependency module +**once**, as a run of its own with a hidden alias, and reaching it through forwarding stubs +— [ADR-0023](../../adrs/adr-0023.md)'s #2955 amendment. Measured before and after (Apple +M-series, release, `/usr/bin/time -l`, the issue's own generator, output identical to jq +on every row): + +| chain | before | after | jq | +|----------------------------------|--------|--------|--------| +| 6 levels x 40 defs, 1 call each | 12 MB | 9 MB | 2 MB | +| 8 levels x 6 defs, 3 calls each | 142 MB | 34 MB | 2 MB | +| 12 levels x 4 defs, 2 calls each | 162 MB | 36 MB | 2 MB | +| 14 levels x 4 defs, 2 calls each | 681 MB | 112 MB | 2 MB | + +The 14-level row also went from 0.38 s to 0.03 s. The processed program is now linear in +chain depth — `link_size_guard_2955` (`src/bin/succinctly/jq_runner.rs`) counts its nodes +at 6, 8 and 10 levels and asserts a constant per-level delta; against the copying loader it +read 1253 / 5093 / 20453. `test_fan_out_module_chain_stays_linear_2955` +(`tests/jq_cli_tests.rs`) runs the issue's two shapes end to end. + +What remains above jq's 2 MB is the **evaluator's**, not the loader's, and it needs no +module to appear: a `DefCall` node caches its bound body, so a call tree of `F^L` calls +leaves `F^L` cached copies behind for the program's lifetime. The same 56 defs written in +one file cost 58 MB, `def fib(n): if n < 2 then n else fib(n-1) + fib(n-2) end; fib(20)` +costs 234 MB against jq's 2 MB, and `fib(21)` overflows the stack. Filed as +[#3148](https://github.com/rust-works/succinctly/issues/3148). + +**One shape pays for the linking:** a wide chain of which the filter uses *everything*. With +20 modules of 50 defs each including the one below, `include "s19"; s19_0` starts in 9 ms +(20 ms before), but a filter naming all 50 top-level defs links all 950 dependency defs into +the top-level chain and starts in 136 ms (32 ms before; Apple M4 Pro, idle, interleaved +medians of 25 reps). A module-heavy loop, `[range(1e5) | f]` with `f` calling one +dependency through a stub, is neutral within noise (+1.6%, against +3.3% drift on the same +defs written inline). +Each chain def is installed over the whole program below it when bound, so the chain's +length is quadratic at startup -- the same pre-existing evaluator cost a single 3000-def +`include` pays today (1 GB, 0.5 s; also #3148) -- where the copying loader had kept those +bodies nested inside the defs that used them. Only the defs the filter reaches are linked, which is what +keeps the common shapes at or below their old cost. ### A wrapped dependency sits inside the including def's scope — closed (#2962) @@ -7440,22 +7459,41 @@ against jq 1.7.1, with what `succinctly jq` answered before #2962: | `gb` = `def g: b;`, `hb` = `include "gb"; def h(b): g; def q: h(99);`, then `q` | `b/0 is not defined`, exit 3 | `99`, exit 0 | | `inner3` = `def g: 42; def k: [g];`, `h3` = `include "inner3"; def h($g): [g, k]; def q: h(7);`, then `q` | `[7,[42]]` | `[7,[7]]` | -**Closed** by wrapping each dependency group as a flooring run (the #2951 marker, so a -dependency body sees nothing of the def it is wrapped into, for `$variables` as well as -calls), and by **renaming** a dependency that shares the def's (name, arity) or a -parameter's name, rather than excluding it. The exclusion was what produced the first and -third rows: it kept the def's own binding for the body but stranded the other dependency -that called the excluded one. [ADR-0023](../../adrs/adr-0023.md)'s #2962 amendment records -the mechanism. `test_dependencies_are_bound_in_their_own_scope_2962` -(`tests/jq_cli_tests.rs`) pins 29 rows, each byte for byte against jq 1.7.1 (stdout, stderr -and exit code): these three, the scope leaks the floor closes (a parameter, a sibling -origin's group, a `$`-parameter), and the cases the rename must respect (a nested def or -parameter of the same name, a dependency's own dependency, a later same-name entry, and -declaration order). +**Closed** first by wrapping each dependency group as a flooring run (the #2951 marker, so +a dependency body sees nothing of the def it is wrapped into, for `$variables` as well as +calls) with a clashing dependency **renamed** rather than excluded, and since #2955 by not +wrapping dependency bodies into defs at all: a dependency module is linked once, in its own +run, and a def reaches it through forwarding stubs that carry no free name to capture. The +exclusion was what produced the first and third rows: it kept the def's own binding for the +body but stranded the other dependency that called the excluded one; under linking, that +other dependency's call is answered inside its own module. +[ADR-0023](../../adrs/adr-0023.md)'s #2962 and #2955 amendments record both mechanisms. +`test_dependencies_are_bound_in_their_own_scope_2962` (`tests/jq_cli_tests.rs`) pins 30 +rows, each byte for byte against jq 1.7.1 (stdout, stderr and exit code): these three, the +scope leaks the floor closes (a parameter, a sibling origin's group, a `$`-parameter), and +the cases binding must respect (a nested def or parameter of the same name, a dependency's +own dependency, a later same-name entry, and declaration order). A module body's unbound `$variable` is now also reported at the module's own file, line and source, as jq reports it and as #2991 already did for calls. +### A dependency's compile error was reported once per copy — closed (#3058) + +A dependency reached from two defs was copied into both, and each copy's body was checked: +`jq: 2 compile errors`, the second without a line. Since #2955 the body exists once and is +checked once, so `include "mid"; a, b` with `mid` = `include "dep"; def a: g; def b: g;` and +`dep` = `def g: $nosuch;` reports jq's one error. Cross-module errors also come out in jq +1.7.1's order now — a dependency's before its includer's, the last-declared dependency's +first, a chain's deepest first — because that is the order the linked runs are wrapped in. +Both pinned in `tests/jq_cli_tests.rs` (`_3058`, `_2955`). + +**Residual:** a module that is both a top-level `include` and another module's dependency +has two copies (its top-level run and its linked run), so a body error in it is reported +twice when the main filter reaches *both* — `include "dep"; include "mid"; [h, bad]` with +`dep` = `def bad: nosuch;` and `mid` = `include "dep"; def h: bad;` prints +`jq: 2 compile errors` where jq prints one. Reaching only the linked copy (`[h, k]`) is one +report, since an unreached body is never checked (#2740). + ### Module-scope gaps that are genuinely open Found while closing #2865, filed rather than recorded as divergences: a dependency named diff --git a/docs/reference/jq-language.md b/docs/reference/jq-language.md index 3df4fc8ad..379125cf1 100644 --- a/docs/reference/jq-language.md +++ b/docs/reference/jq-language.md @@ -321,14 +321,16 @@ as opposed to a trailing same-line comment) are not implemented at all. - [x] Parameterized functions in modules - [x] Transitive `include`/`import` — a module's own directives are processed too (#2865) -A module's dependencies are bound into the bodies of the defs that module -exports, not spliced into its exported chain, which is what real jq does: -a transitively included name is visible *inside* the module and is **not** +A module's dependencies are linked once each and reached through +forwarding stubs wrapped into the bodies of the defs that call them, never +spliced into the module's exported chain, which is what real jq does: a +transitively included name is visible *inside* the module and is **not** re-exported to whoever included it, and it outranks a same-named sibling def in that module (while the module still exports its own). A def's own recursive call binds to itself rather than to a same-named dependency, per (name, arity), and a module's own defs keep the bindings they were -written under however the def that calls them is declared. See +written under however the def that calls them is declared. A chain of +modules costs memory linear in its total source (#2955). See [jq Limitations](../compliance/jq/limitations.md#seven-module-scoping-rules-that-are-matched-and-read-as-bugs-2865) for the full table and the two scoping gaps that remain open. From c43508b105da5b5fa93eba3d403da02d81287111 Mon Sep 17 00:00:00 2001 From: John Ky Date: Fri, 18 Sep 2026 20:45:05 +1000 Subject: [PATCH 4/5] fix(jq): never treat an aliased dependency's bare name as a clash; address #2955 review findings - `dep_stubs_for` compared a dependency's *bare* name against the consuming def's own (name, arity) and parameter names even for an `import ... as ns` group, whose stub is spelt `ns::name` and cannot clash with either. The stub was dropped and a working call became a spurious compile error. Rows R34-R36 in the #2962 matrix pin the three shapes (own name, a parameter, the def's own recursive call), captured from jq 1.7.1. - `hoist_order` takes the top-level directives' already-interned run ids from `process_program` instead of re-resolving every path through the filesystem. - The per-module referenced closure indexes siblings by name once instead of scanning the module per call. - `report_unresolved_call` and the bare-form fallback map a def name through `ModuleRun::display_name`, so a link spelling can never reach stderr. - Doc comments on `link_keys` (why it is keyed by literal path) and on the two twin fixed points. --- src/bin/succinctly/jq_runner.rs | 103 ++++++++++++++++++++++---------- tests/jq_cli_tests.rs | 33 ++++++++++ 2 files changed, 103 insertions(+), 33 deletions(-) diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index d168b887e..2d7ab2b64 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -102,6 +102,16 @@ pub struct ModuleLoader { deps_of: BTreeMap>, /// `run id -> loaded_modules key` for every module some other module /// depends on (#2955): the modules that get a link run of their own. + /// + /// Deliberately keyed the same way [`Self::loaded_modules`] itself is -- + /// the literal path a `dependency_signatures` call was made with, not + /// [`Self::run_origins`]'s canonicalized one -- because + /// [`Self::link_dependency_runs`] uses this only to index straight into + /// `loaded_modules`. If the same module is reached via two literal + /// spellings that canonicalize to one id, the later spelling overwrites + /// the earlier here, but `loaded_modules` always has an entry for + /// whichever spelling wins (`dependency_signatures` inserts both from the + /// same call), so the lookup this feeds never misses. link_keys: BTreeMap, } @@ -634,8 +644,12 @@ fn dep_stubs_for( Some(alias) => format!("{alias}::{dep_name}"), None => dep_name.clone(), }; - let clashes = (dep_name == name && dep_params.len() == arity) - || (dep_params.is_empty() && param_names.contains(dep_name.as_str())); + // A qualified `alias::name` call can never collide with the + // consuming def's own bare (name, arity) or a bare parameter name -- + // only an `include`d (unaliased) dependency's bare spelling can. + let clashes = alias.is_none() + && ((dep_name == name && dep_params.len() == arity) + || (dep_params.is_empty() && param_names.contains(dep_name.as_str()))); if clashes || !called.contains(&stub_name) { continue; } @@ -1044,7 +1058,14 @@ impl ModuleLoader { /// module that is also a top-level `include`/`import` is walked for its /// dependencies but not recorded for itself unless something depends on /// it: its top-level run already carries its defs. - fn hoist_order(&self, program: &Program) -> Vec { + /// + /// `top_ids` is each top-level `include`/non-data `import`'s + /// `(decl_index, run id)`, exactly as [`Self::process_program`] already + /// resolved them via [`Self::run_id_for`] while wrapping their runs -- + /// passed in rather than re-derived from `Program`'s paths so this does + /// not repeat the same `resolve_module_in`/`canonicalize` filesystem + /// lookups a second time for every call. + fn hoist_order(&self, top_ids: &[(usize, u32)]) -> Vec { fn visit( loader: &ModuleLoader, id: u32, @@ -1065,31 +1086,15 @@ impl ModuleLoader { } } - // Top-level directives, last declared first, exactly as their runs - // nest (`process_program` wraps the last-declared include innermost). - let mut top: Vec<(usize, &str)> = program - .includes - .iter() - .map(|i| (i.decl_index, i.path.as_str())) - .chain( - program - .imports - .iter() - .filter(|i| !i.data) - .map(|i| (i.decl_index, i.path.as_str())), - ) - .collect(); + // Last declared first, exactly as their runs nest (`process_program` + // wraps the last-declared include innermost). + let mut top: Vec<(usize, u32)> = top_ids.to_vec(); top.sort_by_key(|(decl, _)| core::cmp::Reverse(*decl)); let mut expanded = BTreeSet::new(); let mut recorded = BTreeSet::new(); let mut order = Vec::new(); - for (_, path) in top { - // Already interned by the load that recorded its dependencies; - // an unresolvable path is not in `deps_of` and contributes nothing. - let Some(&id) = self.run_ids.get(&self.run_key(path)) else { - continue; - }; + for (_, id) in top { visit(self, id, false, &mut expanded, &mut recorded, &mut order); } order @@ -1182,6 +1187,11 @@ impl ModuleLoader { // order below is bit-for-bit what it was before this issue. let mut last_err: Option<(usize, ModuleLoadError)> = None; + // Every top-level `include`/non-data `import`'s (decl_index, run id), + // collected as each is resolved below so `link_dependency_runs` -> + // `hoist_order` can place them without re-resolving the same paths + // through the filesystem a second time. + let mut top_ids: Vec<(usize, u32)> = Vec::new(); // #2682: each `expr = FuncDef { .., then: expr }` wraps the *previous* // `expr` one layer further in, so whichever source is processed @@ -1217,6 +1227,7 @@ impl ModuleLoader { // first -- and reversing the two `include`s made it agree with // jq again, by accident. let id = self.run_id_for(&include.path); + top_ids.push((include.decl_index, id)); expr = wrap_run(expr, defs, id, None); } @@ -1264,6 +1275,7 @@ impl ModuleLoader { // of their bodies is retried as `alias::name` by the resolver, // and only within this run. let id = self.run_id_for(&import.path); + top_ids.push((import.decl_index, id)); expr = wrap_run(expr, defs, id, Some(namespace)); } @@ -1271,7 +1283,7 @@ impl ModuleLoader { return Err(e); } - expr = self.link_dependency_runs(program, expr); + expr = self.link_dependency_runs(program, &top_ids, expr); // Transform NamespacedCall expressions to regular FuncCall expressions expr = rewrite_namespaced_calls(expr); @@ -1318,8 +1330,13 @@ impl ModuleLoader { /// innermost-first rule among them is the module's own (`def c: 7; def /// c: 8; def g: c;` exports `g` as 8, and `def c: 7; def g: c; def c: 8; /// def k: [g, c];` as `[7, 8]`, both as jq answers). - fn link_dependency_runs(&self, program: &Program, mut expr: Expr) -> Expr { - let order = self.hoist_order(program); + fn link_dependency_runs( + &self, + program: &Program, + top_ids: &[(usize, u32)], + mut expr: Expr, + ) -> Expr { + let order = self.hoist_order(top_ids); if order.is_empty() { return expr; } @@ -1335,6 +1352,14 @@ impl ModuleLoader { // same way, or a sibling reached only that way looks unreached and // what *it* depends on is never linked (a compile error naming the // missing link, in a program jq runs). + // + // This is the top-level twin of the per-linked-module fixed point + // below: same "grow `wanted`/`kept` until nothing new resolves" + // shape, over a different candidate list (`top`'s alias-qualified + // entries here, `defs`'s bare-named ones there) because a module can + // be imported under several different aliases at the top level but a + // hoisted link run is keyed by one globally unique id. A retry rule + // fixed in one almost certainly needs the same fix in the other. let mut wanted: BTreeSet = called_func_names(&program.expr); let mut top: Vec<(String, &Expr, Option<&str>, bool)> = Vec::new(); for include in &program.includes { @@ -1388,8 +1413,18 @@ impl ModuleLoader { continue; }; + // Name -> index, built once so the fixed point below is O(log D) + // per sibling call rather than an O(D) scan of the whole module. + let name_index: BTreeMap<&str, usize> = defs + .iter() + .enumerate() + .map(|(i, (name, _, _))| (name.as_str(), i)) + .collect(); + // Seeds: this module's defs some stub already names. Then the - // fixed point over its own bare sibling calls. + // fixed point over its own bare sibling calls -- the per-module + // twin of the top-level closure above; see its comment for why + // the two are not one shared function. let mut kept_names: BTreeSet<&str> = defs .iter() .map(|(name, _, _)| name.as_str()) @@ -1406,12 +1441,8 @@ impl ModuleLoader { for called in called_func_names(body) { if jq::ModuleRun::is_link_name(&called) { wanted.insert(called); - } else if let Some(sibling) = defs - .iter() - .map(|(n, _, _)| n.as_str()) - .find(|n| *n == called) - { - grew |= kept_names.insert(sibling); + } else if let Some(&sibling_i) = name_index.get(called.as_str()) { + grew |= kept_names.insert(defs[sibling_i].0.as_str()); } } } @@ -2502,6 +2533,11 @@ fn report_unresolved_call( occurrence_index: usize, resume_from: &mut usize, ) { + // #2955: a link-run pruning gap could in principle leave a stub's target + // unresolved, surfacing its internal `\0link:::name` spelling here. + // `display_name` is a no-op for every ordinary name, so this costs + // nothing on the common path. + let name = jq::ModuleRun::display_name(name); // #2635: `occurrence_index` (from `resolve::UnresolvedCall`, computed // while walking the same tree in the same source order) counts *every* // earlier call to this `(name, arity)` pair, resolved or not -- not @@ -2771,6 +2807,7 @@ fn report_compile_errors(errors: &[jq::ResolveError], filter: &str, loader: &Mod ); } None => { + let name = jq::ModuleRun::display_name(name); eprintln!("jq: error: {name}/{arity} is not defined at {at}"); } } diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index db45e0461..9c729cc9d 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -27760,6 +27760,39 @@ fn test_dependencies_are_bound_in_their_own_scope_2962() -> Result<()> { "", 0, ), + // R34-R36: an `import ... as ns;` dependency's bare name can equal + // the consuming def's own name, a param name, or the consuming def's + // own recursive call -- none of which can ever clash with the actual + // call spelling `ns::name`, since a qualified name can never equal a + // bare one. `dep_stubs_for`'s clash test used to compare the + // dependency's *bare* name against the consumer's own (name, arity) + // and params regardless of whether the dependency was aliased, + // wrongly dropping the stub and turning a working call into a + // spurious compile error. Live-verified against jq 1.7.1. + ( + "R34", + &[("m34", "import \"dep34\" as ns; def g: ns::g;\n"), ("dep34", "def g: 99;\n")], + "include \"m34\"; g", + "99\n", + "", + 0, + ), + ( + "R35", + &[("m35", "import \"dep35\" as ns; def h(g): ns::g;\n"), ("dep35", "def g: 99;\n")], + "include \"m35\"; h(1)", + "99\n", + "", + 0, + ), + ( + "R36", + &[("m36", "import \"dep36\" as ns; def rec: if . == 0 then ns::rec else (. - 1 | rec) end;\n"), ("dep36", "def rec: 99;\n")], + "include \"m36\"; 1 | rec", + "99\n", + "", + 0, + ), ]; for (id, modules, filter, want_stdout, want_stderr, want_code) in rows { From 0bcfcb408ff1a1a3a2ce31c15d0d0800f200b87a Mon Sep 17 00:00:00 2001 From: John Ky Date: Fri, 18 Sep 2026 21:51:24 +1000 Subject: [PATCH 5/5] fix(jq): sort hoisted dependencies by decl_index and dedupe module loads by canonical path hoist_order relied on deps_of's insertion order, but module_dep_defs records a module's includes before its imports regardless of interleaving, reversing jq's compile-error order when a dependency module mixes both directives. Sorting deps_of entries by their own decl_index, the same way top_ids already is, restores it (#2955 review). Also key loaded_modules by the canonical file (ModuleLoader::run_key) instead of the literal spelling a caller wrote, so two spellings of one module share one load instead of parsing and binding it twice -- this also makes link_keys redundant with run_origins, so it's retired. Finally, factor the two reachability fixed-point loops in link_dependency_runs onto one shared grow_to_fixed_point helper, so a future fix to the iteration itself doesn't need to be applied twice. --- src/bin/succinctly/jq_runner.rs | 252 ++++++++++++++++++++++---------- tests/jq_cli_tests.rs | 27 +++- 2 files changed, 198 insertions(+), 81 deletions(-) diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index 2d7ab2b64..0740eff0a 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -71,7 +71,14 @@ const AUTO_LOAD_RUN_ID: u32 = 0; pub struct ModuleLoader { /// Search path for modules (in order of priority) search_path: Vec, - /// Loaded modules (path -> function definitions: name, params, body) + /// Loaded modules, keyed by [`Self::run_key`] -- the same canonical-file + /// key [`Self::run_ids`]/[`Self::run_origins`] intern by, not the literal + /// path a caller happened to write (function definitions: name, params, + /// body). Two different literal spellings of one module (`"dep"` from one + /// includer, `"./dep"` from another) canonicalize to the same key here, + /// so the module loads once regardless of how many spellings reach it + /// (#2955) -- keying on the literal path used to let each spelling load + /// and cache its own copy. loaded_modules: BTreeMap, /// Auto-loaded ~/.jq file definitions (if file exists): name, params, body auto_loaded_defs: FuncDefList, @@ -96,23 +103,15 @@ pub struct ModuleLoader { run_origins: BTreeMap, /// Next unused run id. `0` is reserved for `~/.jq`. next_run_id: u32, - /// `run id -> the run ids of that module's own dependencies`, in - /// declaration order (#2955). Read by [`Self::hoist_order`] to place each - /// linked module outside everything that depends on it. - deps_of: BTreeMap>, - /// `run id -> loaded_modules key` for every module some other module - /// depends on (#2955): the modules that get a link run of their own. - /// - /// Deliberately keyed the same way [`Self::loaded_modules`] itself is -- - /// the literal path a `dependency_signatures` call was made with, not - /// [`Self::run_origins`]'s canonicalized one -- because - /// [`Self::link_dependency_runs`] uses this only to index straight into - /// `loaded_modules`. If the same module is reached via two literal - /// spellings that canonicalize to one id, the later spelling overwrites - /// the earlier here, but `loaded_modules` always has an entry for - /// whichever spelling wins (`dependency_signatures` inserts both from the - /// same call), so the lookup this feeds never misses. - link_keys: BTreeMap, + /// `run id -> (decl_index, run id)` of that module's own dependencies + /// (#2955), `decl_index` shared across `include`/`import` exactly as + /// [`Import::decl_index`]/[`Include::decl_index`] number them. Read by + /// [`Self::hoist_order`], which sorts by `decl_index` before recursing -- + /// insertion order alone is *not* declaration order here, because + /// [`Self::module_dep_defs`] records all of a module's `include`s before + /// any of its `import`s, regardless of how the two are interleaved in the + /// source. + deps_of: BTreeMap>, } /// A [`ModuleLoader`] failure, structured enough to report in jq's own @@ -768,7 +767,6 @@ impl ModuleLoader { run_origins, next_run_id: AUTO_LOAD_RUN_ID + 1, deps_of: BTreeMap::new(), - link_keys: BTreeMap::new(), } } @@ -823,6 +821,14 @@ impl ModuleLoader { /// splicing the dependencies into the exported chain, is what real jq /// does. fn ensure_module_loaded(&mut self, module_path: &str) -> Result<&FuncDefList, ModuleLoadError> { + // Keyed by the canonical file (#2955), not `module_path` as written: + // two spellings of the same module (`"dep"`, `"./dep"`) must share one + // cache entry, or each distinct spelling pays its own full + // parse-and-bind pass. `run_key` is the same canonicalize-or-fall-back + // helper `run_id_for` interns run ids by, so this map and `run_ids` + // agree on what "the same module" means. + let key = self.run_key(module_path); + // `contains_key` -> load -> `insert` -> re-`get`, rather than the // `entry` spelling this used to have (#2865). `entry` holds a mutable // borrow of `loaded_modules` across the whole load, which the loader @@ -831,14 +837,14 @@ impl ModuleLoader { // comment here was written to avoid comes back, as the cost of // recursion being possible at all. It is genuinely unreachable: the // insert immediately above it is unconditional on this path. - if !self.loaded_modules.contains_key(module_path) { + if !self.loaded_modules.contains_key(&key) { let defs = self.load_and_bind_module(module_path)?; - self.loaded_modules.insert(module_path.to_string(), defs); + self.loaded_modules.insert(key.clone(), defs); } Ok(self .loaded_modules - .get(module_path) + .get(&key) .expect("just inserted above, or already present")) } @@ -981,7 +987,7 @@ impl ModuleLoader { for include in &program.includes { let id = self.run_id_for(&include.path); - let sigs = self.dependency_signatures(&include.path, id, own_id)?; + let sigs = self.dependency_signatures(&include.path, id, own_id, include.decl_index)?; defs.push((id, None, sigs)); } @@ -1017,7 +1023,7 @@ impl ModuleLoader { continue; } let id = self.run_id_for(&import.path); - let sigs = self.dependency_signatures(&import.path, id, own_id)?; + let sigs = self.dependency_signatures(&import.path, id, own_id, import.decl_index)?; defs.push((id, Some(import.alias.clone()), sigs)); } @@ -1028,20 +1034,26 @@ impl ModuleLoader { /// and borrow out its defs' signatures (#2955): the names and parameters /// the stubs are built from. The bodies stay in the cache, to be linked /// once by [`Self::process_program`], which is what this also records: - /// that `id` needs a link run, and that `own_id` depends on it. + /// that `id` needs a link run, and that `own_id` depends on it at + /// `decl_index` -- the position [`Self::hoist_order`] sorts by, since + /// `own_id`'s dependencies are recorded include-block-then-import-block + /// here (see [`Self::module_dep_defs`]), not in true source order. fn dependency_signatures( &mut self, module_path: &str, id: u32, own_id: u32, + decl_index: usize, ) -> Result)>, ModuleLoadError> { let sigs = self .ensure_module_loaded(module_path)? .iter() .map(|(name, params, _)| (name.clone(), params.clone())) .collect(); - self.link_keys.insert(id, module_path.to_string()); - self.deps_of.entry(own_id).or_default().push(id); + self.deps_of + .entry(own_id) + .or_default() + .push((decl_index, id)); Ok(sigs) } @@ -1076,7 +1088,15 @@ impl ModuleLoader { ) { if expanded.insert(id) { if let Some(deps) = loader.deps_of.get(&id) { - for &dep in deps.iter().rev() { + // Sorted here rather than relying on `deps`' insertion + // order: `dependency_signatures` records a module's + // includes before its imports (see `module_dep_defs`), + // which is not source order when the two are + // interleaved. `decl_index` is the true order; last + // declared first, same rule as `top_ids` below. + let mut deps = deps.clone(); + deps.sort_by_key(|(decl, _)| core::cmp::Reverse(*decl)); + for (_, dep) in deps { visit(loader, dep, true, expanded, recorded, order); } } @@ -1354,21 +1374,28 @@ impl ModuleLoader { // missing link, in a program jq runs). // // This is the top-level twin of the per-linked-module fixed point - // below: same "grow `wanted`/`kept` until nothing new resolves" - // shape, over a different candidate list (`top`'s alias-qualified - // entries here, `defs`'s bare-named ones there) because a module can - // be imported under several different aliases at the top level but a - // hoisted link run is keyed by one globally unique id. A retry rule - // fixed in one almost certainly needs the same fix in the other. + // below: both share `grow_to_fixed_point` for the "grow until + // nothing new resolves" iteration itself, but what a visit *does* + // still differs, over a different candidate list (`top`'s + // alias-qualified entries here, `defs`'s bare-named ones there) + // because a module can be imported under several different aliases + // at the top level but a hoisted link run is keyed by one globally + // unique id. A retry rule fixed in one almost certainly needs the + // same fix in the other. let mut wanted: BTreeSet = called_func_names(&program.expr); let mut top: Vec<(String, &Expr, Option<&str>, bool)> = Vec::new(); for include in &program.includes { - if let Some(defs) = self.loaded_modules.get(&include.path) { + // `loaded_modules` is keyed canonically (#2955); `include.path` is + // the literal spelling as written, so it has to go through + // `run_key` the same way `ensure_module_loaded` does, or a + // spelling that differs from whichever one populated the cache + // would miss here even though the module is loaded. + if let Some(defs) = self.loaded_modules.get(&self.run_key(&include.path)) { top.extend(defs.iter().map(|(n, _, b)| (n.clone(), b, None, false))); } } for import in program.imports.iter().filter(|i| !i.data) { - if let Some(defs) = self.loaded_modules.get(&import.path) { + if let Some(defs) = self.loaded_modules.get(&self.run_key(&import.path)) { let ns = import.alias.as_str(); top.extend( defs.iter() @@ -1381,35 +1408,30 @@ impl ModuleLoader { .iter() .map(|(n, _, b)| (n.clone(), b, None, false)), ); - loop { - let mut grew = false; - for (name, body, alias, kept) in &mut top { - if *kept || !wanted.contains(name.as_str()) { - continue; - } - *kept = true; - let before = wanted.len(); - for called in called_func_names(body) { - if let Some(alias) = alias { - if !called.contains("::") { - wanted.insert(format!("{alias}::{called}")); - } + grow_to_fixed_point(top.len(), |i| { + let (name, body, alias, kept) = &mut top[i]; + if *kept || !wanted.contains(name.as_str()) { + return false; + } + *kept = true; + let before = wanted.len(); + for called in called_func_names(body) { + if let Some(alias) = alias { + if !called.contains("::") { + wanted.insert(format!("{alias}::{called}")); } - wanted.insert(called); } - grew |= wanted.len() != before; - } - if !grew { - break; + wanted.insert(called); } - } + wanted.len() != before + }); for &id in order.iter().rev() { - let Some(defs) = self - .link_keys - .get(&id) - .and_then(|k| self.loaded_modules.get(k)) - else { + // `run_origin` and `loaded_modules` are both keyed by the same + // canonical file (#2955), so `id`'s origin is directly a + // `loaded_modules` key -- no separate `id -> loaded_modules key` + // map is needed once both agree on canonicalization. + let Some(defs) = self.run_origin(id).and_then(|k| self.loaded_modules.get(k)) else { continue; }; @@ -1422,34 +1444,39 @@ impl ModuleLoader { .collect(); // Seeds: this module's defs some stub already names. Then the - // fixed point over its own bare sibling calls -- the per-module - // twin of the top-level closure above; see its comment for why - // the two are not one shared function. + // fixed point over its own bare sibling calls, via the same + // `grow_to_fixed_point` the top-level closure above uses -- what + // a visit *does* still differs (this one grows a local + // `kept_names` by sibling name, and only forwards a link name + // into the shared `wanted`, where the top-level one grows + // `wanted` itself and retries an alias), because a module can be + // `import`ed under several different aliases at the top level + // but a hoisted link run is keyed by one globally unique id -- + // see the top-level closure's comment for the oracle rows this + // asymmetry is checked against. A retry rule fixed in one almost + // certainly needs the same fix in the other. let mut kept_names: BTreeSet<&str> = defs .iter() .map(|(name, _, _)| name.as_str()) .filter(|name| wanted.contains(&jq::ModuleRun::link_name(id, name))) .collect(); let mut keep = vec![false; defs.len()]; - loop { + grow_to_fixed_point(defs.len(), |i| { + let (name, _, body) = &defs[i]; + if keep[i] || !kept_names.contains(name.as_str()) { + return false; + } + keep[i] = true; let mut grew = false; - for (i, (name, _, body)) in defs.iter().enumerate() { - if keep[i] || !kept_names.contains(name.as_str()) { - continue; - } - keep[i] = true; - for called in called_func_names(body) { - if jq::ModuleRun::is_link_name(&called) { - wanted.insert(called); - } else if let Some(&sibling_i) = name_index.get(called.as_str()) { - grew |= kept_names.insert(defs[sibling_i].0.as_str()); - } + for called in called_func_names(body) { + if jq::ModuleRun::is_link_name(&called) { + wanted.insert(called); + } else if let Some(&sibling_i) = name_index.get(called.as_str()) { + grew |= kept_names.insert(defs[sibling_i].0.as_str()); } } - if !grew { - break; - } - } + grew + }); let linked: FuncDefList = defs .iter() @@ -1470,6 +1497,26 @@ impl ModuleLoader { } } +/// Visit every index in `0..len` via `expand`, repeating the full pass until +/// one changes nothing -- the "grow until nothing new resolves" fixed point +/// [`ModuleLoader::link_dependency_runs`]'s two reachability closures both +/// need, pulled out once so a fix to the iteration itself (when to stop, +/// what order a pass visits) is made in one place rather than two. `expand` +/// reports whether visiting `i` changed anything it manages; the two callers +/// still decide, separately, what a visit *does* -- see each call site's own +/// comment for why that differs. +fn grow_to_fixed_point(len: usize, mut expand: impl FnMut(usize) -> bool) { + loop { + let mut grew = false; + for i in 0..len { + grew |= expand(i); + } + if !grew { + break; + } + } +} + /// Rewrite every computed-key `Expr` inside a `reduce`/`foreach`/`as {...}` /// pattern list (#2677's `ObjectKey::Expr`) the same way /// [`rewrite_namespaced_calls`] rewrites everywhere else -- see #2957: @@ -8418,6 +8465,53 @@ mod tests { } } + /// #2955 review: `loaded_modules` must be keyed by the canonical file + /// (`ModuleLoader::run_key`), not the literal path a caller wrote, or two + /// spellings of one module reaching it from different includers (`"dep"` + /// from one, `"./dep"` from another) parse and bind it twice. + /// + /// Output was never wrong on this path -- a `dependency_signatures` call + /// records both the `loaded_modules` entry and the `run_id_for`/ + /// `run_origins` entry for the same spelling in the same call, so the + /// linking step downstream always found *a* copy of the module -- but + /// before this test's fix, each distinct literal spelling paid its own + /// full parse-and-bind pass, including recursively loading *that* + /// module's own dependencies again. #2955 exists specifically to bound + /// module-loading cost, so a wide fan-out where many consumers each + /// spell a shared leaf module slightly differently would reintroduce + /// part of the duplication the linking fix eliminates. + mod duplicate_spelling_shares_one_load_2955 { + use super::*; + + #[test] + fn two_spellings_of_one_module_load_once() { + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join("common.jq"), "def cfn: 42;\n").expect("write common"); + std::fs::write( + dir.path().join("s1.jq"), + "include \"common\";\ndef s1fn: cfn + 1;\n", + ) + .expect("write s1"); + std::fs::write( + dir.path().join("s2.jq"), + "include \"./common\";\ndef s2fn: cfn + 2;\n", + ) + .expect("write s2"); + + let mut loader = ModuleLoader::new(&[dir.path().to_path_buf()]); + loader.load_module("s1").expect("load s1"); + loader.load_module("s2").expect("load s2"); + + assert_eq!( + loader.loaded_modules.len(), + 3, + "common.jq must load once despite the two spellings (s1, s2 and one \ + common entry): {:?}", + loader.loaded_modules.keys().collect::>() + ); + } + } + /// #1525: `seq_no_rs_byte_warning` direct unit coverage. Every expected /// value here was live-verified against the pinned jq 1.7.1 binary /// (see `tests/jq_cli_tests.rs`'s CLI-level `_1525` tests for the diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 9c729cc9d..e85c2ee96 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -46706,14 +46706,24 @@ fn run_module_rows(rows: &[ModuleRow], extra: &[&str]) -> Result<()> { /// #2955: compile errors across a module chain come out in jq 1.7.1's order /// -- a dependency's before its includer's, the last-declared dependency's -/// first, and a chain's deepest module first. All three shapes captured -/// whole from the pinned binary. +/// first, and a chain's deepest module first. All shapes captured whole from +/// the pinned binary. /// /// That order is not chosen by the resolver: it is the order the linked /// runs are wrapped in (`ModuleLoader::hoist_order`), outermost first, and /// `resolve::check` walks the chain from the outside in. The copying loader /// printed the includer's own error first, then its dependencies in /// declaration order (`A G K` for the first row, against jq's `K G A`). +/// +/// The last row pins a regression `hoist_order` reintroduced: `deps_of` is +/// recorded by `module_dep_defs` as all of a module's `include`s then all of +/// its `import`s (see that function's own doc comment for why), which is +/// not source order when the two are interleaved. Sorting only `top_ids` +/// (the top-level directives) by `decl_index` and trusting `deps_of`'s +/// insertion order for nested dependencies reported `adep2955` before +/// `bdep2955` -- reversed from jq, which reports the last-declared +/// dependency inside `mid2955` (`bdep2955`) first. `hoist_order` now sorts +/// `deps_of` entries by their own recorded `decl_index` the same way. #[test] fn test_dependency_errors_report_in_jq_order_2955() -> Result<()> { let rows: &[ModuleRow] = &[ @@ -46747,6 +46757,19 @@ fn test_dependency_errors_report_in_jq_order_2955() -> Result<()> { r#"include "l2"; x"#, "jq: error: nosuchZ/0 is not defined at /l0.jq, line 1:\ndef z: nosuchZ; \njq: error: nosuchY/0 is not defined at /l1.jq, line 1:\ninclude \"l0\"; def y: [z, nosuchY]; \njq: error: nosuchX/0 is not defined at /l2.jq, line 1:\ninclude \"l1\"; def x: [y, nosuchX]; \njq: 3 compile errors\n", ), + ( + "a dependency module mixing include and import, last-declared first", + &[ + ("adep2955", "def afn: nosuchA;\n"), + ("bdep2955", "def bfn: nosuchB;\n"), + ( + "mid2955", + "import \"adep2955\" as a; include \"bdep2955\"; def use: a::afn + bfn;\n", + ), + ], + r#"include "mid2955"; use"#, + "jq: error: nosuchB/0 is not defined at /bdep2955.jq, line 1:\ndef bfn: nosuchB; \njq: error: nosuchA/0 is not defined at /adep2955.jq, line 1:\ndef afn: nosuchA; \njq: 2 compile errors\n", + ), ]; run_module_rows(rows, &[]) }