diff --git a/CHANGELOG.md b/CHANGELOG.md index 4722c5fb64..4eb9e73572 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,7 +98,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - In C++, a call made with `->` on a variable, parameter or member declared as a `std::unique_ptr`, `std::shared_ptr` or `std::optional`, like `iter->Valid()` on a `std::unique_ptr iter`, now links to that method of the type it holds, or of a class that type inherits from, with the type looked up the way C++ does it, so a class of the same name nested elsewhere is not mistaken for it. Before, the call went to whichever project method shared its name: in facebook/rocksdb, `iter->Valid()` reached `ArenaWrappedDBIter::Valid` and `fs->GetFileSize(…)` on a `std::shared_ptr` reached a test's stand-in file system, and calls like `it->Next()` were often not linked at all. When what it holds is a template parameter or a standard-library type, like `std::unique_ptr>`, or a class that has no method of that name and neither do the classes it derives from, the call no longer links to a project method; a class derived from something that can't be followed, like a `decltype(…)`, may still have it, so that call is matched as before. Re-index C++ projects after upgrading. - In C++, a call a method makes with no receiver, or through `this->`, now links to the method C++ itself would call: one of the method's own class, else one the class inherits from a base class, else one of a class it is nested in. The same goes for a call in a lambda inside the method, and of a class's overloads the one the call's arguments fit is taken. Before, the call was matched by its name alone, so another class's method of that name could win because it was declared nearer or found first: in protocolbuffers/protobuf, every generated message's `Swap` and move assignment calling `InternalSwap(…)` reached `Any::InternalSwap`, and the `GetArena()` messages inherit from `MessageLite` reached an arena helper's `GetArena`, while googletest's `~linked_ptr() { depart(); }`, vendored in facebook/rocksdb, reached `linked_ptr_internal::depart`. Calls C++ doesn't look up in the class are matched as before: a free function's, a call to a parameter or local of that name, and a template's bare call into a base class that depends on its template parameters. Re-index C++ projects after upgrading. Thanks @danusha2345 for reporting the C++ case in #1952. - In C++, a call on the variable of a range-based `for` loop over `auto` elements, like `cfd->Ref()` inside facebook/rocksdb's `for (auto cfd : cfds)`, now links to the method of the type the loop goes over. That type is read from what the loop iterates: a `std::vector`, `std::set`, `absl::Span` or similar container, protocolbuffers/protobuf's `RepeatedPtrField` or RocksDB's `autovector`, whether a variable, parameter or member holds it, an `auto` variable is set to it or a function or method returns it, and a project class you loop over through its `begin()`, like RocksDB's `ColumnFamilySet`. Before, the call was matched by the variable's name, or by an earlier variable of that name: in RocksDB, `blob_file.GetBlobFileNumber()` over a list of `BlobFileGarbage` reached `BlobFileAddition::GetBlobFileNumber`, and in protobuf's Objective-C generator, calls over a list of `std::unique_ptr` reached other generators' methods. A call on an element of a standard-library type, like `file.find(…)` over a `std::vector`, no longer links to a project method, and `->` on an element held in a `std::unique_ptr` or `std::shared_ptr` reaches the type it holds. Re-index C++ projects after upgrading. -- In C++, a call on a variable, parameter or member declared as one of your classes, like `iter->Valid()` on an `Iterator* iter` or `it.Valid()` on a `const Iterator& it`, now links to that method of the class C++ itself would find for the type where the call is written, or of a class it inherits from. Before, the method was picked by the type's last name, so a class of the same name nested elsewhere could win: in facebook/rocksdb, calls on an `Iterator` reached `MemTableRep::Iterator`, which declares `Valid` and `Next` itself while the `Iterator` the code means inherits them, google/leveldb's reached `SkipList::Iterator`, and in protocolbuffers/protobuf the Java generator's `context->options()` reached the hpb generator's `Context` and `text_parser.Parse(…)` on a `TextFormat::Parser` reached the `.proto` parser. When that class and the classes it derives from have no method of that name, the call no longer links to an unrelated class's method that shares it, as protobuf's `field->full_name()` on a `FieldDescriptor*` did. A type is also found through a `using` declaration the file writes, like protobuf's `using google::protobuf::Descriptor;`, before a class of the same name declared at global scope elsewhere in the repository; this also applies to base classes and to the type a smart pointer holds. A type written from the global scope, like `::upb::Arena& arena`, is looked up there, so a namespace's own `upb::Arena` is not taken for it, nor is an alias of that name the calling function declares. Re-index C++ projects after upgrading. +- In C++, a call on a variable, parameter or member declared as one of your classes, like `iter->Valid()` on an `Iterator* iter` or `it.Valid()` on a `const Iterator& it`, now links to that method of the class C++ itself would find for the type where the call is written, or of a class it inherits from. Before, the method was picked by the type's last name, so a class of the same name nested elsewhere could win: in facebook/rocksdb, calls on an `Iterator` reached `MemTableRep::Iterator`, which declares `Valid` and `Next` itself while the `Iterator` the code means inherits them, google/leveldb's reached `SkipList::Iterator`, and in protocolbuffers/protobuf the Java generator's `context->options()` reached the hpb generator's `Context` and `text_parser.Parse(…)` on a `TextFormat::Parser` reached the `.proto` parser. When that class and the classes it derives from have no method of that name, the call no longer links to an unrelated class's method that shares it, as protobuf's `field->full_name()` on a `FieldDescriptor*` did. A type is also found through a `using` declaration the file writes, like protobuf's `using google::protobuf::Descriptor;`, before a class of the same name declared at global scope elsewhere in the repository; this also applies to base classes and to the type a smart pointer holds. A type written from the global scope, like `::upb::Arena& arena`, is looked up there, so a namespace's own `upb::Arena` is not taken for it, nor is an alias of that name the calling function declares. A type written as a member of a class template, like `SkipList::Iterator iter` in leveldb's and RocksDB's skip-list tests, is now read whole, so its calls reach the skip list's own iterator rather than the general `Iterator`, and RocksDB's skip-list memtable iterator no longer links its calls on the skip list's iterator to its own methods; a standard container's `iterator` written this way is the library's, never a class of yours named `iterator`. Re-index C++ projects after upgrading. - In C++, text inside a string literal is no longer taken for a variable's declaration. That covers the C code protocolbuffers/protobuf's upb generator writes in a raw string (`R"cc( … upb_MiniTableField field = $field_init$; … )cc"`) and log messages like facebook/rocksdb's `"… in WriteBatch handler; …"`. Before, a later call like `field.ctype()` on the function's own `upb::FieldDefPtr field` parameter, or `handler->PutCF(…)` on a `WriteBatch::Handler* handler`, was matched against the type that text seemed to name, so it linked to nothing or to another class's method. Re-index C++ projects after upgrading. - In C++, a call on an `auto` variable now links to the method of the class C++ finds for the type its initializer gives it, as a call on a variable declared with that type does. The type is read from a cast or smart pointer written out, like facebook/rocksdb's `auto iter = std::unique_ptr(…)` or `auto* mutex = static_cast(arg)`, from `new` or `std::make_unique` of a class, and from what a function or method is declared to return, like the `Iterator*` of `txn->GetIterator(…)`, read to the end of an initializer that goes on to the next line. Before, such a variable took the type of an earlier variable of the same name, or none: in RocksDB, `iter->SeekToFirst()` reached `MemTableRep::Iterator` and `mutex->Lock()` reached `port::Mutex`. A chained call like `cfd->current()->storage_info()` now looks up the class the inner call is declared to return the same way, through a smart pointer too, so protocolbuffers/protobuf's Java generator no longer reaches the C# generator's `EnumGenerator`. A type from outside the project no longer links to a class of the project that shares its name: protobuf's `status.ok()` on an `absl::Status` reached `upb::Status::ok`, and fmtlib/fmt's `os.flush()` on a `std::ostream&` reached fmt's own `ostream::flush`. The same holds for a variable declared in a macro body, or typed by what a function in another file returns, and for a class the index doesn't hold whose name only another class shares: protobuf's `_status.ok()` in its `RETURN_IF_ERROR` macro reached `upb::Status::ok`, and `range.options()` on its generated `DescriptorProto::ExtensionRange` reached `Descriptor::ExtensionRange::options`. A namespace a macro stands for is the project's under either name: facebook/rocksdb declares its classes in `namespace ROCKSDB_NAMESPACE` with `#define ROCKSDB_NAMESPACE rocksdb`, so `db->Put(…)` on a `rocksdb::DB*` links to RocksDB's own `DB::Put`. Re-index C++ projects after upgrading. - In C++, a call on a data member of the calling method's class, like `versions_->GetColumnFamilySet()` in facebook/rocksdb's `db_impl_write.cc`, now links to the method of the member's type wherever the class is declared. Before, only the call's own file and the header of the same name were read for the member, and RocksDB declares `DBImpl` in `db_impl.h` while its methods are spread over `db_impl_write.cc`, `db_impl_compaction_flush.cc`, `db_impl_open.cc` and more, so such calls were guessed by the member's name. A member declared below an inline method that uses it, one a base class declares, and, in a gtest `TEST_F` or `TEST_P` body, one of the test's fixture, like RocksDB's `db_`, are found the same way, and a member of a class nested in the caller's class is no longer taken for the caller's own. A call on a member of a standard-library type, like `std::atomic shutting_down_`, no longer links to a project method that only shares the method's name. Re-index C++ projects after upgrading. diff --git a/__tests__/cpp-template-member-types.test.ts b/__tests__/cpp-template-member-types.test.ts new file mode 100644 index 0000000000..0682b3e035 --- /dev/null +++ b/__tests__/cpp-template-member-types.test.ts @@ -0,0 +1,221 @@ +/** + * A C++ receiver declared as a member type of a class template, like + * google/leveldb's and facebook/rocksdb's `SkipList::Iterator iter(&list);`. The declaration was read from its + * last name alone, `Iterator`, which C++ lookup from the calling function + * then found in the enclosing namespace: leveldb's skip-list tests linked + * `iter.Valid()` to `leveldb::Iterator::Valid`, and rocksdb's to + * `IteratorBase::Valid`, instead of the skip list's own iterator. The type is + * now read whole, and the member found in the class template it is written in. + */ +import { describe, it, expect, afterEach } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { CodeGraph } from '../src'; +import type { Node } from '../src/types'; + +const roots: string[] = []; +afterEach(() => { + for (const root of roots.splice(0)) fs.rmSync(root, { recursive: true, force: true }); +}); + +async function indexed(files: Record): Promise { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-cpp-template-member-')); + roots.push(root); + for (const [rel, content] of Object.entries(files)) { + fs.mkdirSync(path.dirname(path.join(root, rel)), { recursive: true }); + fs.writeFileSync(path.join(root, rel), content); + } + return CodeGraph.init(root, { index: true }); +} + +function callable(cg: CodeGraph, qualifiedName: string): Node { + const node = [...cg.getNodesByKind('method'), ...cg.getNodesByKind('function')].find((n) => n.qualifiedName === qualifiedName); + if (!node) throw new Error(`no function or method ${qualifiedName}`); + return node; +} + +/** `calls` callees of a function or method, as `qualifiedName`, without constructors. */ +function calls(cg: CodeGraph, caller: string): string[] { + return cg + .getCallees(callable(cg, caller).id) + .filter((r) => r.edge.kind === 'calls' && !/::(\w+)::\1$/.test(r.node.qualifiedName)) + .map((r) => r.node.qualifiedName) + .sort(); +} + +const lines = (...l: string[]): string => [...l, ''].join('\n'); + +const SKIPLIST = { + 'include/iterator.h': lines( + 'namespace leveldb {', + 'class Iterator {', + ' public:', + ' virtual ~Iterator() {}', + ' virtual bool Valid() const { return false; }', + ' virtual void SeekToFirst() {}', + '};', + '} // namespace leveldb', + ), + 'db/skiplist.h': lines( + '#include "include/iterator.h"', + 'namespace leveldb {', + 'template ', + 'class SkipList {', + ' public:', + ' class Iterator {', + ' public:', + ' explicit Iterator(const SkipList* list) : list_(list) {}', + ' bool Valid() const { return true; }', + ' void SeekToFirst() {}', + ' private:', + ' const SkipList* list_;', + ' };', + '};', + '} // namespace leveldb', + ), +}; + +describe('a C++ receiver declared as a member type of a class template', () => { + it('reproduction: `SkipList::Iterator iter` is the skip list\'s iterator', async () => { + const cg = await indexed({ + ...SKIPLIST, + 'db/skiplist_test.cc': lines( + '#include "db/skiplist.h"', + 'namespace leveldb {', + 'typedef unsigned long Key;', + 'struct Comparator {};', + 'void Empty() {', + ' SkipList list;', + ' SkipList::Iterator iter(&list);', + ' if (!iter.Valid()) iter.SeekToFirst();', + '}', + 'void Split() {', + ' SkipList list;', + ' SkipList::Iterator', + ' iter(&list);', + ' iter.Valid();', + '}', + '} // namespace leveldb', + ), + }); + try { + expect(calls(cg, 'leveldb::Empty')).toEqual(['leveldb::SkipList::Iterator::SeekToFirst', 'leveldb::SkipList::Iterator::Valid']); + expect(calls(cg, 'leveldb::Split')).toEqual(['leveldb::SkipList::Iterator::Valid']); + } finally { + cg.close(); + } + }); + + it('written with `typename` in a template, and through a pointer', async () => { + const cg = await indexed({ + ...SKIPLIST, + 'db/concurrent_test.cc': lines( + '#include "db/skiplist.h"', + 'namespace leveldb {', + 'template ', + 'class ConcurrentTest {', + ' public:', + ' void ReadStep() {', + ' typename SkipList::Iterator iter(&list_);', + ' iter.SeekToFirst();', + ' }', + ' bool Check(typename SkipList::Iterator* it) {', + ' return it->Valid();', + ' }', + ' private:', + ' SkipList list_;', + '};', + '} // namespace leveldb', + ), + }); + try { + expect(calls(cg, 'leveldb::ConcurrentTest::ReadStep')).toEqual(['leveldb::SkipList::Iterator::SeekToFirst']); + expect(calls(cg, 'leveldb::ConcurrentTest::Check')).toEqual(['leveldb::SkipList::Iterator::Valid']); + } finally { + cg.close(); + } + }); + + it('a standard container\'s iterator is the library\'s, not a project class named `iterator`', async () => { + const cg = await indexed({ + 'util/ring.h': lines( + 'class Ring {', + ' public:', + ' class iterator {', + ' public:', + ' int base() const { return 0; }', + ' };', + '};', + ), + 'src/touch.cc': lines( + '#include ', + '#include "util/ring.h"', + 'void Touch(std::vector::iterator it) {', + ' it.base();', + '}', + ), + }); + try { + expect(calls(cg, 'Touch')).toEqual([]); + } finally { + cg.close(); + } + }); + + it('a library template\'s member that names another type may be one of the project\'s', async () => { + const cg = await indexed({ + 'include/slice.h': lines( + 'class Slice {', + ' public:', + ' int size() const { return 0; }', + '};', + ), + 'src/keys.cc': lines( + '#include ', + '#include "include/slice.h"', + 'int First(const std::vector& keys) {', + ' std::vector::value_type key = keys[0];', + ' return key.size();', + '}', + ), + }); + try { + expect(calls(cg, 'First')).toEqual(['Slice::size']); + } finally { + cg.close(); + } + }); + + it('a member alias of a class template is the class it names', async () => { + const cg = await indexed({ + 'cache/clock_cache.h': lines( + 'class Shard {', + ' public:', + ' int GetTable() const { return 0; }', + '};', + 'namespace cache {', + 'template ', + 'class ClockCacheShard {', + ' public:', + ' int GetTable() const { return 1; }', + '};', + 'template ', + 'class BaseHyperClockCache {', + ' public:', + ' using Shard = ClockCacheShard;', + ' int Report(const BaseHyperClockCache
::Shard* shard) const {', + ' return shard->GetTable();', + ' }', + '};', + '} // namespace cache', + ), + }); + try { + expect(calls(cg, 'cache::BaseHyperClockCache::Report')).toEqual(['cache::ClockCacheShard::GetTable']); + } finally { + cg.close(); + } + }); +}); diff --git a/src/resolution/name-matcher.ts b/src/resolution/name-matcher.ts index 02f3266170..2726405ffa 100644 --- a/src/resolution/name-matcher.ts +++ b/src/resolution/name-matcher.ts @@ -9189,12 +9189,14 @@ const CPP_FUNCTION_WRAPPER_TYPE = String.raw`(?:::\s*)?(?:std\s*::\s*(?:function const CPP_TEMPLATE_ARGUMENTS = String.raw`<(?:[^<>;=(){}]|<(?:[^<>;=(){}]|<[^<>;=(){}]*>)*>)*>`; /** * A declared type as the declarator regex reads it, what it carries included: - * `const FieldDescriptor* PROTOBUF_NULLABLE*`, and a leading `::` that names - * the global scope (`::upb::Arena arena`), which C++ lookup starts from. A - * `::` after a name, a template's arguments or a `)` is no such `::`: in - * `SkipList::Iterator iter`, `Iterator` is SkipList's. - */ -const CPP_DECLARATOR_TYPE = String.raw`(?:${CPP_FUNCTION_WRAPPER_TYPE}|(?:(?):])::\s*)?[A-Za-z_][\w:]*(?:\s*${CPP_TEMPLATE_ARGUMENTS})?)(?:\s*(?:${CPP_DECLARATOR_CV}|[*&]+(?:\s*${CPP_DECLARATOR_ANNOTATION})*))*`; + * `const FieldDescriptor* PROTOBUF_NULLABLE*`, a leading `::` that names the + * global scope (`::upb::Arena arena`), which C++ lookup starts from, and the + * class template a member type is written in: `SkipList::Iterator iter` is SkipList's `Iterator`, which read from its + * `Iterator` alone was the namespace's own. (A `::` after a name, a + * template's arguments or a `)` is no leading `::`.) + */ +const CPP_DECLARATOR_TYPE = String.raw`(?:${CPP_FUNCTION_WRAPPER_TYPE}|(?:(?):])::\s*)?[A-Za-z_][\w:]*(?:\s*${CPP_TEMPLATE_ARGUMENTS}(?:\s*::\s*[A-Za-z_][\w:]*)?)?)(?:\s*(?:${CPP_DECLARATOR_CV}|[*&]+(?:\s*${CPP_DECLARATOR_ANNOTATION})*))*`; /** A line that is a declared type and nothing else, what may come before a type included: `const std::map&`. */ const CPP_TYPE_LINE = new RegExp(String.raw`^\s*(?:(?:const|volatile|static|mutable|constexpr|inline)\s+)*${CPP_DECLARATOR_TYPE}\s*$`); /** An annotation macro's call after a declared name: leveldb's `VersionSet* const versions_ GUARDED_BY(mutex_);`. */ @@ -10197,7 +10199,10 @@ function cppTypesNamed(name: string, context: ResolutionContext): Node[] { * `_t` name has no members of its own: it is a scalar (`std::size_t`, so * the declaration is another variable's), a tag, or a trait that names * another type (rocksdb's `std::conditional_t - * dict`). + * dict`). So is a member of a template's specialization other than an + * iterator: `std::vector::value_type` is a `Slice`, and googletest's + * `typename std::tuple_element::type matcher` one of its + * matcher classes. */ function isCppLibraryType(raw: string, context: ResolutionContext): boolean { if ((raw.match(//g)?.length ?? 0)) return false; @@ -10205,6 +10210,7 @@ function isCppLibraryType(raw: string, context: ResolutionContext): boolean { if (!segments || segments.length < 2) return false; const name = segments[segments.length - 1]!; if (!/^[a-z]/.test(name) || /_t$/.test(name)) return false; + if (!CPP_ITERATOR_NAME.test(name) && isCppSpecializationMember(raw)) return false; if (segments.slice(0, -1).some((s) => cppTypesNamed(s, context).length > 0)) return false; const declared = cppTypesNamed(name, context); if (segments[0] !== 'std') return declared.length === 0 || cppFindsOnlyNamesakes(raw, context); @@ -10212,6 +10218,22 @@ function isCppLibraryType(raw: string, context: ResolutionContext): boolean { return !declared.some((n) => n.qualifiedName === spelled || n.qualifiedName.endsWith(`::${spelled}`)); } +/** A container's iterator type: `iterator`, `const_reverse_iterator`, `local_iterator`. */ +const CPP_ITERATOR_NAME = /^(?:const_)?(?:reverse_|local_)?iterator$/; + +/** + * Is a C++ type written as a member of a template's specialization: + * `std::vector::iterator`, `SkipList::Iterator`? + */ +function isCppSpecializationMember(raw: string): boolean { + let depth = 0; + for (let i = 0; i < raw.length; i++) { + if (raw[i] === '<') depth++; + else if (raw[i] === '>' && --depth === 0 && /^\s*::/.test(raw.slice(i + 1))) return true; + } + return false; +} + /** Library types whose `->` reaches the value they hold. */ const CPP_HOLDERS: ReadonlySet = new Set(['std::unique_ptr', 'std::shared_ptr', 'std::optional', 'absl::optional', 'absl::StatusOr', 'absl::NoDestructor']);