Repository navigation
fix(java): resolve record accessor calls, and never bind a call to data - #2295
Conversation
A record's accessors are implicit, so `record Point(double x, double y, double z)` declares no `x()` and extraction emitted no node for one. The call `p.x()` had nothing to resolve to, fell through to the short-name registry, and bound to the first `x` in the project — typically an unrelated class's private field. Measured on two indexed Java trees, that was 278 of 2444 and 2649 of 29509 CALLS edges. Extraction now emits one Method definition per record component, skipping any accessor the record declares itself (JLS 8.10.3), so the accessor has a node and the call resolves to it. And a Java call may no longer bind to a Variable or a Field. A Java call expression never names data, so such an edge is a spelling collision, not a call. The guard follows cbm_go_suppress_bare_field_ref — a pure, label-keyed predicate — and is called from both pass_calls.c and pass_parallel.c so the sequential and parallel resolvers stay identical. Java-gated: Kotlin properties hold function values and C has function pointers, so both legitimately call through data. Signed-off-by: MopicMP <obshiq123@gmail.com> Co-authored-by: Claude Opus 5 <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. |
|
Written by an AI agent (Claude Code) working for @MopicMP. The only red here is Of the last 61 PR-workflow runs that ran this guard, it was green 56 times and red 5. All five reds are that same section, and four of them are on branches that cannot reach it:
Everything else is green: all nine Could a maintainer re-run that job? I have no permission to re-run it from here. Happy to rebase instead if you would rather see a fresh run of the whole leg. |
|
Thanks for the careful flake analysis, @MopicMP, and sorry for the wait. You're right that |
…ex, not one poll watcher_poll_only_watched_projects failed on the macOS LSan job of PR DeusData#2295, whose diff was Java-only, with `FAIL tests/test_watcher.c:2971: index_call_count == 0, expected 1` (8211 passed, 1 failed). The LSan report that followed points into the aborted test body (watcher and store never freed after the early FAIL): a consequence, not a separate leak. The assert expected ONE touch+poll after a dirty change to reindex exactly once. That needs every git probe of the poll to succeed. A poll whose git spawn is refused under runner load is a fail-closed no-op by design (DeusData#937: the baseline stays uncommitted and the next poll retries), and that path returns without a log line. This is the same test race that DeusData#2376 fixed in watcher_unwatch_drains_pending_free and watcher_sustained_failure_logs_once_issue2015, not a production defect. Holding one exhausted spawn with cbm_subprocess_force_spawn_eagain_for_testing at that poll reproduces the CI output exactly: watch, baseline, then `FAIL tests/test_watcher.c:2971: index_call_count == 0, expected 1 == 1`, with no watcher.changed and no watcher.git.failed. The old test was also partly inert. The Go original (TestPollAllOnlyWatched, from the DeusData#49 fix that replaced scan-all with an explicit watch list) registered BOTH projects in the store and asserted that the unwatched one gets no watcher state. The C port dropped the store registration, so nothing let the watcher learn about B, and the B half could not fail. The test now: - registers A and B in the store, and watches only A; - asserts that the baseline poll indexes nothing and does not adopt B; - makes both worktrees dirty (a dirty change, not a commit, so a late baseline still sees it); - polls until A's reindex has happened, asserting after EVERY poll that B has no watcher state (cbm_watcher_index_failure_count == -1), was never reindexed, and that the watch count is still 1; - asserts that A was reindexed exactly once. No timer or deadline decides the verdict. A watcher that never reindexes A hangs, which the runner reports as a failure, never a pass. Evidence (macOS arm64, ASan/UBSan test-runner, CBM_SANITIZED, so one exhausted spawn = 10 injected refusals), with temporary injection that has since been removed: - CI-shaped (baseline OK, first change poll refused): consumed=10 at loop poll 1, watcher.changed at loop poll 2, PASS. - Heavy (baseline poll plus loop polls 1-3 refused): loop poll 4 takes the baseline, poll 5 reindexes A; loop polls=5 watched=1 other=0, PASS. - Mutation, the pre-DeusData#49 scan-all (poll_once adopts every stored project): `FAIL tests/test_watcher.c:3001: cbm_watcher_index_failure_count(w, "projB-ow") == 0, expected -1 == -1`, both unforced and with the heavy refusals (A's and B's baseline spawns both refused). The other 82 watcher tests stay green under that mutation, so this is the only test guarding DeusData#49. Final tree: build/c/test-runner watcher, 5/5 runs, 83 passed each. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thanks again for the record-accessor fix. The last maintainer update records passing local tests and says no rebase is needed. The remaining landing decision is still on the maintainer side; no new changes are requested in this sweep. |
|
Written by an AI agent (Claude Code) working for @MopicMP. Thanks for the update. Two things I noticed while checking where this stands:
I can resolve the conflicts and push to the branch, with the full suite rerun first, or leave it to you as with the last merge — whichever is easier on your side. |
|
Thanks for checking this and offering to handle the update. Maintainer edits are enabled on this PR, so I will handle the scoped conflict resolution and validation directly on this branch; no additional changes are needed from you for that update. I will follow up with what changed and the test results. Thank you again for the contribution. |
Preserve the current C/C++ field-binding policy while integrating the Java record accessor change. Handle varargs record components with array return types and reject data targets before sequential LSP call emission, matching the parallel path. Add focused regressions for both cases. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for the Java record accessor fix and the regression coverage! I added maintainer commit I also merged current main and resolved the conflicts while preserving its C/C++ callable-field behavior. The new regressions failed before the fixes and again with the production changes reverted; all 694 affected tests passed on the integrated tree. Required GitHub CI is now the remaining merge gate. Thanks again for getting this improvement started! |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thanks again, @MopicMP, for the Java record-accessor work! I merged the latest main in e2660c4 because another merged change touched the shared extraction code and tests. The merge was conflict-free and the Java fix has an identical patch ID. Fresh required GitHub CI will now check the combined head before merge. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for the Java record accessor work! I added a small follow-up fix so comments inside an otherwise empty accessor parameter list do not create a duplicate implicit accessor. The regression tests cover both block and line comments, retain the explicit method source span, and check that a real-parameter overload still keeps its implicit accessor. The fix passed all 503 extraction and call-reference contract tests on macOS; reverting the production change reproduced the regression with the same tests. I also refreshed the branch with main so it picks up the shared CI repairs. Fresh GitHub checks will verify the combined branch. Thanks again for the contribution! |
|
Thank you again for the Java accessor fix. The October updates record the maintainer integration and the varargs, sequential-path, and comment-parameter regressions as completed. No repeat contributor update is needed. Sorry for the delay in the remaining maintainer follow-up—I’ll follow up soon. |
A record's implicit accessors are members the compiler writes and the source never spells. Their nodes now carry the property generated=record, and every registry registration path files them in the exact (qualified-name) map only, never in the short-name index. The Java type-aware resolver still binds p.y() on a Point to Point.y; a call such as v.y() on a type from outside the project no longer binds the accessor through a unique_name / suffix_match / field_type_hint / fuzzy guess. - extract_defs.c: each implicit accessor carries extra_props generated=record (key CBM_PROP_GENERATED, cbm.h). - registry.c: cbm_registry_add_qn_only, and the pure predicates cbm_def_props_generated / cbm_node_props_generated. - pass_definitions.c, pass_parallel.c, pipeline_incremental.c: the three registration sites route a generated member to the exact map only. - pipeline_delta.c: the incremental preseed proxy keeps the mark, so an incremental re-index registers the member exactly as a full one does (the proxy carried only a few properties and the accessor came back into the short-name index). Tests: pipeline_java_record_accessors_generated_qn_only (sequential, parallel and incremental venues) and two registry unit tests. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The import resolver's symbol-name fallback tries the trailing segments of an import path that names no project symbol, and a package segment that merely spells a record accessor bound the import to it. On elasticsearch, `import javax.crypto.spec.PBEKeySpec` bound a record's spec() accessor, and `new PBEKeySpec(...)`, annotations and type uses then followed the import onto it: 6,302 IMPORTS edges into accessors, plus CALLS, USAGE, DECORATES and THROWS through the import map. A generated member is registered by qualified name only, and no Java import can name an instance accessor, so the fallback now skips it exactly as it skips a Python def-local (DeusData#1548). The record pipeline fixture gains an import whose package segment spells an accessor. Without the change it fails in the sequential, parallel and incremental venues: an IMPORTS edge into Point.y, and make() -> Point.y. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
When a JVM LSP callee has no exact node and no Class.method tail match, cbm_pipeline_lsp_target_node takes the one project callable with that leaf name. A record accessor became such a callable: on elasticsearch `Objects.nonNull(x)` (37 CALLS) and `filter(Objects::nonNull)` (230 CALL_REFERENCE) bound a test record's nonNull() accessor. A generated member is registered by qualified name only. It still joins through its owner's Class.method tail, but the leaf-name fallback no longer considers it, either as the unique callable or as a rival that makes a same-named project callable ambiguous, so every other bind is decided exactly as before the accessors existed. The record pipeline fixture gains a record Seen(boolean nonNull) with `Objects.nonNull(o)` and `filter(Objects::nonNull)` callers. Without the change it fails in the sequential, parallel and incremental venues. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you again, @MopicMP, for this fix and for the care that went into it. You found the real cause (a record's accessors had no node, so p.x() fell through to a project-wide name guess and landed on unrelated fields), fixed both halves, kept the sequential and parallel resolvers identical, and measured the damage on real trees. That made the remaining work here small. Since you opened the PR we settled the design for members the compiler writes but the source never spells (record accessors now, Lombok members later): they are Method nodes marked with the property generated, and they are registered by qualified name only, so only a resolver that knows the owner type can bind them and no name guess can land on them. Your data-call veto lands with it unchanged. We pushed three commits and a merge of main on top of your branch to bring the PR in line with that:
Each commit comes with tests that run one record fixture through the sequential, parallel and incremental indexing paths, plus small unit tests for the registry helpers. Each test failed before its change for the reason it names, and your veto's tests fail when the veto is switched off, so they really bind. On a full index of elasticsearch, Java calls into fields and variables go from 42,900 to 0, 8,363 accessors appear as generated Method nodes, and every one of the 6,033 calls that now reach them comes from the type-aware resolver, none from a name guess. Please pull before pushing again, so these commits are not lost. Thanks again for the contribution, and for your patience with the review queue! |
Windows headers define near and far as empty macros, so the locals named near and far in the generated-accessor pipeline test did not compile on the Windows leg. Rename them to near_m and far_m; the node names stay. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Sorry, @MopicMP, one more small commit from our side. The test we added for the generated accessors named two locals near and far, and the Windows headers define both as empty macros, so the test file did not compile on the Windows leg. 8f36ea6 renames them to near_m and far_m; the node names and every assertion stay the same, and your changes are untouched. Please pull before pushing again. |
Conflict in src/pipeline/pass_calls.c: DeusData#2305 moved the inline LSP-target emission of resolve_single_call into calls_emit_lsp_target() and added the unresolved-call evidence around emit_classified_edge. The Java data-member veto of this branch is ported into that structure: - calls_emit_lsp_target: the veto runs once the target node is known, before the resolution, the evidence and any spawn or edge. It returns true (handled), so as before no edge is emitted and no registry fallback runs after it. - resolve_single_call: the veto stays in front of main's candidate_qn/calls_emitted bookkeeping, matching the parallel resolver, whose veto also precedes that bookkeeping. pass_parallel.c and the remaining files merged cleanly. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thanks again @MopicMP for this PR - the record accessor work is careful and well tested, and it keeps holding up as main moves underneath it. We merged current main into your branch once more (merge commit 2aaef23). Main had just landed #2305, which moved the LSP-target part of the sequential call resolver in src/pipeline/pass_calls.c into a new helper, calls_emit_lsp_target(), and added unresolved-call bookkeeping around the final edge emission. That collided with this PR's Java data-member check in two places. Here is exactly what we did:
Nothing else changed: every other file merged cleanly with your changes intact. The Java, pipeline, registry, call-reference, Kotlin, extraction, parallel and incremental suites all pass locally. call_java_static_import_does_not_call_field_sequential still fails if the check is taken out of the new helper, so your test still guards it. If you push to this branch again, please pull first so this merge commit is kept. Thanks a lot for your patience with all the moving parts here! |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you again, @MopicMP, for your patience with this one. Two pull requests merged into main a few minutes ago (#2754 and #2090), and they touch src/pipeline/pass_parallel.c, pipeline.h and registry.c, which this pull request also changes. So we merged current main into your branch once more (one merge commit, 7ee3124; it merged cleanly and your own change is untouched), checked that the merged files still compile, and started a fresh CI run. We will merge as soon as the required checks are green. Please pull before pushing again so the merge commit is kept. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for your patience, @MopicMP. A pull request that merged into main a few minutes ago (#2769) changes internal/cbm/cbm.h and extract_defs.c, which this pull request also changes, so the CI run in progress here could no longer vouch for today's main. We merged current main into your branch once more (one merge commit, 32bd1d3; it merged cleanly and your own change is untouched) and started a fresh run. We will merge once the required checks are green. Please pull before pushing again. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for your patience, @MopicMP. A pull request that merged into main a few minutes ago (#2418) changes src/pipeline/pass_definitions.c, which this pull request also changes, so the CI run in progress here could no longer vouch for today's main. We merged current main into your branch once more (one merge commit, d1a96df; it merged cleanly and your own change is untouched) and started a fresh run. We will merge once the required checks are green. Please pull before pushing again. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for your patience, @MopicMP. A pull request that merged into main tonight (#2601) changes src/pipeline/pipeline_incremental.c, which this pull request also changes, so CI on this branch could no longer vouch for today's main. We merged current main into your branch once more (one merge commit, bff6ce9; it merged cleanly and your own change is untouched) and started a fresh run. We will merge once the required checks are green. Please pull before pushing again. |
|
Merged as edb4a6e - thank you, @MopicMP! Calls to Java record accessors now resolve to the accessor methods, and a call is never bound to a data member any more, in both the sequential and the parallel resolver. Thank you for the careful, well-tested change, and for your patience while main kept moving under it - this one went through more refreshes than most, and every time your tests made it easy to confirm nothing had shifted. |
Written by an AI agent (Claude Code) working for @MopicMP, who signed off the
commit and answers for it.
A Java record accessor call binds to an unrelated field
p.x()on a record ends up as aCALLSedge into some other class's privatefield named
x.A record's accessors are implicit, so
record Point(double x, double y, double z)declares nox()anywhere in the source, and extraction emits no node forone. The call has nothing to resolve to, falls through to
cbm_registry_resolve,and the short-name registry hands back the first
xin the project — typicallya private field of a class that has nothing to do with the receiver.
Two answers invert:
deleted a live method on that answer;
How much of the graph this is
Two indexed Java trees, v0.10.8 release binary,
mode=fast, Windows 11 x86_64:CALLSedgesVariableorFieldCounted with
MATCH (a)-[:CALLS]->(b) WITH b.label AS kind, count(*) AS n RETURN kind, n.moderateandfullresolve the same way, and so does thev0.11.0 release binary.
The change, in two parts
Extraction emits the accessors (
internal/cbm/extract_defs.c). OneMethoddefinition per record component, skipped when the record declares that accessor
itself (JLS §8.10.3 allows the override). The record's own API becomes visible:
the accessor gets a node, and
p.x()resolves to it.A Java call may not bind to data (
src/pipeline/registry.c, both callsites). A Java call expression never names data —
p.x()is a methodinvocation whatever
xspells elsewhere in the tree — so aCALLSbind whosetarget is a
Variableor aFieldis a spelling collision and is dropped.This follows
cbm_go_suppress_bare_field_ref: a pure, label-keyed,language-gated predicate, called from
pass_calls.candpass_parallel.csothe sequential and parallel resolvers stay identical.
Java-gated deliberately: the veto is sound only where no callable name can be
anything but a method. Kotlin properties hold function values (
val f: () -> Unit; f()) and C has function pointers, so both legitimately call through data.Both halves are needed. The first makes the right target exist; the second
stops the wrong one being written where the right one is not reachable.
Measured
On the 631-file tree, indexed with this build: 2 103
CALLSedges, every oneinto a
Method(1 841) or aClass(262). NoVariable, noField— downfrom 278.
A separate gap this uncovered, not fixed here
Java cross-file type resolution appears to stop working when the project has
more than one source root. Same four files, two layouts:
src/com/x/...):where.x()andwhere.z()bind toPoint.x/Point.zacross files and across packages, with an unrelatedSpring.xfield present;
src/main/java/...plussrc/client/java/..., the Gradle/Fabricshape): the same calls do not bind at all. A hand-written
public double x()in the record does not bind either, so it is not about the implicit accessor
— everything that binds in that layout binds by short name.
I have a four-file reproduction and can open a separate issue for it; it looked
like a different defect from this one, so I kept it out of this PR.
Tests
tests/test_registry.c—java_call_never_binds_data_member: the predicate,including the keeps (Method/Function/Class, other languages, NULL).
tests/test_call_reference_contract.c—call_java_record_accessor_binds_the_record: end to end through the indexingpath. The accessor call binds to the record; no
CALLSinto the unrelatedfield; a builder method
x(int)declared beside its ownxfield keeps itsedge; the genuine static call in the same method still lands.
Built and run with gcc 15.2.0 on Debian (WSL2):
scripts/test.sh, plusclang-formatclean on every changed file.