fix(extraction): reduce A::B.new callee to the constant - #1710
fix(extraction): reduce A::B.new callee to the constant#1710benrfairless wants to merge 6 commits into
Conversation
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>
|
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. |
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>
db9656a to
944b229
Compare
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>
|
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 And the correction deserves saying out loud. You went back and contradicted your own #1706 description — the pre-fix callee is the whole The thing that does not belong
- 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: 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
Cleared and approved on the Ruby change. Split the daemon hunk out and this merges. |
What does this PR do?
Slice 2 of 5 splitting #1706. Stands alone; depends on nothing else in the series.
A Ruby
Const.newcall is rewritten to name the constant rather than the method, so the call links the class the way every other language'snew T()does — the constructor body lives ininitialize, so namingnewlinks nothing.That rewrite only fired for bare
constantreceivers. 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 fullAdmin::User.newexpression. 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:
The test pins both spellings — scope-resolved reduces to
Admin::User, bare staysWidgetas a regression guard — and rejects any callee that isnewor retains a.newsuffix. There was previously no test anywhere for this rewrite in either form.Where this sits in the series
feat/ruby-lsp-per-filefeat/ruby-lsp-cross-filedocs/ruby-hybrid-lspCI baseline
mainat 4d7d9f1 is red for reasons unrelated to this change: #1181 gavelist_projectspagination 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-okwill be red here for that reason.Refs #1701
Checklist
git commit -s)scripts/test.sh) — see fix(extraction): keep Ruby superclass names bare #1709 for the full-leg numbers; all failures are pre-existing on unmodifiedorigin/main.make -f Makefile.cbm lint-ci)Assisted by OpenCode using anthropic/claude-opus-5.