fix(extract): model Nix attrpaths (leaf name, scoped QN) and mint Variables for module-level bindings - #1305
Open
sini wants to merge 7 commits into
Open
fix(extract): model Nix attrpaths (leaf name, scoped QN) and mint Variables for module-level bindings#1305sini wants to merge 7 commits into
sini wants to merge 7 commits into
Conversation
… are reached
A Nix library or module file's root expression is normally itself a function —
`{ pkgs, lib, ... }: <body>` — the near-universal header for the ecosystem.
That node matches nix_func_types, so walk_defs handed it to extract_func_def,
which resolved no name for it (the Nix resolver requires the function's parent
to be a `binding`; the root lambda's parent is the file root, `source_code`) and
minted nothing. walk_defs then hit `if (!descend_into_func) continue;` and
abandoned the whole subtree, so no binding below the header was ever visited.
The effect was total for the dominant file shape, and invisible for the rest:
a file opening with a bare `let` or attrset was walked normally, so the language
looked supported. Measured on a 320-file Nix repository, 253 of whose files
carry a function header:
before after
Function 48 1900
CALLS 56 1443
files w/defs 8 258
Adding CBM_LANG_NIX to descend_into_func is sufficient. Descending does not
over-mint from curried lambdas: in `f = a: b: ...` the inner `b:` has a
function_expression parent, resolves no name, and stays out. Checked clean for
over-minting on map/foldl' arguments, `with` bodies, `inherit`, and
parenthesized lambdas; rec attrsets, nested attrsets, `let` inside a function
body, and factory-returned attrsets all mint correctly.
Call attribution is unaffected. CALLS scope is computed by a separate traversal
(push_boundary_scopes -> compute_func_qn) that calls the same shared
cbm_resolve_func_name this path uses, so definition names and call-scope names
agree by construction, and a scope is pushed only when that name resolves
non-NULL. The edge-count rise is previously-dropped edges being written, not
re-attributed ones: before the fix those edges named a source node that did not
exist.
The change is gated on ctx->language, so no other language's walk is affected.
Signed-off-by: Jason Bowman <jason@json64.dev>
… of error
`nix_function` was the only Nix extraction test, and it could not fail for the
bug the previous commit fixes. Three separate reasons:
- its fixture is function-headed — the exact broken shape;
- its binding, `hello = pkgs.writeShellScriptBin "hello" ''...''`, is an
application, not a function, so no Function def is expected from it either
way;
- it asserts only ASSERT_NOT_NULL and ASSERT_FALSE(has_error), omitting the
has_def check every adjacent language test makes.
So the suite reported green while definitions were being dropped for the
dominant file shape in the ecosystem.
This keeps `nix_function` as-is — it usefully pins the "parses, yields no def"
case — and adds one test per root shape, each asserting its own definitions so a
regression names the shape it broke:
nix_defs_in_let_rooted_file let ... in
nix_defs_in_attrset_rooted_file { ... }
nix_defs_in_nested_let let inside let
nix_defs_survive_function_header_let { prelude }: let ... in
nix_defs_survive_function_header_attrset { prelude }: { ... }
nix_defs_survive_curried_header final: prev: { ... }
nix_curried_lambda_mints_one_def guards the other direction
The curried-header case is the nixpkgs overlay signature, the most common
multi-arm header in the ecosystem: two nested function_expressions sit between
the file root and the body. The last test exists because the fix descends past
the header — it pins that `iota = a: b: a + b` mints exactly one Function, so a
change that starts minting the inner arm as a second def is caught.
Verified as a negative control — with the previous commit's one-line change
reverted, the three function-header tests fail on their own assertions (gamma,
eta, iota) while the pre-existing shapes stay green. With it applied the
extraction suite is 260 passed, 0 failed.
Signed-off-by: Jason Bowman <jason@json64.dev>
A Nix binding's name is a PATH, and the resolver took only its first segment
(`child_by_field_name(attrpath, "attr")`). Three defects followed from that one
modelling gap:
- `setA = { dup = …; }` and `setB = { dup = …; }` both minted the qualified
name proj.file.dup. The second definition, and every CALLS edge sourced from
it, was silently discarded at write.
- `a.b.fn = …` minted a definition named `a`, colliding with every other
binding whose path began `a`.
- `"kebab-case" = …` minted a name with the quote characters in it, so every
consumer keying on the name had to know to re-quote.
Adopts the convention already used for C++ namespaces — name is the leaf
segment, qualified_name is enclosing scope plus leaf, so `ns::serialize` is name
`serialize` and QN proj.file.ns.serialize. For Nix that means:
- the resolver returns the LAST attrpath segment;
- leading segments become scope, so `a.b.fn = …` and the nested spelling
`a = { b = { fn = …; }; }` produce the same QN, as they must — one is sugar
for the other;
- a binding whose value is an attribute set is a scope, mirroring
is_namespace_scope_kind's treatment of a C++ namespace. `let` bindings are
deliberately NOT scopes: they are lexical, and C++ does not qualify by block
scope either;
- a quoted segment is unquoted, and an interpolated one (`"${x}" = …`) mints
nothing at all, since it has no statically knowable name — the same call the
Makefile dot-prefix guard makes.
The two scope sources compose: `setA = { a.b.fn = …; }` is
proj.file.setA.a.b.fn.
Definition QNs and call-scope QNs are computed by two different functions —
extract_defs.c's compute_class_qn and extract_unified.c's own. If they disagree
by one segment the CALLS edge names a source node that was never minted and is
dropped at write, with no error and no failing extraction assertion. Rather than
implement the rule twice, the Nix computation lives in helpers.c and both call
it, which makes that class of drift impossible instead of merely unlikely. Note
also that the def-side enclosing-QN gate is language-gated while the call-side is
not, so widening one without the other would have produced exactly that mismatch.
Signed-off-by: Jason Bowman <jason@json64.dev>
Four extraction cases and one pipeline case, each asserting a distinct claim so a
regression names what it broke:
nix_attrset_scope_disambiguates_leaf_names
two attrsets each holding `dup` produce two definitions with distinct QNs.
Unqualified they shared one QN and the second was discarded.
nix_dotted_attrpath_qualifies_like_nested
`wrap.deep.fn = …` and `wrap = { deep = { fn = …; }; }` produce the SAME
QN. This is the equality that makes attrpath qualification a correctness
fix rather than a preference — the two are the same expression.
nix_quoted_attr_name_strips_quotes
`"kebab-case"` is named kebab-case, and asserts the quoted form is absent
rather than merely that the bare form is present.
nix_interpolated_attr_mints_no_def
`"${dynamic}" = …` mints nothing, and the sibling binding still does — so
the test cannot pass by extracting nothing at all.
pipeline_nix_scoped_binding_calls_resolve (tests/test_pipeline.c)
a call inside a scoped binding reaches its target through the store, for
both routes into a qualified name: an enclosing attrset and a dotted
attrpath.
The pipeline case has to live at that level. Definition QNs and call-scope QNs
come from two separate functions, and if they disagree the edge is dropped at
write with no error — every extraction-level assertion still passes while the
graph quietly loses edges. Only the store shows it.
Adds has_def_qn alongside the existing has_def. find_def_by_name returns the
first match by NAME and so cannot tell two same-named definitions in different
scopes apart, which is exactly what these tests exist to separate.
Extraction and pipeline suites: 491 passed, 0 failed.
Signed-off-by: Jason Bowman <jason@json64.dev>
`nix_var_types` has declared `binding` since the language was added, but no Nix
case existed in extract_var_names and no path reached a binding in the first
place, so the Variable count for Nix was unconditionally zero.
Two things were in the way. extract_variables iterates only the file root's
DIRECT children, and a Nix file's root child is its header lambda
(`{ pkgs, lib, ... }:`), so the generic loop saw nothing — the same shape as the
definition bug. And extract_var_names had no Nix branch, so a binding could not
have been named even if reached.
Scope follows the rule the other languages already use rather than inventing one.
extract_variables mints FILE scope and never locals: a C++ declaration inside a
function body is not a Variable. For Nix, file scope is the `let` bindings and
the returned attrset — a let binding is file scope in the same sense a C++
file-static is, and the attrset is the exported surface. A binding in a deeper
attrset is not.
That bound is load-bearing. Every Nix binding's parent is a binding_set at any
depth, so admitting them all would mint a node per `enable = true` in a NixOS
module's settings tree — the per-leaf flood the Helm values.yaml case in this
same function already exists to avoid.
Bindings whose value is a lambda or an attribute set are skipped: the first is
already minted as a Function by the def walk, the second is a scope
(is_namespace_scope_kind), so minting either here would double-count it.
Names follow the attrpath convention from the previous commit — leaf as the name,
whole path in the qualified name, so `services.nginx.enable = true` is name
`enable`, QN proj.file.services.nginx.enable. push_var_def grows a _qn variant
for that; the plain form delegates to it and is unchanged for every other
language.
Extraction and pipeline suites: 494 passed, 0 failed.
Signed-off-by: Jason Bowman <jason@json64.dev>
…ariable split
nix_module_level_bindings_mint_variables
a `let` binding and an attrset binding both mint Variables, and the QN
carries the attrpath (services.nginx.enable).
nix_nested_bindings_are_not_module_level
a binding two attrsets deep mints nothing, and neither `deep` nor `nested`
becomes a Variable since both are scopes.
nix_lambda_binding_is_function_not_variable
a lambda-valued binding is a Function and NOT a Variable; a scalar binding
is the reverse.
Every absence assertion is paired with a positive one on the same predicate in
the same result — `topLevel` in the nesting test, `val` in the split test — so
none of them can pass by extracting nothing at all. Several false "clean" results
in this area came from predicates that could not have matched.
Signed-off-by: Jason Bowman <jason@json64.dev>
LABEL_GOLDENS pins the node labels each grammar emits. The nix case is
`{ foo = 1; bar = 2; }` — two module-level scalar bindings — and its golden was
Module:1, recorded when no Nix binding could be named and none was minted.
Both are Variables now, so the golden is Module:1,Variable:2. The mismatch CI
reported is the intended behaviour change, not a regression.
Extraction, pipeline, grammar_labels, grammar_regression and lang_contract:
534 passed, 0 failed.
Signed-off-by: Jason Bowman <jason@json64.dev>
sini
force-pushed
the
feat/nix-attrpath-and-variables
branch
from
July 28, 2026 00:08
6fc96a0 to
ebfc9ea
Compare
DeusData
added
bug
Owner
|
Thank you for the contribution and for separating the Nix attrpath and Variable work while clearly documenting its dependency on #1304. This is now triaged as a high-priority parsing-quality bug for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Models Nix attrpaths, and mints Variable nodes for Nix module-level bindings.
Built on top of #1304. That PR makes definitions reachable in function-headed files at all; this
one fixes how they are named once reachable, and adds the Variable half. The two are separate
concerns and the four commits here are extracted cleanly for independent review — but the branch
carries #1304's two commits as its base, so please merge that first. The one hard dependency is
pipeline_nix_scoped_binding_calls_resolve, whose fixture is function-headed and mints nothingwithout #1304.
Filing under the CONTRIBUTING carve-out for focused bug fixes, as with #1304. Commits 3–4 (the
Variable half) add a node class rather than repair one, so if you would rather see that split out or
discussed in an issue first, say the word and I will pull it.
1. Attrpath names — a merge bug and an encoding bug
A Nix binding's name is a PATH. The resolver took only its first segment
(
child_by_field_name(attrpath, "attr")), and three defects followed from that one modelling gap:setA = { dup = …; }dup, QNproj.f.dupproj.f.setA.dupsetB = { dup = …; }proj.f.setB.dupa.b.fn = …afn, QNproj.f.a.b.fn"kebab-case" = …"kebab-case"(quotes included)kebab-case"${dyn}" = …"${dyn}"The collision is the serious one: two bindings sharing a leaf name in one file collapsed onto a
single node, and the second definition and every CALLS edge sourced from it were silently
discarded at write. On a 320-file Nix repository a conservative scan found ≥54 files affected and
≥275 bindings collapsed.
The convention adopted is the one already used for C++ namespaces —
ns::serializeis nameserialize, QNproj.file.ns.serialize. For Nix: name is the leaf segment, qualified_name isenclosing scope plus leaf. A binding whose value is an attribute set becomes a scope, mirroring
is_namespace_scope_kind's treatment of a namespace.letbindings deliberately do not scope —they are lexical, and C++ does not qualify by block scope either.
The correctness argument is that Nix has two spellings of the same thing:
Before this they produced different, both-wrong answers.
nix_dotted_attrpath_qualifies_like_nestedpins that they now agree.
2. Variables
nix_var_typeshas declaredbindingsince the language was added, butextract_var_nameshad noNix case and
extract_variableswalks only the file root's direct children — which for afunction-headed file is the lambda. So the Nix Variable count was unconditionally zero.
Scope follows the existing rule rather than a new one:
extract_variablesmints file scope andnever locals, exactly as a C++
declarationinside a function body is not a Variable. For Nix,file scope is the
letbindings and the returned attrset; a binding in a deeper attrset is not.That bound is load-bearing. Every Nix binding's parent is a
binding_setat any depth, so admittingthem all would mint a node per
enable = truein a NixOS module's settings tree — the per-leaf floodthe Helm
values.yamlspecial case in this same function already exists to avoid. Measured on a499-file NixOS configuration: 226 Nix Variables, at most 9 in any one file. No flood.
Lambda-valued and attrset-valued bindings are skipped — the first is already a Function, the second
is a scope — so nothing is double-counted.
The constraint that shaped the implementation
Definition QNs and call-scope QNs are computed by two different functions:
compute_class_qninextract_defs.c and a separate one in extract_unified.c. If they disagree by one segment, the CALLS
edge names a source node that was never minted and is dropped at write — no error, and every
extraction-level assertion still passes.
They are also asymmetric today: the def-side enclosing-QN gate is language-gated, the call-side is
not. So widening one without the other would have produced exactly that silent mismatch.
Rather than implement the rule twice, the Nix computation lives in
helpers.cand both sides callit, which makes that class of drift impossible rather than merely unlikely.
pipeline_nix_scoped_binding_calls_resolveguards it end to end through the store, because noextraction-level test can see a dropped edge.
Impact
Cumulative with #1304, same binary and
mode=fullthroughout,Function/Variable/CALLS:mainVariable counts are Nix-sourced only; the system config also carries ~7.4k pre-existing JSON
Variables, unchanged by this.
The framework repository's Function count rising 1900 → 2213 with no new files is the collision fix
alone — 313 definitions that previously shared a qualified name with a sibling and were discarded.
That independently corroborates the ≥275 estimate from the static scan.
Tests
Seven new cases. Extraction: leaf-name disambiguation across two attrsets; dotted-vs-nested QN
equality; quote stripping (asserting the quoted form is absent, not merely that the bare form is
present); interpolated attrpath minting nothing; module-level Variables including the attrpath QN;
nested bindings minting nothing; and the Function/Variable split in both directions.
Every absence assertion is paired with a positive one on the same predicate in the same result, so
none can pass by extracting nothing at all.
Pipeline:
pipeline_nix_scoped_binding_calls_resolvecovers both routes into a qualified name — anenclosing attrset and a dotted attrpath — asserting each call still reaches its target through the
store.
Extraction and pipeline suites: 494 passed, 0 failed.
Checklist
git commit -s) — required, CI rejectsunsigned commits (DCO, see CONTRIBUTING.md)
make -f Makefile.cbm test) — same pre-existing LeakSanitizer caveat asfix(extract): Nix definitions are dropped in any file whose root expression is a function #1304:
3679 byte(s) leaked in 5 allocation(s)from the test harness, identical atd587deabuntouched
make -f Makefile.cbm lint-ci)