Skip to content

fix(c,cpp): a name in a C or C++ test links to what the test's own includes declare - #2675

Merged
colbymchenry merged 2 commits into
mainfrom
claude/focused-wilson-c2e63b
Oct 19, 2026
Merged

colbymchenry merged 2 commits into
mainfrom
claude/focused-wilson-c2e63b

Conversation

@colbymchenry

Copy link
Copy Markdown
Owner

What

When several files declare a name a C or C++ test uses, the test now links to one its translation unit declares, when there is one:

Before, those candidates were ranked by directory and index order, so a test took whichever namesake came first.

  • protobuf's C# generator tests called test_util2.h's TestUtil::TestSourceDir(), a test suite none of them includes. The TestSourceDir() they call is the one googletest.h declares. This is the issue's own example.
  • protobuf's upb and hpb tests left upb_Arena_New() and the rest of upb's C API unlinked (about 2,300 calls). The copies bundled for PHP and Ruby (php-upb.h, ruby-upb.h) won the ranking and were then rejected.
  • rocksdb's db/c_test.c left rocksdb_open() and the rest of the C API in db/c.cc unlinked (about 1,400 calls).
  • rocksdb's histogram test sent clock->SleepForMicroseconds() to emulated_clock.h's EmulatedSystemClock, a header it doesn't include.
  • llm.c's test_gpt2_fp32.cu includes train_gpt2_fp32.cu directly, yet its gpt2_update(…) and cublasCheck(…) went to the other training programs.

Scope, and what it deliberately doesn't do

  • Test files only. Production references keep fix(c,cpp): a file named like a test is visible to the code that includes it #2421's rule unchanged: a test suite is visible to them only through includes, checked after ranking.
  • Where it applies: the exact-name strategy, typed receivers (resolveMethodOnType), and matchMethodCall's class-name and receiver-word strategies. The qualified-name strategy is unchanged (see the qualified-name note below).
  • Never guesses outside the unit. When the unit declares none of the candidates, they stay as they were. A test whose includes don't resolve, like jemalloc's tests including a generated test/jemalloc_test.h, keeps the one helper there is. I measured a "drop out-of-TU test suites first" step and a veto variant. Both removed right edges or added wrong ones where main had correctly linked nothing:
    • vendored gtest's amalgamated gmock-gtest-all.cc defines what gtest.h declares under another name;
    • breakpad's EXPECT_CALL(memory, GetBase()) reached a production GetBase;
    • rocksdb's table_factory->GetOptions<…>() reached DBImpl::GetOptions.
  • A name written alone (no receiver, no qualifier) is narrowed only to what C++ finds for it outside a class: a function, a namespace-scope type, or a constructor. Never another class's method or nested type: gmock's Value(result, …) matcher is no RepeatedFieldAccessor::Value, and breakpad's EXPECT_CALL(handler, Entry(…)) is no CallFrameInfo::Entry struct. A construction narrows to types and constructors only (std::string buffer(n, c) is no enum's string). A call the extractor recorded without the receiver it is written on (result.columns()[0].value()) is left as it was, read with fix(cpp): a call a method makes with no receiver links to its class's own or inherited method #2479's cppImplicitReceiver.
  • C++ method calls to same-named classes in different files all resolve to the first definition #1079 is unchanged: the call site's own file is in its unit and still ranks first.

Re-verified on current main first

Most of the issue's original symptom was already fixed on main 3695818 by #2538 (TU-local classes), #2537 (TEST_F bodies reach the fixture) and #2479. Measured with the issue's own script, test-noinclude.mjs <db> '_test\.(cc|cpp|h)$'. It counts edges from test files into test files the source's translation unit doesn't include:

at 1fb8691 (issue) main 3695818 this branch
facebook/rocksdb 23,003 4 4
protocolbuffers/protobuf 48 4 3
google/leveldb 13 0 0
chromium/breakpad 1 0 0
google/glog 0 0 0
protobuf, _unittest\.(cc|cpp|h)$ sources (18 with #2434) 20 14

rocksdb's 4 are synthesized base→override links from point_lock_manager_test.h into the tests that derive from it, which is by design.

The 14 protobuf unittest edges left are of two kinds:

  • 10 constructions of generated messages (TestAllTypes, TestMessageSetExtension1) whose only in-repo namesakes sit in wire_format_unittest.h, a header these tests don't include (9 of them in an unnamed namespace);
  • 4 TestUtil::ExpectClear calls taken by the qualified-name boundary bug (see the notes).

What this PR fixes is the rest:

  • test-suite headers a test doesn't include, the TestSourceDir case;
  • declarations in the test's own unit losing to namesakes outside it, which main left unlinked or wrong.

A/B (main 3695818 vs this branch), site-keyed edge diffs

Full indexes of 13 repos with the same kernel. A site is (kind, source node, line, col, refName).

Repo Added Retargeted Removed Confidence only (same target)
facebook/rocksdb 1,442 47 12 2,519
protocolbuffers/protobuf 2,316 55 67 546
fmtlib/fmt 33 40 0 128
chromium/breakpad 13 14 0 135
google/glog 22 0 0 0
google/leveldb 10 0 0 103
karpathy/llm.c 0 6 0 43
redis/redis 0 0 0 248
jqlang/jq, tokio, excalidraw, okhttp, typeorm byte-identical

Triage. Every (before → after target) group was read. The added sites, by group:

  • C API calls into the defining files: rocksdb rocksdb_* → db/c.cc; protobuf upb_* → upb's own .h/.c, not the PHP/Ruby bundles.
  • glog's SetLogDestination(), FlushLogFiles() and LogToStderr() → logging.cc.
  • leveldb's crc32c_test.cc Value(…) → crc32c::Value.
  • fmt's format(…) → fmt::format, test_string(…) → util.h's alias, open_buffered_file() → test/util.cc.
  • breakpad's WriteFile()/ReadFile() → common/tests/file_utils.cc.
  • rocksdb's NewWritableFile(fs, …) → read_write_util.cc, and db_->Get(…)/wp_db->ReleaseSnapshot(…) → DB::.

Wrong among the 3,836 added: 11.

  • protobuf: 7 arena_message1->GetArena() on a generated message → FieldWithArena::GetArena (receiver-word guess).
  • rocksdb: 3 get<0>(GetParam()) → vendored gtest's std::tr1::get.
  • fmt: 1 std::string_view construction → fmt's string_view alias.

Retargeted, 162 sites. About 145 are right or equivalent:

  • right: TestSourceDir ×6; clock->SleepForMicroseconds → SystemClock ×12; CreateFile/Info/Log/ValidateOptions/Roundup/FindFile → their free functions; VisitFields, Prefetch<…>, ValidateEnum, the json:: stream functions; llm.c's included-source functions ×6; breakpad's per-platform CrashGenerationServer/ExceptionHandler and WriteMinidump → the free function; range_map->StoreRange → RangeMap;
  • equivalent: a declaration in an included header in place of its out-of-line definition (DB::Put, Matcher::Matcher).

The rest of the retargets:

  • wrong → wrong: rocksdb swap(…) ×1, the 4 trie_iter->Seek/Next on auto x = make_iter(…), protobuf FileDescriptorProto::descriptor() ×2, FieldOptions::default_instance() ×2;
  • unclear: protobuf info.Clear() on a generic lambda's auto ×2;
  • right → wrong: protobuf Varint(127) ×1 in upb's conformance harness, whose upb/wire/test_util/wire_message.h include doesn't resolve.

Removed, 79 sites. All were wrong receiver-word guesses: rocksdb result.size() → gtest's TestPartResultArray::size ×8 and 4 std_iter->Seek/Next guesses; protobuf FileDescriptorProto::descriptor()-style calls into java_features.pb.h / cpp_file_options.pb.h ×67.

Confidence only: the same target, now the only candidate the test's unit declares, so exact-match 0.4/0.7 → 0.9. Also 2 protobuf instance-method 0.8 → 0.65 with the same target.

Cost

Full indexes, interleaved main/branch, 3 rounds, same kernel:

  • rocksdb: median 51.2s on main vs 48.3s here;
  • protobuf: median 96.3s vs 100.5s.

The ranges overlap heavily (protobuf branch 80–115s, main 93–104s). The check runs only for a test's reference with two or more candidates, and reuses the memoized include graph (cppIncluders), the same-stem header list and the memoized test-path checks.

Tests

__tests__/cpp-test-translation-unit.test.ts, 5 cases. On main, 3 fail:

  • the issue's TestSourceDir case;
  • the upb copy;
  • the rocksdb clock guess.

Two guards pass on both:

Full suite on 08b8361 (this change on main 3695818), with the kernel built, CODEGRAPH_KERNEL_EXPECT=1 and --maxWorkers=4 --minWorkers=1: 7,481 passed, 1 failed, 80 skipped.

Notes

  • Qualified names. matchByQualifiedName's partial match isn't anchored at a :: boundary, so DB::Open(…) also matches BlobDB::Open and TransactionDB::Open, and Status::OK() matches IOStatus::OK. Narrowing those candidates to the unit retargeted 16 rocksdb calls right and 3 wrong, which is an artifact of the boundary bug. So this PR leaves that strategy alone. Branch claude/strange-greider-bd9f76 holds a boundary fix that never became a PR.
  • Follow-up. The same "prefer what the unit declares" rule would help production C/C++ code too: production upb callers hit the same PHP/Ruby duplicates. It needs its own A/B. An earlier experiment that dropped out-of-TU test suites from production candidates before ranking added mostly wrong edges, so I'm filing it separately.

🤖 Generated with Claude Code

colbymchenry and others added 2 commits October 19, 2026 13:49
…cludes declare

When several files declare a name a C or C++ test uses, the test now links
to one its translation unit declares, when there is one: a definition in its
own file, in a file it includes (directly or through other includes), or in
the source file that implements such a header (the same-stem file that
includes it), unless that source keeps the definition to itself (a `static`,
an unnamed namespace, a class it declares). Before, the candidates were
ranked by directory and index order, so a test took whichever namesake came
first:

- protobuf's C# generator tests called test_util2.h's
  `TestUtil::TestSourceDir()`, a test suite they never include, instead of
  the `TestSourceDir()` googletest.h declares;
- protobuf's upb and hpb tests left `upb_Arena_New()` and the rest of upb's
  C API unlinked: the copies bundled for PHP and Ruby won the ranking and
  were then rejected;
- rocksdb's c_test.c left `rocksdb_open()` and the rest of the C API in
  db/c.cc unlinked, and its histogram test's `clock->SleepForMicroseconds()`
  went to emulated_clock.h's `EmulatedSystemClock`, which it doesn't include.

The narrowing applies to test files only, to the exact-name strategy, typed
receivers (resolveMethodOnType), and the class-name and receiver-word
strategies of matchMethodCall. When the translation unit declares none of
the candidates, as when a test's includes don't resolve (jemalloc's tests
include a generated header), the candidates stay as they were. A name a
test writes alone (no receiver, no qualifier) is narrowed only to what C++
finds for it outside a class: a function, a namespace-scope type or a
constructor, never another class's method or nested type, and a call the
extractor recorded without the receiver it is written on is left as it
was. The qualified-name strategy is unchanged.

Validated on main 3695818 (site-keyed edge diffs, every group triaged):
rocksdb +1,442 / 47 retargeted / -12, protobuf +2,316 / 55 / -67,
fmt +33 / 40, breakpad +13 / 14, glog +22, leveldb +10, llm.c 6
retargeted; jq, tokio, excalidraw, okhttp and typeorm byte-identical.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@colbymchenry
colbymchenry merged commit bb8a0f7 into main Oct 19, 2026
@colbymchenry
colbymchenry deleted the claude/focused-wilson-c2e63b branch October 19, 2026 19:22
colbymchenry added a commit that referenced this pull request Oct 19, 2026
…ile's own includes declare (#2684)

PR #2675 narrowed a C or C++ test's same-named candidates to the ones its
translation unit declares. Production code now gets the same rule:
protobuf's Python extension called the copies of upb's C API that the PHP
extension bundles in php-upb.c, and its Java generator's calls of the free
HasHasbit() went to ImmutableFieldGenerator::HasHasbit, a method beside the
caller.

Refinements the wider scope needed:
- a source implements the same-stem header beside it, when it includes one:
  pyext/message.cc is no implementation of google/protobuf/message.h;
- a header's own overloads reach the definitions its source holds:
  conformance/binary_wireformat.h's template Varint calls the .cc's;
- narrowing never leaves a call with only functions its arguments can't
  fill, past one outside the unit that they can: glog's three-argument
  GetStackTrace(...) is not utilities.h's GetStackTrace(). This also mends
  9 breakpad unittest calls #2675 had narrowed the same way;
- nor, for a C call, to definitions only a flag nothing sets compiles,
  past one the build without it does: that stays #2615's choice.
A file-level "call" (a macro, or a declaration read as a call) is left as
it was.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant