fix(cpp): a `->` call on a smart pointer or optional reaches the type it holds

Merged
CO
8d ago
+776-135

Summary

A C++ call made with -> on a receiver declared as a std::unique_ptr<T>, std::shared_ptr<T> or std::optional<T> now links to T's method, or to a method of a class T derives from.

Before, receiver inference read the declared type as unique_ptr, found no method on it, and the call fell through to Strategy 3's guess by method name and receiver words:

  • facebook/rocksdb: std::unique_ptr<Iterator> iter; iter->Valid() reached ArenaWrappedDBIter::Valid. std::shared_ptr<FileSystem> fs; fs->GetFileSize(…) reached the test double test::StringFS::GetFileSize. std::unique_ptr<Transaction> writer; writer->Put(…) reached SstFileWriter::Put.
  • protocolbuffers/protobuf: std::unique_ptr<FieldGeneratorBase> generator; generator->GenerateMembers(…) reached MapFieldGenerator::GenerateMembers. The Kotlin generator's std::unique_ptr<FileGenerator> reached java::FileGenerator.

This is the follow-up #2413 left open. It uses that PR's CppReceiverDeclaration (raw, callers, shadowed) and cppMemberOperator. It applies only when all of these hold:

  • the declaration is the caller's own and not shadowed;
  • it is not an alias and not a pointer;
  • the call is ->;
  • the holder is std::-qualified, as isCppLibraryType reads it.

Why the held type is looked up as C++ does, not by its last name

The plan was resolveMethodOnType on T's last segment. Measured on rocksdb, that sends about 3,800 calls to the wrong class with a 0.9 typed edge (2,998 retargets, 611 additions, 154 confidence bumps):

  • Iterator names 22 classes in rocksdb: MemTableRep::Iterator, SkipList::Iterator, InlineSkipList::Iterator and gtest's generator Iterators among them.
  • The Iterator these calls mean declares none of Valid / Next / Seek / key; it inherits them from IteratorBase.
  • So getMethodMatches('Iterator', 'Valid') finds MemTableRep::Iterator::Valid, and the supertype walk never runs.

So T goes through #2407's C++ lookup, now shared as cppClassWritten, from where the call is written:

  • the calling function first (a class local to it);
  • its class and that class's bases;
  • the enclosing namespaces;
  • the file's using declarations, directives and namespace aliases;
  • aliases followed;
  • failing all that, the only class of that name the file can see (that edge gets 0.85).

The method is then taken from that class, or from its bases through the class's own extends edges. A new node-keyed getSupertypeNodes reads those edges; the name-keyed getSupertypes unions every same-named class's bases.

Each shape, decided by measurement

held typeoutcomerocksdbprotobuffmt
a project class with the method, its own or inheritedtyped edge at 0.9, or 0.85 through the unique-name fallback7,680 retargeted · 2,500 added · 3,509 same target, now 0.962 · 54 · 861 · 3 · 1
a template parameter (std::unique_ptr<IterType>)no edge25 removed00
a library type (std::deque<…>, std::vector<…>), or an alias of one (using VectorWalPtr = std::vector<…>)no edge9 removed00
a project class that has no such method, nor do its basesno edge; a class template falls through, since a specialization may declare it (as in #2399)no such call had an edge before00
a type the project doesn't declare, or an alias the lookup can't followunchanged38 guesses kept12 guesses kept0

  • Types the project doesn't declare keep their guesses. In the probe, those guesses were about half right:

    • right: generated unittest::TestAllTypes → Message::SpaceUsedLong; std::unique_ptr<Rep> rep_ → methods of an out-of-line struct BlockBasedTableBuilder::Rep, which the index names <anonymous>;
    • wrong: ZeroCopyInputStream → a test stream, while PROTOBUF_EXPORT hid the class. #2449 now indexes it, so those 24 calls resolve through this PR to ZeroCopyInputStream / ZeroCopyOutputStream.

  • An alias the lookup can't follow keeps its guess. rocksdb's port::Thread is std::thread in port_posix.h and WindowsThread in port_win.h. Only an alias of a library type or of a template parameter is silenced.

  • Iterators: unchanged. No receiver in the five repos is declared as std::…<T>::iterator and called with ->. All 31 -> calls on an iterator go through aliases:

    • gmock's ExpectationSet::const_iterator;
    • rocksdb's ColumnFamilySet::iterator, which is a project class.

    Reading the element type through an alias needs the alias's template arguments, which cpp-type-aliases.ts doesn't keep. Left as a follow-up.

  • -> on a raw pointer to a smart pointer: unchanged. std::unique_ptr<T>* p; p->reset() calls the holder's own member, and #2413's gate already silences it. #2413's gate log has 549 such calls in rocksdb and 1 in protobuf. All are reset / get / release / swap / has_value / value / emplace. A test pins it.

Validation

Setup.

  • Every arm ran codegraph init -y with the native kernel, on this Windows box.
  • Base is main b8b3eb97, which includes #2413, #2421, #2426, #2434, #2439, #2443, #2445, #2446 and #2449; the other arm is this branch merged with it. Both use one kernel built from that source.
  • The A/B was re-run on each C++ change main took while this PR was open: ed199e60, 9a1d4bd9 (#2449), 16344af8 (#2434, protobuf only), e47cb258 (#2443, #2445, #2446) and b8b3eb97 (#2439).
    • #2449 added 29 protobuf sites: the ZeroCopy*Stream calls above, msg->GetReflection() turning from confidence bumps into additions where main stopped guessing, and DynamicMessageFactory's override (below). It also dropped one rocksdb removal: main no longer guesses WindowsThread::join for a std::optional<std::thread>.
    • #2439 reads typed range-for loop variables, so 8 rocksdb calls on smart-pointer loop variables now take this path. Seven are for (const std::shared_ptr<Cache>& cache : …) in db_block_cache_test.cc, which go from 0.8 to 0.9 on the same Cache::* targets. The eighth is for (const std::shared_ptr<TableFactory>& tf : …) tf->Name(), newly linked to Customizable::Name.
    • The other C++ commits left every changed site as it was.
  • Edges are diffed per call site (kind, source, line, col, refName), and each changed site is joined to a probe log of that call's declaration and held type.
  • The same A/B on the earlier base (d8a7f868 + #2413) gave identical changed sites once qualified names are compared without the namespaces #2426 restores. The one exception is protobuf's std::unique_ptr<TestMapField>. On the old base its alias led to a MapField whose namespace the index had lost, so the call kept its guess. With #2426 the alias is followed, and the same MapFieldBase::GetRepeatedField target goes from 0.65 to 0.9.

What didn't change.

  • leveldb and redis (the C control): dumps byte-identical.
  • No extends, implements or synthesized edge changed in any repo, the cpp-override edges included, so the matchCppSupertype refactor changes nothing on real data.
  • Every changed site is a -> call on a caller-owned std:: holder.

Removed (rocksdb 34; protobuf and fmt none), every one read:

  • 25 template-parameter calls:
    • StressTest::TestIterateImpl's std::unique_ptr<IterType> → ArenaWrappedDBIter::*;
    • BlockBasedTable::PutDataBlockToCache's std::unique_ptr<TBlocklike> → Block::own_bytes and BlockBasedTable::ApproximateMemoryUsage.
  • 6 calls on library types:
    • std::unique_ptr<std::deque<BlockHandleInfo>> → Block::size / BlockBuilder::empty / autovector::emplace_back;
    • std::optional<std::vector<WriteBatch>> → autovector::emplace_back.
  • 3 std::unique_ptr<VectorWalPtr> calls → CompactionInputFiles::size.

Retargeted and added.

  • An independent check re-derived each target from the declaration's spelling, the caller's scopes and the extends edges. It agrees for rocksdb's 6,913 + 2,450 class sites and for every class site in fmt and protobuf; the 29 protobuf sites #2449 added and the 8 rocksdb range-for sites #2439 added were read by hand.

  • The ~1,000 it can't place go through:

    • aliases (InternalIterator → InternalIteratorBase, LogFile → WalFile, MergerMaxIterHeap → BinaryHeap, UserDefinedIndexBuilder → IndexFactoryBuilder);
    • using namespace ROCKSDB_NAMESPACE; (std::unique_ptr<DB> in tools and examples);
    • or the unique-name fallback (a fixture's nested TestItem, CountingFileReadListener).

    I grouped them by declaration and target and read each group.

  • Read by hand:

    • all of protobuf's 62 retargets and 54 additions;
    • all 5 fmt changes;
    • in rocksdb: 40 random retargets, 25 random additions, 38 retargets and 37 additions each with a different held type, and every held type resolved through a base class (36).

    All were right. The old targets were implementations, test doubles or namesakes:

    • ModelDB::CompactRange for a std::unique_ptr<DB>;
    • CuckooTableReader::NewIterator for a std::unique_ptr<TableReader>;
    • ChargedCache::Lookup for a std::shared_ptr<SecondaryCache>;
    • BlobFileBuilder::Finish;
    • KeyGenerator::Next for a std::unique_ptr<LockTracker::KeyIterator>.

  • DynamicMessageFactory: on main ed199e60, protobuf's std::unique_ptr<DynamicMessageFactory> reached the base virtual MessageFactory::GetPrototype, because the index stored the class's own override under a name PROTOBUF_NONNULL mangled. #2449 fixed the name, and the call now reaches DynamicMessageFactory::GetPrototype itself.

Cost. Process CPU time (all threads) for a full index, interleaved runs on the earlier base:

  • rocksdb: main 165.8 / 164.2 / 164.4 s, this branch 163.6 / 159.7 / 156.7 s;
  • protobuf: main 422.0 / 409.4 s, this branch 399.4 / 393.6 s.

No measurable cost; fewer calls reach the by-name search.

Code

  • src/resolution/name-matcher.ts: matchCppHeldTypeCall and cppMethodOf, called from matchMethodCall right after #2413's gate.
  • src/resolution/cpp-supertypes.ts: matchCppSupertype's lookup is now cppClassWritten(written, site), parameterized by where the name is written: scope, file and line, template parameters, and the class it can't be. matchCppSupertype is a thin wrapper over it, with unchanged behavior.
  • src/resolution/cpp-namespaces.ts (new): cppNamespaceAliases and cppMacroNamespaceFrames, moved byte for byte out of name-matcher.ts. That way cpp-supertypes.ts no longer imports name-matcher, and name-matcher imports cpp-supertypes without a cycle.
  • src/resolution/types.ts and index.ts: getSupertypeNodes(id), memoized per supertype generation like getSupertypes.
  • src/resolution/cpp-type-aliases.ts: cppTemplateArguments.

Tests

  • New file: __tests__/cpp-smart-pointer-receiver.test.ts, 8 tests. It covers:

    • rocksdb's reproduction, with MemTableRep::Iterator and ArenaWrappedDBIter as decoys;
    • shared_ptr, optional, a parameter, and a member the class declares in its header, with decoys named after each receiver;
    • a nested class found first from inside the class that declares it;
    • an alias held type, a using namespace, and a class local to the function;
    • template parameters, library types and std aliases getting no edge;
    • a class lacking the method;
    • what stays as it was: a generated message, a class template's specialization, and -> on a std::unique_ptr<T>*.

    Against main, 6 of the 8 fail. The other 2 pin unchanged behavior.

  • All 19 C/C++ test files pass on the merged branch (216 tests), among them #2407's cpp-base-class-lookup against the refactored lookup, and #2426's and #2421's new suites. tsc --noEmit is clean.

  • Full suite on the merged branch. This box ran at 100% CPU, with other sessions' suites and indexing alongside.

    • The parallel run passed 6,549 of 6,674 tests (82 skipped). All 43 failures were timeouts (41) or EBUSY temp-dir removals (2), in 17 load-sensitive files such as sync, wal-deferral, worktree-detection, mcp-*, git-index-currency and function-ref.
    • Rerun one at a time with --testTimeout=120000, those 17 files pass: 938 tests, 0 failed, 1 skipped.

Overlap

  • #2426 (merged) is why protobuf's TestMapField call now gets its typed edge, as described above. #2449 (merged) made the ZeroCopy*Stream and DynamicMessageFactory calls resolvable. #2443 (nested template arguments), #2445 (::-qualified names) and #2439 (range-for loop variables) touch the same files. Each sync conflicted only on name-matcher's import line or CHANGELOG, and the A/B on top of them is unchanged apart from #2439's eight range-for sites.

Follow-ups (not in this PR)

  • Main's typed path for raw pointers has the same namesake problem. Iterator* it; it->Valid() resolves by the last segment, so in rocksdb 1,364 of the 44,915 calls that path types reach MemTableRep::Iterator rather than the class C++ lookup finds. Routing that path through cppClassWritten and cppMethodOf is the fix.
  • An alias of a smart pointer gets no edge today, because #2399's rule silences a capitalized alias that names no class. In rocksdb that's about 90 -> calls: 60 through ScopedArenaPtr<InternalIterator>, an alias template of std::unique_ptr, and 28 through block_test.cc's typedef std::unique_ptr<DataBlockIter> IterPtr;. Following it needs the alias's template arguments.
  • Iterators through aliases: 31 calls (above).
  • absl::StatusOr<T> and absl::optional<T> also hand -> to T. protobuf has about 75 such calls, most unlinked today.
  • The declarator regex spans two templated parameters on one line. In f(std::shared_ptr<A> a, std::unique_ptr<B> b), b's type reads as std::shared_ptr<A> a, std::unique_ptr<B>. That's 31 -> calls in the probes, 1 of them on a smart pointer.

🤖 Generated with Claude Code