Skip to content

fix(cpp): every name a declaration lists has its type, like strings_found in ByteBuffer symbols_found, strings_found; - #2698

Merged
colbymchenry merged 3 commits into
mainfrom
claude/nice-lalande-36c37c
Oct 20, 2026
Merged

colbymchenry merged 3 commits into
mainfrom
claude/nice-lalande-36c37c

Conversation

@colbymchenry

Copy link
Copy Markdown
Owner

What

C++ receiver inference now reads the type of every name a declaration lists, not only the first. ByteBuffer symbols_found, strings_found; gives strings_found the type ByteBuffer, so google/breakpad's strings_found.Size() (macho_reader_unittest.cc) links ByteBuffer::Size.

The declarator regex wants the type right before the name, and a list writes the type once, before its first name, so the later names were never read. A call on one of them then linked nothing (breakpad's two strings_found.Size() sites on current main), or reached the class of an earlier variable with the same name that the backward scan found further up.

How

matchCppListedDeclarator reads such a declaration from its start:

  • decl-specifiers;
  • the type, with template arguments paired, std::function<void(int, int)> included;
  • then each declarator with its own */&, cv-qualifiers and annotations (* PROTOBUF_NULLABLE, GUARDED_BY(mu_), as the declarator regex reads past them), array bounds, and an = …, (…) or {…} initializer.

Names are told apart only by commas at the statement's own depth. A later name gets the declaration's type plus its own pointer and reference marks: Foo *a, b; declares one pointer, which #2413's library-type rule depends on (. vs ->).

It returns a match shaped like matchCppDeclarator's and runs only where the declarator regex finds nothing on a line, in all three declaration readers:

  • inferCppReceiverType, in the backward scan and the header scan;
  • cppDataMemberIn, so a class's Table *index_, *data_; types data_ for member lookup and member chains (leveldb's struct Output { …; InternalKey smallest, largest; } → out.largest.Clear());
  • isCppLocalName, so a bare call on a later-listed local steps aside from the implicit-this lookup, as a call on the first name already did.

What lists no name:

  • template arguments: std::map<K, V> m;, Cache<int, Writer> writers, table; (table is a Cache);
  • a call's or constructor's arguments: Writer w(log, table, 3);, Use(a, b, c);, Status s = Copy(source, table, 3);;
  • a constructor's initializer list: : writer_(w), table_(t) {};
  • expressions and other statements: a = 1, b = 2;, return a, b;, typedef Foo A, B;, lambda captures, structured bindings, bit-fields;
  • a parameter list's tail: const Message& message, int, int).

A comma inside an initializer's template arguments (f<A, B>(x), b) makes the reader give up on that line rather than guess, since < may be a comparison.

Validation

Regression test __tests__/cpp-listed-declarator-receiver.test.ts (6 cases): 5 fail on main and all 6 pass with the fix. The sixth, on commas that list no name, passes on both as a guard against over-reading.

A/B on main 036832d (the only later main commit, PR #2696, is Objective-C doc comments). Kit D:\cgwork\cgmdecl, shallow clones of current upstream. Site-keyed diff:

repo removed added retargeted confidence only
breakpad 0 30 5 66
protobuf 9 97 1 2
rocksdb 5 43 0 0
leveldb 0 4 0 1
glog (control) dump byte-identical
redis (C control) dump byte-identical

Triage (every removed and retargeted site by hand; every added and confidence-only site mechanically, by re-deriving the declaration the scan settles on, then reading each exception):

  • Removed, rocksdb 5, all wrong. const std::optional<Slice> start, end; in SubcompactionState: sub_compact->end.has_value() had gone to rocksdb's OptSlice::has_value by name. Now it is std::optional's own, as start already was.
  • Removed, protobuf 9. 5 were wrong: NestedTestAllTypes nested_src, nested_dst; → nested_dst.payload() had gone to MapFieldBase::payload. 4 were right by luck: single-candidate guesses of MessageLite::SerializeAsString from FileDescriptorProto file1, file2a, file2b; and proto2_unittest::Test…Lite message, message2;. Those classes aren't in the C++ index (build-time generated, or only the C#/PHP namesakes are indexed), so the declared-type rule declines to guess. The first names in the same lists (file1, message) are unresolved on main already, and the later names now match them.
  • Retargeted, breakpad 5, wrong → right. Overloads chosen by argument type: GetMemoryAtAddress(last_rbp, &caller_rbp) after uint64_t caller_rip, caller_rbp; now picks the uint64_t* overload, where main picked the uint8_t* one. The MIPS and x86 walkers' uint32_t caller_pc, caller_sp, caller_fp; likewise get the uint32_t* overload.
  • Retargeted, protobuf 1, wrong → right. RepeatedPtrField<std::string> source, destination; → destination.CopyFrom(source) was Field::CopyFrom, read from an older Field* destination in an earlier test; now RepeatedPtrField::CopyFrom.
  • Added. Typed calls on later names (WriteBatch b1, b2; → b2.Put, RepeatedField<int> source, destination;, Label a, b, c, d;) and inherited methods (breakpad's LoadedSection/SegmentLoadCommand → test_assembler::Section::Append). Also held types (std::unique_ptr<Message> msg(…), msg2(…); → msg2->GetReflection(); rocksdb's EnvMirror members std::unique_ptr<WritableFile> a_, b_;, where b_ now mirrors a_), chains (section2.address().Value(), largest.user_key().ToString()), and 3 calls reached through main's existing fallback past an unreadable multi-line auto (right target, Arena::Create<RepeatedPtrField<std::string>>). No wrong addition found.
  • Confidence only. Same target, a 0.4–0.7 guess now typed at 0.9 (breakpad Label locals, leveldb out.largest.Clear()).

Cost: while indexing protobuf, the new reader runs 874,954 times (251,021 past its comma prefilter) and takes 2.7 s in total across the 7 resolver threads, about 0.6% of the index's ~450 s CPU (an instrumented copy of the fix arm, whose graph is byte-identical to the fix arm's). Whole-index CPU runs, interleaved, are within this box's noise: main 418 s and 466 s, fix 478 s and 457 s.

Not done (measured)

  • A list continued on the next line (Foo a, then b;): 6 such declarations in the five repos, and none of their names is used as a call receiver.
  • A later name after *const or an annotation (, * PROTOBUF_NULLABLE b), where the declarator regex already matches, with the annotation read as the type: 0 occurrences in the five repos, other than 3 lines of inline asm in protobuf's upb headers.
  • Current rocksdb upstream has split almost all of its multi-name declarations, so its numbers are small. An earlier run of this change, on an older rocksdb clone and on main e47cb25, removed 14 wrong guesses (std::string rtype1, rtype2, row, val; → rtype2.c_str() had gone to gtest's FilePath::c_str) and added 79 right edges.

Overlap

None open. The C++ receiver PRs that landed since this was first written (#2493, #2536, #2540, #2555 and the member-chain series #2626–#2628) read one declarator per line, with qualifiers, over two lines, or in member chains. This change plugs into the same readers through the declarator match they share.

🤖 Generated with Claude Code

colbymchenry and others added 3 commits October 19, 2026 20:28
…found` in `ByteBuffer symbols_found, strings_found;`

C++ receiver inference read a declaration with a regex that wants the type
right before the name. A declaration that lists several names writes the type
once, before the first, so the later names were never read: google/breakpad's
`strings_found.Size()` after `ByteBuffer symbols_found, strings_found;`
linked nothing, and a later name could take the type of an earlier variable
of the same name further up.

matchCppListedDeclarator reads such a declaration from its start (specifiers,
the type with its template arguments paired, then each declarator with its
own pointer and reference marks, qualifiers, annotations, array bounds and
initializer) and gives a later name the declaration's type plus its own `*`s
and `&`s: `Foo *a, b;` declares one pointer. Names are told apart only by
commas at the statement's own depth, so template arguments, a call's or a
constructor's arguments, an initializer list or an expression list no name.
It runs only where the declarator regex finds nothing on a line, in all three
declaration readers: the receiver scan (and its header scan), a class's data
members (`Table *index_, *data_;`), and the check that a bare call names a
local.

A/B on main 036832d with current upstream clones: breakpad +30 added,
5 overload retargets wrong to right, 66 guesses now typed; protobuf +97,
1 retarget wrong to right, 9 removed (5 wrong, 4 single-candidate guesses on
message classes the C++ index lacks, now like the first name in their list);
rocksdb +43, 5 wrong removed; leveldb +4. glog and redis byte-identical.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@colbymchenry
colbymchenry merged commit 2043152 into main Oct 20, 2026
@colbymchenry
colbymchenry deleted the claude/nice-lalande-36c37c branch October 20, 2026 01:52
colbymchenry added a commit that referenced this pull request Oct 20, 2026
…othing, not a function elsewhere of its name (#2708)

A C++ call written by its bare name on a name the calling function binds
itself (a parameter, a lambda's parameter, a local lambda or functor, a
range-for variable) calls that local: C++ lookup finds it before any function
or method of that name. The name strategies linked such calls by name:
protocolbuffers/protobuf's code generators' `format(...)` on their
`Formatter& format` parameter went to the conformance suite's
`TestResult::format`, and facebook/rocksdb's tests' local
`auto key = [](int i) {...};` calls to `ArenaWrappedDBIter::key`.

isCppLocalCallableCall, checked in matchReference beside the JS/TS rule,
reads the caller's declarations of the name: its parameters, a declaration
that starts a statement (`auto x = ...`, `T x = ...`, `T x(args)`, a later
name in a list), a range-for variable or a lambda's parameter, the nearest one
in a block still open at the call deciding. A local type or a function the
body declares is not a local callable (the call constructs it or calls the
function), and an expression that reads like a declaration (`a && f(x)`)
declares nothing. A local's type may carry a function type in its template
arguments (`std::function<void()> done, failed;`), so the first name of such
a list counts as the later ones do, and the #2698 test of that list now
expects no edge for either. The looser isCppLocalName used by implicit-this
is unchanged.

A/B on main 5530676: protobuf 269 and rocksdb 51 edges removed, all calls
on locals, nothing else changed; glog, leveldb, breakpad and redis
byte-identical. The check costs 3.2 s of about 450 s CPU on protobuf.

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