Repository navigation
fix(elixir): guarded def heads, multi-clause spans, and typespecs walked as code - #2371
BobbieBarker wants to merge 15 commits into
Conversation
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Thank you, @BobbieBarker. This is exactly the split we hoped for. The cross-language pieces are out, and the one shared-walk change (is_elixir_typespec_attribute) is gated on CBM_LANG_ELIXIR in its first line, which made it easy to confirm that nothing else changes. The clause fold only looks at the previous definition, so there's no quadratic risk. The corpus numbers (2,303 phantom edges gone, none added, and typespec-anchored CALLS down to 0) and the disclosed costs are exactly what we like to see.
Two small things before merge:
- Stale comment. The block above
elixir_call_is_definition_roleininternal/cbm/extract_calls.cstill saysis_elixir_def_bindingis "deliberately WIDER than the unwrap -- it covers the guard expression too". With this PR,extract_usages.cunwraps to the head, so guard reads now count as usages. Please update the comment to match the code, or tell us if we've misread it. - Final-tip number. The
self_recursive129 → 349 figure inextract_defs.cwas measured before the typespec skip. Could you give the count on the final tip?
On ordering with #1731: you're right that the two conflict semantically, since complexity comes from the first clause only. Thank you for offering to handle whichever lands second. We'll coordinate that on our side and let you know which goes first. Thanks again!
|
@BobbieBarker, following up on ordering with #1731, as promised. This PR goes first. Our carry of #1731 (Elixir complexity and fingerprints) will rebase onto it afterwards, and we'll resolve the overlap on our side. You don't need to do anything for it beyond the two small items in the review above. The reasoning: the two conflict textually in |
|
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. |
0dba492 to
eafaab1
Compare
|
You read it correctly on both counts, and the second one turned up two more numbers that needed fixing. 1. The stale commentThe unwrap in What it says now: all three walks find the head through one unwrap chain in 2. The final-tip count389, with 8 carrying no self-call form. Two corrections come with that. The 129 → 349 pair was measured on the pre-split chain, where the typespec skip sat earlier, so it describes no commit in this PR. My detector was also wrong. It skipped every line starting with Per commit on this stack, same 970-file tree, corrected detector:
All 8 at the tip are the phantom a local variable sharing the function's name mints, as in I dropped the @doc half of the stated cause. After the typespec skip the residue holds no @doc-driven flag, so @SPEC was doing the work; a @doc heredoc is a string, not a One intermediate matters if you read commit by commit: at the guarded-def-heads commit the count peaks at 1,342, because that commit gives 1,059 guarded clauses nodes of their own and the head suppression lands in the commit after it. A bisect that stops there sees the worst state in the stack. The figures now sit in the commit that produces them. 3. A number you did not ask forRe-measuring exposed that 2,303 was taken on the pre-split chain too, where the def-head extraction sat elsewhere. Measured against its own parent on current main, the fold and the head suppression remove 2,436 edges, 2,104 CALLS and 332 USAGE, and add none. 1,054 of the 119,696 survivors record a different The CONFIGURES edge in that sentence is gone: While I was in there I re-checked every figure in this PR's description against a fresh index of 4. OrderingUnderstood on #1731 going second, and thank you for taking the overlap. Nothing needed from me there. The stack is force-pushed. Every change in it is a comment or a commit message; the diff against the version you reviewed contains no non-comment line. |
`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 this commit's parent 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,851 -> 57,129
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
this commit's parent and with this one, and diffing the `nodes` and `edges` rows
out of the two SQLite stores:
4,135 functions stop being reported as one line of source, and none starts, 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
for any line that names it as a call, a capture or a pipe target, and counting a
one-line clause's body after `, do:` as such a line:
self_recursive Function nodes 129 -> 430
...with no self-call form in span 6 -> 48
The lines responsible are the typespec heads a widened span now covers: `@spec
f(t) :: u` puts the declared name in a `call` node and the unified walk reads it
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`. Of the 119,696 edges that survive this commit, 1,054
do. The 2,436 that go are 2,104 CALLS and 332 USAGE, measured against this
commit's parent, and none is added back.
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 the base, 5df8b04, 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 that base, 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,061 -> 5,840 (-5,221) CALLS sourced from a Function node 42,659 -> 49,579 (+6,920) CALLS sourced from a File node 2,131 -> 1,269 (-862) 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. Against this commit's own parent, 5,825 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 at this commit is 12,098 against 11,220 on the base; the typespec commit at the end of this stack brings it to 11,207. 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. A typespec head a widened span covers was also being read as a self-call, which is the other half of the cost the multi-clause span commit disclosed. Same 970-file tree, same method it used (each flagged node's own source span, counting a one-line clause's body after `, do:` as a line that names the function): self_recursive Function nodes 430 -> 389 ...with no self-call form in span 48 -> 8 The base is 129 and 6. All 8 that remain are the phantom a local variable sharing the function's name mints (`defp slug(value) do slug = ...`), which nothing in this stack touches: 6 are on the base as well, and the other 2 are guarded clauses that only have a node of their own to be flagged on because of the earlier commits here. `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>
eafaab1 to
4d21c3b
Compare
|
Rebased onto Every figure in the description reproduces exactly on the new base, as does the per-commit Re-taking them did turn up six figures inside the commit messages that were stale, all in the three commits that still measured against
The One more sentence in the scope commit was wrong rather than stale. It said corpus WRITES returns to No reference to Suites on the rebased branch: 144 of 144, 8,216 passed, 0 failed, 10 skipped, 15 more than before the rebase from main's new tests. Separately, |
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Thank you, @BobbieBarker. Checking these numbers again was more than we asked for, and it paid off: the corrected detector, the per-commit table, and flagging the 1,342 peak for anyone who bisects are exactly the kind of care that keeps review honest. We diffed the force-push against the version we reviewed and confirmed it changes comments only. The new comment above elixir_call_is_definition_role now describes what the code does: one unwrap helper, with "is the head" and "inside the head" as separate questions. Both items are done, so we're approving. As agreed, this lands first and we'll rebase our #1731 carry onto it. Thanks again!
DeusData
added
bug
|
Thanks for the careful rebase and corrected test counts. The existing approval and the agreed ordering ahead of #1731 are noted. The remaining verification and landing step is on the maintainer side; I am not requesting another change in this sweep. |
Brings the branch up to date with main (171f99b). One conflict, in extract_elixir_call (internal/cbm/extract_defs.c): main added a walk into compile-time if/unless branches (42a9320, one identity per conditional definition) at the same place this branch passes the fold scope to extract_elixir_func_def. Resolved by keeping both: the def call carries &def_scope and the if/unless branch walk is kept unchanged. The two compose because fold_elixir_clause folds a clause only into one written in the same block, and each branch is its own block, so a branch folds its own clauses and the branches stay separate definitions that the graph buffer merges as variants of one identity. A comment at the branch walk records that. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
A multi-clause function defined in both branches of a module-level if must fold each branch's clauses into one definition and keep the two branches as variants of one identity: one pick node, CALLS to both branch bodies, and exactly the variant spans 5-6 and 8-9. Without the fold the node carries one span per clause (4); without the if/unless branch walk from main there is no pick node at all. Covers the combination the main merge brought together. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Brings the branch up to date with main (5a4af9c, the Go method receiver QN stack DeusData#1907/DeusData#1913). No conflicts; the Go changes and this branch's Elixir changes touch the same files in disjoint, language-gated regions. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you, @BobbieBarker, and thank you for your patience while this waited on our side after the approval. Main had moved underneath it, so we brought the branch up to date ourselves, on top of your commits. Nothing of yours was rewritten. Here is exactly what we did and why:
How we checked it, on phoenixframework/phoenix and livebook-dev/livebook, comparing main with the merged branch:
Nothing is needed from you. Thanks again for splitting this so cleanly, and for the careful numbers throughout. |
|
Thank you again for the Elixir clause and typespec fixes. The October 20 update records the branch integration and conditional-variant coverage as completed; the agreed ordering ahead of #1731 remains. Sorry for the delay in our remaining follow-up—I’ll follow up soon. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you, @BobbieBarker, and thank you again for your patience. Main has moved since our last update, so we merged current main into the branch once more (one signed-off merge commit, c822dce, on top of your commits). Nothing of yours was rewritten. The merge was clean: the main changes that touch extract_defs.c, helpers.c and test_extraction.c (C# 14 extension members and Haskell infix definition names) sit in other languages' code paths and do not reach the Elixir clause fold, the guard unwrap or the typespec skip. The earlier CI failures were not caused by this branch: every test leg stopped at the published language-count check on documentation files this PR does not touch, and that check was repaired on main after the last run. Nothing is needed from you. Once CI is green on the updated branch we will land it, ahead of #1731 as agreed. If you push again, please pull first so the merge commit is kept. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you again, @BobbieBarker, for the Elixir clause and typespec fixes. Tonight's merges on main touched tests/test_extraction.c (a new Elixir channels test was registered right next to yours), so we merged current main into the branch once more (signed-off merge commit 3096a18). We kept both sides: your two guarded-def-head tests and main's new test are all registered, and your changes are untouched. Nothing is needed from you. If you push to this branch again, please pull first so the merge commit is kept. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for your patience, @BobbieBarker. CI went green on this branch, but tonight's merges on main touched tests/test_extraction.c, which this pull request also changes, so that green no longer covered today's main. We merged current main into the branch once more (one merge commit, c4bb4b5; your changes are untouched) and will merge once the fresh checks are green. Please pull before pushing again. |
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.
Part of #2312. Split out of #2310 at your request.
This is the extraction half: three of the five defects in #2312, and the three that need no change to
qualified_name. It carries no index-format bump and no identity change, so an existing store keeps resolving. #2312 stays open until the identity work follows in its own PR.5 commits, 7 files, +1206/-11. Source is +428, tests are +778 across 11 new cases. It stands alone on current
main: zero file overlap with #2370, and it deletes only 11 lines that exist onmain.What it fixes
A guarded def never reaches the graph.
def f(x) when gparses its whole head as onewhenbinary_operator, soextract_elixir_func_deffinds nocalloridentifierwhere it looks and returns. A function whose clauses all carry guards gets no node at all, andsearch_graphanswers "No nodes match", which reads identically to dead code.A multi-clause function returns one clause. The node is anchored at the last clause, so
get_code_snippetreturns that clause as the whole function. A two-clauseadmin?/1comes back asdef admin?(%__MODULE__{}), do: false, so the graph asserts a predicate that always returns false.A typespec is walked as code.
@spec foo(t) :: tputsfoo(t)in the samecallnode an invocation occupies, so the unified walk sources a reference from the enclosing module. On a codebase with@specon every public function,fan_innever reaches 0 and "which exported functions does nothing call" cannot be answered.Why these five commits are one PR
Two of them exist to close regressions the others open, and both pairs are inside this set.
fix(elixir): extract a def head wrapped in a when guardgives a guarded clause a node, and that node is where two pre-existing phantoms land that previously had nowhere to go.fix(elixir): open a function scope for a guarded clause bodycloses them.fix(elixir): span a multi-clause function's node over all of its clauseswidens the span to cover the@specand@docheads above the first clause, which the unified walk then reads as code.fix(elixir): stop walking @spec and @type subtrees as codecloses that, and reports 0 CALLS edges anchored on a typespec-attribute line against 4,565 without it.Splitting either pair ships a state where
mainis measurably worse than before. Both commit messages carry the counts on both sides.Re-measured on current main
Every figure in these commit messages was taken against
e783f73d, which is now 17 commits behind. I re-took them against64c23fabon the same 970-file Elixir tree, darwin arm64. Two corrections, and one result the commit messages undersold.Pristine CALLS is
55,851, not the55,812the commits cite. Andfix(elixir): open a function scope for a guarded clause bodyno longer returns WRITES exactly to pristine: it lands at11,207, thirteen below. That net is the residue of a much larger movement,895edges withdrawn and882added, which makes it a reattribution. The corrected sentence is above.The undersold result is which node an edge is sourced from, which matters more here than the edge total. On main, a quarter of Elixir reference edges are attributed to a Module or a File rather than to the function that performs them, because a guarded clause body opened no function scope:
Module-sourced CALLS fall from
11,061to541, and File-sourced from2,131to169. So the CALLS total falling by5,621and Function-sourced CALLS rising by6,861are the same change seen twice: fewer edges overall, far more of them answering "which function does this". For 13,192 call sites the graph could name the file or the module but not the caller.The corpus indexes deterministically: indexing pristine main twice gives identical counts on every metric above, so these deltas carry no noise.
Blast radius
Nine of the eleven deleted lines are on the Elixir path. The other two are in
cbm_extract_unified, the walk every grammar runs, and the replacement is gated:is_elixir_typespec_attributeopens withif (ctx->language != CBM_LANG_ELIXIR || ...) return false;. No other grammar changes behaviour, and because the gate sits in the predicate's first line there is no call-site check to miss.Verification
Full default suite, plain build, darwin arm64:
11 new cases, each named in the commit that adds it:
Two things this host could not run, so CI is the authority:
ASan aborts at
sanitizer_malloc_mac.inc:189on this macOS box for every suite including ones this change does not touch, so the run above is a plain build. Thearenasuite aborts identically on unmodified code, which is how I established it is the host.cppcheckreports two warnings,src/mcp/mcp.c:870andsrc/daemon/application.c:403. Both reproduce on pristinemainat64c23fabwith the same two lines, and neither file is in this diff.One step of the full run also fails on this host for an unrelated reason: Step 5e refuses to start because ten copies of the installed
codebase-memory-mcpare live and the guard compares build hashes across running processes.A note on the #1731 overlap you flagged
#1731 lands complexity, fingerprint and line count on Elixir functions, in the same region of
extract_defs.cthis PR edits, and GitHub currently reports itCONFLICTINGagainstmain. The interaction is not textual. It computes those values from a folded function's first clause, and this PR changes what a folded function's span is.pass_complexity.cthen propagatesloop_depthinterprocedurally along CALLS edges, so a clause-1-only value does not stay on its node. Whichever of us lands second should recompute against the other's span rather than rebase the hunks. I am happy to take that side if you would rather merge #1731 first.