From 6c7c2bc6da0af0400d3284a44919be38c02268d3 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 14:03:51 +1000 Subject: [PATCH 01/11] fix(jq): process a module's own include/import directives transitively `ModuleLoader::ensure_module_loaded` parsed each module with `jq::parse_program` -- which returns a full `Program`, `includes` and `imports` included -- and then read only `program.expr`, silently discarding the module's own directives. A module that said `include "inner"; def h: g;` therefore exported an `h` whose `g` was genuinely undefined, where real jq answers 42. The issue's suggested fix ("apply `process_program` recursively") gets three of jq's own rows wrong, so the semantics were re-derived from a 16-row oracle matrix captured live against jq 1.7.1. The shape that buys all of them from one mechanism is to wrap each exported def's *body* in its module's dependencies, rather than splicing them into the exported chain: the dependency is then visible inside the module (row 1/2), not re-exported to the includer (row 3), and innermost -- so it beats the module's own same-name sibling (row 4) while that sibling is still what the module exports (row 5). `deps_excluding_self` drops any dependency matching the wrapped def's own (name, arity), because jq binds a def's recursive call to itself first (row 6), arity-scoped so a dep `g/1` and an own `g/0` stay both reachable (row 7). Making the loader re-entrant required replacing `ensure_module_loaded`'s `entry()` spelling, which holds a mutable borrow of the cache across the whole load; the doc comment justifying it is rewritten rather than left contradicting the code. Cycles are a deliberate ADR-0018 rule-4 divergence under the "matching would take the host process down" carve-out: jq 1.7.1 does not diagnose `ca` <-> `cb` at all, it recurses until it dies (exit 139, SIGSEGV, no output on either stream), and a self-including module does the same. succinctly reports `module cycle detected: ca -> cb -> ca` and leaves through the same exit-3 door as the other two compile-error kinds. Detection keys on the resolved file, so `include "./m"` inside `m.jq` is caught too; it cannot use the memo cache, since a module is absent from that cache for exactly as long as its dependencies are loading. Dependencies are filtered to the transitive closure of names the body actually calls -- jq's own `block_bind_referenced` rule, and a sizing requirement rather than a micro-optimisation here: unfiltered, each level's bodies carry the level below, so a synthetic 3-module x 40-def chain cost 359 MB peak RSS against jq's 2.5 MB, and a fourth level would be tens of gigabytes. Filtered, that chain is 10 MB and a 5-level one 11.6 MB. The closure keys on name alone, not (name, arity): a module's own source is parsed with no shadow-candidate seeding, so a call site can still carry #2036's un-resolved `shadow_fallback`, whose `args` is empty by construction and whose arity therefore reads 0. `wrap_defs` gives the "last-declared wins" ordering rule one definition; it had been written out three times in `process_program` and this fix would have made a fourth. `~/.jq` is deliberately not wrapped into module bodies -- jq does not make it visible there -- and `unqualified_def_names` deliberately does not gain transitive names, since row 3 says they are not visible unqualified at the top level either. Refs #2865 --- src/bin/succinctly/jq_runner.rs | 375 +++++++++++++++++++++++++++----- 1 file changed, 324 insertions(+), 51 deletions(-) diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index 862f7ecde..d8fe6fc05 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -63,6 +63,16 @@ pub struct ModuleLoader { loaded_modules: BTreeMap, /// Auto-loaded ~/.jq file definitions (if file exists): name, params, body auto_loaded_defs: FuncDefList, + /// The modules whose own dependencies are currently being loaded, as + /// `(resolved canonical file, module path as written)`, outermost first + /// (#2865). + /// + /// The cycle guard, and it has to be separate from `loaded_modules`: a + /// module is *absent* from that cache for exactly as long as its own + /// dependencies are loading, which is precisely the window in which a + /// cycle closes. Keyed on the resolved file rather than the written path + /// so `include "./m"` inside `m.jq` is still caught. + loading: Vec<(PathBuf, String)>, } /// A [`ModuleLoader`] failure, structured enough to report in jq's own @@ -76,6 +86,24 @@ pub(crate) enum ModuleLoadError { /// then the usual `jq: 1 compile error` trailer. Confirmed live against /// jq 1.7.1, byte-for-byte. NotFound { module_path: String }, + /// A module's own `include`/`import` chain leads back to a module already + /// being loaded (`ca` includes `cb` includes `ca`, or a module including + /// itself). + /// + /// **A deliberate ADR-0018 rule-4 divergence** (#2865), under the explicit + /// "matching would take the host process down" carve-out: real jq 1.7.1 + /// does not diagnose this at all, it recurses until it dies -- + /// `jq -L . -n 'include "ca"; a'` exits **139** (SIGSEGV) with no output on + /// either stream, and a self-including module does the same. Reproducing + /// that faithfully is not an option, so succinctly reports the cycle and + /// leaves through the same `jq: 1 compile error` / exit 3 door as the other + /// two compile-error kinds. + /// + /// `chain` is the module paths **as written in the `include`/`import` + /// directives**, from the outermost module in the cycle through to the + /// repeat that closed it (`["ca", "cb", "ca"]`); detection itself keys on + /// the *resolved* file, so two spellings of one module still close a cycle. + Cycle { chain: Vec }, /// The module could not be read, or its own contents failed to parse. /// jq's shape for this case additionally names the resolved *absolute* /// path and echoes the module's own source line -- #2703's fix does not @@ -102,6 +130,11 @@ fn report_module_load_error(e: &ModuleLoadError) { eprintln!(); eprintln!("jq: 1 compile error"); } + ModuleLoadError::Cycle { chain } => { + eprintln!("jq: error: module cycle detected: {}", chain.join(" -> ")); + eprintln!(); + eprintln!("jq: 1 compile error"); + } ModuleLoadError::Other(inner) => { eprintln!("jq: module error: {inner}"); } @@ -153,6 +186,144 @@ fn extract_and_stamp_func_defs(expr: &Expr, resolved_path: PathBuf) -> FuncDefLi .collect() } +/// Wrap `expr` in one `Expr::FuncDef` per entry of `defs`, so that `defs`' +/// **last** entry ends up innermost -- the one jq's innermost-first scoping +/// resolves a name to -- and its first entry outermost. +/// +/// One definition of that ordering rule (#2865). It used to be written out +/// three times in [`ModuleLoader::process_program`] (includes, `~/.jq`, +/// imports) and this fix adds a fourth site inside the module loader itself; +/// four copies of "which end wins" is exactly the duplicated-predicate trap +/// `CLAUDE.md` calls out, and the copies are individually correct only by +/// inspection. +fn wrap_defs(mut expr: Expr, defs: FuncDefList) -> Expr { + for (name, params, body) in defs.into_iter().rev() { + expr = Expr::FuncDef { + name, + params, + body: Box::new(body), + then: Box::new(expr), + bound: FuncDefBound::default(), + }; + } + expr +} + +/// Every function name called anywhere inside `expr`, including inside nested +/// def bodies (#2865). +/// +/// **Names only, deliberately not (name, arity).** A module's own source is +/// parsed by plain `jq::parse_program` with no shadow-candidate seeding, so a +/// 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 +/// called) where keying on arity could silently *drop* a dependency a call +/// genuinely needs, turning a program that compiles into a compile error. +/// The self-recursion filter in [`deps_excluding_self`] 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 +/// comment is about short-circuiting, which cannot happen here. +fn called_func_names(expr: &Expr) -> BTreeSet { + let mut names = BTreeSet::new(); + succinctly::jq::walk::any_subexpr(expr, &mut |node| { + match node { + Expr::FuncCall { name, .. } => { + names.insert(name.clone()); + } + Expr::NamespacedCall { + namespace, name, .. + } => { + names.insert(format!("{namespace}::{name}")); + } + _ => {} + } + false + }); + names +} + +/// The dependencies a def's body can actually reach: `deps` minus any entry +/// that would capture the def's own recursive calls, minus any entry nothing +/// in the body transitively calls (#2865). +/// +/// ### The self-recursion filter +/// +/// jq binds a def's own name inside its own body before it binds a dependency +/// of the same name, and the match is on **(name, arity)**, not name alone -- +/// so a dependency `g/1` survives when wrapping an own `g/0`, and both stay +/// reachable from that body. +/// +/// ### The referenced-closure filter, and why it is not optional +/// +/// This is jq's own `block_bind_referenced` rule, and here it is a +/// **correctness-adjacent sizing requirement, not a micro-optimisation**: +/// wrapping every dependency into every exported body multiplies down a +/// chain, because each level's bodies already carry the level below. Measured +/// on a synthetic chain of 3 modules x 40 defs (Apple M-series, release +/// build), resolving one def at the top: +/// +/// | chain depth | peak RSS | wall | +/// |--------------------------|----------|-------| +/// | 1 module (no wrapping) | 9 MB | 0.00s | +/// | 2 modules (one wrap) | 18 MB | 0.01s | +/// | 3 modules (two wraps) | 359 MB | 0.28s | +/// | jq 1.7.1, same 3 modules | 2.5 MB | 0.00s | +/// +/// A fourth level would be tens of gigabytes. With the filter the same +/// 3-module chain is flat, because each `c_i` calls exactly one `b_i`. +/// +/// Dropping an unreferenced dependency is unobservable: it is reachable from +/// nothing, so no name resolves to it, and jq agrees an unreferenced +/// dependency whose own body calls an undefined function is an error in +/// neither tool. The closure is seeded from the body and widened through the +/// body of every dependency it pulls in, which over-approximates (a +/// dependency's body was already bound at its own load, so some of its names +/// are satisfied internally) -- over-approximating only keeps something +/// harmless, while under-approximating would drop something needed. +fn deps_excluding_self(deps: &FuncDefList, name: &str, arity: usize, body: &Expr) -> FuncDefList { + let mut wanted = called_func_names(body); + let mut keep = vec![false; deps.len()]; + + // Fixed point: pulling a dependency in can widen `wanted` past + // dependencies already scanned, so rescan until a pass adds nothing. + loop { + let mut grew = false; + for (i, (dep_name, dep_params, dep_body)) in deps.iter().enumerate() { + if keep[i] || (dep_name == name && dep_params.len() == arity) { + continue; + } + if wanted.contains(dep_name) { + keep[i] = true; + let before = wanted.len(); + wanted.extend(called_func_names(dep_body)); + grew |= wanted.len() != before; + } + } + if !grew { + break; + } + } + + deps.iter() + .zip(keep) + .filter_map(|(dep, keep)| keep.then(|| dep.clone())) + .collect() +} + +/// `path` resolved through the filesystem, falling back to `path` itself. +/// +/// `canonicalize` can fail (a module deleted between resolving and reading it, +/// a race no real program depends on); a non-canonical key only weakens the +/// cycle guard's aliasing coverage, it never turns a legal program into an +/// error, so falling back is strictly better than failing a load that already +/// succeeded. +fn canonical_or_self(path: &std::path::Path) -> PathBuf { + std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()) +} + impl ModuleLoader { /// Create a new module loader with the given search paths. pub fn new(library_paths: &[PathBuf]) -> Self { @@ -198,6 +369,7 @@ impl ModuleLoader { search_path, loaded_modules: BTreeMap::new(), auto_loaded_defs, + loading: Vec::new(), } } @@ -209,34 +381,156 @@ impl ModuleLoader { /// does not pay for a deep clone of every def body it is about to drop /// (#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. fn ensure_module_loaded(&mut self, module_path: &str) -> Result<&FuncDefList, ModuleLoadError> { - // `entry` rather than `get`-then-insert: the borrow checker cannot - // see that an early `return` of `get`'s borrow ends it, so the - // `contains_key` spelling would need an unreachable `expect` on the - // re-lookup. The key allocation on the cached path is one short - // module-path string, against the file read and parse it replaces. - match self.loaded_modules.entry(module_path.to_string()) { - std::collections::btree_map::Entry::Occupied(entry) => Ok(entry.into_mut()), - std::collections::btree_map::Entry::Vacant(entry) => { - // Resolve the module path - let file_path = - resolve_module_in(&self.search_path, module_path).ok_or_else(|| { - ModuleLoadError::NotFound { - module_path: module_path.to_string(), - } - })?; - - // Read and parse the module - let contents = std::fs::read_to_string(&file_path) - .with_context(|| format!("failed to read module: {}", file_path.display()))?; + // `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 + // now has to be able to re-enter to pull in the module's own + // dependencies -- so the `expect` on the re-lookup that the old doc + // 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) { + let defs = self.load_and_bind_module(module_path)?; + self.loaded_modules.insert(module_path.to_string(), defs); + } - let program = jq::parse_program(&contents).map_err(|e| { - anyhow::anyhow!("parse error in module '{}': {}", file_path.display(), e) - })?; + Ok(self + .loaded_modules + .get(module_path) + .expect("just inserted above, or already present")) + } - Ok(entry.insert(extract_and_stamp_func_defs(&program.expr, file_path))) + /// Read, parse and bind one module: its own defs, each wrapped in whatever + /// its own `include`/`import` directives bring into scope (#2865). + /// + /// ### Why wrap each *body* rather than splice into the exported chain + /// + /// The issue's own suggested fix ("apply `process_program` recursively") + /// gets three of jq's rows wrong at once. Wrapping the bodies buys all of + /// them from one mechanism, with no special-casing (every row captured + /// live against jq 1.7.1, fixtures `inner.jq` = `def g: 42;`): + /// + /// - `include "outer"; h` where `outer.jq` is `include "inner"; def h: g;` + /// answers `42` -- the dependency is visible inside the module. + /// - `include "outer"; g` is a **compile error**: the dependency is *not* + /// re-exported to the includer, which splicing into the chain would do. + /// - where the module is `include "inner"; def g: 7; def h: g;`, `h` + /// answers **`42`, not `7`** -- the dependency is innermost, so it beats + /// the module's own same-name sibling. (At the top level the same + /// collision goes the *other* way, since a filter's own defs bind at + /// parse time; that row already passes and is unchanged.) + /// - ...while that module's own `g` is still what it exports: `7`. + /// + /// ### The self-recursion filter + /// + /// [`deps_excluding_self`] drops any dependency whose **(name, arity)** + /// matches the def being wrapped, because jq binds a def's own recursive + /// call to itself before it binds the dependency: with `inner`'s `g/0` in + /// scope, `def g: if . == 0 then "base" else (. - 1 | g) end;` still + /// answers `"base"`, not `42`. It is arity-scoped, not name-scoped -- a + /// dependency `g/1` alongside an own `g/0` leaves both reachable. + /// + /// ### Search-path resolution + /// + /// A nested `include` resolves against the global search path only, never + /// the including module's own directory -- `sub/usedeep.jq` saying + /// `include "deep"` reports `module not found: deep` even with `deep.jq` + /// sitting next to it. So reusing the search path verbatim is both the + /// simple implementation and the faithful one. + fn load_and_bind_module(&mut self, module_path: &str) -> Result { + // Resolve the module path + let file_path = resolve_module_in(&self.search_path, module_path).ok_or_else(|| { + ModuleLoadError::NotFound { + module_path: module_path.to_string(), } + })?; + + let canonical = canonical_or_self(&file_path); + if self.loading.iter().any(|(seen, _)| *seen == canonical) { + let mut chain: Vec = self + .loading + .iter() + .map(|(_, as_written)| as_written.clone()) + .collect(); + chain.push(module_path.to_string()); + return Err(ModuleLoadError::Cycle { chain }); + } + + // Read and parse the module + let contents = std::fs::read_to_string(&file_path) + .with_context(|| format!("failed to read module: {}", file_path.display()))?; + + let program = jq::parse_program(&contents).map_err(|e| { + anyhow::anyhow!("parse error in module '{}': {}", file_path.display(), e) + })?; + + // Stamp `$__loc__` BEFORE wrapping, never after: `stamp_loc_file` + // overwrites `file` unconditionally, so a wrap-then-stamp order would + // silently re-stamp an inner module's already-correct `$__loc__` with + // *this* module's path, regressing #2774. + let own = extract_and_stamp_func_defs(&program.expr, file_path); + + self.loading.push((canonical, module_path.to_string())); + let deps = self.module_dep_defs(&program); + self.loading.pop(); + let deps = deps?; + + Ok(own + .into_iter() + .map(|(name, params, body)| { + let visible = deps_excluding_self(&deps, &name, params.len(), &body); + let body = wrap_defs(body, visible); + (name, params, body) + }) + .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). + /// + /// Declaration order is what produces that: a later `include` is appended + /// later, so it lands nearer the end and therefore nearer the body -- + /// matching jq, where a module with `include "pa"; include "pb";` and both + /// defining `foo` resolves `foo` to `pb`'s. That is the same rule + /// [`Self::process_program`] documents for the top level, which is why + /// both go through `wrap_defs`. + /// + /// `~/.jq` is deliberately **not** included here: its defs are not visible + /// inside a module body in real jq (`def uh: hj;` in a module reports + /// `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: FuncDefList = Vec::new(); + + for include in &program.includes { + defs.extend(self.load_module(&include.path)?); + } + + // 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. + for import in &program.imports { + let namespace = &import.alias; + defs.extend( + self.load_module(&import.path)? + .into_iter() + .map(|(name, params, body)| (format!("{namespace}::{name}"), params, body)), + ); } + + Ok(defs) } /// Load a module and return an owned copy of its function definitions @@ -306,29 +600,13 @@ impl ModuleLoader { for include in program.includes.iter().rev() { let defs = self.load_module(&include.path)?; // Wrap expression with function definitions from the included module - for (name, params, body) in defs.into_iter().rev() { - expr = Expr::FuncDef { - name, - params, - body: Box::new(body), - then: Box::new(expr), - bound: FuncDefBound::default(), - }; - } + expr = wrap_defs(expr, defs); } // `~/.jq`'s own defs: lowest priority of the two unqualified // sources (loses to any `include`d module of the same name, but // still beats a name that was never `include`d at all). - for (name, params, body) in self.auto_loaded_defs.clone().into_iter().rev() { - expr = Expr::FuncDef { - name, - params, - body: Box::new(body), - then: Box::new(expr), - bound: FuncDefBound::default(), - }; - } + expr = wrap_defs(expr, self.auto_loaded_defs.clone()); // Process imports (definitions available under namespace::) // Load modules and add their functions with namespace prefixes @@ -337,16 +615,11 @@ impl ModuleLoader { let namespace = &import.alias; // Add each function with a namespaced name (namespace::funcname) - for (name, params, body) in defs.into_iter().rev() { - let namespaced_name = format!("{namespace}::{name}"); - expr = Expr::FuncDef { - name: namespaced_name, - params, - body: Box::new(body), - then: Box::new(expr), - bound: FuncDefBound::default(), - }; - } + let defs = defs + .into_iter() + .map(|(name, params, body)| (format!("{namespace}::{name}"), params, body)) + .collect(); + expr = wrap_defs(expr, defs); } // Transform NamespacedCall expressions to regular FuncCall expressions From 8125bad0e5ad6fd2be9e72b0abccb3c2bf3e4469 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 14:10:05 +1000 Subject: [PATCH 02/11] test(jq): cover transitive module includes and the cycle divergence Twelve cases over a new `run_jq_with_modules` helper (the multi-file sibling of `run_jq_with_module`, which every transitive case needs), one per row of the oracle matrix #2865's semantics were derived from: transitive include and import; the dependency not leaking to the includer; the dependency outranking the module's own sibling while that sibling is still exported; the top-level collision going the opposite way; self-recursion beating a same-named dependency, arity-scoped; the last-declared include winning inside a module; nested includes resolving against the global search path only, in both directions; a three-level chain and a diamond; `$__loc__` still naming the inner module (the #2774 regression the stamping order could have caused silently); and the shadow-candidate boundary that must not gain transitive names. The cycle test pins all four shapes (two-module, self-include, aliased spelling, import-side). It asserts succinctly's own shape rather than jq's, because jq has none: it exits 139 with nothing on either stream. The chain-size test is the regression lock for the referenced-closure filter -- 4 levels x 12 defs, which the unfiltered build could not have completed. It asserts completion rather than wall-clock, which would flake on a loaded CI box. Docs: a new ADR-0018 rule-4 entry for the cycle divergence, with the SIGSEGV transcript as the carve-out evidence; a second entry recording the three module-scoping quirks that *are* matched, since they read as bugs to the next person to touch `ModuleLoader` and the self-recursion filter has no other explanation; and the Module System section of the jq-language reference gains the transitive row plus pointers to both. Two gaps found while closing this are filed rather than recorded as divergences: #2950 (a dependency named after a builtin cannot shadow it inside the module body, since a module's own source is parsed with no shadow-candidate seeding) and #2951 (a module body sees `~/.jq` and sibling modules' defs, which real jq keeps out). Refs #2865 --- docs/compliance/jq/limitations.md | 69 +++++ docs/reference/jq-language.md | 16 ++ tests/jq_cli_tests.rs | 455 ++++++++++++++++++++++++++++++ 3 files changed, 540 insertions(+) diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 7d3a68536..578ea7a2b 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -6288,6 +6288,75 @@ change (`eval_label`/`each_label`/`each_label_generic` and both owned-identity/p `Label` arms would each need their non-matching-escape fallthrough routed through a resolvable call), out of #2687's stated scope. +### A module `include` cycle is a compile error, where jq segfaults — accepted divergence, ADR-0018 rule 4 (#2865) + +[#2865](https://github.com/rust-works/succinctly/issues/2865) made a module's own +`include`/`import` directives load transitively, which makes a cycle between two modules +reachable for the first time. Real jq 1.7.1 does not diagnose one at all — it recurses +until the process dies: + +```console +$ cat ca.jq +include "cb"; +def a: 1; +$ cat cb.jq +include "ca"; +def b: 2; + +$ jq -L . -n 'include "ca"; a'; echo "exit=$?" +exit=139 # SIGSEGV, nothing on stdout or stderr + +$ succinctly jq -L . -n 'include "ca"; a'; echo "exit=$?" +jq: error: module cycle detected: ca -> cb -> ca + +jq: 1 compile error +exit=3 +``` + +A module that includes itself behaves the same way in jq (`exit=139`), and reports +`module cycle detected: selfinc -> selfinc` here. + +This is the **cleanest** of ADR-0018 rule 4's carve-outs rather than a policy stretch: +the rule permits refusing the reference's behaviour where "matching would take the host +process down," and matching here means exactly a SIGSEGV. There is no reference *output* +to be faithful to — jq writes nothing to either stream — so the only open question was +which shape to leave through, and the answer is the one the other two compile-error kinds +already use (`jq: N compile error`, exit 3, per "Undefined functions and arity +mismatches" above). + +Detection keys on the **resolved** file rather than the module path as written, so one +module reachable under two spellings still closes a cycle +(`alia.jq` containing `include "./alia"` reports `alia -> ./alia`); the chain in the +message keeps the spellings, since those are what the source actually says. It cannot use +the module memo cache as its guard: a module is absent from that cache for exactly as long +as its own dependencies are loading, which is precisely the window in which a cycle closes. + +`test_module_cycle_is_a_compile_error_not_a_hang_2865` (`tests/jq_cli_tests.rs`) pins all +four shapes (two-module cycle, self-include, aliased spelling, `import`-side cycle). + +### Three module-scoping quirks 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 self-recursion filter in +`deps_excluding_self` has no other explanation. All three captured live against jq 1.7.1, +with `inner.jq` = `def g: 42;`: + +| Case | jq, and succinctly | +|--------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------| +| Module is `include "inner"; def g: 7; def h: g;` — what does `h` answer? | **`42`** — the dependency is innermost, so it beats the module's own same-name sibling one line above | +| ...and what does that module still *export* as `g`? | `7` — its own def | +| Same collision at the **top level**: `include "inner"; def g: 7; g` | `7` — the **opposite** way, because a filter's own defs bind at parse time before the module block is spliced in | +| A def's own recursive call, with a same-named dependency in scope | binds to **itself**: `def g: if . == 0 then "base" else (. - 1 \ | + +Two further module-scope gaps found while closing #2865 are genuine and still open, filed +rather than recorded as divergences: a dependency named after a builtin cannot shadow that +builtin *inside* the module body, because a module's own source is parsed with no +shadow-candidate seeding +([#2950](https://github.com/rust-works/succinctly/issues/2950)); and a module body can see +names it should not — `~/.jq`'s defs, and sibling `include`d modules' defs in a +declaration-order-dependent way — because every module is inlined into one flat def chain +([#2951](https://github.com/rust-works/succinctly/issues/2951)). + ## Provenance | Artifact | Path | diff --git a/docs/reference/jq-language.md b/docs/reference/jq-language.md index 025578ff9..8f2958f31 100644 --- a/docs/reference/jq-language.md +++ b/docs/reference/jq-language.md @@ -309,6 +309,22 @@ as opposed to a trailing same-line comment) are not implemented at all. - [x] `~/.jq` auto-loading (file or directory) - [x] `namespace::func` - Namespaced function calls - [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** +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). See +[jq Limitations](../compliance/jq/limitations.md#three-module-scoping-quirks-that-are-matched-and-read-as-bugs-2865) +for the full table and the two scoping gaps that remain open. + +An `include` cycle is reported as `module cycle detected: a -> b -> a` +(exit 3) — a deliberate ADR-0018 rule-4 divergence, since real jq +segfaults instead; see +[jq Limitations](../compliance/jq/limitations.md#a-module-include-cycle-is-a-compile-error-where-jq-segfaults--accepted-divergence-adr-0018-rule-4-2865). ### Succinctly Extensions These are succinctly-specific extensions not available in standard jq or yq: diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 7970ef095..93e1ab867 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -24758,6 +24758,461 @@ fn test_include_appends_jq_suffix_unconditionally_2702() -> Result<()> { Ok(()) } +/// #2865: write each `(name, contents)` pair as `.jq` in a fresh temp +/// dir and run `succinctly jq -L `. +/// +/// The multi-file sibling of [`run_jq_with_module`], which every transitive- +/// `include` case needs: the whole point is a module that itself names +/// another one. Same `TempDir`-outlives-the-spawn and raw-`Command` shape, +/// for the same reasons its doc comment gives. +fn run_jq_with_modules(modules: &[(&str, &str)], args: &[&str]) -> Result<(String, String, i32)> { + let temp_dir = tempfile::tempdir()?; + for (name, contents) in modules { + let path = temp_dir.path().join(format!("{name}.jq")); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + std::fs::write(path, contents)?; + } + let (output, code) = spawn_with_signal_retry( + || { + let mut command = Command::new(succinctly_bin()); + command.args(["jq", "-L"]).arg(temp_dir.path()).args(args); + command + }, + None, + )?; + Ok(( + String::from_utf8(output.stdout)?, + String::from_utf8(output.stderr)?, + code, + )) +} + +/// #2865: a module's own `include` is processed transitively -- the third +/// module's defs are available to the second module's body. +/// +/// `ModuleLoader` used to read only `program.expr` from a loaded module's +/// parse, discarding the `Program`'s own `includes`/`imports`, so `h`'s +/// reference to `g` was genuinely undefined by the time resolve ran. +/// +/// Both rows captured live from jq 1.7.1. +#[test] +fn test_module_include_is_transitive_2865() -> Result<()> { + let modules = [ + ("inner", "def g: 42;\n"), + ("outer", "include \"inner\";\ndef h: g;\n"), + ]; + + let (stdout, stderr, code) = run_jq_with_modules(&modules, &["-nc", r#"include "outer"; h"#])?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "42"); + + // A module's own `import` is processed the same way, under its namespace. + let imports = [ + ("inner", "def g: 42;\n"), + ("impouter", "import \"inner\" as i;\ndef h: i::g;\n"), + ]; + let (stdout, stderr, code) = + run_jq_with_modules(&imports, &["-nc", r#"include "impouter"; h"#])?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "42"); + + Ok(()) +} + +/// #2865: a transitively included def is **not** re-exported to the includer. +/// +/// The load-bearing half of why the fix wraps each exported def's *body* +/// rather than splicing the dependency into the exported chain -- splicing +/// would make `g` visible here, where jq reports a compile error. +#[test] +fn test_transitive_include_does_not_leak_to_the_includer_2865() -> Result<()> { + let modules = [ + ("inner", "def g: 42;\n"), + ("outer", "include \"inner\";\ndef h: g;\n"), + ]; + let (_, stderr, code) = run_jq_with_modules(&modules, &["-nc", r#"include "outer"; g"#])?; + assert_eq!(code, 3, "stderr: {stderr:?}"); + assert!(stderr.contains("g/0 is not defined"), "stderr: {stderr:?}"); + Ok(()) +} + +/// #2865: inside a module, a dependency **outranks the module's own +/// same-name sibling** -- and that sibling is still what the module exports. +/// +/// Both rows captured live from jq 1.7.1, and both are surprising enough to +/// be worth stating: `h` answers `42`, not the `7` sitting one line above it. +/// The same collision at the *top level* goes the other way (a filter's own +/// def wins), because a filter's defs bind at parse time; that row is locked +/// separately below so the two cannot be conflated. +#[test] +fn test_module_dependency_outranks_own_sibling_but_is_not_exported_2865() -> Result<()> { + let modules = [ + ("inner", "def g: 42;\n"), + ("shadow", "include \"inner\";\ndef g: 7;\ndef h: g;\n"), + ]; + + let (stdout, stderr, code) = run_jq_with_modules(&modules, &["-nc", r#"include "shadow"; h"#])?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!( + stdout.trim_end(), + "42", + "dependency must beat the own sibling" + ); + + let (stdout, stderr, code) = run_jq_with_modules(&modules, &["-nc", r#"include "shadow"; g"#])?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "7", "the module still exports its own g"); + + Ok(()) +} + +/// #2865: the top-level direction of the same collision, unchanged by this +/// fix and locked because the fix moves the code that decides it. +/// +/// `include "inner"; def g: 7; g` answers `7` in jq -- the filter's own def +/// wins -- where the module-internal collision above answers `42`. +#[test] +fn test_top_level_def_still_outranks_an_included_one_2865() -> Result<()> { + let (stdout, stderr, code) = run_jq_with_modules( + &[("inner", "def g: 42;\n")], + &["-nc", r#"include "inner"; def g: 7; g"#], + )?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "7"); + Ok(()) +} + +/// #2865: a def's own recursive call binds to **itself**, not to a +/// same-named dependency -- and the rule is arity-scoped. +/// +/// With `inner`'s `g/0` in scope, `def g: if . == 0 then "base" else (. - 1 | +/// g) end;` still recurses into itself and reaches `"base"`; without the +/// `deps_excluding_self` filter it would answer `42` on the first step. +/// +/// The arity half: a dependency `g/1` alongside an own `g/0` leaves both +/// reachable from the same body (`["own0","dep1arg"]`). Both captured live +/// from jq 1.7.1. +#[test] +fn test_module_def_self_recursion_beats_dependency_per_arity_2865() -> Result<()> { + let (stdout, stderr, code) = run_jq_with_modules( + &[ + ("inner", "def g: 42;\n"), + ( + "recself", + "include \"inner\";\ndef g: if . == 0 then \"base\" else (. - 1 | g) end;\n", + ), + ], + &["-nc", r#"include "recself"; 1 | g"#], + )?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), r#""base""#); + + let (stdout, stderr, code) = run_jq_with_modules( + &[ + ("inner1", "def g(x): \"dep1arg\";\n"), + ( + "arity", + "include \"inner1\";\ndef g: \"own0\";\ndef h: g;\ndef i2: g(1);\n", + ), + ], + &["-nc", r#"include "arity"; [h, i2]"#], + )?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), r#"["own0","dep1arg"]"#); + + Ok(()) +} + +/// #2865: among several `include`s *inside a module*, the last-declared one +/// wins a name collision -- the same rule #2682 established for the top +/// level, which is why both now go through the one `wrap_defs` ordering. +#[test] +fn test_last_declared_include_wins_inside_a_module_2865() -> Result<()> { + let (stdout, stderr, code) = run_jq_with_modules( + &[ + ("pa", "def foo: \"pa\";\n"), + ("pb", "def foo: \"pb\";\n"), + ("twoinc", "include \"pa\";\ninclude \"pb\";\ndef h: foo;\n"), + ], + &["-nc", r#"include "twoinc"; h"#], + )?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), r#""pb""#); + Ok(()) +} + +/// #2865: a nested `include` resolves against the **global search path +/// only**, never the including module's own directory. +/// +/// `sub/usedeep.jq` saying `include "deep"` fails even with `sub/deep.jq` +/// sitting right beside it, and succeeds once `sub` is itself on the search +/// path -- so reusing the search path verbatim is the faithful implementation +/// as well as the simple one. Both rows captured live from jq 1.7.1, and the +/// not-found case reports jq's own `module not found:` shape. +#[test] +fn test_nested_include_uses_the_global_search_path_only_2865() -> Result<()> { + let temp_dir = tempfile::tempdir()?; + std::fs::create_dir_all(temp_dir.path().join("sub"))?; + std::fs::write(temp_dir.path().join("sub/deep.jq"), "def d: 99;\n")?; + std::fs::write( + temp_dir.path().join("sub/usedeep.jq"), + "include \"deep\";\ndef ud: d;\n", + )?; + + let run = |extra_lib: bool| { + let temp_path = temp_dir.path().to_path_buf(); + spawn_with_signal_retry( + move || { + let mut command = Command::new(succinctly_bin()); + command.args(["jq", "-L"]).arg(&temp_path); + if extra_lib { + command.arg("-L").arg(temp_path.join("sub")); + } + command.args(["-nc", r#"include "sub/usedeep"; ud"#]); + command + }, + None, + ) + }; + + let (output, code) = run(false)?; + let stderr = String::from_utf8(output.stderr)?; + assert_eq!(code, 3, "stderr: {stderr:?}"); + assert!( + stderr.contains("module not found: deep"), + "stderr: {stderr:?}" + ); + + let (output, code) = run(true)?; + let stdout = String::from_utf8(output.stdout)?; + let stderr = String::from_utf8(output.stderr)?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "99"); + + Ok(()) +} + +/// #2865: a three-level chain, and a diamond where one module includes both +/// another module and that module's own dependency. +/// +/// The diamond is the case a naive memo-plus-recursion gets wrong by loading +/// `inner` twice into incompatible scopes; `84` is `g + h` with both reaching +/// the same `42`. Both captured live from jq 1.7.1. +#[test] +fn test_transitive_include_chain_and_diamond_2865() -> Result<()> { + let base = [ + ("inner", "def g: 42;\n"), + ("outer", "include \"inner\";\ndef h: g;\n"), + ("mid", "include \"inner\";\ndef m: g;\n"), + ("top", "include \"mid\";\ndef t: m;\n"), + ( + "dia", + "include \"inner\";\ninclude \"outer\";\ndef dd: g + h;\n", + ), + ]; + + let (stdout, stderr, code) = run_jq_with_modules(&base, &["-nc", r#"include "top"; t"#])?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "42", "three-level chain"); + + let (stdout, stderr, code) = run_jq_with_modules(&base, &["-nc", r#"include "dia"; dd"#])?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "84", "diamond"); + + Ok(()) +} + +/// #2865: a module cycle is reported and terminates. +/// +/// **A deliberate ADR-0018 rule-4 divergence**, under the explicit "matching +/// would take the host process down" carve-out: real jq 1.7.1 does not +/// diagnose this at all, it recurses until it dies -- +/// `jq -L . -n 'include "ca"; a'` exits **139** (SIGSEGV) with nothing on +/// either stream, and a self-including module does the same. So there is no +/// reference output to match; what this test pins is that succinctly leaves +/// through the same exit-3 `jq: 1 compile error` door as the other two +/// compile-error kinds, and that it leaves at all. +/// +/// The third row is why detection keys on the *resolved* file rather than the +/// module path as written: `alia.jq` including `"./alia"` is the same cycle +/// under two spellings. +#[test] +fn test_module_cycle_is_a_compile_error_not_a_hang_2865() -> Result<()> { + for (modules, filter, want_chain) in [ + ( + vec![ + ("ca", "include \"cb\";\ndef a: 1;\n"), + ("cb", "include \"ca\";\ndef b: 2;\n"), + ], + r#"include "ca"; a"#, + "ca -> cb -> ca", + ), + ( + vec![("selfinc", "include \"selfinc\";\ndef s: 5;\n")], + r#"include "selfinc"; s"#, + "selfinc -> selfinc", + ), + ( + vec![("alia", "include \"./alia\";\ndef q: 3;\n")], + r#"include "alia"; q"#, + "alia -> ./alia", + ), + ( + vec![ + ("ia", "import \"ib\" as b;\ndef x: 1;\n"), + ("ib", "import \"ia\" as a;\ndef y: 2;\n"), + ], + r#"include "ia"; x"#, + "ia -> ib -> ia", + ), + ] { + let (stdout, stderr, code) = run_jq_with_modules(&modules, &["-nc", filter])?; + assert_eq!(code, 3, "{filter}: stderr: {stderr:?}"); + assert_eq!(stdout, "", "{filter}"); + assert!( + stderr.contains(&format!("module cycle detected: {want_chain}")), + "{filter}: stderr: {stderr:?}" + ); + assert!( + stderr.contains("jq: 1 compile error"), + "{filter}: stderr: {stderr:?}" + ); + } + Ok(()) +} + +/// #2865 / #2774: `$__loc__` inside a **transitively** included def still +/// reports the innermost module's own path, not the module that included it. +/// +/// The regression this fix could most easily have caused silently: +/// `stamp_loc_file` overwrites `file` unconditionally, so binding a module's +/// dependencies before stamping its own defs would re-stamp the inner +/// module's already-correct `$__loc__` with the outer module's path. +#[test] +fn test_loc_in_a_transitively_included_def_names_the_inner_module_2865() -> Result<()> { + let temp_dir = tempfile::tempdir()?; + std::fs::write(temp_dir.path().join("locinner.jq"), "def li: $__loc__;\n")?; + std::fs::write( + temp_dir.path().join("locouter.jq"), + "include \"locinner\";\ndef lo: li;\n", + )?; + // The `TempDir` path can itself sit under a symlink (`/var` -> `/private/var` + // on macOS), and `$__loc__` reports the canonical path -- compare against + // the canonicalised dir, the same way #2774's own tests do. + let canonical = 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", r#"include "locouter"; lo"#]); + command + }, + None, + )?; + let stdout = String::from_utf8(output.stdout)?; + let stderr = String::from_utf8(output.stderr)?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!( + stdout.trim_end(), + format!( + r#"{{"file":"{}","line":1}}"#, + canonical.join("locinner.jq").display() + ) + ); + Ok(()) +} + +/// #2865 (risk R3): a transitively included name must **not** join the +/// shadow-candidate set `unqualified_def_names` seeds the main filter's +/// re-parse with -- it is not visible unqualified at the top level, so +/// `length` there is still the builtin. +/// +/// Stated as a test because the "helpful" edit that adds transitive names to +/// that set would break nothing else visibly. +/// +/// The first element pins the residual gap in the other direction, filed as +/// **#2950**: the dependency's `length` *should* shadow inside the module +/// body (jq answers `"dep-length"`), but a module's own source is parsed with +/// no shadow-candidate seeding, so the call lowers to the builtin and answers +/// `0` (`null | length`). Update this row when #2950 lands. +#[test] +fn test_transitive_names_do_not_seed_top_level_shadow_candidates_2865() -> Result<()> { + let (stdout, stderr, code) = run_jq_with_modules( + &[ + ("shlen", "def length: \"dep-length\";\n"), + ("usesh", "include \"shlen\";\ndef h: length;\n"), + ], + &["-nc", r#"include "usesh"; [h, ([1,2]|length)]"#], + )?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "[0,2]"); + + // ...while a module's *own* def still shadows for its includer (#2395), + // which this fix must not have disturbed. + let (stdout, stderr, code) = run_jq_with_modules( + &[("ownlen", "def length: \"own-length\";\n")], + &["-nc", r#"include "ownlen"; [1,2] | length"#], + )?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), r#""own-length""#); + + Ok(()) +} + +/// #2865 (plan step 6): a chain of modules does not multiply in size. +/// +/// Wrapping every dependency into every exported body compounds down a chain, +/// because each level's bodies already carry the level below: unfiltered, a +/// 3-module x 40-def chain measured 359 MB peak RSS against jq's 2.5 MB, and +/// a fourth level would have been tens of gigabytes. `deps_excluding_self`'s +/// referenced-closure filter (jq's own `block_bind_referenced` rule) is what +/// keeps it flat. +/// +/// Deliberately modest -- 4 levels x 12 defs, which the unfiltered build +/// could not have completed in any reasonable time or memory, while the +/// filtered one is instant. A wall-clock assertion would be a flake on a +/// loaded CI box; completing at all is the signal. +#[test] +fn test_transitive_include_chain_does_not_blow_up_2865() -> Result<()> { + const LEVELS: usize = 4; + const DEFS: usize = 12; + + let mut modules: Vec<(String, String)> = Vec::new(); + let mut base = String::new(); + for i in 0..DEFS { + base.push_str(&format!("def f0_{i}: {i};\n")); + } + modules.push(("chain0".to_string(), base)); + for level in 1..LEVELS { + let mut contents = format!("include \"chain{}\";\n", level - 1); + for i in 0..DEFS { + contents.push_str(&format!("def f{level}_{i}: f{}_{i} + 1;\n", level - 1)); + } + modules.push((format!("chain{level}"), contents)); + } + + let borrowed: Vec<(&str, &str)> = modules + .iter() + .map(|(name, contents)| (name.as_str(), contents.as_str())) + .collect(); + let filter = format!( + r#"include "chain{}"; f{}_{}"#, + LEVELS - 1, + LEVELS - 1, + DEFS - 1 + ); + let (stdout, stderr, code) = run_jq_with_modules(&borrowed, &["-nc", &filter])?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + // `f0_11` is 11, and each of the 3 levels above it adds 1. + assert_eq!(stdout.trim_end(), (DEFS - 1 + LEVELS - 1).to_string()); + 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. From 922fd0379c0e0d084e33237eef8522e07fae863f Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 14:17:36 +1000 Subject: [PATCH 03/11] style(jq): avoid bool::then in filter_map in deps_excluding_self --- src/bin/succinctly/jq_runner.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index d8fe6fc05..1d191b321 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -309,7 +309,8 @@ fn deps_excluding_self(deps: &FuncDefList, name: &str, arity: usize, body: &Expr deps.iter() .zip(keep) - .filter_map(|(dep, keep)| keep.then(|| dep.clone())) + .filter(|(_, keep)| *keep) + .map(|(dep, _)| dep.clone()) .collect() } From 69dca075c90ad1b2b962cd4473b3330be3ac93a5 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 14:46:17 +1000 Subject: [PATCH 04/11] fix(jq): correct module dependency binding after PR review Four behavioural findings from /code-review on PR #2954, each reproduced live against jq 1.7.1 before being acted on. 1. A dependency captured a def's own **parameter**. The wrap nests inside the parameter's binding, so `def f(g): g; def q: f(7);` answered 42 where jq answers 7, and `def f($g): [$g, g]` answered [7,42] where jq answers [7,7] -- a `$`-spelled parameter binds the bare call-site namespace too. `visible_defs_for` now also excludes anything named after one of the def's parameters, which covers a same-named sibling as well (jq gives the parameter there too). 2. `called_func_names` missed every sub-expression behind a `builtin_fallback`. `walk::any_subexpr`'s `FuncCall` arm does not descend into that field, which is sound only after `resolve::check` has run -- and a module's source has only just been parsed when its dependencies are chosen. A module defining `def limit:` turns `limit(1; g)` into a shadowable-call node with empty `args`, hiding `g`, so its dependency was filtered out and a program jq compiles became a compile error. Exactly the failure direction the filter's own doc comment claims cannot happen. 3. An exported def was not self-contained. Dropping the same-(name, arity) dependency removed it from the whole closure, not just from the def's own binding, so a sibling that needed it lost it: with `inner.jq` = `def g: 42; def k: g;` and `outer.jq` = `include "inner"; def g: k;`, jq answers 42 while succinctly recursed until it hit the depth cap. Each exported body now carries its module's own earlier siblings (taken from the already-bound list, so they are themselves self-contained) alongside its dependencies, which fixes this by construction and makes an exported body independent of its wrap site. `deps_excluding_self` becomes `visible_defs_for` accordingly. 4. The cycle chain was built from the whole load stack, so modules outside the cycle were named in it (`x -> ca -> cb -> ca`). It now starts at the repeat, matching the `Cycle` doc comment. Also from review, non-behavioural: the "keeps it flat" claim for the referenced-closure filter was shape-specific. It flattens the one-call-per-def chain completely (359 MB -> 10 MB), but a module whose defs each call several defs below still compounds -- measured 91 MB at 8x6 fan-out 3 and 361 MB at 14x4 fan-out 2, against jq's 2.6 MB. Correct output throughout, and not a regression (none of those programs compiled before this fix); filed as #2955 and recorded in limitations.md rather than claimed away. Two doc comments citing the `entry()` mutable borrow that #2865 removed are updated, and the limitations table that lost its last row to an escaped pipe is rewritten as a list. Refs #2865 --- docs/compliance/jq/limitations.md | 75 ++++++++--- docs/reference/jq-language.md | 2 +- src/bin/succinctly/jq_runner.rs | 199 ++++++++++++++++++++---------- tests/jq_cli_tests.rs | 126 +++++++++++++++++++ 4 files changed, 318 insertions(+), 84 deletions(-) diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 578ea7a2b..4dff3b0c2 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -6334,24 +6334,67 @@ as its own dependencies are loading, which is precisely the window in which a cy `test_module_cycle_is_a_compile_error_not_a_hang_2865` (`tests/jq_cli_tests.rs`) pins all four shapes (two-module cycle, self-include, aliased spelling, `import`-side cycle). -### Three module-scoping quirks that *are* matched, and read as bugs (#2865) +### Five 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 self-recursion filter in -`deps_excluding_self` has no other explanation. All three captured live against jq 1.7.1, -with `inner.jq` = `def g: 42;`: - -| Case | jq, and succinctly | -|--------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------| -| Module is `include "inner"; def g: 7; def h: g;` — what does `h` answer? | **`42`** — the dependency is innermost, so it beats the module's own same-name sibling one line above | -| ...and what does that module still *export* as `g`? | `7` — its own def | -| Same collision at the **top level**: `include "inner"; def g: 7; g` | `7` — the **opposite** way, because a filter's own defs bind at parse time before the module block is spliced in | -| A def's own recursive call, with a same-named dependency in scope | binds to **itself**: `def g: if . == 0 then "base" else (. - 1 \ | - -Two further module-scope gaps found while closing #2865 are genuine and still open, filed -rather than recorded as divergences: a dependency named after a builtin cannot shadow that -builtin *inside* the module body, because a module's own source is parsed with no -shadow-candidate seeding +otherwise read them as ones, and because the exclusions in `visible_defs_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 + `include "inner"; def g: 7; def h: g;`, `h` answers **`42`** — not the `7` one line + above it. The dependency is bound innermost. +2. **...while that module still exports its own `g`**, which answers `7`. +3. **The same collision at the top level goes the opposite way.** `include "inner"; def + g: 7; g` answers `7`, because a filter's own defs bind at parse time, before the + module block is spliced in. +4. **A def's own recursive call binds to itself, not to a same-named dependency, per + (name, arity).** With `inner`'s `g/0` in scope, a module's + `def g: if . == 0 then "base" else (. - 1 | g) end;` still answers `"base"`, never + `42`. Arity-scoped: a dependency `g/1` alongside an own `g/0` leaves both reachable + from the same body (`["own0","dep1arg"]`). +5. **A parameter beats both.** `def f(g): g; def q: f(7);` answers `7` even with a + dependency or a sibling named `g` in scope, and `def f($g): [$g, g]` answers `[7,7]` + — a `$`-spelled parameter binds the bare call-site namespace too. + +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. +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` = +`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`, +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_defs_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, and not a +regression — none of these programs compiled at all before #2865. 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. + +### Two module-scope gaps that are genuinely open + +Found while closing #2865, filed rather than recorded as divergences: a dependency named +after a builtin cannot shadow that builtin *inside* the module body, because a module's +own source is parsed with no shadow-candidate seeding ([#2950](https://github.com/rust-works/succinctly/issues/2950)); and a module body can see names it should not — `~/.jq`'s defs, and sibling `include`d modules' defs in a declaration-order-dependent way — because every module is inlined into one flat def chain diff --git a/docs/reference/jq-language.md b/docs/reference/jq-language.md index 8f2958f31..b50c725f5 100644 --- a/docs/reference/jq-language.md +++ b/docs/reference/jq-language.md @@ -318,7 +318,7 @@ 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). See -[jq Limitations](../compliance/jq/limitations.md#three-module-scoping-quirks-that-are-matched-and-read-as-bugs-2865) +[jq Limitations](../compliance/jq/limitations.md#five-module-scoping-rules-that-are-matched-and-read-as-bugs-2865) for the full table and the two scoping gaps that remain open. An `include` cycle is reported as `module cycle detected: a -> b -> a` diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index 1d191b321..8f2e72c8f 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -143,10 +143,13 @@ fn report_module_load_error(e: &ModuleLoadError) { /// Resolve a module path to a file path within `search_path`. /// -/// A free function rather than a `ModuleLoader` method (#2395): its only -/// caller, [`ModuleLoader::ensure_module_loaded`], holds a mutable borrow of -/// the module cache across the call, which an `&self` method could not -/// coexist with. It reads nothing but the search path either way. +/// A free function rather than a `ModuleLoader` method (#2395). The original +/// reason -- its caller holding a mutable borrow of the module cache across +/// the call -- went away with #2865, which had to drop that `entry()` +/// spelling to make the loader re-entrant; it stays a free function because +/// it reads nothing but the search path, and its caller +/// ([`ModuleLoader::load_and_bind_module`]) does re-enter `&mut self` around +/// it to load the module's own dependencies. fn resolve_module_in(search_path: &[PathBuf], module_path: &str) -> Option { // #2702: real jq appends `.jq` unconditionally -- `include "m.jq"` looks // for `m.jq.jq`, never `m.jq` itself. Confirmed live against jq 1.7.1. @@ -171,8 +174,8 @@ fn resolve_module_in(search_path: &[PathBuf], module_path: &str) -> Option Expr { /// over-keeps a little (a dependency `g/1` survives 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 self-recursion filter in [`deps_excluding_self`] still keys on +/// The self-recursion filter in [`visible_defs_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 @@ -230,8 +233,25 @@ fn called_func_names(expr: &Expr) -> BTreeSet { let mut names = BTreeSet::new(); succinctly::jq::walk::any_subexpr(expr, &mut |node| { match node { - Expr::FuncCall { name, .. } => { + Expr::FuncCall { + name, + builtin_fallback, + .. + } => { names.insert(name.clone()); + // `any_subexpr`'s own `FuncCall` arm does not descend into + // `builtin_fallback`, which is sound only *after* + // `resolve::check` has run -- and a module's source has just + // been parsed here, so it has not. A module that defines a + // builtin's name turns every call to that builtin into a + // shadowable-call node whose real sub-expressions live in the + // fallback with `args` left empty, so skipping it hides them: + // `def limit: "s"; def h: [limit(1; g)];` would not see `g` at + // all and would drop the dependency that defines it, turning a + // program jq compiles into a compile error. + if let Some(fallback) = builtin_fallback { + names.extend(called_func_names(fallback)); + } } Expr::NamespacedCall { namespace, name, .. @@ -245,60 +265,93 @@ fn called_func_names(expr: &Expr) -> BTreeSet { names } -/// The dependencies a def's body can actually reach: `deps` minus any entry -/// that would capture the def's own recursive calls, minus any entry nothing -/// in the body transitively calls (#2865). +/// The defs a module's exported def can actually reach, in [`wrap_defs`] +/// order: its module's own earlier siblings and its module's dependencies, +/// minus everything that must not capture a name in this body, minus +/// everything nothing in the body transitively calls (#2865). +/// +/// ### Why `siblings` is here at all /// -/// ### The self-recursion filter +/// Every def's body has to be resolved in **its own module's scope**, which +/// means a def handed to another module must already be self-contained -- +/// its sibling references cannot be left to find their target in whatever +/// chain it lands in. 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. Wrapping only dependencies left `k`'s `g` to resolve outwards +/// into `outer`'s own `g`, which is `k` -- unbounded recursion where jq +/// returns a number. Carrying the siblings closes that by construction, and +/// makes every exported body independent of its wrap site. /// -/// jq binds a def's own name inside its own body before it binds a dependency -/// of the same name, and the match is on **(name, arity)**, not name alone -- -/// so a dependency `g/1` survives when wrapping an own `g/0`, and both stay -/// reachable from that body. +/// Only siblings declared *before* the def are in scope, which is also what +/// makes this terminate: `def x: 1; def y: x; def x: 2;` binds `y`'s `x` to +/// the first one, inside a module exactly as at the top level. /// -/// ### The referenced-closure filter, and why it is not optional +/// ### Ordering /// -/// This is jq's own `block_bind_referenced` rule, and here it is a -/// **correctness-adjacent sizing requirement, not a micro-optimisation**: -/// wrapping every dependency into every exported body multiplies down a -/// chain, because each level's bodies already carry the level below. Measured -/// on a synthetic chain of 3 modules x 40 defs (Apple M-series, release -/// build), resolving one def at the top: +/// `siblings` first, `deps` second, so a dependency ends up **innermost** and +/// beats a same-named sibling -- jq's answer for a module written +/// `include "inner"; def g: 7; def h: g;` is `42`, not the `7` one line +/// above. /// -/// | chain depth | peak RSS | wall | -/// |--------------------------|----------|-------| -/// | 1 module (no wrapping) | 9 MB | 0.00s | -/// | 2 modules (one wrap) | 18 MB | 0.01s | -/// | 3 modules (two wraps) | 359 MB | 0.28s | -/// | jq 1.7.1, same 3 modules | 2.5 MB | 0.00s | +/// ### The three exclusions /// -/// A fourth level would be tens of gigabytes. With the filter the same -/// 3-module chain is flat, because each `c_i` calls exactly one `b_i`. +/// - **The def itself, by (name, arity).** jq binds a def's own recursive +/// call to itself before anything else: with `inner`'s `g/0` in scope, +/// `def g: if . == 0 then "base" else (. - 1 | g) end;` still answers +/// `"base"`. Arity-scoped, so a dependency `g/1` alongside an own `g/0` +/// leaves both reachable. +/// - **Anything named after one of this def's parameters, at arity 0.** A +/// parameter binds the bare call-site namespace (`Param::Dollar`'s `$g` +/// binds `g` too), and it wins: `def f(g): g; def q: f(7);` answers `7` in +/// jq even with a dependency or sibling `g` in scope. Without this the +/// wrap, which nests *inside* the parameter's own binding, would capture +/// the parameter's every use. +/// - **Anything nothing in the body transitively calls.** 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. Measured on a +/// synthetic chain of 40-def modules where each def calls one def from the +/// level below, resolving one def at the top: 9 MB peak RSS at one module, +/// 18 MB at two, **359 MB** at three (jq: 2.5 MB), and a fourth level would +/// have been tens of gigabytes. Filtered, the same chain is 10 MB and a +/// five-level one 11.6 MB. It is a *mitigation*, not a cure -- a module +/// whose defs each call several defs below still compounds; see +/// [`docs/compliance/jq/limitations.md`]. /// -/// Dropping an unreferenced dependency is unobservable: it is reachable from -/// nothing, so no name resolves to it, and jq agrees an unreferenced -/// dependency whose own body calls an undefined function is an error in -/// neither tool. The closure is seeded from the body and widened through the -/// body of every dependency it pulls in, which over-approximates (a -/// dependency's body was already bound at its own load, so some of its names -/// are satisfied internally) -- over-approximating only keeps something -/// harmless, while under-approximating would drop something needed. -fn deps_excluding_self(deps: &FuncDefList, name: &str, arity: usize, body: &Expr) -> FuncDefList { +/// Dropping an unreferenced def 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. The closure is seeded from the body +/// and widened through the body of everything it pulls in, which +/// over-approximates -- over-approximating only keeps something harmless, +/// where under-approximating would drop something a call genuinely needs. +fn visible_defs_for( + siblings: &FuncDefList, + deps: &FuncDefList, + name: &str, + params: &[Param], + body: &Expr, +) -> FuncDefList { + let param_names: BTreeSet<&str> = params.iter().map(Param::name).collect(); + let arity = params.len(); + let candidates: Vec<&(String, Vec, Expr)> = siblings.iter().chain(deps.iter()).collect(); + let mut wanted = called_func_names(body); - let mut keep = vec![false; deps.len()]; + let mut keep = vec![false; candidates.len()]; - // Fixed point: pulling a dependency in can widen `wanted` past - // dependencies already scanned, so rescan until a pass adds nothing. + // Fixed point: pulling one in can widen `wanted` past candidates already + // scanned, so rescan until a pass adds nothing. loop { let mut grew = false; - for (i, (dep_name, dep_params, dep_body)) in deps.iter().enumerate() { - if keep[i] || (dep_name == name && dep_params.len() == arity) { + for (i, (cand_name, cand_params, cand_body)) in candidates.iter().enumerate() { + let excluded = (cand_name == name && cand_params.len() == arity) + || (cand_params.is_empty() && param_names.contains(cand_name.as_str())); + if keep[i] || excluded { continue; } - if wanted.contains(dep_name) { + if wanted.contains(cand_name) { keep[i] = true; let before = wanted.len(); - wanted.extend(called_func_names(dep_body)); + wanted.extend(called_func_names(cand_body)); grew |= wanted.len() != before; } } @@ -307,10 +360,11 @@ fn deps_excluding_self(deps: &FuncDefList, name: &str, arity: usize, body: &Expr } } - deps.iter() + candidates + .into_iter() .zip(keep) .filter(|(_, keep)| *keep) - .map(|(dep, _)| dep.clone()) + .map(|(def, _)| (*def).clone()) .collect() } @@ -430,14 +484,14 @@ impl ModuleLoader { /// parse time; that row already passes and is unchanged.) /// - ...while that module's own `g` is still what it exports: `7`. /// - /// ### The self-recursion filter + /// ### What each body is wrapped in /// - /// [`deps_excluding_self`] drops any dependency whose **(name, arity)** - /// matches the def being wrapped, because jq binds a def's own recursive - /// call to itself before it binds the dependency: with `inner`'s `g/0` in - /// scope, `def g: if . == 0 then "base" else (. - 1 | g) end;` still - /// answers `"base"`, not `42`. It is arity-scoped, not name-scoped -- a - /// dependency `g/1` alongside an own `g/0` leaves both reachable. + /// [`visible_defs_for`] decides that per def: the module's own earlier + /// siblings (so an exported body is self-contained wherever it is spliced + /// in) then its dependencies (innermost, so they win), minus the def + /// itself by (name, arity), minus anything named after one of the def's + /// parameters, minus anything the body never transitively calls. Its own + /// doc comment carries the oracle row behind each of those. /// /// ### Search-path resolution /// @@ -455,9 +509,13 @@ impl ModuleLoader { })?; let canonical = canonical_or_self(&file_path); - if self.loading.iter().any(|(seen, _)| *seen == canonical) { - let mut chain: Vec = self - .loading + if let Some(at) = self.loading.iter().position(|(seen, _)| *seen == canonical) { + // From the repeat, not from the bottom of the stack: the modules + // that merely *led* to the cycle are not part of it, and naming + // them makes the chain read as a longer cycle than it is. Loading + // `x` (which includes `ca`, which includes `cb`, which includes + // `ca`) reports `ca -> cb -> ca`, not `x -> ca -> cb -> ca`. + let mut chain: Vec = self.loading[at..] .iter() .map(|(_, as_written)| as_written.clone()) .collect(); @@ -484,14 +542,21 @@ impl ModuleLoader { self.loading.pop(); let deps = deps?; - Ok(own - .into_iter() - .map(|(name, params, body)| { - let visible = deps_excluding_self(&deps, &name, params.len(), &body); - let body = wrap_defs(body, visible); - (name, params, body) - }) - .collect()) + // `bound` doubles as the sibling list: walking the module's defs in + // declaration order means def `i` sees exactly `bound[..i]`, the same + // lexical rule a filter's own defs follow. Siblings are taken from + // `bound` rather than from `own` deliberately -- an *already bound* + // sibling carries its own dependencies inside its body, so splicing it + // into a later def's wrap keeps it self-contained. The unbound form + // would arrive with its references still looking outward, into a chain + // that no longer has them. + let mut bound: FuncDefList = Vec::with_capacity(own.len()); + for (name, params, body) in own { + let visible = visible_defs_for(&bound, &deps, &name, ¶ms, &body); + let wrapped = wrap_defs(body, visible); + bound.push((name, params, wrapped)); + } + Ok(bound) } /// Every def one module's own `include`/`import` directives bring into diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 93e1ab867..5ed0ba24e 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -25164,6 +25164,132 @@ fn test_transitive_names_do_not_seed_top_level_shadow_candidates_2865() -> Resul Ok(()) } +/// #2865 (PR review): a def's **parameter** beats a same-named dependency or +/// sibling, in both spellings. +/// +/// The wrap nests inside the parameter's own binding, so without an explicit +/// exclusion it captured every use of the parameter: `def f(g): g; def q: +/// f(7);` answered `42` where jq answers `7`. A `$`-spelled parameter binds +/// the bare call-site namespace too (`def f($g): [$g, g]` is `[7,7]` in jq), +/// so the exclusion is by name, not by spelling. +#[test] +fn test_module_def_parameter_beats_dependency_and_sibling_2865() -> Result<()> { + for (module, want) in [ + ("include \"inner\";\ndef f(g): g;\ndef q: f(7);\n", "7"), + ( + "include \"inner\";\ndef f($g): [$g, g];\ndef q: f(7);\n", + "[7,7]", + ), + // ...and over a sibling of the module's own, not just a dependency. + ( + "include \"inner\";\ndef g2: 5;\ndef f(g2): g2;\ndef q: f(7);\n", + "7", + ), + ] { + let (stdout, stderr, code) = run_jq_with_modules( + &[("inner", "def g: 42;\n"), ("pshadow", module)], + &["-nc", r#"include "pshadow"; q"#], + )?; + assert_eq!(code, 0, "{module}: stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), want, "{module}"); + } + Ok(()) +} + +/// #2865 (PR review): a module that defines a builtin's name does not lose +/// the dependencies used *inside* that builtin's arguments. +/// +/// `walk::any_subexpr`'s `FuncCall` arm does not descend into +/// `builtin_fallback`, which is sound only after `resolve::check` has run -- +/// and a module's source has only just been parsed when its dependencies are +/// chosen. A module defining `def limit:` turns `limit(1; g)` into a +/// shadowable-call node with empty `args` and its real sub-expressions in the +/// fallback, so the reference to `g` was invisible and its dependency got +/// filtered out: jq answers `[42]`, succinctly raised `g/0 is not defined` +/// and exited 3. Exactly the "under-keeping turns a compiling program into a +/// compile error" case the filter must not produce. +#[test] +fn test_builtin_named_module_def_keeps_dependencies_in_its_arguments_2865() -> Result<()> { + for module in [ + "include \"inner\";\ndef limit: \"shadowed\";\ndef h: [limit(1; g)];\n", + "include \"inner\";\ndef error: \"shadowed\";\ndef h: [first(g)];\n", + ] { + let (stdout, stderr, code) = run_jq_with_modules( + &[("inner", "def g: 42;\n"), ("blt", module)], + &["-nc", r#"include "blt"; h"#], + )?; + assert_eq!(code, 0, "{module}: stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "[42]", "{module}"); + } + Ok(()) +} + +/// #2865 (PR review): an exported def is self-contained -- its references to +/// its own module's siblings travel with it, rather than resolving wherever +/// it gets spliced in. +/// +/// `inner.jq` = `def g: 42; def k: g;` and `outer.jq` = `include "inner"; def +/// g: k;`: jq answers `42`, since `k`'s `g` is `inner`'s. Wrapping only +/// dependencies left `k`'s `g` to resolve outward into `outer`'s own `g`, +/// which is `k` -- unbounded recursion (`exceeded maximum recursion depth`) +/// where jq returns a number. +#[test] +fn test_exported_module_def_carries_its_own_siblings_2865() -> Result<()> { + let (stdout, stderr, code) = run_jq_with_modules( + &[ + ("inner2", "def g: 42;\ndef k: g;\n"), + ("outer2", "include \"inner2\";\ndef g: k;\n"), + ], + &["-nc", r#"include "outer2"; g"#], + )?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "42"); + + // The same shape one step further out: a sibling that needs the module's + // own dependency, reached only through another sibling. + let (stdout, stderr, code) = run_jq_with_modules( + &[ + ("inner", "def g: 42;\n"), + ("sibdep", "include \"inner\";\ndef x: g;\ndef y: x;\n"), + ], + &["-nc", r#"include "sibdep"; y"#], + )?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "42"); + + Ok(()) +} + +/// #2865 (PR review): the reported cycle chain starts at the repeat, not at +/// the bottom of the load stack. +/// +/// A module that merely *leads* to a cycle is not part of it: loading `x`, +/// which includes `ca`, which includes `cb`, which includes `ca`, reports +/// `ca -> cb -> ca` and not `x -> ca -> cb -> ca`. The existing cycle test +/// only covers cycles rooted at the first module loaded, where the two spell +/// the same thing. +#[test] +fn test_cycle_chain_starts_at_the_repeat_not_the_stack_bottom_2865() -> Result<()> { + let (_, stderr, code) = run_jq_with_modules( + &[ + ("x", "include \"ca\";\ndef xx: 1;\n"), + ("ca", "include \"cb\";\ndef a: 1;\n"), + ("cb", "include \"ca\";\ndef b: 2;\n"), + ], + &["-nc", r#"include "x"; xx"#], + )?; + assert_eq!(code, 3, "stderr: {stderr:?}"); + assert!( + stderr.contains("module cycle detected: ca -> cb -> ca"), + "stderr: {stderr:?}" + ); + assert!( + !stderr.contains("x -> ca"), + "chain must not name modules outside the cycle: {stderr:?}" + ); + Ok(()) +} + /// #2865 (plan step 6): a chain of modules does not multiply in size. /// /// Wrapping every dependency into every exported body compounds down a chain, From aab65259c579752138d133fed3f044869afa9b1a Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 15:11:52 +1000 Subject: [PATCH 05/11] fix(jq): drop transitive closure widening, walk patterns, skip data imports Three more defects from a second /code-review pass on PR #2954, each reproduced live before being acted on. 1. **A regression against `main`, not the disclosed #2955 shape.** The referenced closure was widened transitively through each kept candidate's body. But every candidate is already *bound* -- its own references are satisfied inside it -- so re-deriving names from a bound body re-adds what it has already captured, pulls in the whole preceding sibling set, and doubles the body per def. A module with no `include` at all and 20 chained defs (`def f0: 0; def f1: f0 + 1; ...`) reached **13 GB** resident and six seconds, where `main` does it in milliseconds. One pass over the body, no widening, is both correct (self-containment is exactly what makes it sufficient) and linear: the same case is now 10 MB and instant. 2. `called_func_names` inherited `any_subexpr`'s blind spot for `Pattern`, whose "patterns hold only destructuring names, never an `Expr`" comment has been stale since #2677 gave object patterns computed keys. A dependency reached only from such a key was invisible and got filtered out: `def h: . as {(kf): $v} | $v;` in a module answered `undefined function: kf/0` where jq prints `1`. Same for `?//` alternatives and reduce/foreach pattern keys -- three separate `Expr` variants, all now walked via `map_pattern_subexprs` rather than a hand-rolled fourth copy of what a pattern contains. 3. `import "f" as $d;` is a *data* import, and `parse_import` drops the `$`, so `Import` cannot tell it from a module import. Loading one as a module fails with `module not found`, so a module that merely declared one stopped compiling -- harmless before this fix, since a module's own imports were never looked at. `module_resolves` skips an unresolvable import in the transitive path only; a missing `include` still reports jq's own clear error there. Data imports remain unimplemented at the top level, filed as #2956. Also: the fan-out table in limitations.md gets its re-measured 8x6 number (98 MB), and two test doc comments still naming `deps_excluding_self` follow the rename to `visible_defs_for`. Refs #2865 --- docs/compliance/jq/limitations.md | 2 +- src/bin/succinctly/jq_runner.rs | 98 +++++++++++++++------- tests/jq_cli_tests.rs | 131 +++++++++++++++++++++++++++++- 3 files changed, 197 insertions(+), 34 deletions(-) diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 4dff3b0c2..7a0574add 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -6377,7 +6377,7 @@ 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 | +| 8 levels x 6 defs, 3 calls each | 98 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 diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index 8f2e72c8f..3e1c02ae9 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -258,6 +258,26 @@ fn called_func_names(expr: &Expr) -> BTreeSet { } => { names.insert(format!("{namespace}::{name}")); } + // `any_subexpr` does not descend into a `Pattern` either -- its + // own comment that a pattern holds only destructuring names has + // been stale since #2677 gave object patterns computed keys. A + // call reached only from such a key (`. as {(kf): $v} | $v`) would + // otherwise be invisible, and its dependency dropped. + // + // `map_pattern_subexprs` rather than a hand-rolled walk: it is the + // one exhaustive definition of what a pattern contains, and a + // fourth copy of that is exactly what would drift. The rebuilt + // `Pattern` it returns is discarded. + Expr::Reduce { patterns, .. } + | Expr::Foreach { patterns, .. } + | Expr::AsPattern { patterns, .. } => { + for pattern in patterns { + succinctly::jq::walk::map_pattern_subexprs(pattern, &mut |key| { + names.extend(called_func_names(key)); + key.clone() + }); + } + } _ => {} } false @@ -320,10 +340,20 @@ fn called_func_names(expr: &Expr) -> BTreeSet { /// /// Dropping an unreferenced def 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. The closure is seeded from the body -/// and widened through the body of everything it pulls in, which -/// over-approximates -- over-approximating only keeps something harmless, -/// where under-approximating would drop something a call genuinely needs. +/// function is an error in neither tool. +/// +/// **One pass over the body suffices, with no transitive widening**, and the +/// widening version was actively harmful: every candidate here is already +/// bound, so its own references are satisfied *inside* its body and it needs +/// nothing further from this scope. Re-deriving names from a bound body +/// therefore re-adds what it has already captured, which pulls in the whole +/// preceding sibling set and doubles the body per def -- a module with **no +/// `include` at all** and 20 chained defs (`def f0: 0; def f1: f0 + 1; ...`) +/// reached 13 GB of resident memory, against milliseconds before #2865 +/// touched this code. Correctness rests on that self-containment, which is +/// why siblings are taken from the bound list; the cost is the one under +/// "Anything nothing in the body transitively calls" above, and is tracked as +/// #2955. fn visible_defs_for( siblings: &FuncDefList, deps: &FuncDefList, @@ -335,36 +365,16 @@ fn visible_defs_for( let arity = params.len(); let candidates: Vec<&(String, Vec, Expr)> = siblings.iter().chain(deps.iter()).collect(); - let mut wanted = called_func_names(body); - let mut keep = vec![false; candidates.len()]; - - // Fixed point: pulling one in can widen `wanted` past candidates already - // scanned, so rescan until a pass adds nothing. - loop { - let mut grew = false; - for (i, (cand_name, cand_params, cand_body)) in candidates.iter().enumerate() { - let excluded = (cand_name == name && cand_params.len() == arity) - || (cand_params.is_empty() && param_names.contains(cand_name.as_str())); - if keep[i] || excluded { - continue; - } - if wanted.contains(cand_name) { - keep[i] = true; - let before = wanted.len(); - wanted.extend(called_func_names(cand_body)); - grew |= wanted.len() != before; - } - } - if !grew { - break; - } - } + let wanted = called_func_names(body); candidates .into_iter() - .zip(keep) - .filter(|(_, keep)| *keep) - .map(|(def, _)| (*def).clone()) + .filter(|(cand_name, cand_params, _)| { + let excluded = (cand_name == name && cand_params.len() == arity) + || (cand_params.is_empty() && param_names.contains(cand_name.as_str())); + !excluded && wanted.contains(cand_name) + }) + .cloned() .collect() } @@ -575,6 +585,19 @@ 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. + /// Whether `module_path` names a file the search path can actually + /// resolve (#2865). + /// + /// Used only to skip a module's own *data* imports, which cannot be + /// distinguished from namespace imports on the `Import` node itself -- + /// see the call site. Deliberately not applied to `include`s: a missing + /// `include` is a genuine error jq reports too, and silently skipping one + /// would swap a clear `module not found` for a later, unexplained + /// `is not defined`. + fn module_resolves(&self, module_path: &str) -> bool { + resolve_module_in(&self.search_path, module_path).is_some() + } + fn module_dep_defs(&mut self, program: &Program) -> Result { let mut defs: FuncDefList = Vec::new(); @@ -587,7 +610,20 @@ impl ModuleLoader { // 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 *data* import (`import "f" as $d;`, which binds a `$`-variable to + // the file's parsed JSON rather than a namespace of defs) contributes + // no defs and must be skipped: `parse_import` drops the `$`, so + // `Import` cannot tell the two apart, and loading one as a module + // fails with `module not found`. Before #2865 a module declaring one + // was simply ignored; making it fatal would be a regression, and a + // directive that binds a variable has no business in a *def* + // dependency list either way. Data imports remain unimplemented at the + // top level too (#2956) -- unchanged here. for import in &program.imports { + if !self.module_resolves(&import.path) { + continue; + } let namespace = &import.alias; defs.extend( self.load_module(&import.path)? diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 5ed0ba24e..078b3d3ed 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -24889,7 +24889,7 @@ fn test_top_level_def_still_outranks_an_included_one_2865() -> Result<()> { /// /// With `inner`'s `g/0` in scope, `def g: if . == 0 then "base" else (. - 1 | /// g) end;` still recurses into itself and reaches `"base"`; without the -/// `deps_excluding_self` filter it would answer `42` on the first step. +/// `visible_defs_for` exclusion it would answer `42` on the first step. /// /// The arity half: a dependency `g/1` alongside an own `g/0` leaves both /// reachable from the same body (`["own0","dep1arg"]`). Both captured live @@ -25290,12 +25290,139 @@ fn test_cycle_chain_starts_at_the_repeat_not_the_stack_bottom_2865() -> Result<( Ok(()) } +/// #2865 (PR review, round 2): a module with **no `include` at all** does not +/// blow up as its own defs chain. +/// +/// The referenced closure used to be widened transitively through each kept +/// candidate's body. Every candidate is already *bound*, though, so its own +/// references are satisfied inside it -- re-deriving names from a bound body +/// re-adds what it has already captured, pulls in the whole preceding sibling +/// set, and doubles the body per def. A 20-def chain (`def f0: 0; def f1: f0 + +/// 1; ...`) reached **13 GB** of resident memory and six seconds, against +/// milliseconds before #2865 touched this code at all -- a regression against +/// `main`, and not the (disclosed, module-chain) shape of #2955. One pass over +/// the body, with no widening, is both correct and linear. +/// +/// 24 defs would have been minutes and hundreds of gigabytes on the widening +/// version, and is instant now; asserting completion is the signal, since a +/// wall-clock bound would flake on a loaded CI box. +#[test] +fn test_module_own_def_chain_does_not_blow_up_2865() -> Result<()> { + const DEFS: usize = 24; + let mut module = String::from("def f0: 0;\n"); + for i in 1..DEFS { + module.push_str(&format!("def f{i}: f{} + 1;\n", i - 1)); + } + let filter = format!(r#"include "chain"; f{}"#, DEFS - 1); + let (stdout, stderr, code) = run_jq_with_modules(&[("chain", &module)], &["-nc", &filter])?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), (DEFS - 1).to_string()); + Ok(()) +} + +/// #2865 (PR review, round 2): a dependency reached only from a **computed +/// destructuring key** survives the referenced-closure filter. +/// +/// `walk::any_subexpr` does not descend into a `Pattern` -- its own comment +/// that a pattern holds only destructuring names has been stale since #2677 +/// gave object patterns computed keys. A call appearing only there was +/// invisible, so its dependency was dropped and jq's answer became +/// `undefined function: kf/0`. All three pattern-bearing constructs are +/// covered, since they are three separate `Expr` variants. +#[test] +fn test_dependency_reached_only_from_a_pattern_key_survives_2865() -> Result<()> { + for (module, want) in [ + // `as` pattern + ("include \"kfin\";\ndef h: . as {(kf): $v} | $v;\n", "1"), + // `reduce` pattern + ( + "include \"kfin\";\ndef h: reduce (.,.) as {(kf): $v} (0; . + $v);\n", + "2", + ), + // `?//` alternatives + ( + "include \"kfin\";\ndef h: . as [$x] ?// {(kf): $v} | ($v // $x);\n", + "1", + ), + ] { + let (stdout, stderr, code) = run_jq_with_modules( + &[("kfin", "def kf: \"a\";\n"), ("kfout", module)], + &["-nc", r#"include "kfout"; {"a":1} | h"#], + )?; + assert_eq!(code, 0, "{module}: stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), want, "{module}"); + } + Ok(()) +} + +/// #2865 (PR review, round 2): a module that merely *declares* a data import +/// still loads. +/// +/// `import "f" as $d;` binds a `$`-variable to a file's parsed JSON rather +/// than a namespace of defs, and `parse_import` drops the `$` -- so `Import` +/// cannot tell it from a module import, and resolving one as a module fails +/// with `module not found`. Before #2865 a module's own imports were never +/// looked at, so declaring one was harmless; processing them transitively made +/// it fatal. Data imports themselves remain unimplemented (#2956); this only +/// pins that declaring one does not break the module around it. +#[test] +fn test_module_declaring_a_data_import_still_loads_2865() -> Result<()> { + let temp_dir = tempfile::tempdir()?; + std::fs::write(temp_dir.path().join("data.json"), "{\"z\":9}\n")?; + std::fs::write( + temp_dir.path().join("dimp.jq"), + "import \"data\" as $d;\ndef h: 7;\n", + )?; + + let (output, code) = spawn_with_signal_retry( + || { + let mut command = Command::new(succinctly_bin()); + command + .args(["jq", "-L"]) + .arg(temp_dir.path()) + .args(["-nc", r#"include "dimp"; h"#]); + command + }, + None, + )?; + let stdout = String::from_utf8(output.stdout)?; + let stderr = String::from_utf8(output.stderr)?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "7"); + + // A missing *include*, by contrast, is still the clear error jq reports -- + // `module_resolves` is deliberately not applied there. + std::fs::write( + temp_dir.path().join("bad.jq"), + "include \"nosuchmod\";\ndef h: 7;\n", + )?; + let (output, code) = spawn_with_signal_retry( + || { + let mut command = Command::new(succinctly_bin()); + command + .args(["jq", "-L"]) + .arg(temp_dir.path()) + .args(["-nc", r#"include "bad"; h"#]); + command + }, + None, + )?; + let stderr = String::from_utf8(output.stderr)?; + assert_eq!(code, 3, "stderr: {stderr:?}"); + assert!( + stderr.contains("module not found: nosuchmod"), + "stderr: {stderr:?}" + ); + + Ok(()) +} + /// #2865 (plan step 6): a chain of modules does not multiply in size. /// /// Wrapping every dependency into every exported body compounds down a chain, /// because each level's bodies already carry the level below: unfiltered, a /// 3-module x 40-def chain measured 359 MB peak RSS against jq's 2.5 MB, and -/// a fourth level would have been tens of gigabytes. `deps_excluding_self`'s +/// a fourth level would have been tens of gigabytes. `visible_defs_for`'s /// referenced-closure filter (jq's own `block_bind_referenced` rule) is what /// keeps it flat. /// From ca0f58d03ad4bd7f5be1e7705f5b7191429f801c Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 15:38:12 +1000 Subject: [PATCH 06/11] fix(jq): bind module siblings in local form, and separate data imports Two more defects from a third /code-review pass on PR #2954, both reproduced live and both regressions against `main`. 1. **Sibling wrapping duplicated a module's AST exponentially.** Each exported def was wrapped in the *sealed* form of its earlier siblings, nesting a full copy of every earlier sibling inside every later one. With one callee per def that is linear and the existing test passed; with two it doubles per def. A directive-free Fibonacci module, merely `include`d and never called, reached 190 MB at 18 defs, 4.1 GB at 24 and 32 GB at 28, against a flat 14.5 MB on `main` -- so the limitations entry claiming "not a regression, none of these programs compiled before" was false for that shape. A def now has two bound forms (`ModuleDef`): `local`, its body plus the module's dependencies, and the sealed form that leaves the module. A sibling splice uses `local`, because the chain it lands in already supplies the module's earlier siblings in the right lexical order; only what crosses a module boundary needs sealing, since the receiving scope cannot supply another module's internals. Binding is linear again: the Fibonacci module now measures 56 MB against `main`'s 53 MB at 20 defs, and the closure's fixed point -- restored, since a `local` sibling does still reach outward -- widens over the unbound `source` body, never a bound one. 2. **An unresolvable namespace import inside a module was silently ignored**, exiting 0 and later reporting `m::q/0 is not defined` instead of jq's `module not found: nosuchmod` and exit 3. The previous commit skipped on *resolvability* because `Import` could not tell a data import from a module import. It can now: `parse_import` recorded and discarded the `$`, and `Import::data` keeps it. Data imports are skipped because they contribute no defs; everything else resolves or errors exactly as jq does. The `module_dep_defs` doc comment, which the removed `module_resolves` had displaced onto itself, is back on its own function. The fan-out table in limitations.md gets its re-measured numbers and a corrected claim: the remaining blow-up needs a chain of *modules*, and a directive-free module now binds exactly as cheaply as before #2865. Filed while reviewing: #2957 -- `rewrite_namespaced_calls` has the same `Pattern` blind spot this PR fixed in `called_func_names`, so `. as {(m::kf): $v}` fails with `module 'm' not loaded`. Pre-existing and independent of transitive loading. Refs #2865 --- docs/compliance/jq/limitations.md | 9 +- src/bin/succinctly/jq_runner.rs | 225 ++++++++++++++++++------------ src/jq/expr.rs | 14 +- src/jq/parser.rs | 14 +- tests/jq_cli_tests.rs | 59 +++++++- 5 files changed, 213 insertions(+), 108 deletions(-) diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 7a0574add..b5ccd7c56 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -6378,14 +6378,17 @@ blocks, so it does not. Measured at #2865's own head (Apple M-series, release): |----------------------------------|---------------------|-------------| | 6 levels x 40 defs, 1 call each | 10 MB | 2.5 MB | | 8 levels x 6 defs, 3 calls each | 98 MB | 2.6 MB | -| 14 levels x 4 defs, 2 calls each | 361 MB | 2.6 MB | +| 14 levels x 4 defs, 2 calls each | 382 MB | 2.6 MB | The referenced-closure filter (jq's own `block_bind_referenced` rule, in `visible_defs_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, and not a -regression — none of these programs compiled at all before #2865. Tracked as +answer; this is a scalability limit of AST inlining, not a wrong result. It needs a +*chain of modules* to appear: a module's own defs are spliced into each other in their +`local` form (body plus dependencies, no siblings), which stays linear however many +siblings each one calls, so a directive-free module is bound exactly as cheaply as +before #2865. 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. diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index 3e1c02ae9..1493570a6 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -285,33 +285,40 @@ fn called_func_names(expr: &Expr) -> BTreeSet { names } -/// The defs a module's exported def can actually reach, in [`wrap_defs`] -/// order: its module's own earlier siblings and its module's dependencies, -/// minus everything that must not capture a name in this body, minus -/// everything nothing in the body transitively calls (#2865). +/// One of a module's own defs, in the two bound forms the loader needs +/// (#2865). /// -/// ### Why `siblings` is here at all -/// -/// Every def's body has to be resolved in **its own module's scope**, which -/// means a def handed to another module must already be self-contained -- -/// its sibling references cannot be left to find their target in whatever -/// chain it lands in. 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. Wrapping only dependencies left `k`'s `g` to resolve outwards -/// into `outer`'s own `g`, which is `k` -- unbounded recursion where jq -/// returns a number. Carrying the siblings closes that by construction, and -/// makes every exported body independent of its wrap site. -/// -/// Only siblings declared *before* the def are in scope, which is also what -/// makes this terminate: `def x: 1; def y: x; def x: 2;` binds `y`'s `x` to -/// the first one, inside a module exactly as at the top level. +/// The split is what keeps binding from growing exponentially. A def has to +/// be **sealed** before it leaves its module -- carrying everything it +/// references, so it resolves the same wherever it is spliced -- but a def +/// spliced as a *sibling*, into another def of the same module, must not be: +/// the chain it lands in already supplies the module's earlier siblings, in +/// the right lexical order. Splicing the sealed form there instead nests a +/// full copy of each sibling inside every later one, which for a module whose +/// defs each call two earlier siblings doubles per def. +struct ModuleDef { + name: String, + params: Vec, + /// The def's body exactly as parsed, binding nothing. The reference + /// graph is read off *this*, never off a bound form -- a bound body has + /// already captured names internally, so re-deriving from it re-adds + /// what it has satisfied and pulls in the whole preceding sibling set. + source: Expr, + /// The body wrapped in the module's own **dependencies** only. What gets + /// spliced when this def is needed as a sibling. + local: Expr, +} + +/// The defs one of a module's own defs can actually reach, in [`wrap_defs`] +/// order: the module's earlier siblings (in their `local` form) and the +/// module's dependencies, minus everything that must not capture a name in +/// this body, minus everything the body never transitively calls (#2865). /// /// ### Ordering /// -/// `siblings` first, `deps` second, so a dependency ends up **innermost** and -/// beats a same-named sibling -- jq's answer for a module written -/// `include "inner"; def g: 7; def h: g;` is `42`, not the `7` one line -/// above. +/// Siblings first, dependencies second, so a dependency ends up **innermost** +/// and beats a same-named sibling -- jq's answer for a module written +/// `include "inner"; def g: 7; def h: g;` is `42`, not the `7` one line above. /// /// ### The three exclusions /// @@ -328,34 +335,25 @@ fn called_func_names(expr: &Expr) -> BTreeSet { /// the parameter's every use. /// - **Anything nothing in the body transitively calls.** 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. Measured on a -/// synthetic chain of 40-def modules where each def calls one def from the -/// level below, resolving one def at the top: 9 MB peak RSS at one module, -/// 18 MB at two, **359 MB** at three (jq: 2.5 MB), and a fourth level would -/// have been tens of gigabytes. Filtered, the same chain is 10 MB and a -/// five-level one 11.6 MB. It is a *mitigation*, not a cure -- a module -/// whose defs each call several defs below still compounds; see -/// [`docs/compliance/jq/limitations.md`]. +/// a micro-optimisation: without it 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 def 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. /// -/// **One pass over the body suffices, with no transitive widening**, and the -/// widening version was actively harmful: every candidate here is already -/// bound, so its own references are satisfied *inside* its body and it needs -/// nothing further from this scope. Re-deriving names from a bound body -/// therefore re-adds what it has already captured, which pulls in the whole -/// preceding sibling set and doubles the body per def -- a module with **no -/// `include` at all** and 20 chained defs (`def f0: 0; def f1: f0 + 1; ...`) -/// reached 13 GB of resident memory, against milliseconds before #2865 -/// touched this code. Correctness rests on that self-containment, which is -/// why siblings are taken from the bound list; the cost is the one under -/// "Anything nothing in the body transitively calls" above, and is tracked as -/// #2955. +/// ### Why the closure widens over siblings but not dependencies +/// +/// A dependency arrives **sealed** from its own module, so it needs nothing +/// further from this scope and contributes nothing to the closure. A sibling +/// arrives in `local` form and does still reach outward for the module's +/// earlier siblings, so the closure widens through its +/// [`ModuleDef::source`] -- the unbound body, whose free names are the real +/// reference graph. fn visible_defs_for( - siblings: &FuncDefList, + siblings: &[ModuleDef], deps: &FuncDefList, name: &str, params: &[Param], @@ -363,18 +361,54 @@ fn visible_defs_for( ) -> FuncDefList { let param_names: BTreeSet<&str> = params.iter().map(Param::name).collect(); let arity = params.len(); - let candidates: Vec<&(String, Vec, Expr)> = siblings.iter().chain(deps.iter()).collect(); + let excluded = |cand_name: &str, cand_params: usize| { + (cand_name == name && cand_params == arity) + || (cand_params == 0 && param_names.contains(cand_name)) + }; - let wanted = called_func_names(body); + let mut wanted = called_func_names(body); + let mut keep = vec![false; siblings.len()]; + + // Fixed point over the siblings only: pulling one in can name an earlier + // sibling not yet scanned. Bounded and linear -- each sibling is admitted + // at most once, and widening reads its unbound `source`, never a bound + // body. + loop { + let mut grew = false; + for (i, sibling) in siblings.iter().enumerate() { + if keep[i] || excluded(&sibling.name, sibling.params.len()) { + continue; + } + if wanted.contains(&sibling.name) { + keep[i] = true; + let before = wanted.len(); + wanted.extend(called_func_names(&sibling.source)); + grew |= wanted.len() != before; + } + } + if !grew { + break; + } + } - candidates - .into_iter() - .filter(|(cand_name, cand_params, _)| { - let excluded = (cand_name == name && cand_params.len() == arity) - || (cand_params.is_empty() && param_names.contains(cand_name.as_str())); - !excluded && wanted.contains(cand_name) + siblings + .iter() + .zip(keep) + .filter(|(_, keep)| *keep) + .map(|(sibling, _)| { + ( + sibling.name.clone(), + sibling.params.clone(), + sibling.local.clone(), + ) }) - .cloned() + .chain( + deps.iter() + .filter(|(dep_name, dep_params, _)| { + !excluded(dep_name, dep_params.len()) && wanted.contains(dep_name) + }) + .cloned(), + ) .collect() } @@ -552,21 +586,42 @@ impl ModuleLoader { self.loading.pop(); let deps = deps?; - // `bound` doubles as the sibling list: walking the module's defs in - // declaration order means def `i` sees exactly `bound[..i]`, the same - // lexical rule a filter's own defs follow. Siblings are taken from - // `bound` rather than from `own` deliberately -- an *already bound* - // sibling carries its own dependencies inside its body, so splicing it - // into a later def's wrap keeps it self-contained. The unbound form - // would arrive with its references still looking outward, into a chain - // that no longer has them. - let mut bound: FuncDefList = Vec::with_capacity(own.len()); - for (name, params, body) in own { - let visible = visible_defs_for(&bound, &deps, &name, ¶ms, &body); - let wrapped = wrap_defs(body, visible); - bound.push((name, params, wrapped)); - } - Ok(bound) + // Two passes, for the reason [`ModuleDef`] gives. First each def's + // `local` form -- its body wrapped in the module's dependencies alone, + // which is what a *sibling* splice needs, since the chain it lands in + // already carries the module's earlier siblings. + let locals: Vec = own + .into_iter() + .map(|(name, params, body)| { + let dep_wrap = visible_defs_for(&[], &deps, &name, ¶ms, &body); + let local = wrap_defs(body.clone(), dep_wrap); + ModuleDef { + name, + params, + source: body, + local, + } + }) + .collect(); + + // Then the sealed form that leaves the module: each def wrapped in the + // siblings declared *before* it plus the dependencies, so it resolves + // identically wherever it is later spliced. Walking in declaration + // order means def `i` sees exactly `locals[..i]` -- the same lexical + // rule a filter's own defs follow. + Ok(locals + .iter() + .enumerate() + .map(|(i, def)| { + let visible = + visible_defs_for(&locals[..i], &deps, &def.name, &def.params, &def.source); + ( + def.name.clone(), + def.params.clone(), + wrap_defs(def.source.clone(), visible), + ) + }) + .collect()) } /// Every def one module's own `include`/`import` directives bring into @@ -585,19 +640,6 @@ 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. - /// Whether `module_path` names a file the search path can actually - /// resolve (#2865). - /// - /// Used only to skip a module's own *data* imports, which cannot be - /// distinguished from namespace imports on the `Import` node itself -- - /// see the call site. Deliberately not applied to `include`s: a missing - /// `include` is a genuine error jq reports too, and silently skipping one - /// would swap a clear `module not found` for a later, unexplained - /// `is not defined`. - fn module_resolves(&self, module_path: &str) -> bool { - resolve_module_in(&self.search_path, module_path).is_some() - } - fn module_dep_defs(&mut self, program: &Program) -> Result { let mut defs: FuncDefList = Vec::new(); @@ -613,17 +655,14 @@ impl ModuleLoader { // // A *data* import (`import "f" as $d;`, which binds a `$`-variable to // the file's parsed JSON rather than a namespace of defs) contributes - // no defs and must be skipped: `parse_import` drops the `$`, so - // `Import` cannot tell the two apart, and loading one as a module - // fails with `module not found`. Before #2865 a module declaring one - // was simply ignored; making it fatal would be a regression, and a - // directive that binds a variable has no business in a *def* - // dependency list either way. Data imports remain unimplemented at the - // top level too (#2956) -- unchanged here. - for import in &program.imports { - if !self.module_resolves(&import.path) { - continue; - } + // no defs, and resolving one as a module reports `module not found` + // where jq reads `.json`. Before #2865 a module's own imports + // were never looked at, so declaring one was harmless; processing them + // transitively made it fatal. `Import::data` (#2865) is what separates + // the two -- skipping on *resolvability* instead would have silently + // swallowed a genuinely missing module, which jq reports and exits 3 + // for. Data imports themselves remain unimplemented (#2956). + for import in program.imports.iter().filter(|import| !import.data) { let namespace = &import.alias; defs.extend( self.load_module(&import.path)? diff --git a/src/jq/expr.rs b/src/jq/expr.rs index d22236761..4666dcb26 100644 --- a/src/jq/expr.rs +++ b/src/jq/expr.rs @@ -1073,8 +1073,20 @@ pub struct Import { /// unconditionally when resolving it to a file (#2702), so a path that /// already ends in `.jq` is not stripped or treated specially here. pub path: String, - /// The namespace alias + /// The namespace alias, without the `$` a data import spells it with -- + /// see [`Self::data`]. pub alias: String, + /// Whether this was written `import "f" as $d;` (a **data** import, + /// binding the file's parsed JSON to a `$`-variable) rather than + /// `import "m" as m;` (a **module** import, binding a namespace of defs). + /// + /// The two are different directives that happen to share a keyword, and + /// `alias` cannot tell them apart because the `$` is not part of the + /// identifier. Recorded here (#2865) so a module loader can decline to + /// resolve a data import as a module -- resolving one that way reports + /// `module not found`, where jq reads `.json`. Data imports are + /// otherwise still unimplemented; see #2956. + pub data: bool, /// Optional metadata overrides pub metadata: Option>, } diff --git a/src/jq/parser.rs b/src/jq/parser.rs index 14cbbcd59..10f98d0fb 100644 --- a/src/jq/parser.rs +++ b/src/jq/parser.rs @@ -6845,13 +6845,14 @@ impl<'a> Parser<'a> { self.consume_keyword("as"); self.skip_ws(); - // Parse the alias (optionally prefixed with $) - let alias = if self.peek() == Some('$') { + // Parse the alias (optionally prefixed with $). The `$` is what + // distinguishes a *data* import from a module import, so record it + // rather than just consuming it (#2865) -- `Import::data`. + let data = self.peek() == Some('$'); + if data { self.next(); - self.parse_ident()? - } else { - self.parse_ident()? - }; + } + let alias = self.parse_ident()?; self.skip_ws(); @@ -6874,6 +6875,7 @@ impl<'a> Parser<'a> { Ok(Import { path, alias, + data, metadata, }) } diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 078b3d3ed..0984987fd 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -25303,12 +25303,24 @@ fn test_cycle_chain_starts_at_the_repeat_not_the_stack_bottom_2865() -> Result<( /// `main`, and not the (disclosed, module-chain) shape of #2955. One pass over /// the body, with no widening, is both correct and linear. /// -/// 24 defs would have been minutes and hundreds of gigabytes on the widening -/// version, and is instant now; asserting completion is the signal, since a -/// wall-clock bound would flake on a loaded CI box. +/// A second round of review found the single-callee chain above too weak a +/// lock: the *sealed* form of each def was being spliced where a sibling was +/// needed, nesting a full copy of every earlier sibling inside every later +/// one. That only shows when a def calls **more than one** earlier sibling, +/// so the second shape below is a Fibonacci chain -- merely `include`d, never +/// called, since the blow-up happens at load time. It reached 190 MB at 18 +/// defs, 4.1 GB at 24 and 32 GB at 28, against a flat 14.5 MB on `main`. +/// Splicing the `local` form (body plus dependencies, no siblings) keeps it +/// linear, because the chain a sibling lands in already supplies the earlier +/// siblings. +/// +/// Asserting completion rather than a wall-clock bound, which would flake on +/// a loaded CI box. #[test] fn test_module_own_def_chain_does_not_blow_up_2865() -> Result<()> { const DEFS: usize = 24; + + // One callee per def. let mut module = String::from("def f0: 0;\n"); for i in 1..DEFS { module.push_str(&format!("def f{i}: f{} + 1;\n", i - 1)); @@ -25317,6 +25329,18 @@ fn test_module_own_def_chain_does_not_blow_up_2865() -> Result<()> { let (stdout, stderr, code) = run_jq_with_modules(&[("chain", &module)], &["-nc", &filter])?; assert_eq!(code, 0, "stderr: {stderr:?}"); assert_eq!(stdout.trim_end(), (DEFS - 1).to_string()); + + // Two callees per def, and no directives at all in the module. Loading it + // is the whole test -- `1` never calls any of them. + let mut fib = String::from("def f0: 0;\ndef f1: 1;\n"); + for i in 2..DEFS { + fib.push_str(&format!("def f{i}: f{} + f{};\n", i - 1, i - 2)); + } + let (stdout, stderr, code) = + run_jq_with_modules(&[("fib", &fib)], &["-nc", r#"include "fib"; 1"#])?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "1"); + Ok(()) } @@ -25390,8 +25414,33 @@ fn test_module_declaring_a_data_import_still_loads_2865() -> Result<()> { assert_eq!(code, 0, "stderr: {stderr:?}"); assert_eq!(stdout.trim_end(), "7"); - // A missing *include*, by contrast, is still the clear error jq reports -- - // `module_resolves` is deliberately not applied there. + // A missing *namespace* import, by contrast, is still jq's own clear + // error -- the skip keys on `Import::data`, not on whether the path + // happens to resolve, so an unresolvable `import "m" as m;` is not + // swallowed (jq exits 3 with `module not found` here too). + std::fs::write( + temp_dir.path().join("badimp.jq"), + "import \"nosuchmod\" as m;\ndef q: 1;\n", + )?; + let (output, code) = spawn_with_signal_retry( + || { + let mut command = Command::new(succinctly_bin()); + command + .args(["jq", "-L"]) + .arg(temp_dir.path()) + .args(["-nc", r#"include "badimp"; q"#]); + command + }, + None, + )?; + let stderr = String::from_utf8(output.stderr)?; + assert_eq!(code, 3, "stderr: {stderr:?}"); + assert!( + stderr.contains("module not found: nosuchmod"), + "stderr: {stderr:?}" + ); + + // ...and so is a missing `include`. std::fs::write( temp_dir.path().join("bad.jq"), "include \"nosuchmod\";\ndef h: 7;\n", From d7ce31aa8ab93b0aea5977063c6c503cb645fbb8 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 15:54:38 +1000 Subject: [PATCH 07/11] fix(jq): keep an excluded sibling name excluded for its fellow siblings Two defects from a fourth /code-review pass on PR #2954, one root cause, both regressions against `main` (which splices a module's defs flat). `visible_defs_for`'s exclusions -- the def's own (name, arity), and any sibling sharing a parameter's name -- are decided on the *including* def's behalf. But a sibling spliced in `local` form carries its own references outward into that same scope, where the excluded name now resolves to whatever displaced it: - `def f: 1; def k: f; def h(f): k;` -- `h`'s parameter displaces the sibling `f`, and `k`'s own `f` then found the parameter. `include "m"; h(99)` answers `1` in jq; this branch answered `99`, and `[1,7]` became `[7,7]` in the `$f` spelling. - `def h: "first"; def g: h; def h: "second-" + g;` -- the second `h` excludes the first, and `g`'s `h` then found the second, recursively. jq answers `"second-first"`; this branch hit the depth cap and exited 5. A sibling that names something the including def excludes is now spliced in its `sealed` form, where that reference is already bound and cannot be recaptured. `ModuleDef` carries both forms, built in one declaration-order walk so an earlier sibling's sealed body is available when a later def needs it. Nothing is excluded for almost any def, so the sealed fallback is confined to these two shapes: a Fibonacci-shaped module with a parameter collision stacked on top still binds in 10 MB, against `main`'s 9 MB for the same module without one. Tests cover both shapes in both parameter spellings, with the equivalent single-filter programs as controls -- they never went through the module loader and have always matched jq. limitations.md's matched-rules list gains the two rules and the reason the fallback exists. Refs #2865 --- docs/compliance/jq/limitations.md | 12 ++- src/bin/succinctly/jq_runner.rs | 127 ++++++++++++++++++++---------- tests/jq_cli_tests.rs | 59 ++++++++++++++ 3 files changed, 157 insertions(+), 41 deletions(-) diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index b5ccd7c56..4a7acb4a5 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -6334,7 +6334,7 @@ as its own dependencies are loading, which is precisely the window in which a cy `test_module_cycle_is_a_compile_error_not_a_hang_2865` (`tests/jq_cli_tests.rs`) pins all four shapes (two-module cycle, self-include, aliased spelling, `import`-side cycle). -### Five module-scoping rules that *are* matched, and read as bugs (#2865) +### 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_defs_for` have no @@ -6356,8 +6356,18 @@ other explanation. All captured live against jq 1.7.1, with `inner.jq` = `def g: dependency or a sibling named `g` in scope, and `def f($g): [$g, g]` answers `[7,7]` — a `$`-spelled parameter binds the bare call-site namespace too. +6. **An excluded name stays excluded for the siblings spliced alongside it.** In + `def f: 1; def k: f; def h(f): k;`, `h(99)` answers `1` — `k`'s `f` is the module's + `f`, not `h`'s parameter, even though the parameter displaces `f` for `h`'s own body. +7. **An in-module redefinition does not capture an earlier sibling's call.** In + `def h: "first"; def g: h; def h: "second-" + g;`, `h` answers `"second-first"` — + `g`'s `h` is the first one, bound where `g` was declared. + 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. +Rules 6 and 7 are why a sibling that *names* one of those excluded defs is spliced in its +own fully-bound form rather than the cheaper one — the exclusion is decided for the +including def, and a sibling carries its references outward into that same scope. Rules 1-3 are why the wrap goes around each exported def's **body** rather than being spliced into the module's exported chain. diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index 1493570a6..87d7688a7 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -305,8 +305,14 @@ struct ModuleDef { /// what it has satisfied and pulls in the whole preceding sibling set. source: Expr, /// The body wrapped in the module's own **dependencies** only. What gets - /// spliced when this def is needed as a sibling. + /// spliced when this def is needed as a sibling, in the ordinary case + /// where the chain it lands in can supply its own sibling references. local: Expr, + /// The fully bound body -- dependencies *and* earlier siblings -- i.e. + /// what this def exports. Also what a sibling splice falls back to when + /// the receiving def excludes a name that sibling references; see + /// [`visible_defs_for`]'s "when a sibling has to be sealed anyway". + sealed: Expr, } /// The defs one of a module's own defs can actually reach, in [`wrap_defs`] @@ -344,6 +350,27 @@ struct ModuleDef { /// jq agrees an unreferenced dependency whose own body calls an undefined /// function is an error in neither tool. /// +/// ### When a sibling has to be sealed anyway +/// +/// The exclusions are decided on the *including* def's behalf, but a kept +/// sibling is spliced into that same scope and carries its own references +/// outward into it -- so an excluded name is excluded for the sibling too, +/// and resolves to whatever took its place. Both shapes are real: +/// +/// - `def f: 1; def k: f; def h(f): k;` -- `h`'s parameter `f` displaces the +/// sibling `f`, and `k`'s own `f` then resolves to the parameter. +/// `include "m"; h(99)` answers `1` in jq; splicing `k` in `local` form +/// answered `99`. +/// - `def h: "first"; def g: h; def h: "second-" + g;` -- the second `h` +/// excludes the first, and `g`'s `h` then resolves to the second, +/// recursively. jq answers `"second-first"`; `local` form recursed until it +/// hit the depth cap. +/// +/// A sibling naming an excluded name is therefore spliced in its **sealed** +/// form, where that reference is already bound and cannot be recaptured. +/// Nothing is excluded for almost every def, so the sealed fallback -- and +/// the nesting it reintroduces -- is confined to the two shapes above. +/// /// ### Why the closure widens over siblings but not dependencies /// /// A dependency arrives **sealed** from its own module, so it needs nothing @@ -366,6 +393,16 @@ fn visible_defs_for( || (cand_params == 0 && param_names.contains(cand_name)) }; + // The sibling names this def excludes and that some sibling therefore + // cannot be allowed to reach past. Empty for almost every def -- it takes + // either an in-module redefinition of the same (name, arity) or a + // parameter sharing a sibling's name. + let shadowed: BTreeSet<&str> = siblings + .iter() + .filter(|sibling| excluded(&sibling.name, sibling.params.len())) + .map(|sibling| sibling.name.as_str()) + .collect(); + let mut wanted = called_func_names(body); let mut keep = vec![false; siblings.len()]; @@ -396,11 +433,24 @@ fn visible_defs_for( .zip(keep) .filter(|(_, keep)| *keep) .map(|(sibling, _)| { - ( - sibling.name.clone(), - sibling.params.clone(), - sibling.local.clone(), - ) + // A `local` sibling still reaches outward for its own sibling + // references, and they land in *this* def's scope -- where an + // excluded name resolves to something else entirely (this def + // itself, or one of its parameters). Splice the sealed form for + // any sibling that names one, so its references are already bound + // and cannot be recaptured. `shadowed` is empty for almost every + // def, so this costs nothing in the ordinary case; it is checked + // against the unbound `source`, the real reference graph. + let body = if !shadowed.is_empty() + && called_func_names(&sibling.source) + .iter() + .any(|called| shadowed.contains(called.as_str())) + { + sibling.sealed.clone() + } else { + sibling.local.clone() + }; + (sibling.name.clone(), sibling.params.clone(), body) }) .chain( deps.iter() @@ -586,41 +636,38 @@ impl ModuleLoader { self.loading.pop(); let deps = deps?; - // Two passes, for the reason [`ModuleDef`] gives. First each def's - // `local` form -- its body wrapped in the module's dependencies alone, - // which is what a *sibling* splice needs, since the chain it lands in - // already carries the module's earlier siblings. - let locals: Vec = own - .into_iter() - .map(|(name, params, body)| { - let dep_wrap = visible_defs_for(&[], &deps, &name, ¶ms, &body); - let local = wrap_defs(body.clone(), dep_wrap); - ModuleDef { - name, - params, - source: body, - local, - } - }) - .collect(); + // Both bound forms per def, for the reason [`ModuleDef`] gives. The + // `local` form is the body wrapped in the module's dependencies alone, + // which is what a *sibling* splice normally needs, since the chain it + // lands in already carries the module's earlier siblings... + // ...then, in the same declaration-order walk, the sealed form that + // leaves the module: each def wrapped in the siblings declared + // *before* it plus the dependencies, so it resolves identically + // wherever it is later spliced. Def `i` sees exactly `defs[..i]` -- + // the same lexical rule a filter's own defs follow -- and each entry + // there already carries its own `sealed` form, which + // `visible_defs_for` falls back to for a sibling that names something + // this def excludes. + let mut defs: Vec = Vec::with_capacity(own.len()); + for (name, params, body) in own { + let dep_wrap = visible_defs_for(&[], &deps, &name, ¶ms, &body); + let local = wrap_defs(body.clone(), dep_wrap); + + let visible = visible_defs_for(&defs, &deps, &name, ¶ms, &body); + let sealed = wrap_defs(body.clone(), visible); + + defs.push(ModuleDef { + name, + params, + source: body, + local, + sealed, + }); + } - // Then the sealed form that leaves the module: each def wrapped in the - // siblings declared *before* it plus the dependencies, so it resolves - // identically wherever it is later spliced. Walking in declaration - // order means def `i` sees exactly `locals[..i]` -- the same lexical - // rule a filter's own defs follow. - Ok(locals - .iter() - .enumerate() - .map(|(i, def)| { - let visible = - visible_defs_for(&locals[..i], &deps, &def.name, &def.params, &def.source); - ( - def.name.clone(), - def.params.clone(), - wrap_defs(def.source.clone(), visible), - ) - }) + Ok(defs + .into_iter() + .map(|def| (def.name, def.params, def.sealed)) .collect()) } diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 0984987fd..bd77b47b4 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -25344,6 +25344,65 @@ fn test_module_own_def_chain_does_not_blow_up_2865() -> Result<()> { Ok(()) } +/// #2865 (PR review, round 3): a name this def excludes stays excluded for +/// the siblings spliced alongside it. +/// +/// The exclusions are decided on the *including* def's behalf, but a sibling +/// spliced in `local` form carries its own references outward into that same +/// scope -- where the excluded name resolves to whatever displaced it. Both +/// shapes are regressions against `main`, which splices flat: +/// +/// - a parameter displacing a sibling that another sibling calls +/// (`def f: 1; def k: f; def h(f): k;` -- jq says `h(99)` is `1`, the +/// `local` splice said `99`), in both parameter spellings; +/// - an in-module redefinition (`def h: "first"; def g: h; def h: "second-" + +/// g;` -- jq says `"second-first"`, the `local` splice recursed into the +/// second `h` until it hit the depth cap). +/// +/// Both are fixed by splicing the sibling's *sealed* form when it names +/// something excluded. The equivalent top-level filters have always answered +/// correctly, and are included as controls. +#[test] +fn test_excluded_sibling_name_stays_excluded_for_other_siblings_2865() -> Result<()> { + for (module, filter, want) in [ + ( + "def f: 1;\ndef k: f;\ndef h(f): k;\n", + r#"include "m"; h(99)"#, + "1", + ), + ( + "def f: 1;\ndef k: f;\ndef h($f): [k, $f];\n", + r#"include "m"; h(7)"#, + "[1,7]", + ), + ( + "def h: \"first\";\ndef g: h;\ndef h: \"second-\" + g;\n", + r#"include "m"; h"#, + r#""second-first""#, + ), + ] { + let (stdout, stderr, code) = run_jq_with_modules(&[("m", module)], &["-nc", filter])?; + assert_eq!(code, 0, "{module}: stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), want, "{module}"); + } + + // Controls: the same programs written as one filter, which never went + // through the module loader and have always matched jq. + for (filter, want) in [ + ("def f: 1; def k: f; def h(f): k; h(99)", "1"), + ( + r#"def h: "first"; def g: h; def h: "second-" + g; h"#, + r#""second-first""#, + ), + ] { + let (stdout, stderr, code) = run_jq_full(&["-nc", filter], None)?; + assert_eq!(code, 0, "{filter}: stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), want, "{filter}"); + } + + Ok(()) +} + /// #2865 (PR review, round 2): a dependency reached only from a **computed /// destructuring key** survives the referenced-closure filter. /// From 43121194af686fcde9b6ec0666b476881869224d Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 16:22:41 +1000 Subject: [PATCH 08/11] fix(jq): select a module's deps by direct reference, resolve data imports Two defects from a fifth /code-review pass on PR #2954. 1. **The innermost dependency block was filtered by the *widened* closure.** That closure is widened through kept siblings' sources so they can reach each other -- but a sibling spliced in `local` form already wraps every dependency it needs, so selecting the innermost block that way materialized each of those subtrees a second time, doubling per module level. A chain of two-def modules calling one another (`def MnA: M(n-1)B; def MnB: MnA;`) cost 111 MB at 13 levels and tens of gigabytes beyond, against jq's 2.5 MB; a 40-def chain hit 2.18 GB at six levels. The innermost block exists for the def's own body alone, so it is now selected by that body's *direct* references. 21 levels of the two-def module is 10 MB and the 7-level 40-def chain 12 MB. The remaining fan-out blow-up (#2955) is unchanged, and is once again the only one: it needs a def calling more than one def below, which neither this shape nor the existing size test had. 2. **A data import whose file is missing loaded silently.** Skipping the *binding* also skipped the resolution, so `import "nodatafile" as $d;` in a module exited 0 where jq resolves `nodatafile.json`, reports `module not found: nodatafile` and exits 3. `data_file_exists` now checks the `.json` file (not `.jq` -- the unconditional-suffix rule of #2702 is a module-import rule), so stderr and exit code are again byte-identical to jq and there is no divergence to record. The size test gains the two-def chain it was missing, the data-import test gains the missing-file row, and limitations.md's fan-out entry names both mechanisms that keep the blow-up confined to wide closures rather than deep ones, with re-measured numbers. Refs #2865 --- docs/compliance/jq/limitations.md | 19 ++++++---- src/bin/succinctly/jq_runner.rs | 48 ++++++++++++++++++++++-- tests/jq_cli_tests.rs | 62 +++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 10 deletions(-) diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 4a7acb4a5..1b1f79766 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -6387,18 +6387,23 @@ 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 | 98 MB | 2.6 MB | -| 14 levels x 4 defs, 2 calls each | 382 MB | 2.6 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_defs_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: a module's own defs are spliced into each other in their -`local` form (body plus dependencies, no siblings), which stays linear however many -siblings each one calls, so a directive-free module is bound exactly as cheaply as -before #2865. Tracked as +answer; this is a scalability limit of AST inlining, not a wrong result. Two things keep +it confined to genuinely wide closures rather than merely deep ones, both found by +review after a first attempt got them wrong: a module's own defs are spliced into each +other in their `local` form (body plus dependencies, no siblings), so a directive-free +module is bound exactly as cheaply as before #2865 however many siblings each def calls; +and the innermost dependency block is selected by a def's *direct* references rather than +the closure widened through its siblings, since a `local` sibling already carries the +dependencies it needs. Without the second, a chain of two-def modules calling one another +cost 111 MB at 13 levels and tens of gigabytes beyond; with it, 21 levels is 10 MB and 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. diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index 87d7688a7..b4356428c 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -166,6 +166,20 @@ fn resolve_module_in(search_path: &[PathBuf], module_path: &str) -> Option bool { + let data_file = format!("{module_path}.json"); + search_path + .iter() + .any(|base| base.join(&data_file).is_file()) +} + /// Extract a module's function definitions and stamp every `$__loc__` in /// each def's body with that module's own canonical (symlink-resolved) /// path (#2774) -- confirmed live against jq 1.7.1, and the same shape a @@ -379,6 +393,14 @@ struct ModuleDef { /// earlier siblings, so the closure widens through its /// [`ModuleDef::source`] -- the unbound body, whose free names are the real /// reference graph. +/// +/// The widened set therefore selects **siblings only**. Dependencies are +/// selected by the body's own *direct* references, because a `local` sibling +/// already carries every dependency it needs: filtering the innermost +/// dependency block by the widened set instead materializes each of those +/// subtrees twice, which doubles per module level. A chain of two-def modules +/// calling one another (`def MnA: M(n-1)B; def MnB: MnA;`) cost 111 MB at 13 +/// levels that way, against jq's 2.5 MB, and grew from there. fn visible_defs_for( siblings: &[ModuleDef], deps: &FuncDefList, @@ -403,7 +425,13 @@ fn visible_defs_for( .map(|sibling| sibling.name.as_str()) .collect(); - let mut wanted = called_func_names(body); + // The body's own direct references, kept separate from the widened set + // below: a `local` sibling already wraps every dependency *it* needs, so + // filtering the innermost dependency block by the widened set materializes + // each of those subtrees a second time, doubling per module level. The + // innermost block exists for this body alone. + let directly_called = called_func_names(body); + let mut wanted = directly_called.clone(); let mut keep = vec![false; siblings.len()]; // Fixed point over the siblings only: pulling one in can name an earlier @@ -455,7 +483,7 @@ fn visible_defs_for( .chain( deps.iter() .filter(|(dep_name, dep_params, _)| { - !excluded(dep_name, dep_params.len()) && wanted.contains(dep_name) + !excluded(dep_name, dep_params.len()) && directly_called.contains(dep_name) }) .cloned(), ) @@ -709,7 +737,21 @@ impl ModuleLoader { // the two -- skipping on *resolvability* instead would have silently // swallowed a genuinely missing module, which jq reports and exits 3 // for. Data imports themselves remain unimplemented (#2956). - for import in program.imports.iter().filter(|import| !import.data) { + for import in &program.imports { + if import.data { + // A data import contributes no defs, but jq still *resolves* + // it: with no `nodatafile.json` anywhere on the search path, + // `import "nodatafile" as $d;` reports `module not found: + // nodatafile` and exits 3, exactly as a missing module does. + // Check the same thing so a typo is not silently swallowed, + // without implementing the binding itself (#2956). + if !data_file_exists(&self.search_path, &import.path) { + return Err(ModuleLoadError::NotFound { + module_path: import.path.clone(), + }); + } + continue; + } let namespace = &import.alias; defs.extend( self.load_module(&import.path)? diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index bd77b47b4..91d8bbc90 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -25473,6 +25473,32 @@ fn test_module_declaring_a_data_import_still_loads_2865() -> Result<()> { assert_eq!(code, 0, "stderr: {stderr:?}"); assert_eq!(stdout.trim_end(), "7"); + // A data import whose file is missing is still jq's own error, byte for + // byte: jq resolves `.json` and reports `module not found` with + // exit 3 when there is none, so skipping the *binding* must not also skip + // the resolution check. + std::fs::write( + temp_dir.path().join("dmiss.jq"), + "import \"nodatafile\" as $d;\ndef h: 7;\n", + )?; + let (output, code) = spawn_with_signal_retry( + || { + let mut command = Command::new(succinctly_bin()); + command + .args(["jq", "-L"]) + .arg(temp_dir.path()) + .args(["-nc", r#"include "dmiss"; h"#]); + command + }, + None, + )?; + let stderr = String::from_utf8(output.stderr)?; + assert_eq!(code, 3, "stderr: {stderr:?}"); + assert!( + stderr.contains("module not found: nodatafile"), + "stderr: {stderr:?}" + ); + // A missing *namespace* import, by contrast, is still jq's own clear // error -- the skip keys on `Import::data`, not on whether the path // happens to resolve, so an unresolvable `import "m" as m;` is not @@ -25571,6 +25597,42 @@ fn test_transitive_include_chain_does_not_blow_up_2865() -> Result<()> { assert_eq!(code, 0, "stderr: {stderr:?}"); // `f0_11` is 11, and each of the 3 levels above it adds 1. assert_eq!(stdout.trim_end(), (DEFS - 1 + LEVELS - 1).to_string()); + + // The shape above has no *intra*-module calls, which hid a second + // doubling found in review: the innermost dependency block was filtered by + // the closure widened through kept siblings, so every dependency a sibling + // already carried in its own `local` wrap was materialized a second time + // -- 111 MB at 13 levels of the two-def module below, growing to tens of + // gigabytes, against jq's 2.5 MB. Filtering dependencies by the body's + // *direct* references instead keeps it flat (9 MB at 13, 10 MB at 21). + const MINI_LEVELS: usize = 21; + let mut mini: Vec<(String, String)> = vec![( + "mini0".to_string(), + "def m0a: 1;\ndef m0b: m0a;\n".to_string(), + )]; + for level in 1..MINI_LEVELS { + mini.push(( + format!("mini{level}"), + format!( + "include \"mini{}\";\ndef m{level}a: m{}b;\ndef m{level}b: m{level}a;\n", + level - 1, + level - 1 + ), + )); + } + let borrowed: Vec<(&str, &str)> = mini + .iter() + .map(|(name, contents)| (name.as_str(), contents.as_str())) + .collect(); + let filter = format!( + r#"include "mini{}"; m{}b"#, + MINI_LEVELS - 1, + MINI_LEVELS - 1 + ); + let (stdout, stderr, code) = run_jq_with_modules(&borrowed, &["-nc", &filter])?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "1"); + Ok(()) } From 16bbbd84624ea8064fecadec5311643f2250966f Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 16:58:15 +1000 Subject: [PATCH 09/11] fix(jq): stop nesting a module's own defs inside each other Four defects from a sixth /code-review pass on PR #2954, three of them one root cause: binding a module's own defs into each other at all. A module's defs 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 anything free in the copy is then captured by them -- not only module-level names, which rounds 3 and 4 chased with the `local` and `sealed` forms, but **builtins**, which no pre-bound form can protect: def a: length; jq: [1,2] | h(9) is 2; nesting gave 9 def h(length): a; def a: type; jq: h is "null"; nesting gave "q" def type: "q"; def h: a; def a: b; jq: b/0 is not defined, exit 3; def h(b): a; nesting gave 99, exit 0 -- an error swallowed All three hit modules with no `include`/`import` at all, so all three were regressions against `main`. Only dependencies are wrapped into a body now; `ModuleDef` and the local/sealed split are gone, and `visible_defs_for` becomes `visible_deps_for`. The transitive closure that rounds 1-3 needed sibling wrapping for is recovered without it: a dependency's references to its own module's siblings are satisfied by pulling those siblings in as dependencies too -- they are exports of the same module, so they are already in the list. The two exclusions are correspondingly 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. That is what makes `inner.jq` = `def g: 42; def k: g;` with `outer.jq` = `include "inner"; def g: k;` answer 42 while self-recursion still binds to itself. Fourth defect: `process_program`'s top-level import loop ignored the new `Import::data` flag, so `import "dat" as $d;` still reported `module not found` at the top level while the identical line inside a module loaded. Both paths now take the same branch -- resolve `.json`, contribute no defs -- so `import "dat" as $d; 1` answers `1` as jq does, and a missing file is jq's own error and exit 3 on both. Also fixes the jq-language reference's anchor, left behind when the limitations heading went from five rules to seven. Sizes, against `main` on the same shapes: 24-def Fibonacci module 9 MB (main 9 MB), 21-level two-def chain 9 MB, 7-level 40-def chain 12 MB. The fan-out blow-up (#2955) is unchanged and is again the only one. Refs #2865 --- docs/compliance/jq/limitations.md | 41 ++-- docs/reference/jq-language.md | 5 +- src/bin/succinctly/jq_runner.rs | 317 +++++++++++------------------- tests/jq_cli_tests.rs | 109 +++++++--- 4 files changed, 220 insertions(+), 252 deletions(-) diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 1b1f79766..6d8a6345b 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -6356,18 +6356,23 @@ other explanation. All captured live against jq 1.7.1, with `inner.jq` = `def g: dependency or a sibling named `g` in scope, and `def f($g): [$g, g]` answers `[7,7]` — a `$`-spelled parameter binds the bare call-site namespace too. -6. **An excluded name stays excluded for the siblings spliced alongside it.** In - `def f: 1; def k: f; def h(f): k;`, `h(99)` answers `1` — `k`'s `f` is the module's - `f`, not `h`'s parameter, even though the parameter displaces `f` for `h`'s own body. -7. **An in-module redefinition does not capture an earlier sibling's call.** In - `def h: "first"; def g: h; def h: "second-" + g;`, `h` answers `"second-first"` — - `g`'s `h` is the first one, bound where `g` was declared. +6. **A module's own def keeps the bindings it was written under**, whatever the def + that calls it declares. `def f: 1; def k: f; def h(f): k;` answers `1` for `h(99)` — + `k`'s `f` is the module's, not `h`'s parameter. `def h: "first"; def g: h; def h: + "second-" + g;` answers `"second-first"` — `g`'s `h` is the first one. `def a: length; + def h(length): a;` answers `2` for `h(9)` on `[1,2]` — `a`'s `length` is the builtin. +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. -Rules 6 and 7 are why a sibling that *names* one of those excluded defs is spliced in its -own fully-bound form rather than the cheaper one — the exclusion is decided for the -including def, and a sibling carries its references outward into that same scope. +(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. @@ -6394,16 +6399,12 @@ The referenced-closure filter (jq's own `block_bind_referenced` rule, in `visible_defs_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. Two things keep -it confined to genuinely wide closures rather than merely deep ones, both found by -review after a first attempt got them wrong: a module's own defs are spliced into each -other in their `local` form (body plus dependencies, no siblings), so a directive-free -module is bound exactly as cheaply as before #2865 however many siblings each def calls; -and the innermost dependency block is selected by a def's *direct* references rather than -the closure widened through its siblings, since a `local` sibling already carries the -dependencies it needs. Without the second, a chain of two-def modules calling one another -cost 111 MB at 13 levels and tens of gigabytes beyond; with it, 21 levels is 10 MB and a -7-level 40-def chain is 12 MB. Tracked as +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. diff --git a/docs/reference/jq-language.md b/docs/reference/jq-language.md index b50c725f5..0ab2c1a67 100644 --- a/docs/reference/jq-language.md +++ b/docs/reference/jq-language.md @@ -317,8 +317,9 @@ 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). See -[jq Limitations](../compliance/jq/limitations.md#five-module-scoping-rules-that-are-matched-and-read-as-bugs-2865) +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 +[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. An `include` cycle is reported as `module cycle detected: a -> b -> a` diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index b4356428c..f7618912e 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -237,7 +237,7 @@ fn wrap_defs(mut expr: Expr, defs: FuncDefList) -> Expr { /// over-keeps a little (a dependency `g/1` survives 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 self-recursion filter in [`visible_defs_for`] still keys on +/// The self-recursion filter in [`visible_deps_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 @@ -299,155 +299,95 @@ fn called_func_names(expr: &Expr) -> BTreeSet { names } -/// One of a module's own defs, in the two bound forms the loader needs -/// (#2865). -/// -/// The split is what keeps binding from growing exponentially. A def has to -/// be **sealed** before it leaves its module -- carrying everything it -/// references, so it resolves the same wherever it is spliced -- but a def -/// spliced as a *sibling*, into another def of the same module, must not be: -/// the chain it lands in already supplies the module's earlier siblings, in -/// the right lexical order. Splicing the sealed form there instead nests a -/// full copy of each sibling inside every later one, which for a module whose -/// defs each call two earlier siblings doubles per def. -struct ModuleDef { - name: String, - params: Vec, - /// The def's body exactly as parsed, binding nothing. The reference - /// graph is read off *this*, never off a bound form -- a bound body has - /// already captured names internally, so re-deriving from it re-adds - /// what it has satisfied and pulls in the whole preceding sibling set. - source: Expr, - /// The body wrapped in the module's own **dependencies** only. What gets - /// spliced when this def is needed as a sibling, in the ordinary case - /// where the chain it lands in can supply its own sibling references. - local: Expr, - /// The fully bound body -- dependencies *and* earlier siblings -- i.e. - /// what this def exports. Also what a sibling splice falls back to when - /// the receiving def excludes a name that sibling references; see - /// [`visible_defs_for`]'s "when a sibling has to be sealed anyway". - sealed: Expr, -} - -/// The defs one of a module's own defs can actually reach, in [`wrap_defs`] -/// order: the module's earlier siblings (in their `local` form) and the -/// module's dependencies, minus everything that must not capture a name in -/// this body, minus everything the body never transitively calls (#2865). -/// -/// ### Ordering -/// -/// Siblings first, dependencies second, so a dependency ends up **innermost** -/// and beats a same-named sibling -- jq's answer for a module written -/// `include "inner"; def g: 7; def h: g;` is `42`, not the `7` one line above. -/// -/// ### The three exclusions -/// -/// - **The def itself, by (name, arity).** jq binds a def's own recursive -/// call to itself before anything else: with `inner`'s `g/0` in scope, -/// `def g: if . == 0 then "base" else (. - 1 | g) end;` still answers -/// `"base"`. Arity-scoped, so a dependency `g/1` alongside an own `g/0` -/// leaves both reachable. -/// - **Anything named after one of this def's parameters, at arity 0.** A -/// parameter binds the bare call-site namespace (`Param::Dollar`'s `$g` -/// binds `g` too), and it wins: `def f(g): g; def q: f(7);` answers `7` in -/// jq even with a dependency or sibling `g` in scope. Without this the -/// wrap, which nests *inside* the parameter's own binding, would capture -/// the parameter's every use. -/// - **Anything nothing in the body transitively calls.** jq's own -/// `block_bind_referenced` rule, and here a sizing requirement rather than -/// a micro-optimisation: without it 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 def 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. -/// -/// ### When a sibling has to be sealed anyway -/// -/// The exclusions are decided on the *including* def's behalf, but a kept -/// sibling is spliced into that same scope and carries its own references -/// outward into it -- so an excluded name is excluded for the sibling too, -/// and resolves to whatever took its place. Both shapes are real: -/// -/// - `def f: 1; def k: f; def h(f): k;` -- `h`'s parameter `f` displaces the -/// sibling `f`, and `k`'s own `f` then resolves to the parameter. -/// `include "m"; h(99)` answers `1` in jq; splicing `k` in `local` form -/// answered `99`. -/// - `def h: "first"; def g: h; def h: "second-" + g;` -- the second `h` -/// excludes the first, and `g`'s `h` then resolves to the second, -/// recursively. jq answers `"second-first"`; `local` form recursed until it -/// hit the depth cap. -/// -/// A sibling naming an excluded name is therefore spliced in its **sealed** -/// form, where that reference is already bound and cannot be recaptured. -/// Nothing is excluded for almost every def, so the sealed fallback -- and -/// the nesting it reintroduces -- is confined to the two shapes above. -/// -/// ### Why the closure widens over siblings but not dependencies -/// -/// A dependency arrives **sealed** from its own module, so it needs nothing -/// further from this scope and contributes nothing to the closure. A sibling -/// arrives in `local` form and does still reach outward for the module's -/// earlier siblings, so the closure widens through its -/// [`ModuleDef::source`] -- the unbound body, whose free names are the real -/// reference graph. -/// -/// The widened set therefore selects **siblings only**. Dependencies are -/// selected by the body's own *direct* references, because a `local` sibling -/// already carries every dependency it needs: filtering the innermost -/// dependency block by the widened set instead materializes each of those -/// subtrees twice, which doubles per module level. A chain of two-def modules -/// calling one another (`def MnA: M(n-1)B; def MnB: MnA;`) cost 111 MB at 13 -/// levels that way, against jq's 2.5 MB, and grew from there. -fn visible_defs_for( - siblings: &[ModuleDef], - deps: &FuncDefList, - name: &str, - params: &[Param], - body: &Expr, -) -> FuncDefList { +/// The dependencies one of a module's own defs can reach, in [`wrap_defs`] +/// order (#2865): everything its body transitively calls, minus whatever must +/// not capture a name it uses directly. +/// +/// ### 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**: +/// +/// ```text +/// def a: length; jq: [1,2] | h(9) is 2 +/// def h(length): a; nested: 9, the parameter captured a's call +/// +/// def a: b; jq: b/0 is not defined (exit 3) +/// def h(b): a; nested: 99, the error silently swallowed +/// ``` +/// +/// Sealing cannot repair that, because the captured name is free in the sealed +/// form too. Leaving siblings in the flat chain is what keeps them bound where +/// they were written. +/// +/// ### The transitive closure, and why the exclusions are direct-only +/// +/// 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`. +/// +/// That is also why the two exclusions apply to a name only when the def's +/// body calls it **directly**: +/// +/// - **The def's own (name, arity).** jq binds a def's own recursive call to +/// itself: with `inner`'s `g/0` in scope, `def g: if . == 0 then "base" +/// else (. - 1 | g) end;` answers `"base"`, so the dependency must not be +/// wrapped. In the `outer.jq` case above the same dependency *must* be +/// wrapped -- and there the body never calls `g` itself, so nothing of the +/// def's own is at stake and it cannot be captured. +/// - **A parameter's name, at arity 0.** A parameter binds the bare call-site +/// 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. +/// Again scoped to direct calls, so a dependency reached only through +/// another one still resolves to the dependency, as it does in jq. +/// +/// A body that both uses such a name directly *and* reaches a dependency +/// needing the other binding cannot be expressed in one chain; that pairing +/// has no test and no known real shape, and the direct use wins. +/// +/// ### 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, name: &str, params: &[Param], body: &Expr) -> FuncDefList { let param_names: BTreeSet<&str> = params.iter().map(Param::name).collect(); let arity = params.len(); - let excluded = |cand_name: &str, cand_params: usize| { - (cand_name == name && cand_params == arity) - || (cand_params == 0 && param_names.contains(cand_name)) - }; - - // The sibling names this def excludes and that some sibling therefore - // cannot be allowed to reach past. Empty for almost every def -- it takes - // either an in-module redefinition of the same (name, arity) or a - // parameter sharing a sibling's name. - let shadowed: BTreeSet<&str> = siblings - .iter() - .filter(|sibling| excluded(&sibling.name, sibling.params.len())) - .map(|sibling| sibling.name.as_str()) - .collect(); - - // The body's own direct references, kept separate from the widened set - // below: a `local` sibling already wraps every dependency *it* needs, so - // filtering the innermost dependency block by the widened set materializes - // each of those subtrees a second time, doubling per module level. The - // innermost block exists for this body alone. let directly_called = called_func_names(body); + let mut wanted = directly_called.clone(); - let mut keep = vec![false; siblings.len()]; + let mut keep = vec![false; deps.len()]; - // Fixed point over the siblings only: pulling one in can name an earlier - // sibling not yet scanned. Bounded and linear -- each sibling is admitted - // at most once, and widening reads its unbound `source`, never a bound - // body. + // 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, sibling) in siblings.iter().enumerate() { - if keep[i] || excluded(&sibling.name, sibling.params.len()) { + for (i, (dep_name, dep_params, dep_body)) in deps.iter().enumerate() { + let excluded = directly_called.contains(dep_name) + && ((dep_name == name && dep_params.len() == arity) + || (dep_params.is_empty() && param_names.contains(dep_name.as_str()))); + if keep[i] || excluded { continue; } - if wanted.contains(&sibling.name) { + if wanted.contains(dep_name) { keep[i] = true; let before = wanted.len(); - wanted.extend(called_func_names(&sibling.source)); + wanted.extend(called_func_names(dep_body)); grew |= wanted.len() != before; } } @@ -456,37 +396,10 @@ fn visible_defs_for( } } - siblings - .iter() + deps.iter() .zip(keep) .filter(|(_, keep)| *keep) - .map(|(sibling, _)| { - // A `local` sibling still reaches outward for its own sibling - // references, and they land in *this* def's scope -- where an - // excluded name resolves to something else entirely (this def - // itself, or one of its parameters). Splice the sealed form for - // any sibling that names one, so its references are already bound - // and cannot be recaptured. `shadowed` is empty for almost every - // def, so this costs nothing in the ordinary case; it is checked - // against the unbound `source`, the real reference graph. - let body = if !shadowed.is_empty() - && called_func_names(&sibling.source) - .iter() - .any(|called| shadowed.contains(called.as_str())) - { - sibling.sealed.clone() - } else { - sibling.local.clone() - }; - (sibling.name.clone(), sibling.params.clone(), body) - }) - .chain( - deps.iter() - .filter(|(dep_name, dep_params, _)| { - !excluded(dep_name, dep_params.len()) && directly_called.contains(dep_name) - }) - .cloned(), - ) + .map(|(dep, _)| dep.clone()) .collect() } @@ -608,12 +521,15 @@ impl ModuleLoader { /// /// ### What each body is wrapped in /// - /// [`visible_defs_for`] decides that per def: the module's own earlier - /// siblings (so an exported body is self-contained wherever it is spliced - /// in) then its dependencies (innermost, so they win), minus the def - /// itself by (name, arity), minus anything named after one of the def's - /// parameters, minus anything the body never transitively calls. Its own - /// doc comment carries the oracle row behind each of those. + /// [`visible_deps_for`] decides that per def: the module's dependencies + /// that the body transitively calls, minus the def itself by (name, + /// arity) and minus anything named after one of the def's parameters when + /// the body calls that name directly. 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. /// /// ### Search-path resolution /// @@ -664,38 +580,16 @@ impl ModuleLoader { self.loading.pop(); let deps = deps?; - // Both bound forms per def, for the reason [`ModuleDef`] gives. The - // `local` form is the body wrapped in the module's dependencies alone, - // which is what a *sibling* splice normally needs, since the chain it - // lands in already carries the module's earlier siblings... - // ...then, in the same declaration-order walk, the sealed form that - // leaves the module: each def wrapped in the siblings declared - // *before* it plus the dependencies, so it resolves identically - // wherever it is later spliced. Def `i` sees exactly `defs[..i]` -- - // the same lexical rule a filter's own defs follow -- and each entry - // there already carries its own `sealed` form, which - // `visible_defs_for` falls back to for a sibling that names something - // this def excludes. - let mut defs: Vec = Vec::with_capacity(own.len()); - for (name, params, body) in own { - let dep_wrap = visible_defs_for(&[], &deps, &name, ¶ms, &body); - let local = wrap_defs(body.clone(), dep_wrap); - - let visible = visible_defs_for(&defs, &deps, &name, ¶ms, &body); - let sealed = wrap_defs(body.clone(), visible); - - defs.push(ModuleDef { - name, - params, - source: body, - local, - sealed, - }); - } - - Ok(defs + // 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. + Ok(own .into_iter() - .map(|def| (def.name, def.params, def.sealed)) + .map(|(name, params, body)| { + let visible = visible_deps_for(&deps, &name, ¶ms, &body); + let wrapped = wrap_defs(body, visible); + (name, params, wrapped) + }) .collect()) } @@ -841,6 +735,19 @@ impl ModuleLoader { // Process imports (definitions available under namespace::) // Load modules and add their functions with namespace prefixes for import in &program.imports { + // The same data-import handling the module path uses (#2865), so + // the two agree: a data import binds a `$`-variable rather than a + // namespace of defs, so it contributes nothing here, but jq still + // resolves its `.json` and reports `module not found` when + // there is none. Binding the variable itself is #2956. + if import.data { + if !data_file_exists(&self.search_path, &import.path) { + return Err(ModuleLoadError::NotFound { + module_path: import.path.clone(), + }); + } + continue; + } let defs = self.load_module(&import.path)?; let namespace = &import.alias; diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 91d8bbc90..475c29559 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -25304,15 +25304,15 @@ fn test_cycle_chain_starts_at_the_repeat_not_the_stack_bottom_2865() -> Result<( /// the body, with no widening, is both correct and linear. /// /// A second round of review found the single-callee chain above too weak a -/// lock: the *sealed* form of each def was being spliced where a sibling was -/// needed, nesting a full copy of every earlier sibling inside every later -/// one. That only shows when a def calls **more than one** earlier sibling, -/// so the second shape below is a Fibonacci chain -- merely `include`d, never -/// called, since the blow-up happens at load time. It reached 190 MB at 18 -/// defs, 4.1 GB at 24 and 32 GB at 28, against a flat 14.5 MB on `main`. -/// Splicing the `local` form (body plus dependencies, no siblings) keeps it -/// linear, because the chain a sibling lands in already supplies the earlier -/// siblings. +/// lock. Successive attempts at binding a module's own defs into each other +/// nested a copy of every earlier sibling inside every later one, which only +/// shows when a def calls **more than one** earlier sibling -- so the second +/// shape below is a Fibonacci chain, merely `include`d and never called, +/// since the blow-up happens at load time. It reached 190 MB at 18 defs, 4.1 +/// GB at 24 and 32 GB at 28, against a flat 14.5 MB on `main`. A module's own +/// defs are now not wrapped into each other at all (see `visible_deps_for`): +/// they are emitted as siblings in the top-level chain, where jq's lexical +/// rule already relates them. /// /// Asserting completion rather than a wall-clock bound, which would flake on /// a loaded CI box. @@ -25344,24 +25344,75 @@ fn test_module_own_def_chain_does_not_blow_up_2865() -> Result<()> { Ok(()) } -/// #2865 (PR review, round 3): a name this def excludes stays excluded for -/// the siblings spliced alongside it. +/// #2865 (PR review, round 5): the two capture shapes that need something +/// other than a plain zero-exit comparison -- a builtin resolved against +/// stdin, and a compile error that must stay one. /// -/// The exclusions are decided on the *including* def's behalf, but a sibling -/// spliced in `local` form carries its own references outward into that same -/// scope -- where the excluded name resolves to whatever displaced it. Both -/// shapes are regressions against `main`, which splices flat: +/// `def a: length; def h(length): a;` -- jq answers `2` for `h(9)` on +/// `[1,2]`, because `a`'s `length` is the builtin it was written against. +/// Nesting `a` inside `h`'s body put it under `h`'s parameter scope and +/// answered `9`. /// -/// - a parameter displacing a sibling that another sibling calls -/// (`def f: 1; def k: f; def h(f): k;` -- jq says `h(99)` is `1`, the -/// `local` splice said `99`), in both parameter spellings; -/// - an in-module redefinition (`def h: "first"; def g: h; def h: "second-" + -/// g;` -- jq says `"second-first"`, the `local` splice recursed into the -/// second `h` until it hit the depth cap). +/// `def a: b; def h(b): a;` -- `b` is defined nowhere, so jq reports +/// `b/0 is not defined` and exits 3. Nesting let `h`'s parameter satisfy it, +/// printing `99` and exiting 0: a compile error silently swallowed, which is +/// the worse half of the same bug. +#[test] +fn test_module_def_scope_is_not_captured_by_its_caller_2865() -> Result<()> { + let temp_dir = tempfile::tempdir()?; + std::fs::write( + temp_dir.path().join("cap.jq"), + "def a: length;\ndef h(length): a;\n", + )?; + let (output, code) = spawn_with_signal_retry( + || { + let mut command = Command::new(succinctly_bin()); + command + .args(["jq", "-L"]) + .arg(temp_dir.path()) + .args(["-c", r#"include "cap"; h(9)"#]); + command + }, + Some(b"[1,2]"), + )?; + let stdout = String::from_utf8(output.stdout)?; + let stderr = String::from_utf8(output.stderr)?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "2"); + + let (_, stderr, code) = run_jq_with_modules( + &[("m", "def a: b;\ndef h(b): a;\n")], + &["-nc", r#"include "m"; h(99)"#], + )?; + assert_eq!(code, 3, "stderr: {stderr:?}"); + assert!(stderr.contains("b/0 is not defined"), "stderr: {stderr:?}"); + + Ok(()) +} + +/// #2865 (PR review, rounds 3, 4 and 5): a module's own def keeps the +/// bindings it was written under, whatever the def that calls it declares. +/// +/// While a module's siblings were being nested inside each other's bodies, +/// anything free in the nested copy was captured by the scopes it landed in. +/// Every shape below was a regression against `main`, which emits a module's +/// defs flat: +/// +/// - a parameter capturing a sibling's call to another sibling +/// (`def f: 1; def k: f; def h(f): k;` -- jq says `h(99)` is `1`, nesting +/// said `99`), in both parameter spellings; +/// - an in-module redefinition capturing an earlier sibling's call +/// (`def h: "first"; def g: h; def h: "second-" + g;` -- jq says +/// `"second-first"`, nesting recursed into the second `h` until it hit the +/// depth cap); +/// - a later-declared sibling capturing an earlier one's call to a +/// **builtin**, which no amount of pre-binding the nested copy could have +/// repaired, since a builtin call is free in every form of it. /// -/// Both are fixed by splicing the sibling's *sealed* form when it names -/// something excluded. The equivalent top-level filters have always answered -/// correctly, and are included as controls. +/// `test_module_def_scope_is_not_captured_by_its_caller_2865` covers the +/// parameter-versus-builtin and swallowed-compile-error shapes, which need a +/// stdin input and a non-zero exit respectively. The equivalent top-level +/// filters have always answered correctly, and are included as controls. #[test] fn test_excluded_sibling_name_stays_excluded_for_other_siblings_2865() -> Result<()> { for (module, filter, want) in [ @@ -25380,6 +25431,14 @@ fn test_excluded_sibling_name_stays_excluded_for_other_siblings_2865() -> Result r#"include "m"; h"#, r#""second-first""#, ), + // A later-declared sibling must not capture an earlier one's call to + // a builtin: `a`'s `type` is the builtin, so on `null` input it is + // `"null"`, never the sibling's `"q"`. + ( + "def a: type;\ndef type: \"q\";\ndef h: a;\n", + r#"include "m"; h"#, + r#""null""#, + ), ] { let (stdout, stderr, code) = run_jq_with_modules(&[("m", module)], &["-nc", filter])?; assert_eq!(code, 0, "{module}: stderr: {stderr:?}"); @@ -25601,7 +25660,7 @@ fn test_transitive_include_chain_does_not_blow_up_2865() -> Result<()> { // The shape above has no *intra*-module calls, which hid a second // doubling found in review: the innermost dependency block was filtered by // the closure widened through kept siblings, so every dependency a sibling - // already carried in its own `local` wrap was materialized a second time + // already carried in its own dependency wrap was materialized a second time // -- 111 MB at 13 levels of the two-def module below, growing to tens of // gigabytes, against jq's 2.5 MB. Filtering dependencies by the body's // *direct* references instead keeps it flat (9 MB at 13, 10 MB at 21). From 7e214cf368bdd5b2d5b10da048c2f23e246c9b26 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 17:32:23 +1000 Subject: [PATCH 10/11] docs(jq): record the dependency-capture boundary, fix stale names A seventh /code-review pass found three more shapes in one family, all of them the boundary of this fix's own mechanism rather than regressions: a dependency is wrapped *inside* the including def's `Expr::FuncDef`, so that def's name and parameters are enclosing binders for it, and a name the dependency reaches on its own can be captured. `visible_deps_for`'s two exclusions are a partial mitigation -- they keep self-recursion and parameters working for names the body uses directly -- and they cannot cover a name a dependency reaches by itself, because the two cases pull opposite ways: excluding strands the other dependency, keeping it would shadow the def's own binding. All three need a transitive `include`, which did not work at all before this change (`main` answers `g/0 is not defined` / `k/0 is not defined` for each), so none is a regression. None of ADR-0018's four conditions covers them either, so per rule 4 they are recorded in `docs/compliance/jq/limitations.md` as a still-open gap rather than an accepted divergence, filed as #2962, and pinned by `test_dependency_capture_by_the_including_defs_scope_2962` with the jq answers each assertion should become. Closing it needs a targeted rename of the excluded dependency, or #2951's sealed module scope, which subsumes it. Also from that pass: six references spelled `visible_defs_for` for a function renamed to `visible_deps_for`, and the limitations entry on the open module-scope gaps now names #2956 alongside #2950/#2951. Refs #2865 --- docs/compliance/jq/limitations.md | 42 +++++++++++++++++-- tests/jq_cli_tests.rs | 69 ++++++++++++++++++++++++++++++- 2 files changed, 106 insertions(+), 5 deletions(-) diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 6d8a6345b..9aeb590b1 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -6337,7 +6337,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_defs_for` have no +otherwise read them as ones, and because the exclusions in `visible_deps_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 @@ -6396,7 +6396,7 @@ blocks, so it does not. Measured at #2865's own head (Apple M-series, release): | 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_defs_for`) flattens the one-call-each shape completely — without it the +`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 @@ -6409,6 +6409,39 @@ two-def module is 9 MB; a 7-level 40-def chain is 12 MB). Tracked as splicing bound bodies by handle (the `Rc`-shaded opaque sub-expression #1371 already introduced) instead of by clone. +### A wrapped dependency sits inside the including def's scope — no carve-out; recorded as a still-open gap (#2962) + +A module's dependencies are bound by wrapping them around the body of each def that +reaches them, and that wrap nests **inside** the def's own `Expr::FuncDef` — so the def's +own name (for self-recursion) and its parameters are enclosing binders for every +dependency in the block. Real jq binds a module's block in its own scope and only then +links it, so nothing of the caller is ever in scope for it. + +`visible_deps_for`'s two exclusions are a partial mitigation: they keep a def's own +recursion and its parameters working for names the body uses **directly**. They cannot +help a name a dependency reaches on its own, and the two cases pull opposite ways — +excluding strands the other dependency, keeping it would shadow the def's own binding. +Three shapes, all confirmed live against jq 1.7.1: + +| fixtures | jq 1.7.1 | succinctly | +|----------|----------|------------| +| `inner` = `def c: 7; def g: c;`, `mid` = `include "inner"; def c: if . == 0 then g else (. - 1 \| c) end;`, then `0 \| c` | `7` | `g/0 exceeded maximum recursion depth`, exit 5 | +| `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]]` | + +None of ADR-0018's four conditions covers this (the output is readable, nothing is +corrupted or discarded, and the process does not die), so per rule 4 it is recorded here +as a still-open gap rather than an accepted divergence. It is **not** a regression: every +shape needs a transitive `include`, which did not work at all before #2865 — `main` +answers `g/0 is not defined` / `k/0 is not defined` for each. Closing it needs either a +targeted rename of an excluded dependency (rewriting free calls to it inside the other +kept dependency bodies) or the sealed module scope of +[#2951](https://github.com/rust-works/succinctly/issues/2951), which subsumes it; the +second row additionally needs a module's own body resolved at load time, the way jq +reports it against the module's own file. +`test_dependency_capture_by_the_including_defs_scope_2962` (`tests/jq_cli_tests.rs`) pins +all three so a change to the mechanism cannot make them worse unnoticed. + ### Two module-scope gaps that are genuinely open Found while closing #2865, filed rather than recorded as divergences: a dependency named @@ -6417,7 +6450,10 @@ own source is parsed with no shadow-candidate seeding ([#2950](https://github.com/rust-works/succinctly/issues/2950)); and a module body can see names it should not — `~/.jq`'s defs, and sibling `include`d modules' defs in a declaration-order-dependent way — because every module is inlined into one flat def chain -([#2951](https://github.com/rust-works/succinctly/issues/2951)). +([#2951](https://github.com/rust-works/succinctly/issues/2951)). Data imports +(`import "f" as $d;`) are also still unimplemented: #2865 records the `$` on `Import::data` +and resolves the file so a typo is still jq's own `module not found`, but binding the +variable is [#2956](https://github.com/rust-works/succinctly/issues/2956). ## Provenance diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 475c29559..6afe5fb8d 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -24889,7 +24889,7 @@ fn test_top_level_def_still_outranks_an_included_one_2865() -> Result<()> { /// /// With `inner`'s `g/0` in scope, `def g: if . == 0 then "base" else (. - 1 | /// g) end;` still recurses into itself and reaches `"base"`; without the -/// `visible_defs_for` exclusion it would answer `42` on the first step. +/// `visible_deps_for` exclusion it would answer `42` on the first step. /// /// The arity half: a dependency `g/1` alongside an own `g/0` leaves both /// reachable from the same body (`["own0","dep1arg"]`). Both captured live @@ -25610,12 +25610,77 @@ fn test_module_declaring_a_data_import_still_loads_2865() -> Result<()> { Ok(()) } +/// #2962: the documented boundary of #2865's mechanism -- a dependency is +/// wrapped *inside* the including def's own `Expr::FuncDef`, so that def's +/// name and parameters are enclosing binders for it, and a name the +/// dependency reaches on its own can be captured. +/// +/// Pinned rather than fixed, and recorded in +/// `docs/compliance/jq/limitations.md` as a still-open gap (no ADR-0018 +/// rule-4 condition applies). Not a regression: every shape needs a +/// transitive `include`, which did not work at all before #2865 -- `main` +/// answers `g/0 is not defined` / `k/0 is not defined` for each. Closing it +/// needs a targeted rename of the excluded dependency or #2951's sealed +/// module scope; this test is what makes a change to the mechanism visible. +/// +/// **When #2962 lands, these assertions become jq's own answers**: `7`, +/// exit 3 with `b/0 is not defined`, and `[7,[42]]`. +#[test] +fn test_dependency_capture_by_the_including_defs_scope_2962() -> Result<()> { + // 1. The self-(name, arity) exclusion strands a dependency that another + // kept dependency still calls. jq answers 7. + let (_, stderr, code) = run_jq_with_modules( + &[ + ("inner", "def c: 7;\ndef g: c;\n"), + ( + "mid", + "include \"inner\";\ndef c: if . == 0 then g else (. - 1 | c) end;\n", + ), + ], + &["-nc", r#"include "mid"; 0 | c"#], + )?; + assert_eq!(code, 5, "stderr: {stderr:?}"); + assert!( + stderr.contains("exceeded maximum recursion depth"), + "stderr: {stderr:?}" + ); + + // 2. A parameter satisfies a name the dependency left undefined, so a + // program jq rejects with `b/0 is not defined` (exit 3) answers here. + let (stdout, stderr, code) = run_jq_with_modules( + &[ + ("gb", "def g: b;\n"), + ("hb", "include \"gb\";\ndef h(b): g;\ndef q: h(99);\n"), + ], + &["-nc", r#"include "hb"; q"#], + )?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "99"); + + // 3. A parameter captures a dependency reached only indirectly. `h`'s own + // `g` is correctly the parameter; `k`'s `g` should still be 42. + let (stdout, stderr, code) = run_jq_with_modules( + &[ + ("inner3", "def g: 42;\ndef k: [g];\n"), + ( + "h3", + "include \"inner3\";\ndef h($g): [g, k];\ndef q: h(7);\n", + ), + ], + &["-nc", r#"include "h3"; q"#], + )?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "[7,[7]]"); + + Ok(()) +} + /// #2865 (plan step 6): a chain of modules does not multiply in size. /// /// Wrapping every dependency into every exported body compounds down a chain, /// because each level's bodies already carry the level below: unfiltered, a /// 3-module x 40-def chain measured 359 MB peak RSS against jq's 2.5 MB, and -/// a fourth level would have been tens of gigabytes. `visible_defs_for`'s +/// a fourth level would have been tens of gigabytes. `visible_deps_for`'s /// referenced-closure filter (jq's own `block_bind_referenced` rule) is what /// keeps it flat. /// From 57aad70c4d21824d0d513b2bfb76e84b69d7c0ea Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 17:53:49 +1000 Subject: [PATCH 11/11] test(jq): close the two patch-coverage gaps on the module loader `omni-dev coverage diff` put the patch at 95.7%, with two genuinely untested branches rather than unreachable ones: - the `Expr::Foreach` arm of `called_func_names`' pattern walk. The existing rows covered `as` and `?//` (both `Expr::AsPattern`) and `reduce`, leaving `foreach` -- a third variant, so a third arm -- with zero hits in the raw lcov. Added as a fourth row, oracle-checked: `[foreach (.,.) as {(kf): $v} (0; . + $v)]` is `[1,2]` in jq. - the top-level import loop's data-import branch. Only the module-level path was tested, so nothing pinned that the two agree. The new test covers both directions against jq: `import "dat" as $d; 1` answers `1` with `dat.json` present, and `module not found: nofile` / exit 3 without it. Patch coverage is now 100% (164/164). Refs #2865 --- tests/jq_cli_tests.rs | 52 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 6afe5fb8d..a1e233788 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -25462,6 +25462,53 @@ fn test_excluded_sibling_name_stays_excluded_for_other_siblings_2865() -> Result Ok(()) } +/// #2865 (PR review, round 5): the top-level import loop takes the same +/// data-import branch the module loader does, so the two agree. +/// +/// `import "f" as $d;` binds a `$`-variable to a file's parsed JSON rather +/// than a namespace of defs. It contributes no defs, but jq still resolves +/// `.json`: with the file present `import "dat" as $d; 1` answers `1`, +/// and with it missing jq reports `module not found: dat` and exits 3. Before +/// this, the top level reported `module not found` either way, which would +/// have left it disagreeing with the identical line inside a module. Binding +/// the variable itself is #2956. +#[test] +fn test_top_level_data_import_resolves_but_contributes_no_defs_2865() -> Result<()> { + let temp_dir = tempfile::tempdir()?; + std::fs::write(temp_dir.path().join("dat.json"), "{\"z\":9}\n")?; + + let run = |filter: &'static str| { + let temp_path = temp_dir.path().to_path_buf(); + spawn_with_signal_retry( + move || { + let mut command = Command::new(succinctly_bin()); + command + .args(["jq", "-L"]) + .arg(&temp_path) + .args(["-nc", filter]); + command + }, + None, + ) + }; + + let (output, code) = run(r#"import "dat" as $d; 1"#)?; + let stdout = String::from_utf8(output.stdout)?; + let stderr = String::from_utf8(output.stderr)?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim_end(), "1"); + + let (output, code) = run(r#"import "nofile" as $d; 1"#)?; + let stderr = String::from_utf8(output.stderr)?; + assert_eq!(code, 3, "stderr: {stderr:?}"); + assert!( + stderr.contains("module not found: nofile"), + "stderr: {stderr:?}" + ); + + Ok(()) +} + /// #2865 (PR review, round 2): a dependency reached only from a **computed /// destructuring key** survives the referenced-closure filter. /// @@ -25481,6 +25528,11 @@ fn test_dependency_reached_only_from_a_pattern_key_survives_2865() -> Result<()> "include \"kfin\";\ndef h: reduce (.,.) as {(kf): $v} (0; . + $v);\n", "2", ), + // `foreach` pattern -- a third `Expr` variant, so a third arm + ( + "include \"kfin\";\ndef h: [foreach (.,.) as {(kf): $v} (0; . + $v)];\n", + "[1,2]", + ), // `?//` alternatives ( "include \"kfin\";\ndef h: . as [$x] ?// {(kf): $v} | ($v // $x);\n",