Repository navigation
fix(registry): score a candidate on the extractor's test verdict, not its name - #2427
Conversation
… its name cbm_registry_add discarded the caller's knowledge of whether a symbol is test code and recovered it from the qualified name: is_test_qn scanned the QN for "test", "Test", "mock", "stub", "spec", "fake" or "Fixture" anywhere in the string. REG_TEST_PENALTY then keeps non-test candidates ahead of test ones, so that scan decides which definition an unresolved call resolves to. An unanchored substring match over a path is wrong in both directions, and the false positives are not exotic. On one 970-file Elixir tree the scan calls 794 of 20,882 Function and Method nodes test code. 516 of those match on "spec", and 268 match ONLY because "inspect" contains it: forge_symphony.pipeline.impl .inspect_optional, .pr_tracker.continuation_intent.inspect_record and every other symbol under an operator_inspect module is ordinary production code that was being ranked below real test code. "latest" and "contest" match "test" the same way. In the other direction a test helper under a directory that spells none of those words is scored as production. The extractor already knows. CBMDefinition.is_test is set from the file's test-ness (cbm_is_test_file) OR the definition's own attributes, and it is the value the store filters on. cbm_registry_add now takes it and stores it beside the QN, and is_test_qn is deleted. Three things follow from the verdict no longer being recomputable: candidate_score loses its tri-state `int is_test` and its `-1: ask` fallback, because there is nothing left to ask. qn_test_flags stops returning NULL. Both filtered resolve paths have to carry the verdicts with the candidates. resolve_multi_with_imports already copied a parallel array but guarded it on `flags ?`; filter_import_reachable carried none at all, so the fuzzy path passed NULL whenever it had filtered and silently fell back to the QN scan. index_under_name drops an entry whose verdict it cannot store, rather than leaving the flag array short. A missing verdict scores as non-test, which would let an unranked candidate outrank real code; one missing candidate is the smaller error. pipeline_incremental.c seeds the registry from persisted nodes and has no CBMDefinition, so it reads the same flag out of properties_json through cbm_node_is_test, exported from pass_tests.c. Its comment requires the incremental registry to mirror the full-index one exactly, and both now answer from the same value. Measured by indexing four corpora with a binary built from this commit's parent and with this one, and diffing the `edges` rows keyed on (source QN, target QN, type): corpus moved retargeted lost (resolve) lost (similarity) Elixir 970-file 920 919 1 0 ripgrep 14.1.1 353 193 30 130 tokio 1.40.0 1,548 1,426 86 36 rust-analyzer 5,160 4,817 219 124 Almost every moved edge is the same call site landing on a different target. 336 resolution edges are lost across the four, where the new winner is refused by a check the old one passed. The similarity column is SEMANTICALLY_RELATED, which the post-passes rebuild wholesale over a drifting corpus and which moves on its own. This is a behaviour change across every indexed project, not a signature change, and the gating suite does not see it: 8,236 tests pass with the scan and without it. REG_TEST_PENALTY had no direct coverage, because none of the 95 cbm_registry_add call sites in the suite registered a symbol the scan would flag. tests/test_registry.c: three tests. registry_prefers_non_test_over_test_ candidate pins the penalty itself. registry_test_verdict_comes_from_the_caller _not_the_qn pins where the verdict comes from, using proj.latest.parse (production code the scan reads as a test) against proj.harness.parse (a test the scan reads as production); it fails with the scan restored and the signature kept. registry_sole_test_candidate_still_resolves pins that the penalty orders candidates without vetoing them. Signed-off-by: Chad <[email protected]>
|
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. |
DeusData
added
bug
|
Thank you for quantifying the retargeted and lost edges instead of presenting only a green suite. I checked is_test_qn and candidate scoring on current main; the name-based classification is indeed part of candidate selection. Because this changes winners across languages and indexing modes, we need more time to review the recorded edge changes and incremental behavior before deciding. It is tagged as a parsing-quality bug for deeper review. Thank you for your patience with the full queue. |
No problem, I think you're doing a great job considering the popularity of the repo and how hard OS is to do right. 👍 |
|
Thanks for the kind follow-up and for quantifying the candidate retargets. The cross-language scoring and incremental behavior still need maintainer review. No further work is requested from you in this sweep. |
|
Thank you for the test-verdict scoring change and the measured retargets. The cross-language and incremental review remains on us, with no additional request for you. Sorry for the delay—I’ll follow up soon. |
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Thank you, Chad, for this careful change, and for measuring it instead of trusting a green suite. You are right about the root problem: an unanchored substring scan over the qualified name is wrong in both directions, and inspect, latest and contest are exactly the false positives that make it unreliable. Taking the extractor's verdict instead of re-deriving it is the right foundation, and carrying the verdicts through the filtered resolve paths fixes a real silent fallback. Sorry it took us this long to come back with a decision; this is the follow-up we promised.
The decision: we want this approach. CBMDefinition.is_test becomes the verdict and is_test_qn goes away. We also measured your branch against its base on a large Go project (gardener, about 2,400 non-test Go files) and found one effect your four corpora could not show. Go projects keep many test doubles in ordinary, non-test files: generated fake clientsets, mock packages and mock_*.go files. The old name scan happened to rank those below real code; the extractor's verdict does not mark them as test code. With the change, about 570 call edges newly resolve into fake or mock files (73 of them from production code, for example an init() whose AddToScheme call now lands in the generated fake clientset), and none move out of them. About 3% of all CALLS edges move on that project. So we would like one anchored rule on top of your verdict, and then a port onto current main:
-
Keep
CBMDefinition.is_testas the primary verdict, exactly as you have it, and keep the deletion ofis_test_qn. -
Add a test-double path rule, anchored to whole path segments and file-name prefixes rather than substrings, so it can never match
inspect,latestorfakeroot. A candidate counts as test code for this penalty when the repo-relative path of its defining file has a directory segment named exactlyfake,mock,mocks,testingortestutil, or when the file name matchesmock_*.goorfake_*.go. That is the complete list. -
Keep the rule in one predicate that takes the defining file's path, next to the scoring in
src/pipeline/registry.cand declared inpipeline.h, and have all three registry seeders pass the extractor verdict OR that predicate:pass_definitions.candpass_parallel.cwith the definition's file path, and the incremental seeder inpipeline_incremental.cwithcbm_node_is_test(node)andnode->file_path. One predicate keeps the full and incremental registries answering the same way. Please keep it out ofcbm_is_test_pathandcbm_is_test_file: those decide the storedis_testproperty, TESTS edges and importance, and a generated fake clientset is not a test. This rule only changes how a candidate is ranked when a call is resolved, never what a node is. -
Port the branch onto current main (rebase or merge, whichever you prefer, with sign-offs as before). The registry has changed a lot since your base and five files conflict today, mostly
registry.c. The main differences:- registration now goes through
cbm_registry_add_lang_in_module(language plus module prefix), withcbm_registry_addandcbm_registry_add_langas thin wrappers, sois_testneeds to be threaded through that function; - each name bucket now carries a
langbyte array besideis_test, grown in lockstep and sharingis_test_cap, so your drop-on-failed-growth rule inindex_under_nameshould cover both arrays; best_by_import_distancenow takes the registry and the language array, and more resolve paths call it. The fuzzy path on main still passes NULL verdicts whenever it has filtered by imports, so yourfilter_import_reachablefix is still needed and should carry the languages along with the verdicts.
- registration now goes through
-
Tests that pin the rule, next to your three registry tests:
- a table test of the predicate: positives such as
pkg/client/fake/clientset.go,fake/x.go,internal/mocks/store.go,pkg/testing/fixtures.go,internal/testutil/helpers.go,pkg/mock_store.goandpkg/fake_clock.go; negatives such aspkg/inspect/x.go,pkg/latest/x.go,pkg/fakeroot/x.go,pkg/mockery/x.go,pkg/testingutil/x.go,pkg/mocking.goandpkg/mock_store.py; - a resolution test in which a production caller has two same-named candidates, a real one and one defined under
fake/whose extractor verdict is false, and the real one wins, through both the full and the incremental registry (the incremental one is where a missed seeder would show); - please check that each new test fails with the rule removed, as you did for
registry_test_verdict_comes_from_the_caller_not_the_qn, and that itsRUN_TESTline is in the suite.
- a table test of the predicate: positives such as
What we will do on our side:
- Once the ported head is up, we will re-run the same gardener comparison on it and post the numbers here, so you do not need a large Go corpus yourself.
- If the port turns out to be painful, just say so. Maintainer edits are enabled on your branch, so we can take the main merge, or the whole port, on top of your commits with your authorship kept.
Thank you again for the thorough work, and for your patience with the queue.
|
Hi @BobbieBarker, thank you again for #2371, which landed on main yesterday. With it in, main has settled in the files this pull request touches, so this one is unblocked. Whenever you have time, the points from our review of October 26 still stand: keep the extractor's test verdict and drop |
|
Hi @BobbieBarker, thank you again for this change. Your reading of the problem holds up: scoring a candidate on the extractor's verdict instead of scanning its qualified name is the right fix, and the Main has moved a long way in the registry since your base (six files conflict now), so we are going to take the port ourselves, as we offered on October 26 and November 1. Maintainer edits are enabled, so our commits go on top of your branch: a merge of current main with the conflicts resolved, then the segment-anchored test-double rule and the tests from our review. Your two commits stay exactly as they are, and the change lands under your name. Once it is up we will re-run the gardener comparison and post the numbers here. If you already have a local port in progress, just say so and we will build on yours instead. Otherwise, please pull before pushing again. Thank you for the careful work and for the measurements that made this an easy decision. |
Brings the branch up to main f5d5746. Conflicts and how each was resolved: - pipeline.h: registration now goes through cbm_registry_add_lang_in_module with cbm_registry_add and cbm_registry_add_lang as thin wrappers. The extractor's is_test verdict is threaded through all three as their last parameter (the convention this branch set for cbm_registry_add), and main's doc comment on cbm_registry_add is kept with the branch's verdict note. - pass_definitions.c, pass_parallel.c: main's registration block (generated members -> cbm_registry_add_qn_only, Swift signatures) is kept and cbm_registry_add_lang_in_module receives def->is_test. cbm_registry_add_qn_only needs no verdict: those entries never reach the by-name buckets the scorer reads. - pipeline_incremental.c: main's registry_seed_t visitor is kept and passes cbm_node_is_test(node), so the incremental seed gives the same verdict as the full index. - registry.c includes: both foundation/arena.h (main) and foundation/log.h (branch). - registry.c index_under_name: takes both the verdict and main's language. The branch's drop-on-failed-growth rule now covers the lang side array too, since it shares is_test_cap: an entry that cannot get both bytes is dropped and logged (registry.verdict_alloc_failed). - registry.c best_by_import_distance: the branch's unconditional verdict read plus main's optional lang_flags tie-break input. - registry.c resolve_multi_with_imports: the filtered view always copies both side arrays (no "flags may be absent" case is left). - registry.c cbm_registry_fuzzy_resolve: the branch's filter_import_reachable fix, now carrying the languages along with the verdicts, so the filtered subset is scored with its own verdicts and tie-broken with its own languages. - registry.c cbm_registry_add body: main's layout (the body lives in cbm_registry_add_lang_in_module, cbm_registry_add is a wrapper) with the verdict passed to both index_under_name keys. - tests/test_registry.c: main's renamed `proximity` registry (Windows defines `near`) with the verdict argument. Semantic follow-through in code main added since the branch's base (each relied on "no verdict means ask is_test_qn", which this branch deletes): - qn_lang_flags, candidates_without_members and registry_entry_language no longer guard against side arrays that cover only part of a bucket: under the drop rule every entry of a non-empty bucket has both. - The Swift overload choosers (swift_choose_receiver, swift_choose_candidate) passed NULL verdicts. cbm_registry_swift_candidates now copies each match's stored verdict from its by-name bucket into a parallel byte array (same allocation as the match lists), and the choosers carry it through their receiver and import filters, so every best_by_import_distance call scores with stored verdicts. - Test call sites main added (test_registry.c 74 cbm_registry_add, 17 cbm_registry_add_lang, 9 cbm_registry_add_lang_in_module; test_pipeline.c 6; test_callable_sig.c 3) pass `false`, as the branch's own call sites do; only those lines were re-wrapped. is_test_qn has no remaining definition or call site. Signed-off-by: Martin Vogel <[email protected]>
index_under_name, swift_choose_candidate and the fuzzy path's filter_import_reachable call gained parameters in the merge of main; this re-wraps exactly those lines the way clang-format (LLVM 23) lays them out. No code change. Signed-off-by: Martin Vogel <[email protected]>
…rule The extractor's verdict is the right primary signal, but Go projects keep many test doubles in ordinary, non-test files: generated fake clientsets, mock packages, mock_*.go files. The QN scan this branch removed happened to rank those below real code; the verdict alone does not, so calls from production code started resolving into them (gardener: about 570 call edges newly landed in fake or mock files, 73 of them from production code). cbm_registry_test_double_path is one registry-only predicate: a directory segment named exactly fake, mock, mocks, testing or testutil, or a file named mock_*.go or fake_*.go. It is anchored to whole segments and the file-name prefix, so inspect, latest, contest, fakeroot or mockery never match. All three registry seeders pass the extractor verdict OR this predicate on the path the definition's node stores (pass_definitions.c, pass_parallel.c, and the incremental seed in pipeline_incremental.c), so a full and an incremental index rank alike. register_and_link_def now takes the file's rel path for that, as pass_definitions.c already has it. The rule only orders candidates when a call is resolved. It stays out of cbm_is_test_path and cbm_is_test_file, so the stored is_test property, TESTS edges and importance do not change. Co-authored-by: Chad <[email protected]> Signed-off-by: Martin Vogel <[email protected]>
The incremental registry seed reads each stored node's test verdict back with cbm_node_is_test, as the full build takes CBMDefinition.is_test. Main's closure-repair route seeds that registry from cbm_delta_preseed proxies, which load only the properties a resolver reads, and is_test was not among them: every proxy scored as production, so a re-resolved caller could bind a test helper the full build ranks last. On gardener a comment-only edit of three files made the incremental index differ from a fresh full one by two ranking edges (a WRITES into a _test.go function, a USAGE into pkg/utils/test); with this change the only differences left are the 13 similarity / OVERRIDE / IMPLEMENTS edges main already shows on that edit. The preseed now selects whether a row's properties carry "is_test":true and adds just that member to the proxy's properties. Proxies are never written back. If adding it cannot be allocated, the preseed fails like an id mismatch does, rather than seed a proxy that would rank as production. Co-authored-by: Chad <[email protected]> Signed-off-by: Martin Vogel <[email protected]>
… and incremental - registry_test_double_path_table: the anchored predicate's positives (fake, mock, mocks, testing, testutil directory segments; mock_*.go, fake_*.go) and negatives (inspect, latest, contest, fakeroot, mockery, testingutil, mocking.go, mock_store.py, a file named fake, a different case, NULL). - registry_swift_overload_choice_uses_stored_verdicts: the Swift overload choosers score with the verdicts stored at registration; without them the smaller QN, a test double, wins the tie. - pipeline_registry_ranks_by_extractor_verdict_full_and_incremental: the `latest` vs `_test.go` misranking the QN scan made, through a full index, a closure-repair incremental run and a forced legacy-partial run. - pipeline_registry_ranks_test_doubles_last_full_and_incremental: a generated fake that out-ranks the real package on proximity, through the same three runs. Each fails with the piece it pins reverted (predicate, seeder verdict, incremental seeder rule, closure-repair verdict carry, Swift verdict carry). Co-authored-by: Chad <[email protected]> Signed-off-by: Martin Vogel <[email protected]>
|
Hi @BobbieBarker, the port is now on your branch, on top of your two commits, which are unchanged. Thank you again for the core of this: taking the extractor's test verdict instead of a substring scan, deleting What our commits do, and why:
Gardener (5,333 files), current main against this head, same corpus, fresh caches:
Please pull before pushing again. Thank you for your patience with the queue. |
|
This is now on |
Bring DeusData#2360 up to date with main b3ec08d (DeusData#2427 registry test verdict, DeusData#2962 MSan CI, DeusData#2952 Windows stdio guard). The merge is clean and needs no follow-up edit: DeusData#2360's code makes no cbm_registry_add* call and never used is_test_qn, and Scala definitions register through the shared seeders in pass_definitions.c, pass_parallel.c and pipeline_incremental.c, which DeusData#2427 already passes the extractor verdict through for every language. The branch's own change against main is byte-identical to 28948f2's against f5d5746. Signed-off-by: Martin Vogel <[email protected]>
Brings the branch up to main b3ec08d (DeusData#2962, DeusData#2427). Conflict in src/pipeline/registry.c: main (DeusData#2427) threads the extractor's test verdict through cbm_registry_add_lang_in_module into index_under_name, while this branch records each VB.NET definition's spelling for the case-insensitive retry right after that call. Resolved by keeping both: index_under_name takes main's `is_test`, and vb_index_folded still follows it for VB.NET. VB.NET registrations get the verdict the way every language does: the three registry seeders pass the definition's is_test, which the extractor sets from the file's test-ness, and the *Test.vb / *Tests.vb rule of this branch feeds that. Adapted with the merge, per DeusData#2427's convention: the VB.NET registry tests register with `, false`, and vb_test_file_names now also checks that a method of OrderTests.vb is extracted with is_test set and one of Order.vb without it. Signed-off-by: Martin Vogel <[email protected]>
DeusData#2427 (b3ec08d) ranks registry candidates by the extractor's stored test verdict instead of the old QN substring scan. The extractor gives a definition its file's verdict only for functions, methods and the module (extract_defs.c), so a variable, field, class, struct or interface in a test file keeps is_test=false. The QN scan had masked that: Java and Scala QNs spell src.test and test class names. Since DeusData#2427 such a symbol ranks as production code, and a name-only match (suffix_match) from production code picks it whenever it shares more of the caller's path than the real candidate. finagle's `Future.exception` bound to a Variable in TimeoutFactoryTest; Go structs in _test.go files won the same way. The three registry seeders (pass_definitions.c, pass_parallel.c, pipeline_incremental.c) now also apply cbm_is_test_file, the rule the extractor computes is_test_file with, on the same path and language, so every definition in a test file ranks as test, in full and incremental indexes alike. Ranking only: the stored is_test property, TESTS edges and importance keep their own rules. Proof, production-source edges into test files (cbm_is_test_file on both ends), pre-DeusData#2427 cf337a6 / main b3ec08d / this change: kafka 4.0.0 985bc995 CALLS 849 / 1023 / 750 (onto Variables 6/362/6) USAGE 5936 / 8254 / 5832, WRITES 163 / 218 / 121 gardener 58d1ab91 CALLS 125 / 123 / 52 (onto Structs 75/75/4) USAGE 837 / 730 / 572, WRITES 323 / 254 / 160 finagle ca472deb CALLS 281 / 505 / 273, USAGE 1568 / 2436 / 1541 finagle with DeusData#2360 CALLS 566 -> 300 (Variables 391 -> 107) Every edge DeusData#2427 moved from a test file to production stays there (gardener USAGE 125, WRITES 51, CALLS 1; kafka CALLS 59, USAGE 45, WRITES 13; finagle USAGE 17, CALLS 7). Only suffix_match CALLS move. Two runs are identical, and an incremental re-index of 25 seeded files matches the full index for every caller outside the edited files. On kafka the change also restores about 6,800 suffix_match CALLS from test files, with their TESTS edges, that main had lost because an unmarked test-file field won the ranking and the CALLS guard refused it; they are the edges the index had before DeusData#2427. Signed-off-by: Martin Vogel <[email protected]>
The registry ranks a test candidate below real code. For a reference made by test or test-double code that is the wrong way round: tests and mocks use their own helpers. Since every definition in a test file ranks as test (previous commit), kafka's clients tests calling `new MockTime()` bound TopologyTestDriver.MockTime in streams/test-utils/src/main, a test-support module no rule marks, instead of the clients test helper they import. The calling definition's own verdict (cbm_pipeline_caller_is_test: the extractor's verdict stored on its node, cbm_is_test_file and cbm_registry_test_double_path on its file) now reaches the registry from both call resolvers and both USAGE / READS / WRITES resolvers. For a test or test-double caller no candidate takes the test penalty, so proximity alone ranks; production references keep the full rule. The per-file resolve cache records the bit, since a test and a production function of one file can ask for the same name. Proof: - Production-caller CALLS/USAGE/WRITES on kafka are edge-for-edge equal to the previous commit; gardener production CALLS too. The production->test-file tables and the DeusData#2427 gains are unchanged. - kafka `new MockTime()` from tests: 487 constructor and 17 class calls bind the clients helper again (previous commit: 654 went to TopologyTestDriver.MockTime). - gardener mock packages bind their own symbols: USAGE into test-double files 4959 -> 5132, WRITES 61 -> 137. - Test-caller CALLS that differ from main (16,373 on kafka) are mostly name-only member guesses either way; 24 judged (seed 162): 7 better, 6 worse, 10 wrong both ways, 1 equivalent. kafka TESTS edges: 64,603 on main, 57,518 here. - Two runs identical; incremental matches full for every caller outside the edited files. Signed-off-by: Martin Vogel <[email protected]>
This PR was written by an AI agent working on my behalf. I certify the DCO sign-off on the commit and answer for the change.
Follow-up to #2370. The registry scores candidates on
CBMDefinition.is_testinstead of scanning the qualified name, andis_test_qnis deleted.is_test_qnmatchedtest,Test,mock,stub,spec,fakeorFixtureanywhere in the QN, andREG_TEST_PENALTYuses that to pick which definition an unresolved call resolves to. On one 970-file Elixir tree it calls 794 of 20,882 Function and Method nodes test code. 516 match onspec, 268 of those only becauseinspectcontains it:inspect_optional,inspect_record,inspection_actions.latestandcontestmatchtestthe same way, and a test helper under a directory spelling none of those words is scored as production.Cost
Four corpora, a binary from this commit's parent and one from this branch,
edgesdiffed on (source QN, target QN, type):336 resolution edges are lost across the four, where the new winner is refused by a check the old one passed. The similarity column is
SEMANTICALLY_RELATED, which the post-passes rebuild wholesale and which moves on its own.acceptance_spec_runneris a typical retarget:resultresolved tocritique_report.resultand now resolves tooperator_inspect.reader.result, the function it calls.This is a behaviour change in every indexed project, not a signature change.
Ripples
candidate_scoreloses its-1: asktri-state andqn_test_flagsstops returning NULL, since there is nothing left to recompute from.Both filtered resolve paths now carry verdicts with their candidates.
resolve_multi_with_importscopied a parallel array but guarded it onflags ?;filter_import_reachablecarried none, so the fuzzy path passed NULL whenever it had filtered and fell back to the scan silently.index_under_namedrops an entry whose verdict it cannot store rather than leaving the array short, because a missing verdict scores as non-test and would outrank real code.pipeline_incremental.cseeds from persisted nodes with noCBMDefinition, so it reads the flag out ofproperties_jsonviacbm_node_is_test, exported frompass_tests.c.Tests
REG_TEST_PENALTYhad no direct coverage: of the 95cbm_registry_addcall sites in the suite, none registered a symbol the scan would flag, so the suite passes with the scan and without it.registry_test_verdict_comes_from_the_caller_not_the_qndiscriminates:proj.latest.parseagainstproj.harness.parse. With the scan restored and the signature kept it fails on"proj.harness.parse" != "proj.latest.parse". The other two pin that the penalty exists and that it orders candidates without vetoing them.144 of 144 suites, 8,254 passed, 0 failed, 10 skipped.