fix(jq): process a module's own include/import directives transitively - #2954
fix(jq): process a module's own include/import directives transitively#2954newhoggy wants to merge 11 commits into
Conversation
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
…mports 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
CoverageTotal: 93.53% ⚪ 0.01 pp vs Comparing
🔇 0 ignored region(s), 90 tolerated region(s)
Patch coveragePatch: 100% (164/164 new lines covered)
Indirect coverage changes🔴 0 lines lost coverage, 🟢 1 lines gained coverage on unchanged code. Indirect changes
|
CoverageTotal: 93.43% ⚪ 0.01 pp vs Comparing
🔇 0 ignored region(s), 91 tolerated region(s)
Patch coveragePatch: 100% (164/164 new lines covered)
Indirect coverage changes🔴 0 lines lost coverage, 🟢 1 lines gained coverage on unchanged code. Indirect changes
|
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
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
…orts 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
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 `<path>.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
`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
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
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
…mports 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
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
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
…orts 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
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 `<path>.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
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
9fcb444 to
7e214cf
Compare
`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
Summary
Fixes #2865.
ModuleLoader::ensure_module_loadedparsed each module withjq::parse_program— which returns a fullProgram,includesandimportsincluded — and then read only
program.expr, silently discarding the module'sown directives. A module saying
include "inner"; def h: g;therefore exportedan
hwhosegwas genuinely undefined, where real jq answers42.The issue's own suggested fix direction ("apply
process_programrecursively")gets three of jq's rows wrong, so the semantics were re-derived from the 16-row
oracle matrix in the triage plan and every row re-captured live here against
/usr/bin/jq1.7.1.The shape that buys every row from one mechanism is to wrap each exported
def's body in the dependencies it reaches, rather than splicing them into the
exported chain:
include "outer"; h,outer=include "inner"; def h: g;42— dependency visible inside the moduleinclude "outer"; ginclude "inner"; def g: 7; def h: g;, thenh42, not7— dependency is innermost, beats the module's own siblingg7— its own definclude "inner"; def g: 7; g7— the opposite way, since a filter's defs bind at parse time"base", not42), per (name, arity)def f(g): g;with a dependencyg7— a parameter wins, in bothgand$gspellingsOnly dependencies are wrapped in. A module's own defs are emitted as siblings
in the top-level chain, exactly as a filter's own defs are, because jq's lexical
rule already relates them there — nesting a copy of one inside another's body
puts it under scopes it was never written in, which review showed lets a
parameter or a later sibling capture a call that resolved to a builtin
(
def a: length; def h(length): a;) or swallow a compile error outright(
def a: b; def h(b): a;). A dependency's references to its own module'ssiblings are instead satisfied by pulling those siblings in as dependencies too
— they are exports of the same module — which is what makes
inner.jq=def g: 42; def k: g;withouter.jq=include "inner"; def g: k;answer42while self-recursion still binds to itself.Three supporting changes:
ensure_module_loaded'sentry()spelling helda mutable borrow of the cache across the whole load, structurally forbidding
recursion. It is now
contains_key→ load →insert→ re-get; the doccomment that justified
entry()is rewritten rather than left contradictingthe code.
"matching would take the host process down" carve-out. jq 1.7.1 does not
diagnose
ca⇄cbat all; it recurses until it dies (exit 139, SIGSEGV,nothing on either stream), and a self-including module does the same. There
is no reference output to be faithful to, so succinctly reports
module cycle detected: ca -> cb -> caand leaves through the same exit-3jq: 1 compile errordoor as the other two compile-error kinds. Detectionkeys on the resolved file (so
include "./m"insidem.jqis caught),starts the chain at the repeat rather than the bottom of the load stack, and
cannot use the memo cache, since a module is absent from it for exactly as
long as its dependencies are loading.
Import::data.parse_importread and discarded the$thatdistinguishes a data import (
import "f" as $d;) from a module import, soresolving one as a module reported
module not found. The flag is nowrecorded; both the top-level and module paths resolve the data import's
<path>.jsonand contribute no defs, so a typo is still jq's ownmodule not foundand exit 3 on both, byte for byte. Binding the variableitself remains jq: data imports (import "f" as $d;) are unimplemented — the $ is dropped at parse time, so they resolve as module imports and fail #2956.
Sizing. Dependencies are filtered to the transitive closure of what the body
actually calls — jq's own
block_bind_referencedrule, and here a sizingrequirement rather than a micro-optimisation, since each level's bodies already
carry the level below. Peak RSS, Apple M-series release build, against
mainand jq on the same fixtures:
mainThe last row is the residual: a chain of modules whose defs each call more
than one def below still compounds, because binding copies the AST where jq
shares blocks. Filed as #2955 and recorded in
limitations.md.A documented boundary. A dependency is wrapped inside the including def's
own
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. The two exclusions
in
visible_deps_forcover names the body uses directly; three shapes beyondthat are recorded in
limitations.mdas a still-open gap (no ADR-0018 rule-4condition applies), filed as #2962, and pinned by a test carrying the jq answers
each assertion should become. None is a regression — every one needs a
transitive
include, which did not work at all before this change.Follow-ups filed
dependency named after a builtin cannot shadow it inside the module body (the
module-level twin of yq: jq: a def loaded via include/import/~/.jq cannot shadow a builtin (#2036 follow-up) #2395).
~/.jq's defs and sibling modules' defs, whichreal jq keeps out; needs a real scope boundary.
Import::dataandresolves the file, but does not bind the variable.
. as {(m::f): $v}fails with "module 'm' not loaded" #2957 —rewrite_namespaced_callsandwalk::uses_input_builtinsskipcomputed destructuring keys, the same stale-
Patternassumption this PR fixedin its own dependency walk.
Also closed #2843 as a duplicate of #2840 during board triage (unrelated to this
PR; its posted triage plan already recommended it).
Test plan
tests/jq_cli_tests.rsover a new multi-filerun_jq_with_moduleshelper — one per oracle row, the cycle (four shapes),the
$__loc__/jq: $__loc__ inside an included module reports the top-level filter's file, not the module's #2774 regression the stamping order could have causedsilently, the shadow-candidate boundary, pattern-key dependencies, data
imports and their missing-file error, four scope-capture shapes, two
size locks, and the jq: a wrapped module dependency sits inside the including def's scope, so that def's own name or parameters can capture names free in it #2962 pin
cargo build --features clicargo test --features cli,simd,regex,serde— 8593 tests across 63suites, green
cargo clippy --all-targets --all-features -- -D warningscargo clippy --all-targets --features std,simd,serde,cli,regex,bench-runner,large-tests,mmap-tests -- -D warningscargo build --no-default-features(no_std) andRUSTDOCFLAGS="-D warnings" cargo doc --no-depscargo fmt --check/usr/bin/jq1.7.1 — not justgolden fixtures — including exit codes, re-run after the final rebase.
All match except one whose diagnostic location differs (
<top-level>vs the module's own file), which is the pre-existing jq: unresolved-call diagnostic doesn't name the module a bad def came from #2866
/code-reviewrounds, every finding reproduced live before beingacted on
omni-dev coverage diff— run before enqueueing