Repository navigation
fix(elixir): extract a def head wrapped in a when guard - #2307
Closed
BobbieBarker wants to merge 2 commits into
Closed
BobbieBarker wants to merge 2 commits into
BobbieBarker wants to merge 2 commits into
Conversation
…elper `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, and a disagreement between two of these walks does not fail, 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, not one, 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, and it 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" rather than "the head under the first one". tests/test_extraction.c: 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> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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 -- silently, because a missing definition is not an error anywhere in the pipeline. Guards are ordinary Elixir, not a corner. 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,056 (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,056 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 is the shared helper, not a local peel, because 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, and both belong in this message rather than in a reviewer's lap. 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 -- i.e. the graph asserting that a module mutates a function. Corpus WRITES 11,220 -> 11,302 (+82). Both are closed by the guarded call-scope commit later in this stack, and 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. Both are stated here because this commit is what makes them observable, and 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 #N (the shared def-head helper). Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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. |
Contributor
Author
|
Closing in favour of #2310, which carries this commit plus the rest of the set. I split this out first, then realised the pieces cannot land independently: this change creates nodes for guarded functions, which makes a pre-existing phantom self-edge in the calls walk reachable — the regression documented in this PR's own body. It is fixed by a later commit in the set. Merging this alone would ship that regression. Same commit, same tests, unchanged, now in #2310 with the commits it depends on. Sorry for the noise. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
def name(args) when guarddoes not parse as a call with a guard beside it. tree-sitter-elixir builds onewhenbinary_operatorfor the whole head, and thename(args)call that names the function is that operator's left operand.extract_elixir_func_defreads the def's first argument directly and accepts only acallor anidentifierthere. With a guard it sees abinary_operator, finds no name, and returns — so the clause is never emitted. A function whose clauses all carry guards never reaches the graph at all, silently, because a missing definition is not an error anywhere in the pipeline.Guards are ordinary Elixir, so this is not an edge case.
defguardis always guarded.Why it is hard to notice
search_graphanswers a miss with "No nodes match; check spelling or broaden the regex." So an agent asking about a guarded function is told the symbol does not exist, and an agent asking who calls it gets an empty answer that reads exactly like dead code.The fix
Two commits:
refactor(elixir)adds one shared helper that reads a def head through its guard. Three call sites need the same parse, and two of them had already drifted — only the def side unwrapped the guard — so the helper exists to keep them from disagreeing. They must agree byte-for-byte: 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 a guarded function.fix(elixir)uses it inextract_elixir_func_def. This mirrorsresolve_r_func_name(), which already unwraps the samebinary_operatorshape for R.A regression this creates, stated up front
The calls walk decides a def head is a declaration rather than an invocation by comparing the visited node against the def's first argument. A guard makes that argument the
whenoperator, so the innername(args)no longer compares equal and is recorded as a call to the function being defined — a phantom self-edge, and a falseself_recursiveflag on the new node.That is a pre-existing hole in the calls walk that this commit makes reachable by creating the node it lands on. It is fixed by pointing the calls walk at the same shared helper; I have that change and will open it as a follow-up rather than bundle two concerns here. Both are named in the commit body so the intermediate state is on record.
Verification
tests/test_extraction.c: extract_elixir_guarded_def_headcovers plain, single-guard, multi-clause, compoundandguard,defp, and a paren-lessdef bare when true. It fails without the fix at theguardedassertion and passes with it.Reported and fixed against
main.