fix(cpp): a range-based for loop's variable has the type the loop declares

Merged
CO
8d ago
+558-3

Summary

C++ receiver inference now reads a range-based for loop's declaration, for (ConformanceTestSuite *suite : suites), for (const Foo& x : xs), for (Foo* x : xs). Before, it never did. buildDeclaratorRegex takes a declared name only when ;, =, ,, ), [, { or ( follows it. A loop variable is followed by its :, so x->Method() / x.Method() fell through to Strategy 3's guess by the receiver's name, or the scan read some other declaration of the same name further up.

The motivating case is protocolbuffers/protobuf's conformance runner (conformance/conformance_test_runner.cc, lines 219–291):

  • On main (ed199e60, with #2421). 13 calls in two for (ConformanceTestSuite *suite : suites) loops stay unresolved: suite->SetVerbose(verbose), suite->RunSuite(runner, &output, …), suite->GetFailureListFlagName() and 10 more. inferCppReceiverType returns null. The guess drops ConformanceTestSuite because conformance_test.h is a test path, and the guess's own test filter is stricter than #2421's include rule.
  • Now. All 13 reach ConformanceTestSuite's methods through the typed path, at 0.9.

A small fixture can hide the bug. With a decoy class, the guess picks the right method by word overlap, at 0.65 or 0.7. So the new tests check the edge's confidence, not only its target.

The rule

  • What is read. A line the declarator regex doesn't match is read as a range-based for when its for ( header declares the receiver before a single : (not ::). The declared type is taken the same way the declarator regex takes it (CPP_DECLARED_TYPE_TAIL), so normalization, alias following (#2399) and #2413's std-type gate all apply as they do to any other declaration. A line the declarator regex does match is read exactly as before.
  • Scope. The loop's variable counts only when the call is inside the loop's body (cppForBodyEncloses). That means past the header, and up to the } that closes a braced body or the end of a single-statement body. After the loop, or in a later function, the same name is another variable, and the scan goes on as before. The body is found by a small lexer that skips comments and string and character literals. A for inside a literal (commented-out code, a code generator's template string) has no body, so it declares nothing. Without this check, protobuf/rocksdb's field, cfd and file loop variables would type same-named receivers in later functions.
  • auto. A loop over auto elements has no initializer to read. The scan goes on, as it does for an auto local whose initializer can't be read, and #2413's shadowed mark is set as before.
  • Bit-fields and the scope operator. Neither is ever read as a loop's declaration. Only a for ( header is read, and only its single :. unsigned car : 1; declares nothing new (and no method is called on a bit-field), nor does void parts::Install().
  • The header scan (the same-stem .h read top-down for members) is unchanged: a loop in another file never encloses the call.

Validation

Arms are 7a796541 (main, after #2413, #2421, #2426, #2449, #2443, #2445, #2446 and #2434) and this branch. The changed-site list is identical, edge for edge, to the run on 3de5bef0 before the last four C++ PRs landed. Both were built from the same tree with the same native kernel and indexed with codegraph init -y on this Windows box. Edges are diffed per call site (kind, source node, line, col, refName), and every removed, added and retargeted site was read against the source.

I first ran the A/B on ed199e60. Re-running it after #2449 (C++ attribute macros) left rocksdb, leveldb and fmt unchanged. On protobuf:

  • three find removals dropped out, because main no longer guesses Map::find there;
  • two right adds appeared: FindAllFileNames, and Any::UnpackTo, now that Any is a class.
reporemovedaddedretargetedsame edge, new metadata
protocolbuffers/protobuf10 (all wrong guesses)26 (25 right, 1 wrong)3 (wrong → right)24 (confidence up to 0.9)
facebook/rocksdb25 (all wrong guesses)38 (all right)18 (wrong → right)24 (16 up to 0.9; 8 down 0.9 → 0.8, same target)
google/leveldb, fmtlib/fmt0000 (dumps byte-identical)

protobuf.

  • Added, right (25):
    • the 13 conformance runner calls;
    • upb::MessageDefPtr / EnumDefPtr / FieldDefPtr::full_name in the Rust generator's for (upb::MessageDefPtr m : …) loops;
    • source->FindAllExtensionNumbers / FindAllFileNames on for (DescriptorDatabase* source : sources_);
    • prototype->GetDescriptor(), ->New() and the chained ->GetDescriptor()->FindFieldByName(…) on for (const Message* prototype : …);
    • the chained option.value().UnpackTo(…) → Any::UnpackTo on for (const Option& option : options).
  • Added, wrong (1): file->name() in for (const FileDescriptor* file : parsed_files) (command_line_interface.cc:3037) now goes to CodeGeneratorResponse_File::name at 0.65.
    • The loop is now read correctly, but FileDescriptor::name is a macro-generated accessor that isn't indexed. FileDescriptor is a project class, so the existing fallback guesses, exactly as it does for a local const FileDescriptor* file = …; today.
    • On main the call had no edge only by accident. The scan read an unrelated const FileDescriptorProto& file 33 lines up, in another block. FileDescriptorProto isn't indexed as a class, so it looks like an outside type.
  • Retargeted (3):
    • value->name() on for (const EnumValueDescriptor* value : canonical_values_): from EnumValue::name to EnumValueDescriptor::name (×2);
    • source->FindFileByName on a DescriptorDatabase* loop variable: from SourceTreeDescriptorDatabase::FindFileByName to the declared DescriptorDatabase::FindFileByName.
  • Removed (10), all wrong guesses:
    • file_descriptor.name() / file_proto.name() / descriptor.name() on FileDescriptorProto / DescriptorProto loop variables. These had gone to EnumValueDescriptor::name, CodeGeneratorResponse_File::name and internal::DescriptorNames::name. Those classes aren't indexed as classes, so they read as outside types.
    • message->name() on a MessageAccessInfo (a generated message not in the repo), which had gone to upb::MessageDefPtr::name.
    • field->is_extension() through using Field = const FieldDescriptor*, which had gone to upb::FieldDefPtr.
    • #2413's std-type gate, now that the declaration is read: fragment.remove_prefix(…) on absl::string_view, which had gone to LeftoverBuffer::remove_prefix.

rocksdb.

  • Added (38), all right. Examples:
    • entry->key() / Free on for (LRUHandle* entry : …);
    • cfd->GetName() / current()->storage_info() on ColumnFamilyData*;
    • log->file()->Sync(…) / Close / SyncWithoutFlush / reset_seen_error through log::Writer::file();
    • ag.column_family() / columns() on AttributeGroup;
    • Slice / PinnableSlice / Status / WideColumn methods;
    • be->AsBackupEngine().
  • Retargeted (18), all wrong → right. Examples:
    • log->file() / get_log_number() on for (log::Writer* log : wals_to_sync): from BlobLogWriter to log::Writer (12 sites);
    • value.Reset() on PinnableSlice&: from DBIter::ValueColumnsState::Reset;
    • column.name() on const WideColumn&: from LazyWideColumn::name;
    • child_iter->SetPinnedItersMgr(…) on InternalIterator* / ForwardLevelIterator*: from BlockIter's override to the declared types' methods.
  • Removed (25), all wrong guesses:
    • 7 op.type() on the fuzzers' const DBOperation& (a generated proto), which had gone to gtest's TestPartResult::type;
    • 18 from #2413's gate on std::string / std::shared_ptr loop variables: file.c_str() → gtest's FilePath::c_str, fname.find(…) → toku::omt::find, cache.get() → BaseCacheInterface::get.
  • Same target, confidence 0.9 → 0.8 (8). The loop variable is a smart pointer: for (const std::shared_ptr<Cache>& cache : …) (7) and for (UnownedPtr<Stats> stats : stats_). -> through a smart pointer isn't followed to its element type (the follow-up #2413 left), so these now resolve by the receiver's capitalized name at 0.8. On main, 0.9 came from reading some other cache / stats declaration.

The scope check on real code. An instrumented copy logged every range-for the scan met that declares the receiver's name, and whether its body encloses the call.

  • protobuf: 1,746 such loops, of which 683 enclose the call;
  • rocksdb: 3,837, of which 1,297 enclose it;
  • most loops judged not enclosing are auto loops in other functions.

Every typed "not enclosed" verdict within 60 lines of the call, and every "enclosed" verdict more than 60 lines from it, was read against the source, and all were right. The instrumented runs' dumps are byte-identical to the fix arm's.

Cost. Measured with resolution on the main thread (CODEGRAPH_NO_PARALLEL_RESOLVE=1), using instrumented copies of both arms with hrtime counters, on a box at 100% CPU from other work. Both instrumented runs' dumps are byte-identical to the plain arms'.

reporeceiver inferencestime inferring (main / this branch)range-for checkloop-body check
rocksdb112,21051.4 s / 47.1 s2.18 M lines, 598 ms4,284 loops, 50 ms
protobuf52,48719.8 s / 20.5 s647 k lines, 226 ms1,870 loops, 12 ms

The new code is about 1–1.5% of the time spent inferring receivers, and the hrtime wrapper is a large share of that. Most lines return at the for substring check. The main/branch gap is noise: it goes opposite ways on the two repos.

Tests

New __tests__/cpp-range-for-receiver.test.ts (9 tests):

  • On main, 6 fail:
    • protobuf's runner shape, with a decoy SetVerbose the guess avoids: the calls resolve, but at 0.65 / 0.7, not 0.9;
    • a loop variable named like a member of its class: main reads the member's type;
    • after the loop and in a later function, a same-named receiver is the other variable: main gives both calls the outer type;
    • reference, pointer, * on the name, global-scope :: and a header split over two lines;
    • a brace in a comment or literal inside the body;
    • a call inside a nested classic for.
  • 3 guards pass on main and here: a for in a comment or string literal; a bit-field above a member of the same name; a name followed by ::.
  • Mutation check. Each guard fails when its rule is broken:
    • accepting any loop above the call;
    • a lexer that ignores literals, or comments;
    • a braceless body that never ends;
    • adding : (or :(?!:)) to the declarator regex's lookahead instead.
  • Existing tests. The C++ suites pass (1,098 tests across 15 files, including #2413's, #2421's and #2426's), on the native kernel (CODEGRAPH_KERNEL_EXPECT=1).
  • Full suite. The parallel run on this box (100% CPU from other sessions) reported 109 of 6,675 tests failing in 50 files. Of those, 105 were timeouts, 2 were EBUSY-style filesystem errors, and 2 were daemon/writer-lock lifecycle assertions. A serial rerun of those 50 files with 120 s timeouts passed 1,685 of their 1,702 tests and failed 2. Both pass when run alone:
    • function-ref.test.ts's #1820 shadow case, which took 44 s of its own 60 s limit;
    • mcp-writer-lock.test.ts's two-proxies case.

Not in this PR

  • auto loops (for (const auto& x : xs)) still fall back. Typing them means reading the range's element type: from the container's declaration, or from a call's return type. That is roughly 1,570 calls in protobuf and rocksdb alone.
  • -> through a smart-pointer loop variable reaches the element type only by the receiver's name. This is the same gap #2413 left for any declaration.

🤖 Generated with Claude Code