Conversation
Signed-off-by: Pcristin <xxxokzxxx@protonmail.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. |
|
Thank you @pcristin, both for picking this up so quickly and for keeping it focused on the misleading zero. That was exactly the part that hurt us. I built the PR head and ran it against the repro from #2265, plus one extra case. The inbound side works well. I found three rough edges and traced each one to the source, so I hope this saves you some time. Please take whatever is useful. All line links below point to the PR head How I testedLinux x86_64, gcc, export HOME=$(mktemp -d)
B=./build/c/codebase-memory-mcp
R=/path/to/repro # the 9 files from #2265 (+ anidada.js below)
echo "{\"repo_path\":\"$R\",\"mode\":\"full\"}" | $B cli index_repository
P=<project name printed above>
q() { echo "{\"project\":\"$P\",\"function_name\":\"$1\",\"direction\":\"$2\",\"format\":\"json\"}" | $B cli trace_path; }What works
1. Outbound: nested functions inside a factory still report an exact zeroRepro ( import { ayudante } from "./ayudante.js";
export function crearAnidada({ cliente }) {
function interna(id) {
ayudante(id); // resolved → CALLS edge from `interna`
return cliente.buscar(id); // unresolved → recorded with caller `crearAnidada`
}
return { interna };
}The original repro shows the same thing: Why: the two passes disagree on who the caller is.
I checked this with a temporary Possible directions (you know the codebase much better, so these are only ideas):
This is the case that made me open #2265. Our backend builds services with factory functions and injected dependencies, so almost every method we'd trace outbound is a nested function. 2. A resolved call is also recorded as unresolved, twiceIn [{"caller":"…directo.usarDirecto","leaf":"ayudante","start_byte":84,"end_byte":95,"reason":"import_symbol_not_in_registry"},
{"caller":"…directo.usarDirecto","leaf":"ayudante","start_byte":84,"end_byte":95,"reason":"import_symbol_not_in_registry"}]As a result, Why: the TS LSP runs twice per file, and each run gives a different callee QN for the same site. My trace shows:
Possible fixes:
(Separately, and maybe worth its own issue: the cross-file pass has the correct module QN 3.
|
DeusData
left a comment
There was a problem hiding this comment.
Thank you. This is the follow-up we agreed on in #1682, and persisting unresolved call sites so coverage and trace_path can admit what they do not know is exactly the direction we want. We accept the "unknown" relation value.
@BryanQuiceno, thank you too for that test report. You traced each rough edge to the line that causes it, and that saved us real time. Your three findings overlap with what we were going to ask for, so here they are in one list.
Before it merges:
-
Remove the false
"unknown"s. Two sources drain the signal:- Bryan's finding 2: one call site that resolved (
usarDirecto → ayudante) is also recorded as unresolved, twice, because the per-file and cross-file TS passes use different callee strings. - Inbound matching on any short name: any unresolved site whose leaf matches a visited node's short name marks that trace
"unknown". Common names (get,run,init) would then almost always read"unknown".
Please dedupe coverage rows on
(caller, leaf, start_byte, end_byte), drop an unresolved site whose span already produced a CALLS edge, and link an unresolved site to a traced node only through the caller or a leaf the resolver would actually have considered for it. - Bryan's finding 2: one call site that resolved (
-
Outbound on nested functions must not claim an exact zero (Bryan's finding 1). A factory-built function (
crearAnidada → interna) still reportscallees_total_relation: "eq"while a call is missing. Correcting that"eq"is what this PR is for. Matching the site to the traced function by file and source range, as Bryan suggests in (b), keeps this PR narrow. The deeper fix in the TS LSP walk, setting the enclosing function for nested functions, affects resolved calls too and deserves its own issue and PR, as Bryan proposes. -
Files with unresolved calls must not appear in
skipped[](Bryan's finding 3).add_skipped_summaryshould treatunresolved_callslike the parse-coverage phases, since those files were indexed. -
Real-corpus numbers. Our rule is that a change is shown working on real input before it merges. Please report:
- index time
- peak RSS
- the number and total size of
unresolved_callsrows - the share of
trace_pathcalls that come back"unknown"
Please measure on a Go repository and on the Linux kernel, taken after items 1–3, so they show the signal we will actually ship.
-
A parallel-path test. The two tests use two files, which is below
MIN_FILES_FOR_PARALLEL(50), so only the sequential path runs. Please add a fixture past 50 files, ideally one that forces a result spill, to cover the path most real indexes take. -
yyjson_mut_doc_new(NULL)incbm_pipeline_record_unresolved_callsuses libc's allocator. Please pass the core-backed allocator the rest of the PR already uses.
Bryan has offered to open a PR against your branch with items 1 (dedupe part) and 3, plus tests. That is welcome from our side, and it is your call whether to take it. The 1-based line next to the byte offsets is a good small addition if you have room.
We will call out one behaviour in the release notes on our side: every index built before upgrading reads "unknown" on call traces until it is re-indexed. Also a heads-up: #2294 removes is_test_file() from mcp.c, which this PR calls in new places, so whichever lands second will need a small rebase.
Thank you both. This closes a real honesty gap in the tools.
What does this PR do?
Fixes #2265.
TypeScript and JavaScript receiver calls can be emitted as
lsp_unresolvedwithout a CALLS edge. This patch records those unresolved call sites as per-file index coverage, including the caller, method name, source byte span, and reason.check_index_coverageexposes the records. Incremental indexing preserves them, and CALLS trace totals are reported asunknownwhen unresolved sites or older metadata make an exact total impossible. It does not invent CALLS edges for unresolved targets.The CLI host fixture timeout discovered while validating this change is addressed separately in #2304, following the contributing guide's one-issue-per-PR rule.
Validation
scripts/test.shrun completed all 143 suites: 8,176 passed, 1 failed, 8 skipped. The only failure was the existing CLI fixture host expiring on this slow machine; test(cli): keep fixture host until released #2304 fixes that fixture.clang-format-20 --dry-run --Werror, the memory-core lint, no-skips check, DCO check, and focused cppcheck 2.20.0 on changed production C sources passed.scripts/lint.sh --cidid not finish within 45 minutes in cppcheck; it produced no findings before being stopped.OpenAI Codex assisted with the investigation, implementation, testing, and PR preparation. Pcristin reviewed and certified the signed-off commit.
Checklist