diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 7d3a68536..9aeb590b1 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -6288,6 +6288,173 @@ 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). + +### Seven module-scoping rules that *are* matched, and read as bugs (#2865) + +Not divergences — recorded here because the next person to touch `ModuleLoader` will +otherwise read them as ones, and because the exclusions in `visible_deps_for` have no +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. + +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 — +both scoped to a name the body calls *directly*, since a dependency reached only through +another one is bound where that one was written and is not the def's own to shadow. +Rules 6 and 7 are why a module's own defs are not wrapped into each other at all: they are +emitted as siblings in the top-level chain, exactly as a filter's own defs are, and jq's +lexical rule relates them there. Nesting a copy of one inside another's body puts it under +scopes it was never written in, and rule 6's third row shows sealing cannot repair that — +a call to a builtin is free in every pre-bound form of the copy. +Rules 1-3 are why the wrap goes around each exported def's **body** rather than being +spliced into the module's exported chain. + +One consequence worth stating, since it is the reason an exported body carries its +module's own earlier siblings as well as its dependencies: a def handed to another module +has to be **self-contained**. With `inner.jq` = `def g: 42; def k: g;` and `outer.jq` = +`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_deps_for`) flattens the one-call-each shape completely — without it the +6 x 40 row was 359 MB rather than 10 MB — but it cannot flatten a genuinely wide closure, +because that closure is itself exponential. Every row above produces the **correct** +answer; this is a scalability limit of AST inlining, not a wrong result. It needs a +*chain of modules* to appear, and a wide one: only dependencies are wrapped into a body, +so a module with no `include`/`import` is bound exactly as cheaply as before #2865 +however many of its own defs call each other (a 24-def Fibonacci module is 9 MB, against +`main`'s 9 MB), and a chain whose defs call one def apiece stays flat (21 levels of a +two-def module is 9 MB; a 7-level 40-def chain is 12 MB). Tracked as +[#2955](https://github.com/rust-works/succinctly/issues/2955), whose most promising fix is +splicing bound bodies by handle (the `Rc`-shaded opaque sub-expression #1371 already +introduced) instead of by clone. + +### 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 +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)). 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 | Artifact | Path | diff --git a/docs/reference/jq-language.md b/docs/reference/jq-language.md index 025578ff9..0ab2c1a67 100644 --- a/docs/reference/jq-language.md +++ b/docs/reference/jq-language.md @@ -309,6 +309,23 @@ 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), 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` +(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/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index 862f7ecde..f7618912e 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}"); } @@ -110,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. @@ -130,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 @@ -138,8 +188,8 @@ fn resolve_module_in(search_path: &[PathBuf], module_path: &str) -> Option 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 [`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 +/// 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, + 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, .. + } => { + 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 + }); + names +} + +/// 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 directly_called = called_func_names(body); + + let mut wanted = directly_called.clone(); + let mut keep = vec![false; deps.len()]; + + // Fixed point: a kept dependency's body can name a sibling of its own + // module, which is itself a dependency here and may sit anywhere in the + // list. Each entry is admitted at most once, so this terminates. + loop { + let mut grew = false; + for (i, (dep_name, dep_params, dep_body)) in deps.iter().enumerate() { + let 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(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(|(_, keep)| *keep) + .map(|(dep, _)| 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 +459,7 @@ impl ModuleLoader { search_path, loaded_modules: BTreeMap::new(), auto_loaded_defs, + loading: Vec::new(), } } @@ -209,34 +471,190 @@ 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(), - } - })?; + // `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); + } - // Read and parse the module - let contents = std::fs::read_to_string(&file_path) - .with_context(|| format!("failed to read module: {}", file_path.display()))?; + Ok(self + .loaded_modules + .get(module_path) + .expect("just inserted above, or already present")) + } - let program = jq::parse_program(&contents).map_err(|e| { - anyhow::anyhow!("parse error in module '{}': {}", file_path.display(), e) - })?; + /// 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`. + /// + /// ### What each body is wrapped in + /// + /// [`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 + /// + /// 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 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(); + 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?; + + // 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(|(name, params, body)| { + let visible = visible_deps_for(&deps, &name, ¶ms, &body); + let wrapped = wrap_defs(body, visible); + (name, params, wrapped) + }) + .collect()) + } - Ok(entry.insert(extract_and_stamp_func_defs(&program.expr, file_path))) + /// 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. + // + // 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 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 { + 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)? + .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,47 +724,39 @@ 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 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; // 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 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 7970ef095..a1e233788 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -24758,6 +24758,1060 @@ 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 +/// `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 +/// 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 (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 (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. +/// +/// A second round of review found the single-callee chain above too weak a +/// 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. +#[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)); + } + 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()); + + // 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(()) +} + +/// #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. +/// +/// `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`. +/// +/// `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. +/// +/// `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 [ + ( + "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""#, + ), + // 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:?}"); + 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 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. +/// +/// `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", + ), + // `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", + "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 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 + // 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", + )?; + 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(()) +} + +/// #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_deps_for`'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()); + + // 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 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). + 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(()) +} + /// #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.