Repository navigation
fix(cpp): a range-based for loop's variable has the type the loop declares - #2439
Open
colbymchenry wants to merge 3 commits into
Open
colbymchenry wants to merge 3 commits into
colbymchenry wants to merge 3 commits into
Conversation
…lares
C++ receiver inference read a declaration only when the declared name was
followed by `;`, `=`, `,`, `)`, `[`, `{` or `(`, so a range-based for's
`Type name :` was never read. In protocolbuffers/protobuf's conformance
runner, `suite->SetVerbose(…)`, `suite->RunSuite(…)` and
`suite->GetFailureListFlagName()` inside `for (ConformanceTestSuite *suite :
suites)` fell through to Strategy 3's guess by the receiver's name, and in
RocksDB `log->file()` on a `log::Writer` loop variable went to
`BlobLogWriter::file`.
A line the declarator regex doesn't match is now read as a range-based for
when its `for (` header declares the receiver before a single `:` (not
`::`), and the loop's variable is taken only when the call is inside the
loop's body: past the header, up to the `}` closing a braced body or the end
of a single-statement body, with comments and literals skipped. After the
loop, or in a later function, the same name is another variable, and the
scan goes on as before. A loop over `auto` elements has nothing to read and
keeps the existing fallback. Bit-fields and the scope operator are never
read as a loop's declaration.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-696753 # Conflicts: # CHANGELOG.md # src/resolution/name-matcher.ts
…s not end the loop Receiver inference now reads comment lines blanked (#2413), so the commented-out loops in the range-for tests no longer reach the loop-body scan; this case keeps its own comment and literal handling covered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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.
Summary
C++ receiver inference now reads a range-based
forloop's declaration,for (ConformanceTestSuite *suite : suites),for (const Foo& x : xs),for (Foo* x : xs). Before, it never did.buildDeclaratorRegextakes a declared name only when;,=,,,),[,{or(follows it. A loop variable is followed by its:, sox->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):ed199e60, with fix(c,cpp): a file named like a test is visible to the code that includes it #2421). 13 calls in twofor (ConformanceTestSuite *suite : suites)loops stay unresolved:suite->SetVerbose(verbose),suite->RunSuite(runner, &output, …),suite->GetFailureListFlagName()and 10 more.inferCppReceiverTypereturns null. The guess dropsConformanceTestSuitebecauseconformance_test.his a test path, and the guess's own test filter is stricter than fix(c,cpp): a file named like a test is visible to the code that includes it #2421's include rule.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
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 (fix(cpp): a receiver declared through a typedef or using alias calls the type it names #2399) and fix(cpp): a receiver declared as a std or other outside type with a lowercase name calls that type's own member #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.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. Aforinside a literal (commented-out code, a code generator's template string) has no body, so it declares nothing. Without this check, protobuf/rocksdb'sfield,cfdandfileloop variables would type same-named receivers in later functions.auto. A loop overautoelements has no initializer to read. The scan goes on, as it does for anautolocal whose initializer can't be read, and fix(cpp): a receiver declared as a std or other outside type with a lowercase name calls that type's own member #2413'sshadowedmark is set as before.for (header is read, and only its single:.unsigned car : 1;declares nothing new (and no method is called on a bit-field), nor doesvoid parts::Install()..hread 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 on3de5bef0before the last four C++ PRs landed. Both were built from the same tree with the same native kernel and indexed withcodegraph init -yon 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:findremovals dropped out, because main no longer guessesMap::findthere;FindAllFileNames, andAny::UnpackTo, now thatAnyis a class.protobuf.
upb::MessageDefPtr/EnumDefPtr/FieldDefPtr::full_namein the Rust generator'sfor (upb::MessageDefPtr m : …)loops;source->FindAllExtensionNumbers/FindAllFileNamesonfor (DescriptorDatabase* source : sources_);prototype->GetDescriptor(),->New()and the chained->GetDescriptor()->FindFieldByName(…)onfor (const Message* prototype : …);option.value().UnpackTo(…)→Any::UnpackToonfor (const Option& option : options).file->name()infor (const FileDescriptor* file : parsed_files)(command_line_interface.cc:3037) now goes toCodeGeneratorResponse_File::nameat 0.65.FileDescriptor::nameis a macro-generated accessor that isn't indexed.FileDescriptoris a project class, so the existing fallback guesses, exactly as it does for a localconst FileDescriptor* file = …;today.const FileDescriptorProto& file33 lines up, in another block.FileDescriptorProtoisn't indexed as a class, so it looks like an outside type.value->name()onfor (const EnumValueDescriptor* value : canonical_values_): fromEnumValue::nametoEnumValueDescriptor::name(×2);source->FindFileByNameon aDescriptorDatabase*loop variable: fromSourceTreeDescriptorDatabase::FindFileByNameto the declaredDescriptorDatabase::FindFileByName.file_descriptor.name()/file_proto.name()/descriptor.name()onFileDescriptorProto/DescriptorProtoloop variables. These had gone toEnumValueDescriptor::name,CodeGeneratorResponse_File::nameandinternal::DescriptorNames::name. Those classes aren't indexed as classes, so they read as outside types.message->name()on aMessageAccessInfo(a generated message not in the repo), which had gone toupb::MessageDefPtr::name.field->is_extension()throughusing Field = const FieldDescriptor*, which had gone toupb::FieldDefPtr.fragment.remove_prefix(…)onabsl::string_view, which had gone toLeftoverBuffer::remove_prefix.rocksdb.
entry->key()/Freeonfor (LRUHandle* entry : …);cfd->GetName()/current()->storage_info()onColumnFamilyData*;log->file()->Sync(…)/Close/SyncWithoutFlush/reset_seen_errorthroughlog::Writer::file();ag.column_family()/columns()onAttributeGroup;Slice/PinnableSlice/Status/WideColumnmethods;be->AsBackupEngine().log->file()/get_log_number()onfor (log::Writer* log : wals_to_sync): fromBlobLogWritertolog::Writer(12 sites);value.Reset()onPinnableSlice&: fromDBIter::ValueColumnsState::Reset;column.name()onconst WideColumn&: fromLazyWideColumn::name;child_iter->SetPinnedItersMgr(…)onInternalIterator*/ForwardLevelIterator*: fromBlockIter's override to the declared types' methods.op.type()on the fuzzers'const DBOperation&(a generated proto), which had gone to gtest'sTestPartResult::type;std::string/std::shared_ptrloop variables:file.c_str()→ gtest'sFilePath::c_str,fname.find(…)→toku::omt::find,cache.get()→BaseCacheInterface::get.for (const std::shared_ptr<Cache>& cache : …)(7) andfor (UnownedPtr<Stats> stats : stats_).->through a smart pointer isn't followed to its element type (the follow-up fix(cpp): a receiver declared as a std or other outside type with a lowercase name calls that type's own member #2413 left), so these now resolve by the receiver's capitalized name at 0.8. On main, 0.9 came from reading some othercache/statsdeclaration.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.
autoloops 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'.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
forsubstring 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):SetVerbosethe guess avoids: the calls resolve, but at 0.65 / 0.7, not 0.9;*on the name, global-scope::and a header split over two lines;for.forin a comment or string literal; a bit-field above a member of the same name; a name followed by::.:(or:(?!:)) to the declarator regex's lookahead instead.CODEGRAPH_KERNEL_EXPECT=1).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 Caller/impact graph misses methods passed as first-class references (callbacks), e.g. executor.submit(obj.method, …) #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
autoloops (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 fix(cpp): a receiver declared as a std or other outside type with a lowercase name calls that type's own member #2413 left for any declaration.🤖 Generated with Claude Code