Skip to content

fix(extraction): reduce A::B.new callee to the constant - #1710

Open
benrfairless wants to merge 10 commits into
DeusData:mainfrom
benrfairless:fix/ruby-namespaced-constructor-callee
Open

benrfairless wants to merge 10 commits into
DeusData:mainfrom
benrfairless:fix/ruby-namespaced-constructor-callee

Conversation

@benrfairless

Copy link
Copy Markdown
Contributor

What does this PR do?

Slice 2 of 5 splitting #1706. Stands alone; depends on nothing else in the series.

A Ruby Const.new call is rewritten to name the constant rather than the method, so the call links the class the way every other language's new T() does — the constructor body lives in initialize, so naming new links nothing.

That rewrite only fired for bare constant receivers. A scope-resolved receiver fell through it and kept the whole call expression as its callee: Admin::User.new. That resolves to nothing, and it cannot join the Ruby LSP's constructor rows, whose callee is the class QN.

Correction to the original description. While writing the reproduce-first test I found the claim I made in #1706 — that the callee stayed "new" — is wrong. The actual pre-fix callee is the full Admin::User.new expression. The commit message and test here state the real behaviour; the fix itself is unchanged and still needed.

Reproduce-first. Without the change, the new row prints exactly that:

ruby callees (2):
  'Admin::User.new'
  'Widget'

The test pins both spellings — scope-resolved reduces to Admin::User, bare stays Widget as a regression guard — and rejects any callee that is new or retains a .new suffix. There was previously no test anywhere for this rewrite in either form.

Where this sits in the series

# PR Depends on
1 #1709 —
2 this PR —
3 feat/ruby-lsp-per-file —
4 feat/ruby-lsp-cross-file 3
5 docs/ruby-hybrid-lsp 4

CI baseline

main at 4d7d9f1 is red for reasons unrelated to this change: #1181 gave list_projects pagination parameters, retiring the last empty-properties schema that #1359's guards used as their zero-argument example. #1704 already fixes it. Until that merges, ci-ok will be red here for that reason.

Refs #1701

Checklist

  • Every commit is signed off (git commit -s)
  • Tests pass locally (scripts/test.sh) — see fix(extraction): keep Ruby superclass names bare #1709 for the full-leg numbers; all failures are pre-existing on unmodified origin/main.
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test (reproduce-first for bug fixes)

Assisted by OpenCode using anthropic/claude-opus-5.

A Ruby `Const.new` call is rewritten to name the constant rather than
the method, so the call links the class the way every other language's
`new T()` does — the constructor body lives in `initialize`, so naming
`new` links nothing.

That rewrite only fired for bare `constant` receivers. A scope-resolved
receiver fell through it and kept the whole call expression as its
callee: `Admin::User.new`. That resolves to nothing, and it cannot join
the Ruby LSP's constructor rows, whose callee is the class QN.

Treat `scope_resolution` receivers the same way. The new extraction row
pins both spellings and fails without this change, printing the
'Admin::User.new' callee that is the bug's signature.

Refs DeusData#1701

Assisted-by: OpenCode:anthropic/claude-opus-5
Signed-off-by: Ben Fairless <ben@oaf.org.au>
@benrfairless
benrfairless requested a review from DeusData as a code owner August 18, 2026 09:02
@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.

The previous run hit two transient infrastructure failures:
- security/codeql-gate: gate poller timed out by ~4 seconds; CodeQL
  analysis itself completed successfully
- test/test-unix (macos-15-intel): tests passed but the post-run
  artifact upload hit ENOTFOUND (network unavailability on the runner)

No code changes.

Assisted-by: Claude Code:claude-sonnet-4-6
Signed-off-by: Ben Fairless <ben@oaf.org.au>
@benrfairless
benrfairless force-pushed the fix/ruby-namespaced-constructor-callee branch from db9656a to 944b229 Compare August 19, 2026 09:11
daemon_runtime_connection_cap_covers_slow_hello_and_stopping_is_terminal
failed on macos CI. The test's capacity_rejected condition sampled
active_connections immediately after the overflow connect returned. On a
loaded runner the server's HELLO timeout (5 s) can fire for the
slow_hello raw connection during the overflow client_connect call (up to
2 s), dropping the count from 2 to 1 and falsifying the == 2 assertion.

The capacity rejection message already proves the reason. Drop the
point-in-time active_connections sample; keep active_clients == 1,
which is stable because we still hold the accepted client open.

Same class of race fixed in db91b88 for the forged-identity test.

Assisted-by: Claude Code:claude-sonnet-4-6
Signed-off-by: Ben Fairless <ben@oaf.org.au>
@DeusData

DeusData commented Sep 1, 2026

Copy link
Copy Markdown
Owner

The Ruby fix is right. There is one thing in the diff that does not belong to it.

The fix itself: two lines, gated on CBM_LANG_RUBY, adding scope_resolution beside constant — the same shape as #1709, and the comment explains what resolution actually joins on (the bare leaf) rather than just what changed. The test pinning both spellings, with the bare Widget case kept as a regression guard and an explicit rejection of any callee that is new or retains a .new suffix, is the right shape. There was no test for this rewrite in either form before, so this is a coverage gain as well as a fix.

And the correction deserves saying out loud. You went back and contradicted your own #1706 description — the pre-fix callee is the whole Admin::User.new expression, not "new" — because the reproduce-first test told you so. Most people quietly ship the corrected test under the old story.

The thing that does not belong

tests/test_daemon_runtime.c removes an assertion:

-    cbm_daemon_runtime_service_active_connections(fixture.service) == 2 &&

That is not mentioned anywhere in the PR description, and it has nothing to do with Ruby constructor callees. I also checked the other four slices — none of them touches that file — so it is specific to this one rather than something shared across the series.

I am not disputing the change on its merits. I checked the precedent you cite: db91b88c is real, it is "fix(test): stop asserting a scheduling race in the forged-identity test", and it is the same fault class. Asserting active_connections == 2 in the window between a raw connect and the server's HELLO timeout is asserting a transient state, which is exactly what we do not do here. Your reasoning looks sound and your replacement argument — that the "capacity" substring proves the reason and active_clients == 1 is safe because the test still holds accepted — is the right one to make.

But it needs to be its own PR. A change that removes test coverage gets read on its own terms, by someone thinking about that test rather than about Ruby extraction. Bundled into a language fix it is very easy to wave through, which is precisely the risk. Please lift it out — the commit message you would write for it is already most of the way there in that comment.

I suspect this is a stray from local iteration rather than deliberate bundling, given how carefully the rest of this series is scoped. Worth saying, though, because your own note when closing #1706 was that bundled changes are "exactly the kind of thing an umbrella PR hides" — and this one hid in a slice.

Status

ci-ok should no longer be red for the reason in your baseline note — the #1181 × #1359 cli failures are fixed on main; I confirmed both tests pass in a full-suite run today.

Cleared and approved on the Ruby change. Split the daemon hunk out and this merges.

@DeusData DeusData added bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Sep 3, 2026
Restore tests/test_daemon_runtime.c to the PR's base, as asked in review:
the dropped active_connections assertion is unrelated to the Ruby
constructor callee and belongs in its own change. main has since reworked
this test on its own (flaky-ledger item 12), so the merge of main that
follows takes main's version.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

Copy link
Copy Markdown
Owner

Hi @benrfairless, thank you for this fix, and sorry it sat so long after the review.

The change itself is exactly right: two lines gated on Ruby, putting scope_resolution beside constant, so Admin::User.new names Admin::User the way Widget.new already names Widget. The reproduce-first test pinning both spellings, and your correction of the #1706 description once the test showed the real pre-fix callee, made this easy to trust.

Since the branch allows maintainer edits, we finished the last step ourselves instead of asking you for another round:

  1. We removed the tests/test_daemon_runtime.c change from this PR, as asked in the review on Sep 1, by restoring that file to the branch base in a separate commit. Your reasoning about the racy active_connections read was sound, and main has since reworked that test on its own, so nothing is lost.
  2. We merged current main. The branch was far behind and conflicted; the merge itself was clean, and your change lands unchanged at its new place in extract_calls.c.

We checked it end to end: without the fix the new test fails and prints 'Admin::User.new', and with it the test passes. On a real Ruby codebase (Homebrew's own Library, about 2,000 files) the fix adds about 500 constructor CALLS edges that previously resolved to nothing. A small share of those land on a same-named class in a different namespace, because our resolver falls back to the last name segment. That part is ours to tighten, not something this PR needs to change.

Your commits are untouched. We will take it from here once CI is green. Thank you again!

@benrfairless

Copy link
Copy Markdown
Contributor Author

Thanks @DeusData! It looks like the CI is all green now. Fingers cross we are good to merge soon

Refresh the contributor branch onto current main while preserving the
reviewed constructor extraction change and regression coverage.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

Copy link
Copy Markdown
Owner

Thank you for fixing namespaced Ruby constructor extraction! I merged current main into this branch in 124ce0e130d7a4e9680ce8dcfc60aca2e0a6ea40 because main has changed both touched files since the previous CI run. The merge was conflict-free and preserves the Ruby fix and its regression test.

The integrated diff passed review; fresh required GitHub CI will validate the updated branch before merge. Thanks again for the contribution!

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

Copy link
Copy Markdown
Owner

Thanks again, @benrfairless, for the Ruby constructor fix! I merged the latest main into this branch in b40ed9c because another merged change touched the shared extraction tests. The Ruby fix is unchanged (the patch ID matches); this refresh gives it a fresh required CI run on the current base.

This branch has not been deployed

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

Labels

bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants