Skip to content

fix(elixir): populate first_string_arg for Elixir calls - #1721

Open
henry-hz wants to merge 1 commit into
DeusData:mainfrom
henry-hz:upstream-pr/elixir-call-arguments
Open

fix(elixir): populate first_string_arg for Elixir calls#1721
henry-hz wants to merge 1 commit into
DeusData:mainfrom
henry-hz:upstream-pr/elixir-call-arguments

Conversation

@henry-hz

Copy link
Copy Markdown

The bug

tree-sitter-elixir attaches no field name to a call's arguments node. Its entire field set is:

key, left, operand, operator, quoted_start, quoted_end, right, target, value

There is no arguments field. But handle_calls() in internal/cbm/extract_calls.c reads exactly that:

TSNode args = ts_node_child_by_field_name(node, TS_FIELD("arguments"));

For Elixir this is always null, so call.first_string_arg was never populated for any Elixir call — not once, in any file. Nothing errored; the field was simply always NULL.

Why it matters

Everything downstream that keys off a call's string argument was silently dead for Elixir:

  • Phoenix route pathsget "/wallets", WalletController, :index never yielded a path
  • HTTP / async service URLs — the CBM_SVC_HTTP / CBM_SVC_ASYNC classification needs the URL or topic literal
  • Config keysApplication.get_env("...") is matched as CBM_SVC_CONFIG, which reads the same field

The fix

The definition side has always known about this. extract_defs.c carries elixir_call_args() with a positional second-child fallback for precisely this reason:

static TSNode elixir_call_args(TSNode node) {
    TSNode args = ts_node_child_by_field_name(node, TS_FIELD("arguments"));
    if (ts_node_is_null(args) && ts_node_child_count(node) > SECOND_CHILD_IDX) {
        args = ts_node_child(node, SECOND_CHILD_IDX);
    }
    return args;
}

This adds the same fallback on the call side. It is gated to call nodes, because Elixir's other call kinds — dot, and the |> binary_operator — carry no arguments node in that position and would otherwise pick up the operator token.

Test

elixir_call_string_argument pins first_string_arg directly rather than asserting a downstream effect, so it stays meaningful independently of route or service classification.

Verification

  • make -f Makefile.cbm test-focused TEST_SUITES="extraction registry lang_contract grammar_regression grammar_labels repro_language_registry repro_call_node_manifest" → 388 passed, 0 failed
  • The CALLS-breadth contract across 53 languages is unaffected (0 failures); the change is gated to CBM_LANG_ELIXIR
  • clang-format --dry-run --Werror clean on the changed source. tests/ was deliberately left unformatted, per the note that it is not in LINT_SRCS
  • Exercised end-to-end against elixir-plug/plug and an 829-file Phoenix application

Scope

Submitted as a bug fix under the CONTRIBUTING exception for focused bug fixes and test additions, so there is no prior issue. Additions only — 54 lines, no deletions, no reformatting.

This is the first of four independent Elixir fixes found while auditing Elixir extraction against a real Phoenix codebase. The other three are prepared and gated separately, and I will open them one at a time rather than as a bundle:

  1. this PRfirst_string_arg never populated
  2. Phoenix channel extraction is unreachable in both branches (arguments field again, plus a dispatch on a def node type the grammar does not have)
  3. Phoenix router macros mint no Route nodes (every route_reg_suffixes entry is prefixed . or ::, but the macros are bare) — depends on this PR for the path argument
  4. Elixir functions carry no complexity, fingerprint or line count

Happy to reorder, split further, or drop any of them if you would rather discuss the approach in an issue first.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DpDDX9sFC16mQ9U9wQ3WNd

tree-sitter-elixir attaches no field name to a call's arguments node. Its whole
field set is key, left, operand, operator, quoted_start, quoted_end, right,
target and value, so ts_node_child_by_field_name(node, "arguments") is always
null for Elixir and call.first_string_arg was never populated for any Elixir
call.

Every downstream signal that keys off a call's string argument was therefore
dead for Elixir: Phoenix route paths, HTTP/async service URLs, and config keys.
Nothing reported an error — the field was simply always NULL.

The definition side already knows this: extract_defs.c carries
elixir_call_args() with a positional second-child fallback for exactly this
reason. This adds the same fallback on the call side, gated to `call` nodes,
since Elixir's other call kinds (`dot`, the `|>` binary_operator) carry no
arguments node in that position.

The new test pins the field directly rather than a downstream effect.

Signed-off-by: Henry Hazan <henry@teramine.io>
@henry-hz
henry-hz requested a review from DeusData as a code owner August 19, 2026 05:45
@github-actions

Copy link
Copy Markdown

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. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

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.

@henry-hz

Copy link
Copy Markdown
Author

test / test-windows-guards is red here, and I do not believe it is this change. Recording the evidence rather than just asserting it.

The build step succeeded; the guard script failed:

SETUP FAIL: permanent daemon did not start for the crash check
RED (tests/windows/test_daemon_stability.py): section_crash_recovery failed
REGRESSION: 1 green guard(s) went red: tests\windows\test_daemon_stability.py

section_crash_recovery never reached an assertion — it timed out at daemon start (60s budget) before the kill/recover sequence began. Four other guards in the same job also failed preconditions in that run:

  • test_hook_augment.py — "SETUP FAIL: index did not run"
  • test_cli_non_ascii_arg.py — "SETUP FAIL: ASCII control did not index via CLI"
  • test_ui_drive_listing.py — "HTTP server did not start on port 53503"

Five independent subprocess/daemon startups failing in one Windows job reads like a sick runner rather than a code defect.

Why it should not be reachable from this diff:

  • tests/windows/test_daemon_stability.py contains no Elixir
  • the change is 54 added lines, all inside if (ctx->language == CBM_LANG_ELIXIR), altering only how a call's arguments node is located during extraction
  • it touches no process startup, IPC, socket, or path code

Everything else on the PR is green: DCO, lint, lint-mem, codeql-gate, security-static, license-gate, all test-unix shards (ubuntu ×3, ubuntu-arm ×3, macos-14, macos-15-intel), all three test-tsan variants, test-lsan-macos, test-package-wrappers, and pr-smoke on all three platforms.

I do not have rerun rights on this repo — could a maintainer re-run that single job? Happy to push an empty commit to re-trigger instead if you would rather not spend the click, and equally happy to dig further if you have seen this guard fail for a real reason before.

@henry-hz

Copy link
Copy Markdown
Author

Context for this change now lives in #1729, which lays out four Elixir extraction defects found while auditing against a real Phoenix application, and how they relate to the #1239 / docs/elixir-lsp/PLAN.md workstream. This PR is defect 1 of 4.

Refs #1729

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant