fix(elixir): correct function extraction, identity and reference edges - #2310
BobbieBarker wants to merge 13 commits into
Conversation
2f92214 to
8955d98
Compare
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
8955d98 to
7c24b02
Compare
b2e9199 to
d636aa7
Compare
`def name(args) when guard` does not parse as a call with a guard beside it. tree-sitter-elixir builds one `when` binary_operator for the whole head, and the `name(args)` call that names the function is that operator's left operand. Four walks read that same node, the def's first argument, and each asks a different question of it: the defs walk names the function from it; the unified walk opens the function's call scope from it; the usages walk bounds the head's bindings with it; the calls walk tells a definition head apart from an invocation by it. A private copy in one of them makes the rest disagree about what a guarded clause is. The disagreement is silent, and it mints an edge: a head one walk treats as a declaration and another does not is recorded as a call to the very function being defined. Two questions, so two exported entry points: cbm_elixir_def_head_unwrap_guard(first_arg) returns the head UNDER every guard, or the node unchanged when there is none. The three walks that need a name or a span take this. cbm_elixir_def_head_is(signature, node) is true for that head OR for any `when` operator above it. The calls walk takes this, because a guarded head reaches it by two routes and it sees only one node per visit: `def f(x) when g` arrives as the inner `f(x)` call, and `def f when g` has no inner call at all, so the `when` operator itself arrives. Recognising one route and not the other leaves the phantom in place for the other. Both test the operator TOKEN rather than the node kind. `def a + b` is a binary_operator head too, and peeling it would name the function after its own left parameter. This commit changes no extraction behaviour: nothing calls either helper yet. The walks are separate commits, each with its own measurement. Measured while writing the test, which corrects a claim worth not repeating: tree-sitter-elixir nests multiple guards RIGHT-associatively. `def f(x) when is_atom(x) when is_binary(x)` parses as `when(f(x), when(is_atom(x), is_binary(x)))`, so the head is the outer operator's left operand and one peel reaches it. The peel still runs to a fixed point (one extra token compare), so the helper's contract is "the head under every guard" even where one peel would do. In tests/test_extraction.c, the test elixir_def_head_is_covers_the_head_and_every_guard_above_it drives the helpers on the AST directly rather than through cbm_extract_file. It has to: the two routes into the calls walk are the pair a single-walk test cannot separate, because each walk only ever sees the node its own traversal reaches. It pins the unguarded case, both guarded shapes, the double guard, and that `def a + b` is returned unchanged with its left operand answering false. Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
extract_elixir_func_def reads the def's first argument directly and accepts only a `call` or an `identifier` there. A guard makes that argument a binary_operator, so the extractor found no name and returned. Every guarded clause was dropped, and a function whose clauses ALL carry guards never reached the graph at all. The loss is silent: a missing definition is not an error anywhere in the pipeline. Guards are ordinary Elixir. Measured by indexing one 970-file Elixir lib tree with the binary built from e783f73 and with this one, and diffing the `nodes` and `edges` rows out of the two SQLite stores: Function nodes 20,882 -> 21,941 new qualified names 1,059 (functions with no node at all before) lost qualified names 0 CALLS edges 55,812 -> 57,090 USAGE edges 27,418 -> 28,223 Every one of those 1,059 is a function whose every clause carries a guard: a clause without one already minted the node they collided on. An agent asking "who calls this" about any of them got an empty answer that reads exactly like dead code. The unwrap goes in the shared helper. The calls, unified and usages walks read the same node, and a private copy in one of them makes the rest disagree about what a guarded clause is. Two regressions come with the new node. Neither is minted here: both are pre-existing phantoms that had nowhere to land while guarded clauses were dropped, and now have a node. 1. A phantom CALLS edge onto the function being defined. The calls walk decides a def head is a declaration by comparing the visited node against the def's first argument, and a guard makes that argument the `when` operator, so the inner `name(args)` call no longer compares equal. cbm.c flags self-recursion by line containment, so the phantom lands inside the function's own span: self_recursive Function nodes 129 -> 1,342 ...with no self-call form in span 24 -> 1,241 (18.6% -> 92.5%) (measured by reading each flagged node's own source span and looking for any non-declaration line that names it as a call, a capture or a pipe target.) 2. A phantom WRITES edge onto it, from a paren-less guarded head. `def f when g` has no inner call at all, so the `when` operator reaches the read/write extractor and its first identifier is recorded as an assignment target. Reproduced on `def bare_guarded when true, do: :ok`: one CBMReadWrite row, var_name "bare_guarded", is_write true, scoped to the file's Module node (the graph asserting that a module mutates a function). Corpus WRITES 11,220 -> 11,302 (+82). Both become observable with this commit, and the guarded call-scope commit later in this stack closes them, by different mechanisms. The CALLS phantom goes because that commit admits the `when` operator to the definition-container check, so a paren-less head is recognised as a declaration. The WRITES phantom goes because that commit opens a function scope for the clause: the read/write row is still recorded, but its enclosing_func_qn becomes the guarded clause itself rather than the enclosing module, so resolve_rw_edges finds src->id == tgt->id and drops the self-edge. They are stated here because a stack that lands partially must not hide them. tests/test_extraction.c: extract_elixir_guarded_def_head covers every guarded shape (one guard, two guards, a `defp`, a paren-less `def bare when true`) and both operator-definition forms that must NOT be unwrapped (`def a + b`, `def c - d when is_integer(c)`, neither of which is extracted, as before). It fails on the current tree for each of the guarded names. Depends on the shared def-head helper commit. Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
Every clause of an Elixir function is its own `def` call, and an Elixir QN
carries neither module nor arity, so all clauses compute one qualified name and
collide on a single graph node. cbm_gbuf_upsert_node breaks a same-QN collision
by keeping the largest start_line, so the survivor was whichever clause was
written last, and get_code_snippet returned that clause as the whole function:
def admin?(%__MODULE__{role: :admin}), do: true
def admin?(%__MODULE__{}), do: false
read back as `def admin?(%__MODULE__{}), do: false`, so an agent asking what the
function does is told the predicate always returns false.
One node per function is right, so the fix widens that node's span instead of
splitting each clause onto its own node. Two bounds keep the widened span honest, and a clause folds only
if it clears both: it must sit in the same module body as the previous one (an
Elixir QN carries no module, so `Outer.run` and `Outer.Inner.run` compute the
same name), and it must be adjacent to it (only the immediately preceding
EXTRACTED definition is a candidate, so a def of another name ends the group).
Measured by indexing one 970-file Elixir lib tree with the binary built from
e783f73 and with this one, and diffing the `nodes` and `edges` rows out of the
two SQLite stores:
2,311 functions stop being reported as one line of source, which is the
get_code_snippet defect measured on this corpus.
A widened span is only honest if nothing phantom lives inside it, which is why
the head suppression in extract_calls.c lands with the fold rather than as a
follow-up. A
guarded clause head is a declaration of the name, never a call to it, and the
calls walk did not agree: it suppressed a head only when the head node IS the
def's first named argument, and `def f(x) when g` parses its whole head as a
`when` binary_operator, so the inner `f(x)` was not that argument. It now asks
cbm_elixir_def_head_is, the same helper the defs walk reads the head through.
cbm.c flags self-recursion by line containment, so under the folded span that
phantom would land inside the function's own node, and `recursive` is a
queryable node property that seeds the cycle detection in pass_complexity.
The widened span has a real cost. Reading each flagged node's own source span
and looking for any non-declaration line that names it as a call, a capture or a
pipe target:
self_recursive Function nodes 129 -> 349
...with no self-call form in span 24 -> 86
The lines responsible are the @SPEC and @doc heads a widened span now covers,
which the unified walk reads as code. That is a separate defect with its own fix
later in this stack, and a fold cannot close it.
Two things a reader of the graph will notice:
Edge metadata moves on edges that SURVIVE. `edges` is UNIQUE on
(source, target, type), so where the def-head phantom happened to be the dedup
survivor for a pair, deleting it promotes another row and the surviving edge
takes that row's `line`. 777 of them do; none changes its source, target or
type. The one CONFIGURES edge removed was minted from
`defp tls_unconfigured(%{tls_port: port, trust_anchor: anchor})`, whose head
was read as a configuration call, so removing that edge is correct.
is_exported becomes the OR over the folded clauses. The arity-free QN puts a
public entry point and its private accumulator on one node, and a name
callable from outside must stay exported. Exactly 6 Function nodes change
is_exported on this corpus, all false -> true and none the other way. Each is
a `def` clause written above a `defp` clause of the same name, where the
pre-patch last-clause-wins record reported a publicly callable name as
unexported. Verified against the source in every one: the new value is correct.
The typespec attributes put a declared name in a `call` node too and are
deliberately NOT suppressed in the calls walk. A def head can be, because
is_elixir_def_binding already treats a def's first argument as a binding, so the
identifier handle_calls declines is declined by handle_usages too. Nothing
treats a typespec subject as a binding, so declining one here would not remove
its phantom, it would RELABEL it as a USAGE: a separate row under
UNIQUE(source, target, type) that no longer collides with the real call it used
to hide behind. extract_elixir_declaration_head_mints_no_reference_of_any_kind
pins that as a relation rather than a count, because at extraction the relabel
is invisible.
Depends on the shared def-head helper and guarded def-heads commits.
Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
A guard wraps the whole head in a `when` binary_operator, and three walks that read a def's first argument were not expecting one. The three failures are independent, and none is an error at runtime: The unified walk resolves a def's QN to open the function's call scope, and compute_elixir_func_qn accepts only a `call` or an `identifier` there. Handed a binary_operator it returns NULL, no function scope is pushed, and every call in a guarded body is attributed to the File node. The function reports no outgoing edges at all, and each of its callees gains an in-edge from the file. So "what does this function call" answers empty for a guarded clause, and "who calls this" answers with a file. The calls walk tells a definition head apart from an invocation through cbm_elixir_def_head_is, which covers the head and the `when` operators above it. A paren-less guarded clause (`def bare_guarded when true, do: ...`) never reaches that check: call_node_is_definition_container only admitted a `call` node, and here the head is the operator itself. Elixir's call node types include binary_operator, so the operator reached extract_callee_name and, with no callee of its own, took that function's last resort, the first identifier child, minting a phantom CALLS edge onto the very function being defined. The usages walk asks whether an identifier sits inside the def's signature and treats everything that does as a binding site rather than a reference. A guard makes that signature the whole `when` operator, which spans the guard expression too, so a parameter read by the guard was classified as its own binding and emitted no USAGE row at all. A guard is an expression over parameters that `guarded(a, b)` has already bound, so every mention of one to its right is a read, exactly as the same mention in the body already is. Measured by indexing one 970-file Elixir lib tree with the binary built from e783f73 and with this one, and diffing the `edges` rows out of the two SQLite stores: Counted as the cumulative effect of the first four commits against e783f73, because this commit's attribution change is only observable once guarded clauses have nodes to be attributed to: CALLS sourced from a Module node 11,053 -> 5,840 (-5,213) CALLS sourced from a Function node 42,629 -> 49,542 (+6,913) CALLS sourced from a File node 2,130 -> 1,269 (-861) Calls written inside a guarded body stop being attributed to the enclosing module and are attributed to the function that writes them. The File count falls rather than rises, because the guarded-def-heads commit earlier in this stack gives those functions a node to source from. 5,275 USAGE rows appear for parameters read inside a guard that had been classified as their own binding. Node counts are identical in the two stores: the change moves where an edge comes from and records reads that were dropped. This also closes the WRITES phantom disclosed in the guarded-def-heads commit, though the extraction-level row does not go away. `def bare_guarded when true` still records one CBMReadWrite with var_name "bare_guarded" and is_write true. Its scope changes: with a function scope now open, enclosing_func_qn is the guarded clause where it had been the enclosing module, so resolve_rw_edges finds src->id == tgt->id and drops the self-edge. Measured on that exact fixture: before, one row scoped to `t.store`; after, one row scoped to `t.store.bare_guarded`. Corpus WRITES returns to 11,220. The row is still wrong at extraction and would reappear as an edge the moment anything sourced it elsewhere; removing it means teaching the read/write extractor about `when`, which is a fourth walk and belongs in its own change. tests/test_extraction.c: two tests. extract_elixir_guarded_def_head_scope asserts call attribution by exact scope QN rather than by callee name, because a call the scope walk failed to place carries the FILE QN, and a bare count of the callee passes just as happily with every call homed on the file. The same test pins the file-node count at 0. extract_elixir_guarded_def_head_guard_usages asserts each name both within its scope and in total, so an unwrap that took the guard instead of the head (admitting the binding sites themselves) fails on the totals. Only a `when` operator is admitted to the definition-container check. The generic first-identifier last resort still overrides a deliberate NULL for every other Elixir binary_operator that extract_scripting_callee declines (`=`, `<-`, `->`, `\\`, `::`), so `def g(c \\ 2)` still emits a phantom `c` and `e = target(d)` a phantom `e`. That is pre-existing behaviour for those operators and is untouched here. Depends on the shared def-head helper and multi-clause span commits. Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
An Elixir module attribute parses as `unary_operator(@, call(<attr>, args))`, so the typespec family reaches the unified walk in exactly the shape real code has. `@spec foo(t) :: t` puts `foo(t)` in the same `call` node an invocation would occupy, and `@type entry :: String.t()` does the same to a type name. The walk then reads a declaration as code and mints reference edges from it, sourced at the enclosing Module. Measured on a fixture of three typespec lines (`@type state :: map()`, `@spec fetch(opts) :: state`, `@spec load(User.t()) :: :ok`) beside `def state`, `def opts` and a remote `MyApp.User.t/0`: seven edges, every one phantom and every one landing on a Function. CALLS onto each specified function; CALLS + USAGE + WRITES onto `state`, the WRITES asserting a mutation that does not exist; and USAGE onto `opts` and onto `MyApp.User.t/0`, because a bare or remote TYPE name resolves to the same-named FUNCTION. On a codebase whose convention is @SPEC on every public function, every specced function carries an inbound edge from its own module, so fan_in never reaches 0 and "which exported functions does nothing call" is unanswerable. Measured by indexing one 970-file Elixir lib tree before and after: every specced function loses the inbound reference its own typespec minted, and no node is added or removed. This is one of two mechanisms. It removes the phantom a typespec line mints; the def-head phantom is removed by the calls walk recognising a `when`-wrapped head as a declaration, earlier in this stack. A function carrying both an @SPEC and a `when` clause needs both, which is why this commit is ordered after that one and its test can assert 0 references onto such a function rather than pinning a remaining 1. The skip covers the whole subtree, and two costs come with that. Both were measured by indexing a fixture repo (a declaring file per attribute head plus the file each one reaches) with the pristine binary and with this one: A typespec's type references are not indexed at all. That loses nothing correct: a type declaration mints no node, so such a reference resolves onto nothing or onto a same-named FUNCTION, which is itself the pollution. `@spec build(MyApp.User.t())` where that module's `t` is a `@type` produced no edge in either build; the ones that did resolve landed on a same-named Function, cross-file included. @callback and @macrocallback lose a real relationship. The declared name is a function name, so it resolved cross-file onto the functions implementing the behaviour, and nothing replaces that edge: `@behaviour MyBehaviour` mints none of its own. It goes anyway because it is only name resolution: on a fixture with one @callback and three same-named `handle_it/1` defs, the single edge landed on the one module that implements nothing, leaving both real implementors at fan_in 0. Recording that relationship correctly means minting it from `@behaviour`, which is a separate change. `unquote(...)` inside a typespec does execute at compile time, so `@type t :: unquote(build_type())` loses its genuine `build_type` edge too. Every other attribute VALUE is ordinary compile-time code (`@timeout Application.compile_env(:app, :timeout)` really does call compile_env), so those subtrees are deliberately walked exactly as before, and their attribute NAME still mints one phantom onto a same-named Function. That half of the defect class needs its own fix. tests/test_extraction.c: extract_elixir_typespec_attribute_is_not_code exercises all six heads in the skip list, so deleting any one entry breaks it. It pins the two accepted costs as decisions, and it carries a control name (`without_spec`) that reads 0 on the pristine build too, so an instrument that scored a definition as a reference could not make the other zeroes vacuous. extract_elixir_declaration_head_mints_no_reference_of_any_kind tightens from a relation to 0: with both mechanisms present, a function with an @SPEC and a guard carries no reference of any kind. Depends on the guarded call-scope commit, for the ordering above. It touches no file the earlier commits in this stack touch except extract_unified.c, and none of their lines. Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
cbm_registry_add takes the symbol's `name`, discards it, and derives the bare-name lookup key from the QN's last dot segment. That makes the QN's tail load-bearing for the by-name index every language shares. The tail is already wrong for one of those languages: rust_cfg_qualified_name mints a `#[cfg(test)]` twin as "proj.lib.add#cfg(test)", simple_name() has no '#' handling, so that function is indexed under the literal string "add#cfg(test)" where no bare `add` callee can reach it. The same hole would swallow every arity-fenced Elixir definition the later commits in this stack mint. The caller always knows the name; it is the argument. But the two keys disagree in both directions, so swapping one for the other only moves the hole. A name can equally carry segments the QN's tail drops: grammar def->name QN tail HCL resource.aws_instance.web web TOML tool.poetry.dependencies dependencies INI tool.isort isort Markdown 1.2 Scope 2-Scope Elixir Fx.Store Store all 162 helper.py helper (the file's Module node) The last row is the broad one: a file's Module node is named with the basename including its extension in every grammar, while its QN tail is the extensionless stem a bare module reference is written as. The HCL row is the one with a live reference syntax behind it: `instance = aws_instance.web.id` reaches the block through the tail `web`, and a name-only key drops that edge outright. So the index carries both keys, and pays for the second only where they differ. No key is removed, so no lookup that resolved before stops resolving. `name` is NULL or empty only for callers that have no symbol name to give; those have the derived key and nothing else. A throwaway probe walked tests/grammar_cases.h (one fixture per grammar, all 162) and compared def->name with simple_name(def->qualified_name), then targeted fixtures for the dotted shapes above. The Module row appears in all 162; HCL is the only fixture-level mismatch beyond it; TOML, INI, Markdown and Elixir mismatch on the targeted fixtures. XML namespaced elements (`ns:root`) do not mismatch: ':' is not a QN separator. tests/test_registry.c: registry_indexes_by_passed_name_not_qn_tail resolves a bare `add` against a cfg-fenced QN. registry_indexes_a_dotted_name_under_its_tail_too resolves an HCL block by its tail and a Module node by its stem. tests/test_pipeline.c: pipeline_hcl_block_reference_resolves_to_its_block indexes a two-resource Terraform file end to end and asserts the surviving USAGE edge. registry_indexes_by_passed_name_not_qn_tail fails without this commit. The other two pass on e783f73 and are here as guards: what would break them is the by-name index this stack re-keys, and an HCL block name is dotted where its QN tail is not, so they pin the shape that regression would take. It has no content dependency on any commit below it: it touches src/pipeline/registry.c plus two test files that none of them touch, and it applies to e783f73 cleanly. It is ordered before the module/arity identity commit because the arity-fenced definitions that commit mints need the passed-name key to stay reachable from a bare callee. Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
An Elixir qualified name was the file path plus the bare function name. It
carried neither the module written in the source nor the arity, and Elixir
identifies a function by both: `Fx.Store.fetch/1` and `Fx.Store.fetch/3` differ by
arity and are two functions, not clauses of one, and `Fx.Store.fetch/1` and `Fx.Other.fetch/1`
are two functions that a single-file-per-module convention happens to put in
different files. Every one of them computed the same QN and collapsed onto one
graph node, so the caller sets of unrelated functions were merged and
get_code_snippet returned whichever one won the collision.
cbm_elixir_def_head / cbm_elixir_def_qn replace two hand-written copies of the
same parse, one in extract_defs.c (naming the definition) and one in
extract_unified.c (opening the call scope), which had already drifted: only the
def side unwrapped the `when` guard until earlier commits in this stack pointed
both at the shared helper. They must emit a byte-identical string, because a
call is joined to its source node by exact QN and falls back to the File node on
a miss, so a one-segment disagreement silently reattributes every edge sourced
inside an Elixir function. A def's QN is now `proj.file.Module.name#arity`, and
an `elixir_frame_t` stack threads the enclosing defmodule QN down the walk so a
nested module composes (`Outer.Inner`). push_boundary_scopes pushes the matching
module scope, which it could not do before: `defmodule` is a `call`, so
compute_func_qn correctly returned NULL for it and the class branch was never
reached.
The fence character is '#', following rust_cfg_qualified_name's precedent
("add#cfg(test)"). It cannot be a dot segment: '.' is structurally reserved as
the QN separator across the whole codebase (simple_name, qualified_suffix_match,
the `LIKE '%.'||suffix` store probe, ~90 strrchr(qn, '.') sites). Only an
all-digit tail counts as a fence, so the Rust cfg twin is not mistaken for one.
The arity trait is enum-keyed rather than a CBMLangSpec field, for the build
reason recorded at lang_specs.h:47. src/foundation/constants.h gains exactly the
two names this commit uses: CBM_ARITY_NONE, the sentinel a QN with no fence and
a call site with no usable argument count both report, and CBM_LAST_OFFSET,
which the clause fold needs to index the definition it just pushed.
param_count travels with this because it is the one seam here that could not be
cut. The generic post-pass in cbm.c derives a parameter count from the signature
or the parameter list, and Elixir has neither: the arity is structural. It
zeroed what the extractor had just set, which is the only record of arity
outside the QN and feeds the structural-smell metric. Eight lines, and
extract_elixir_clauses_of_different_arity_do_not_fold fails without them, so
splitting it out would mean a commit whose own test cannot pass.
The container and arity halves are separable in principle and are deliberately
not separated. They are the same three lines of QN composition. An intermediate
state that carries the module but not the arity still merges fetch/1 with
fetch/3, so its own tests would have to pin the defect the next commit removes.
Each half rewrites every Elixir QN assertion in the suite, so splitting costs
two mechanical rewrites of the same assertions to buy one commit that fixes half
a defect. The change is 788 added lines, of which 220 are in
tests/test_extraction.c and 41 are two repro harnesses learning to strip a
fence.
The counts below come from building a product binary from the by-name index commit and one from this
commit, indexing the same 970-file Elixir lib tree with each into its own
sandbox (`cli index_repository --mode full`), and counting rows in the two
SQLite stores. Both stores report 970 File nodes, which pins the corpus:
Function nodes 21,941 -> 22,226
CALLS edges 50,193 -> 54,129
CALLS edges sourced from a Function 49,483 -> 53,375
The Function rise is the collision leaving: 285 net new nodes where one name
used to answer for several arities or several modules. CALLS rises with it,
because a call that used to land on whichever clause won the collision now
lands on a node that exists.
Two rows a pristine comparison would show are deliberately absent from that
table, because they do not move here. Module-sourced CALLS/USAGE/WRITES reads
17,207 on e783f73 and 729 already at the by-name index commit; it is 759 after this commit.
Distinct CALLS targets from a Module reads 8,224 on e783f73, 401 at the by-name index commit, and
401 after this commit. The extraction commits earlier in this stack own that
attribution collapse. Attributing it here would be measuring the stack and
calling it the commit.
CBM_INDEX_FORMAT_VERSION goes 1 -> 2: every Elixir QN in an existing store has a
different shape, so a stale index must be rebuilt rather than merged.
cbm_enclosing_func_qn gains an Elixir branch. Elixir has no class_node_types and
every construct is a `call`, so the generic chain walk cannot find the enclosing
defmodule and cbm_find_enclosing_func stops at the nearest `call` of any kind.
The Elixir walk climbs the ancestors itself, and must agree byte for byte with
extract_elixir_func_def's QN or the TYPE_REF and dbt edges it sources land on
the file's Module node.
Two exported helpers land here with no caller yet, and the resolution commit calls both:
cbm_qn_container_buf (pass_calls.c, pass_usages.c and their pass_parallel.c
twins) and cbm_lang_container_is_source_named (the same four sites). They are
the same fence/container vocabulary as cbm_fqn_with_arity and cbm_qn_strip_arity
and read as one block; splitting them across commits would put half a 30-line
primitive family in each. Every other symbol this commit exports is called
within it.
Tests: tests/test_extraction.c pins the def QN, sibling-module distinctness,
parent_class, the one-identity collapse of several clauses, the def/call-scope
QN agreement, that two arities of one name do not fold, and that a nested
module's clause does not fold into its parent's. Every Elixir QN assertion added
earlier in this stack is rewritten to the new shape, and that rewrite is the
blast radius.
It depends on the by-name index commit, which carries the passed name as well
as the QN's tail: with only the derived key, every fenced definition would be
indexed under "fetch#3" and unreachable from a bare callee.
Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
The def walk pushed only the direct `call` children of a defmodule's do_block and dropped every other head without descending. A def written inside `if Mix.env() == :test do`, `quote`, `case`, `for` or a user macro (all ordinary Elixir) produced no node and no error. elixir_push_block_calls descends through body-block kinds (do/else/rescue/catch/after/block/stab_clause/body) under the same module, bounded by ELIXIR_BLOCK_DESCENT_MAX, so pathological nesting returns at the bound instead of overflowing the C stack. `defmacrop` joins the three macros that already minted a Function. It has to join them in three lists at once: the def walk, the calls walk's definition-role list, and the usages walk's binding list. By contract those three are the same list, and a head one of them declines is relabelled. handle_calls declining a node leaves state->callee_expr unset, and handle_usages then reaches the bare identifier and mints a USAGE onto the same function. Under UNIQUE(source, target, type) that is a separate row, so it no longer collides with the real call it used to hide behind. Before this commit the def walk minted a node for `defmacrop` and the calls walk did not know its head, so a private macro sourced a phantom CALLS edge onto itself from its own def line and cbm.c read it as self-recursion by line containment. Measured by building a product binary from the module/arity identity commit and one from this commit, indexing the same 970-file Elixir lib tree with each into its own sandbox (`cli index_repository --mode full`), and diffing the Function `qualified_name` sets out of the two SQLite stores. Both stores report 970 File nodes: Function nodes 22,226 -> 22,259 new qualified names 33 lost qualified names 0 Nothing is lost: block descent reaches definitions that had no node at all and does not move any definition that already had one. The block descent also completes the fold's "same body block" bound, which was previously unreachable: a def inside a `quote` body computes the same (module, name, arity) QN as a module-body clause of that name and is extracted immediately after it, so only the block check keeps a quoted template out of the real function's span. extract_elixir_quoted_clause_does_not_fold_into_a_sibling pins it and could not have been written before this commit, because the quoted def was not extracted at all. The calls-walk comment was split across two functions and is now in one: the macro list is stated there as a cross-file contract, with the paren-less route beside it and no longer repeated at the definition-container check. recursion_whitelist.h gains elixir_push_block_calls_d and names its bound. Tests: elixir_defs_inside_macro_blocks_are_extracted covers `if`, `quote`, `case` and a user macro body; extract_elixir_private_macro_head_is_not_a_self_call pins the defmacrop node, its privacy, that it is not recursive, and that exactly one reference of any kind to it exists in the file (the real invocation); extract_elixir_quoted_clause_does_not_fold_into_a_sibling pins the block bound. Depends on the module/arity identity commit, whose frame stack carries the module QN this descent threads through the nested blocks. Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
pass_definitions and its parallel twin gated the Class -> member edge on `label == "Method"`, so a language whose members are labelled Function had no containment edge at all, and module membership existed only as a QN prefix. Previous commits in this stack made an Elixir def name its container, and the graph still had no edge for "what does this module contain" to follow. Relabelling Elixir defs to Method would put a falsehood in a column that the FTS rank boost, cbm_label_is_registry_symbol, cbm_label_is_type_like and the search label filter all read. Widening the gate is the smaller blast radius: every other site in extract_defs.c that sets parent_class on a callable already labels it Method, so the only edges this adds are for Function members. Measured by indexing one 970-file Elixir lib tree with the binary built from the previous commit in this stack and with this one, and diffing the `edges` rows out of the two SQLite stores: DEFINES_METHOD edges 0 -> 22,258 every other edge type unchanged 22,258 of the 22,259 Function nodes gain a containment edge; the one that does not is a def written outside any defmodule. Both copies of the gate move together. pass_definitions.c runs the sequential path and register_and_link_def in pass_parallel.c the threaded one; a change to one and not the other is a full-vs-incremental divergence that no single-mode test can see. Elixir's container is labelled Class, and a module is not a class. Fixing that label is deliberately out of scope here: it means deciding what the FTS rank boost and the type-like predicate should do with a new one. tests/test_pipeline.c: pipeline_elixir_container_and_arity_identity indexes a three-file Elixir repo end to end and asserts the DEFINES_METHOD edge from `Fx.Store` to `fetch` and from `Fx.ErrB` to `message`, alongside the identity assertions from the previous commit (three arities, three nodes; three same-named defs in three modules, three nodes). The resolution commit later in this stack extends the same test with its two CALLS assertions. Its content dependency is the module/arity identity commit, which sets parent_class on an Elixir def. Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
cbm_registry_resolve takes a bare (callee, module) pair and throws away two pieces of evidence the call site actually holds: the CALLER's own container, and the number of arguments written. cbm_registry_resolve_ctx takes both, and both fields are optional. A NULL context leaves the two new strategies inert: the container probe needs a container, the arity choice needs a written arity, and a caller that passes NULL has neither. The commit also extends an EXISTING rule to one more strategy, and that extension is unconditional. receiver_chain_admits is already in registry.c on e783f73 and already guards both name-lookup strategies for every language. It did not guard the same-module suffix fallback, which composes its candidate from the caller's module and the callee's SUFFIX and throws the written qualifier away. So `Keyword.get` in a project that defines its own `get` bound to that `get`, at same-module confidence, by a route the guard never saw. This commit routes that fallback through the same predicate. It is not reached through the context argument and does not read one, so a NULL context does NOT reproduce cbm_registry_resolve byte for byte: it differs exactly where a capitalised qualified callee used to bind to the caller's own module, which is the `Foo.bar` shape Java, C#, Python, Ruby and Swift callees take. The difference is deliberate. The fallback discards the qualifier in every language, so the defect is not Elixir's, and gating the guard on the new context would fix one language and leave the other 161 as they were, with the project's own rule already applied two strategies away. For those languages the guard can only withhold a candidate the fallback was about to return. `module_qn` is the FILE's QN, which is the container only for a language whose container IS the path. Where a language names its container in the source, the def QN carries an extra segment a file-QN probe can never match, so an intra-module call fell through to the bare-name scorer at lower confidence. A green test cannot see that degradation, because the scorer often still lands on a right-looking answer. Two arities of one name look like two unrelated candidates to every scorer in resolve_name_lookup, and letting import distance choose between them picks by path proximity among functions that are not interchangeable. When the call site wrote N arguments and exactly one candidate is fenced at N, that IS the answer. Arity is used only to choose among candidates that differ by their fence, never to reject a candidate that carries none. `has_arity_fence` keeps the whole mechanism off for a corpus that has none: no extra bucket scan on the hot same-module path, and no wider cache key. The per-file resolve cache key grows only where a container is present (one file holds several source-named containers, so the callee name alone is no longer sufficient) or where a fence exists. qualified_suffix_match strips a fence before comparing tails, because the fence is a discriminator and no part of the dotted identity the callee text names. Without that, every fenced definition was invisible to the one strategy built to disambiguate a qualified callee. It now counts matches rather than bailing on the second, so an arity hint can choose among them. Measured by building a product binary from the containment commit and one from this commit, indexing the same 970-file Elixir lib tree with each into its own sandbox (`cli index_repository --mode full`), and counting rows in the two SQLite stores. Both stores report 970 File nodes, which pins the corpus: CALLS edges 54,191 -> 55,266 CALLS edges sourced from a Function 53,441 -> 54,502 The +1,075 CALLS are calls the container-aware strategy resolves that a file-QN same-module probe could not see. They are a recovery, not a net gain over pristine: the module/arity identity commit declines a bare callee it can no longer attribute to one arity, and the stack ends at 55,266 against e783f73's 55,812. The receiver-chain guard's cost on this corpus is inside that net. No row here isolates it, because the corpus is Elixir and every one of this commit's mechanisms is live on it at once. Both resolve paths change together. pass_calls.c and pass_usages.c run the sequential path, resolve_file_calls and resolve_file_usages in pass_parallel.c the threaded one; the container recovery is duplicated verbatim across the twins because a divergence there is a full-vs-incremental difference no single-mode test can catch. tests/test_registry.c: resolve_qualified_suffix_sees_through_arity_fence pins that a qualified callee reaches a fenced candidate and that an arity no candidate carries declines rather than guessing; resolve_same_module_uses_caller_container pins the container path and that the file QN alone still cannot compose the candidate; resolve_qualified_is_monotone_under_unrelated_additions pins that adding one unrelated same-named symbol does not re-point an already-resolved edge; resolve_foreign_receiver_does_not_bind_local_name pins that `Keyword.get` resolves to nothing rather than to the project's own get/1; resolve_receiver_guard_is_language_agnostic pins the same refusal on unfenced Java-, Python- and JavaScript-shaped QNs through the NULL-context entry point, so the unconditional half has a test that owes nothing to Elixir, and pins the two carve-outs (a lower-case root and an ALL_CAPS constant root still bind). tests/test_pipeline.c: pipeline_elixir_container_and_arity_identity gains its two CALLS assertions, end to end over a three-file repo. Its shared end-to-end test lives with the containment commit. It also depends on the module/arity identity commit, for the fence. Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
Arity is now part of a node's identity, so adding or removing a parameter renames the node. An edit that changed both a function's body and its signature dropped every inbound cross-file edge into it until each caller's file happened to be re-parsed. A full reindex and an incremental reindex of the same tree disagreed. incr_restore_inbound_edges keeps exact QN as its primary key, as it always was. When that misses and the captured target carries a fence, a node whose QN differs only by its fence is the same function, so the edge follows it. Ambiguity (several arities of the same name in the same container) keeps the old behaviour and drops the edge, because nothing says which arity the caller meant. The lookup goes through an index. Restoring N captured edges against a graph of M nodes scanned all M for every fenced miss, so the pass cost the product of the two largest quantities in an incremental reindex. One walk builds the table and each miss after it is a hash lookup. The walk is deferred to the first fenced miss, so a corpus with no arity fences (every language but Elixir today) builds nothing and pays nothing. An allocation failure mid-build marks the index unusable rather than partial. A partial index would answer "sole match" for a name whose rival arity never got inserted. Measured on a two-file Elixir fixture: caller calls `landing/1`, then the file declaring `landing` is edited to `landing/2` and reindexed incrementally. Before this commit the CALLS edge is gone from the incremental store and present in a full reindex of the same final tree; after, both stores bind the same `...landing#2`. With two surviving arities the incremental store drops the edge and so does the full reindex. The test there asserts that the two stores agree, not that the edge survives. tests/test_pipeline.c: pipeline_incremental_preserves_edges_across_arity_change and pipeline_incremental_ambiguous_arity_successor_matches_full_index. Both compare the target's qualified_name rather than its bare name, because cross_file_edge_exists matches nodes by name and every arity of one function looks alike to it. Depends on the module/arity identity and resolution commits, which make the pre-edit edge exist to be captured. Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
Elixir writes every qualified reference as a `dot` node: `Keyword.get` is dot(alias "Keyword", identifier "get"). The usage extractor kept only the identifier half. The qualifier was destroyed before resolution ever saw it, so the usage arrived at the registry as the bare name "get", receiver_chain_admits returned true at its bare-name early return, and every receiver-chain protection was bypassed by construction. That is how an unrelated local `get/1` became the target of a `Keyword.get` reference, and how adding one unrelated file could re-point an already-resolved edge: with one candidate the bare name resolved by unique_name, and a second same-named candidate anywhere in the tree flipped the strategy to proximity scoring, which picks by path distance. is_member_access already recorded that a selector was there; it never recorded what it named. CBMUsage gains `receiver`, NULL when the reference is unqualified. The pipeline composes "<receiver>.<ref_name>" before resolving, and the edge property still reports ref_name, so nothing downstream sees a renamed reference. Scoped to Elixir on purpose. A receiver fed to the registry changes which candidate a usage binds to, and selector shape is a per-grammar claim. result_compact.c relocates the new string with the rest. Every arena-owned string a CBMUsage points at has to be relocated: the pipeline composes a qualified name from `receiver` before resolving, so a stale pointer corrupts silently. It resolves a reference built from whatever bytes the old arena's memory now holds, and the result is a valid-looking USAGE edge onto an arbitrary same-suffixed function, different on every run of the same input. Measured by building a product binary from the incremental re-link commit and one from this commit, indexing the same 970-file Elixir lib tree with each into its own sandbox (`cli index_repository --mode full`), and diffing the `edges` rows out of the two SQLite stores. A USAGE site is keyed by (source qualified_name, callee), which is stable across both builds: USAGE edges 31,844 -> 25,664 USAGE sites present before and gone after 6,245 USAGE sites present after and not before 0 surviving sites whose target changed 1,025 Nothing new appears, 6,245 bindings that the bare name alone justified are withdrawn, and 1,025 of the survivors move to a different target. A qualified reference that named a module the project does not define now resolves to nothing instead of to a same-named local function. The incremental re-link commit's product binary is byte-identical to the resolution commit's on this toolchain, so the "before" column is equally the value at resolution. tests/test_extraction.c: elixir_usage_records_receiver pins the recorded qualifier. extract_compact_relocates_every_usage_string walks every arena-owned pointer on every CBMUsage after cbm_result_compact and asserts each one lands inside the surviving arena block, so a field added later and not relocated fails the test. Its content dependency is the resolution commit, whose receiver-chain guard is what the recorded qualifier feeds. Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
Two query paths read a bare function name, and neither could see an arity fence.
get_code_snippet returned one body. Before arity was part of identity, fetch/1, fetch/2
and fetch/3 were one node and it returned whichever survived the collision, with
no ambiguity signal. Now they are three nodes, and a bare `fetch` answers that
the name is ambiguous, with one suggestion per arity. pick_resolved_node gains
`fold_arity`, and get_code_snippet passes false.
trace_call_path wants the opposite. "Who calls fetch" wants the callers of every
arity, and bfs_union_same_name already unions the seeds it is given, so it passes
true and candidates whose QNs are equal once the fence is removed fold into one
logical symbol, the same way a body-less `.d.ts` stub already folds into its
implementation. Two genuinely different same-named functions live in different
containers, so their stripped QNs still differ and they are still reported
ambiguous.
The flag gates both tallies, the top-score tie and the rival-definition count,
because either one alone decides the answer. Honouring it in only one left
ambiguity resting on whether two clauses happened to have the same line count.
For the same reason a fenced QN counts as a rival definition even with a
one-line span. The body-less rule exists for ambient declarations; a fence is
minted only from a definition head the extractor actually saw, so
`def size(x), do: byte_size(x)` is a whole definition that merely occupies one
line. Judging it by span would go back to deciding the answer by how many lines
a body happens to take up.
cbm_store_find_nodes_by_qn_suffix gains a third alternative matching the fenced
forms of the same dotted tail. Without it, get_code_snippet("fetch"), the
bare-name convention every agent is told to use, reached no arity of an
overloaded function. '#' is not a LIKE metacharacter, so it is matched
literally.
On a three-arity fixture, get_code_snippet("fetch") answered with one arity's
body and status "ok" before this commit and answers status "ambiguous" with one
suggestion per arity after it, whatever the relative body lengths.
trace_call_path is deliberately unchanged: it keeps folding the arities into one
union, which is why the two paths need a flag between them.
tests/test_mcp.c: snippet_bare_name_every_arity_is_ambiguous_whatever_the_spans
pins that the answer does not depend on relative body length, and
snippet_arity_fold_still_traces_every_arity_under_one_bare_name pins the trace
side of the same flag. tests/test_store_nodes.c:
store_find_by_qn_suffix_matches_arity_fence pins the probe.
Its content dependency is the module/arity identity commit, which mints the
fenced QNs this commit teaches the MCP layer to read.
Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
d636aa7 to
9b10ba2
Compare
DeusData
left a comment
There was a problem hiding this comment.
Thank you for this — it is a serious piece of work, and the Elixir identity problem it attacks is real: guarded heads, multi-clause spans, @spec/@type subtrees walked as code, and arity-blind names all cost us correct edges today. The 36 tests and the incremental re-link across an arity change show real care.
We would like to land the Elixir part, and we have made the call on the two open questions:
- Split it. Please split along the lines you offered, so each piece can be reviewed and verified on its own. In particular the HCL / TOML / Rust-cfg registry fix (
pipeline_hcl_block_reference_resolves_to_its_block) is a separate defect and belongs in its own PR. - Gate the cross-language changes to Elixir. Three changes reach every language even though the comments say the other languages stay byte-identical:
cbm_registry_addindexing every symbol under both the passed name and the QN's last segment;resolve_same_modulerunningreceiver_chain_admitson the same-module suffix path; and widening DEFINES_METHOD toFunctionmembers at both definition sites. Our standing rule for call resolution is that changes of this kind are per-language and gated on the file's language at bothpass_calls.candpass_parallel.c. A 10-language polyglot corpus is a good start, but it cannot prove "unchanged" for the other ~150 languages or at kernel scale, so the gate is the safer contract.
The Module.name#arity QN and the index-format bump are acceptable for Elixir in that shape; we will carry the rebuild note in the release notes.
Smaller items (thank you for already dropping the language count from the comments in today's push):
qualified_ref[512]is built withsnprintfwithout checking for truncation; please handle the truncated case explicitly.- #1731 also touches Elixir functions in
extract_defs.c(complexity, fingerprint, line count); worth a look so the two land cleanly together.
Thank you again — this is the most thorough Elixir pass we have seen, and splitting it is how it gets merged rather than stuck.
🫡 Aye aye captain |
|
Closing this. The branch here is still the pre-split stack you reviewed, so the diff no longer reflects the work. Where it went:
The rest is built and held until those two merge, then goes up in dependency order: the identity keystone together with the MCP bare-name fix, then the container-and-arity resolver, the reference qualifier, and the incremental re-link. The keystone and the MCP fix have to ship as one, which I measured rather than assumed. At the keystone alone all 22,259 Elixir function QNs carry a fence, and All three cross-language changes you flagged are gated in those branches, and Thanks for the review; the split was the right call. |
This PR was written by an AI agent working on my behalf. I certify the DCO sign-off on every commit and answer for the change.
Splitting this as you asked. I converted it to draft so it is not reviewed as a monolith, and it stays open as the integration branch and the home of your review thread until the pieces land.
Tracking issue: #2312.
The split
Module.name#arity, format bump 1 to 2The order is
{#2370, #2371} -> C -> {D, E} -> F, G. I am holding C through G instead of opening all seven at once, so each one's diff is exactly its own change against themainit will merge into.Two things I got wrong in the grouping I offered you
The grouping in #2312 put commits 8 and 9 before the keystone. That does not build:
fix(elixir): extract definitions inside macro and conditional blockscalls the 5-argumentextract_elixir_func_def, the 3-argumentelixir_stack_pushand readsframe.module_qn, all of which the keystone introduces. And commits 1 through 6 are not "independent of each other", they are a chain through the shared def-head helper. Corrected in the table above.I also described
pipeline_hcl_block_reference_resolves_to_its_blockin a way that led you to name it as the separate defect. It passes without the commit and is a guard. The reproducer isregistry_indexes_by_passed_name_not_qn_tailand the defect is Rust, not HCL. #2370 has the before and after.On gating the cross-language changes
Taking them one at a time. Your item 2 covered three changes under one instruction, and they do not all resolve the same way.
cbm_registry_addis in #2370 and is not an Elixir change at all. An Elixir gate there deletes its own reproducer and leaves Rust broken. #2370 gates the second key on the QN tail carrying a#instead, which makes "unchanged for every other grammar" a property of the code, with no appeal to a corpus I indexed. If you still want the enum, it needs aCBMLanguageparameter oncbm_registry_addplus a carve-out for Rust, and I will do it that way on your word.DEFINES_METHODand thereceiver_chain_admitssuffix path both gate cleanly and will be gated in C and D. For the receiver guard I intend to keep it on the new container probe and restore the pre-existing file-QN probe to unguarded:container_qnis non-NULL only wherecbm_lang_container_is_source_named(lang)holds, so that is the Elixir gate with no signature churn. It costs three assertions inresolve_receiver_guard_is_language_agnostic, a test written specifically to pin the property you are asking me to remove, so I will re-point it and say so in the commit.qualified_ref[512]is in F and will carry the truncation check.Two more cross-language reaches, neither in your list
Both are in the two files your standing rule names, and both are worse than the three you caught.
The arity hint is passed unconditionally.
pass_calls.c:563andpass_parallel.c:2930set.arity = call->arg_countwith no language gate, whilecontainer_qnthree lines above is gated.cbm_lang_overloads_by_arityexists and is called from nowhere in the passes. In a polyglot repo a bare Javafetch(a, b), whereby_name["fetch"]holds Java candidates plus oneFx.Store.fetch#2, can bind to the Elixir function:receiver_chain_admitsreturns true immediately for a bare name, andcbm_suppress_cross_language_suffix_matchonly fires onsuffix_match. It reaches through bothqualified_suffix_matchat confidence 0.90 andsole_fenced_candidateat 0.75. Two lines with your own predicate closes both. Going into D.cbm_store_find_nodes_by_qn_suffixmatches a fence that is not one.store.c:4396builds%%.%s#%%, so%.add#%matchesproj.lib.add#cfg(test), while the same commit'sqn_has_arity_fenceinmcp.c:8413is digit-restricted and its comment says the Rust twin is not a fence. The SQL and the C disagree, so a bareget_code_snippet("add")in a pure-Rust project reaches a node it could not onmain. Going into E, as a shared predicate instead of a third copy of the digit check.One of the ones you found has a worse twin.
registry.c:1596builds the resolve-cache keykeybuf[512]with an uncheckedsnprintf.container_qncomes fromcbm_qn_container_buf, which accepts up to 511 bytes, so past about 505 the callee is pushed out of the key entirely and every callee in that container shares one cache entry: the second lookup deterministically returns the first's resolved QN. Going into D, declining the cache on truncation instead of answering from a key that no longer identifies the question.The measurement I owed, and what it changes
fix(pipeline): resolve calls with caller container and written aritysaid the bare callees the keystone declines were never isolated. They are now. Same 970-file Elixir tree,origin/mainat64c23fab, darwin arm64, and the corpus indexes deterministically: pristine main indexed twice gives identical counts on every metric below, so none of these deltas are noise.I measured C against A+B instead of main, so #2371's intentional phantom removal does not muddy it. Keyed on (source file, callee as written):
So the decline is real and it is small: 19 bare callees on 970 files, against 3,010 call sites the container-aware identity resolves that A+B could not. Raw CALLS goes
50,230 -> 54,228. On resolution alone, C ships fine by itself with that cost stated.But C must not ship without E, for the other reason, and it is worse than I described it. At C all 22,259 Elixir Function QNs carry an arity fence, overloaded or not.
cbm_store_find_nodes_by_qn_suffixmatches'%.' || suffixor= suffix, and neither matches a fenced QN. E is the commit that adds the fenced alternative. Run against the C store:handle_get_code_snippet(mcp.c:12398) has that function as its only lookup, so C alone makesget_code_snippetreturn nothing for every Elixir function addressed as anything but its full fenced QN. No test catches it, because the test arrives with E.So C and E merge as one PR. Roughly 1,450 lines, over your guidance again. Last time I argued for the size; this time the measurement above is the reason. If you would rather have them as two, they can be two PRs merged back to back with no release between, and I will say so in both.
One more correction from re-taking the numbers
Pristine CALLS is
55,851, not the55,812every commit message on this branch cites. Main moved 17 commits. I have corrected #2371 and will re-take the rest before C goes up.Re-taking also surfaced something the commit messages undersold, which I have added to #2371: on main a quarter of Elixir reference edges are sourced from a Module or File node rather than the function that performs them (CALLS 76.4% Function-sourced, WRITES 76.6%). After #2371 all three are at 99%. Module-sourced CALLS fall from
11,061to541, which is the "who calls this" answer Elixir was missing, and a better argument for #2371 than anything I first wrote in it.