Skip to content

fix(elixir): Phoenix channel extraction is unreachable in both branches - #1730

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

fix(elixir): Phoenix channel extraction is unreachable in both branches#1730
henry-hz wants to merge 1 commit into
DeusData:mainfrom
henry-hz:upstream-pr/elixir-channels

Conversation

@henry-hz

Copy link
Copy Markdown

Refs #1729 (defect 2 of 4).

The bug

Neither half of extract_channels_elixir() can fire, so an Elixir project produces zero Channel nodes regardless of how much Phoenix.PubSub or Phoenix.Channel it uses.

docs/elixir-lsp/PLAN.md §1.2 currently records this path as "Phoenix channels/PubSub (works today)", which is what prompted me to write it up rather than just patch it.

Emit side

elixir_process_call() reads:

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

tree-sitter-elixir defines no arguments field — its whole field set is key, left, operand, operator, quoted_start, quoted_end, right, target, value. args is always null, so elixir_emit_second_arg() returns early every time.

Listener side

} else if (strcmp(kind, "def") == 0) {
    elixir_process_function_def(ctx, node);
}

There is no def node type in the grammar — an Elixir definition is a call whose target is the macro name. That branch is unreachable, and elixir_process_function_def() compounds it by reading name and parameters fields that also do not exist.

The fix

  • a positional arguments fallback, mirroring elixir_call_args() which extract_defs.c has always used for the same reason
  • dispatch on call + a def-macro target instead of the phantom def node type
  • elixir_process_function_def() now takes the def call, unwraps a when guard, and reads the head's name and parameters positionally — so guarded clauses like def handle_in(e, p, s) when is_map(s) are caught too
  • the callee lookup accepts target or child(0), matching what extract_defs.c does

Test

elixir_channels_pubsub_and_handle_in covers Phoenix.PubSub.broadcast, Phoenix.PubSub.subscribe, a bare push emit, and two handle_in listener clauses, one guarded.

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
  • CALLS-breadth contract across 53 languages unaffected; every change is inside the Elixir path
  • clang-format --dry-run --Werror clean on extract_channels.c. tests/ left unformatted, per its exclusion from LINT_SRCS

Independent of #1721 — these can land in either order.

Submitted as a bug fix under the CONTRIBUTING exception for focused bug fixes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DpDDX9sFC16mQ9U9wQ3WNd

Neither half of extract_channels_elixir could ever fire, so an Elixir project
produced zero Channel nodes regardless of how much Phoenix.PubSub or
Phoenix.Channel it used.

The emit side read ts_node_child_by_field_name(call, "arguments").
tree-sitter-elixir defines no such field — its whole field set is key, left,
operand, operator, quoted_start, quoted_end, right, target and value — so args
was always null and elixir_emit_second_arg() returned early every time.
extract_defs.c has always used a positional second-child fallback for this;
the same fallback is now used here.

The listener side dispatched on strcmp(kind, "def"). There is no `def` node
type in the grammar: an Elixir definition is a `call` whose target is the macro
name. elixir_process_function_def() was therefore dead code, and it also read
`name` and `parameters` fields that do not exist. It now takes the def call,
unwraps a `when` guard, and reads the head's name and parameters positionally,
so guarded clauses like `def handle_in(e, p, s) when is_map(s)` are recognized
too.

Verified against a Phoenix application; the new test covers
Phoenix.PubSub.broadcast/subscribe, a bare push/broadcast emit, and two
handle_in clauses, one of them guarded.

Signed-off-by: Henry Hazan <henry@teramine.io>
@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.

@DeusData

Copy link
Copy Markdown
Owner

Confirmed dead, and the evidence is even cleaner than your report. You wrote that tree-sitter-elixir never produces a def node, so the else if (strcmp(kind, "def") == 0) dispatch at extract_channels.c:969 is unreachable. Our own language spec says the same thing outright — lang_specs.c:499 declares elixir_func_types[] = {"call", "anonymous_function", NULL}. So the file that defines what an Elixir function definition is has always said call, while the channel walker has been checking for def. elixir_process_function_def has never executed once, and every Phoenix handle_in listener has been invisible to the graph for as long as the code has existed.

That is a good find, and it is the kind that hides indefinitely because the dead branch looks like working code in review.

The test is binding on all five assertions. The emit side fails on main because args comes back null; the listener side fails because it is unreachable. And "ping" specifically exercises the when-guard unwrap — def handle_in("ping", _p, socket) when is_map(socket) — which is exactly the case that would have been quietly skipped by a naive first-child read. Covering the guarded clause rather than only the plain one is the difference between a fix and a fix that holds.

Scope is clean: everything is inside extract_channels_elixir() or new elixir_* statics, and the only caller chain is extract_channels.c:1080extract_channels_elixirelixir_process_function_def. Nothing else can reach it, so no other language is affected.

One thing I want you to confirm rather than assume. This is not purely "make the dead branch live" — you restructured the dispatch so a call whose target is def/defp/defmacro/defmacrop routes to elixir_process_function_def instead of elixir_process_call, as an else-branch rather than an addition. So a definition-macro call can no longer also be examined as a PubSub emit.

I worked through it and I believe it is safe: the loop still pushes every child onto the stack, so broadcasts inside a def body are still visited, and a def node's own target could never match a broadcast pattern anyway. But it is a behaviour change beyond the stated fix, it is not called out in the description, and you know this code better than I do — so please confirm that reading, and it would be worth a line in the commit message either way.

Your CI here is not a real failure. The run shows test skipped plus two cancelled jobs plus a red aggregate, which is the signature of an incomplete run rather than a test result. I have re-run it; we will see a real signal shortly.

One heads-up spanning your three PRs. The production files are genuinely disjoint, so nothing here can break #1721 or #1731. But all three insert into tests/test_extraction.c at the same two anchors — right after TEST(elixir_function) closes, and right after RUN_TEST(elixir_function); in the suite. GitHub reports all three MERGEABLE because it compares each against main alone; I expect the first merge to flip the other two to CONFLICTING at both points. Trivial to rebase, just not a surprise worth having.

Also note this adds a third near-identical positional-args helper (elixir_channel_call_args, alongside elixir_call_args in extract_defs.c and #1721's elixir_call_arguments_fallback). I have asked on #1721 whether you would rather hoist one shared helper now or consolidate afterwards — same question, no strong preference from me.

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.

2 participants