Repository navigation
fix(cpp): a call written from the global scope links the declaration of exactly that name - #2445
Merged
Merged
Conversation
…of exactly that name The resolver's name-existence pre-filter read no leading `::`: its `::` branch wanted the separator past the first character, and the single-`:` loop skips names holding `::`. So `::leveldb::RepairDB(…)`, `::ROCKSDB_NAMESPACE::RepairDB(…)` or protobuf's `::_pbi::…` calls were dropped before any strategy ran, unless a detected framework claimed them. protobuf's got through only because it ships three Swift files: the Swift ↔ Objective-C bridge claims any name with a `:` in it, which is why - The pre-filter checks a C or C++ name without its leading `::`. - claimsReference() is asked only of the frameworks that resolve the reference's language (frameworksFor), like resolve() since #2415. - matchByQualifiedName takes a global-qualified C/C++ name as exactly that qualified name. Its suffix match took a namesake nested in another namespace or class: with the pre-filter alone, 23 of the 24 edges it added on leveldb, fmt and rocksdb were wrong, mostly wrappers like fmt's mock `test::open` linked to the `::open(…)` they wrap, and protobuf's `::operator delete` went to `DynamicMessage::operator delete`. A declaration inside a namespace a macro opens (fmt's `struct pipe`) only looks global in the index, so it is skipped; step 5 still reads those. - `::std::…` and `::memset` count as `std::…` and `memset` for the built-in check, and step 5 returns early for a single-segment name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2415.
What was wrong
The resolver's name-existence pre-filter (
hasAnyPossibleMatchinresolveOneInner) never accepted a C/C++ name that starts with::. Its::branch needs the separator past the first character, and the single-:loop skips names holding::. So a global-qualified call like::leveldb::RepairDB(…)was dropped before any resolution strategy ran, unless a detected framework'sclaimsReference()let it through.protobuf's
::_pbi::PrivateAccess::GenerateParseTable(…)calls resolved only because protobuf ships three.swiftfiles. That makes the Swift ↔ Objective-C bridge a detected framework, and the bridge claims any name with a:in it. That dependency is why #2415 left the claims check universal. C++-only repos had no such escape: on main, rocksdb leaves all 729 of its leading-::calls unresolved.The change
::is checked without it.claimsReference()is asked only ofthis.frameworksFor(ref.language), the frameworks whoseresolve()reads the reference since fix(frameworks): a framework resolves only references written in its own languages #2415. fix(frameworks): a framework resolves only references written in its own languages #2415's comment explaining why the claims stayed universal is gone.matchByQualifiedNametreats a global-qualified C/C++ name as exactly that qualified name: C/C++ declarations whose qualified name is the written name without::, the call site's file first. It never falls through to the suffix match, which takes a namesake nested in another namespace or class. It also skips a declaration inside a namespace that a macro opens: the index can't see fmt'sFMT_BEGIN_NAMESPACE, sofmt::pipelooks like a globalpipe. Step 5 (matchCppMacroNamespaced) still handles macro namespaces and namespace aliases, which is where::_pbi::…keeps resolving.::std::…and::memsetcount asstd::…andmemsetin the built-in check. Step 5 returns early for a single-segment name: it used to slice::pipeinto the headpipand the nameipe.Why 3 is needed: the pre-filter alone gives wrong edges
Measured on #2415's head with only 1 and 2 applied, on the same dist and kernel as the baseline:
::crc32c::Extend(the external crc32c library) → leveldb's ownleveldb::crc32c::Extendtest::open,test::fopen,test::pipe, …) linked to themselves from the::open(…)etc. they wrap; gtest's Win32::CreateThread→ThreadWithParamSupport::CreateThread::Compress/::Decompress→ thexpress::Compresswrappers that call them;::gettid→WinEnvThreads::gettid; gtest's::GetThreadPriority→ rocksdb's<anonymous>::GetThreadPriorityThat arm also gained no in-repo
::ns::xcall. A correct top-level targetns::xnever ends with::ns::x, so the suffix match can only return nested namesakes. protobuf showed the same thing: its::google::protobuf::internal::TSanWritecalls (435 of them) already passed the pre-filter on the bridge's claim and still resolved to nothing.Validation
Final A/B on the squash itself. Both arms use the same dist and the same kernel, rebuilt for main's C/C++ changes. The baseline is 235387e, the squash's parent. The fix arm is the same build with this change overlaid (tsc output of
resolution/index.jsandresolution/name-matcher.js), which is the tree of 7a79654. Every index is diffed site by site (source node, line, column, ref name, ref kind), plus unresolved statuses and synthesized edges.::leveldb::RepairDB(…)→leveldb::RepairDB::testing::…→ fmt's vendored googletest/gmock intest/gtest/;::fmt::assert_fail→assert_failinsideFMT_BEGIN_NAMESPACE(step 5)::operator new(…)→ jemalloc's replacement globaloperator new(deps/jemalloc/src/jemalloc_cpp.cpp)::testing::…→ the vendored gtest 1.8.1 inthird-party/(219 of themInitGoogleTest);::ROCKSDB_NAMESPACE::RepairDB::google::…(e.g.::google::protobuf::internal::TSanWrite,WireFormatLite::StringSize,MessageLite::internal_visibility), 128::hpb::…, 4::upb::…, 4::pb::…, 1::upb_Array_DataPtr. Removed: 6::operator delete(ptr)→ a generated class'soperator delete, which were wrong; protobuf declares no globaloperator deleteTriage of every added edge:
::. The one exception is fmt's::fmt::assert_fail, resolved through the macro namespace.testing::targets are in-repo copies. They point to rocksdb'sthird-party/gtest-1.8.1/fused-src/and fmt'stest/gtest/. Unqualifiedtesting::Xcalls already resolve there (403 in rocksdb, 10 in fmt), and so do::testing::Testbases (fix(cpp): a base class links to the class C++ name lookup finds, not a namesake #2407).::testing::stays unresolved: leveldb 3, protobuf 76.::std::…,::absl::…,::crc32c::…and OS calls like::CloseHandleand::fcntlstay unresolved. No edge was added for any of them.::_pbi::…alias calls (924) are identical on both arms.::upb_Array_DataPtr. It lands on the first of three identical C copies of upb (php/ext/google/protobuf/php-upb.h), notupb/message/internal/array.h. That is the same pick the qualified-name path already makes among duplicate definitions. Choosing by the include graph (cpp-includers.ts, fix(c,cpp): a file named like a test is visible to the code that includes it #2421) would fix both paths; left as a follow-up.Earlier rounds, as main moved:
Each round's added sites are a strict superset of the round before. The extras are exact matches to declarations that later extraction fixes index:
::testing::InitGoogleTest, and::google::protobuf::internal::AssignDescriptors, previously indexed asgoogle::internal::….CanUseInternalSwap,WireFormatLite::WriteInt32ToArrayWithField, and fmt's vendoredtesting::Mock::*.Still unresolved in-repo: a handful of names the index doesn't hold under the written qualified name, such as fmt's
::testing::PrintToString, and protobuf's::google::protobuf::EqualsProtoand a name split across a line break. They stay silent rather than wrong.Tests
__tests__/cpp-global-scope-names.test.ts:::store::Repair()from inside a namespace that has its ownstore::Repairreaches the global one.::_pbi::Prefetch()throughnamespace _pbi = ::pb::internal;reachespb::internal::Prefetch.store::posix::closecalling::close(fd)links nothing;::testing::TempDir()with only a nestedstore::testing::TempDirin the project links nothing;test::pipecalling::pipe(fds)does not reach astruct pipeinside a macro-opened namespace;::testing::Testbase stays unresolved.::_pbi::Prefetch()still resolves with the narrowed claims.Mutation-checked:
::ns::f()test fails;::ns::f()test and the Swift/ObjC test fail;::ns::f()and never-a-namesake tests fail (::store::Repair→tools::store::Repair,::close→store::posix::close);::pipe→pipe).Reverting only the claims narrowing changes no test. In the measurements it removed nothing either: on #2415's head, protobuf with the strip and the narrowing was byte-for-byte the baseline, and the 27 framework repos are identical. A ref no part of whose name is a known symbol can't be resolved by name in any case; the narrowing keeps the claims consistent with
resolve()and saves the work.Full suite on Windows, at 100% CPU from other sessions, with the kernel rebuilt for main's C/C++ changes and
CODEGRAPH_KERNEL_EXPECT=1:--testTimeout=120000: 1,731 passed and 2 failed. function-ref's Caller/impact graph misses methods passed as first-class references (callbacks), e.g. executor.submit(obj.method, …) #1820 shadow case hit its own 60 s limit, and one MCP daemon lifecycle case failed.The build's
tscand a type-check of the new test file are clean.Main moved six times while this was being merged. Each sync re-ran
tsc, this PR's test, and the test files of the new main commits; C/C++ commits got a hunk review against this change first. The rounds brought in #2430/#2442, #2449, #2433, #2434/#2444, #2437/#2448, and #2431/#2436/#2441/#2443/#2446/#2447/#2453.🤖 Generated with Claude Code