fix(extract): model Nix attrpaths (leaf name, scoped QN) and mint Variables for module-level bindings - #1305
Conversation
6fc96a0 to
ebfc9ea
Compare
DeusData
added
bug
|
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 |
|
I merged #1304 a moment ago, which means this branch now conflicts — it carried those two commits, and they have since landed on The review itself is done and positive. What I checked, since the whole design rests on it: definition QNs and call-scope QNs both route through the same shared helpers ( A few things I appreciated specifically: the quote test asserts the quoted form is absent rather than merely checking the bare form is present; every absence assertion is paired with a positive control, so none of them can pass vacuously; and On the Variable half you offered to split: I am keeping it. Two asymmetries worth recording for whoever reads this later, neither blocking: Rebase and it goes in. Thank you for the care in both of these — self-reporting your own limitations is rarer than it should be. |
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>
ebfc9ea to
4397833
Compare
|
Rebased onto On the two asymmetries you recorded: leaving both as-is for follow-up, as you called them. For the record on the first one — One disclosure on my local run, since none of it is in the paths this PR touches. |
|
Merged as On the two red suites you flagged: neither is yours, and you were right to flag rather than claim. I checked
And your explanation of the first asymmetry is better than my description of it. "The dotted spelling is one binding whose leaf sits at file scope, whereas the nested spelling puts the leaf inside an attrset expression — same value, different binder depth" is exactly right, and I agree with your conclusion: this wants a stated rule rather than a fix, because the alternative is a scope model that is no longer lexical. I have recorded it that way. Thank you again — self-reporting your own red suites, unprompted and without hedging, is rarer than it should be. |
2 participants
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)