diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f129ff26..a4a95ca46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -131,6 +131,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - 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. A call on a member reached through other members now links the same way, like RocksDB's `thread->rand.Next()` on a `Random64` or `meta->largest.user_key()` on an `InternalKey`. Before, it went to whichever class had a method of that name, like `ArenaWrappedDBIter::Next` or `ClippingIterator::user_key`, or, inside a method, to that method's own class. When the member's class has no such method, the call no longer links to another class's. The member is read from the class the calling file sees: RocksDB's `db_bench_tool.cc` and its stress tool each declare their own `ThreadState`, whose `rand` is a `Random64` in one and a `Random` in the other. It is also found in the classes that class derives from. RocksDB's `Options` inherits its members from `DBOptions` and `ColumnFamilyOptions`, so `options.statistics->getTickerCount(…)` now reaches `Statistics`, and `options.table_factory.reset(…)` on a `std::shared_ptr` no longer links to a project method of that name. Re-index C++ projects after upgrading. - In C++, a member declared inside an anonymous union or struct is now found as a member of the class around it, as C++ looks it up. Every protocolbuffers/protobuf generated message keeps its fields in `union { Impl_ _impl_; };`, so calls like `_impl_.name_.Set(…)` now link to `ArenaStringPtr::Set`. Before, they were guessed by the method's name, and `this_._impl_.type_url_.Destroy()` linked to `Arena::Destroy`. The same goes for fmtlib/fmt's `value`, which keeps its argument in a union. Re-index C++ projects after upgrading. +- In C++, a type a header names for a class its source file defines, like the `Rep` of a pimpl's `std::unique_ptr rep_` or `Rep* rep_`, is now found for the code in that source file, so calls through it link to the right classes. facebook/rocksdb's `rep->write_batch.Put(…)` in `WriteBatchWithIndex` now reaches `WriteBatch::Put`, google/leveldb's `rep_->index_block->NewIterator(…)` reaches `Block::NewIterator` instead of the calling `Table`'s own, and googletest's `impl()->total_test_count()` reaches `UnitTestImpl`. A call on a standard-library member of such a class, like protocolbuffers/protobuf's `tables_->known_bad_files_.clear()`, no longer links to a project method of that name. Re-index C++ projects after upgrading. - In C++, a variable, parameter or member whose declaration puts a qualifier or an annotation around its name is now read for its type, so calls on it link to the declared class's methods: a `const` after the `*` or `&`, like facebook/rocksdb's `Iterator* const iter = iter_guard.get();` or google/leveldb's `Env* const env_;`, a type written before its `const`, like `Slice const& key`, a nullability annotation after the `*`, like protocolbuffers/protobuf's `io::EpsCopyOutputStream* PROTOBUF_NONNULL stream`, and a thread-safety annotation after the name, like LevelDB's `MemTable* imm_ GUARDED_BY(mutex_);`. Before, such a declaration was skipped, or the annotation was taken for its type, so the call took the type of an earlier variable of the same name or was guessed by the method's name: RocksDB's `iter->SeekToFirst()` reached the unrelated `ArenaWrappedDBIter`, and LevelDB's `env_->NewWritableFile(…)` reached `InMemoryEnv`. Re-index C++ projects after upgrading. - In C++, a call on a name a structured binding declares now links to the method of the type C++ gives it. That covers a range-based `for` over pairs or a map, like facebook/rocksdb's `for (auto& [cfh, iter] : cfh_iter_pairs_) { iter->Valid(); }`, and a declaration like protocolbuffers/protobuf's `auto [dst_ext, is_new] = Insert(…);`. A call on what a `std::function` or another function wrapper returns, like `create_iterator()->Valid()` on a `std::function`, links the same way, and so does a call on a class member whose type is written on the line above its name. Before, these calls were guessed by the method's name, and calls on a library container declared that way, like a `std::priority_queue` member's `pop()`, reached a project class's method of that name. Re-index C++ projects after upgrading. - In C++, a call written without a receiver in a gtest `TEST_F` or `TEST_P` body now links to the test fixture's method, its own or one it inherits, as C++ finds it: facebook/rocksdb's tests call `DBTestBase`'s `Put(…)`, `Flush()` and `NewIterator(…)` this way. A variable such a call initializes, like `auto iter = NewIterator(…)`, takes the type the method returns, so `iter->SeekToFirst()` reaches the iterator class. Before, those calls were guessed by name and often reached a namesake in another class, like RocksDB's `ColumnFamilyTestBase::Put` or protocolbuffers/protobuf's `EncodeDecodeTest::ExpectNoErrors`, and `iter->SeekToFirst()` reached the unrelated `MemTableRep::Iterator`. A `->` call on a value of a type from outside the project, like protobuf's `m->value()` on an `absl::StatusOr`, no longer links to a class of the project that shares the type's name, like protobuf's own `hpb::StatusOr`. Re-index C++ projects after upgrading. diff --git a/__tests__/cpp-pimpl-member-chains.test.ts b/__tests__/cpp-pimpl-member-chains.test.ts new file mode 100644 index 000000000..e8a2736f3 --- /dev/null +++ b/__tests__/cpp-pimpl-member-chains.test.ts @@ -0,0 +1,115 @@ +/** + * A C++ call through a member of a pimpl's `Rep` — rocksdb's + * `rep_->index_builder->IndexSize()`, leveldb's `rep_->options.comparator` — + * calls a method of that member's class. The class declares `struct Rep;` and + * `std::unique_ptr rep_;` (or `Rep* rep_;`) in its header, and the source + * file that calls through it defines `struct Table::Rep { … }`. The member's + * type was looked up from the header, which sees no source file's classes, so + * the call was guessed by the method's name. + */ +import { describe, it, expect, beforeAll, afterAll } 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 lines = (...l: string[]): string => [...l, ''].join('\n'); + +describe('a C++ call through a member of a pimpl Rep', () => { + let root: string; + let cg: CodeGraph; + + beforeAll(async () => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-cpp-pimpl-')); + const write = (file: string, text: string): void => { + fs.mkdirSync(path.dirname(path.join(root, file)), { recursive: true }); + fs.writeFileSync(path.join(root, file), text); + }; + write('include/table.h', lines( + '#include ', + 'namespace ns {', + 'class Table {', + ' public:', + ' int Size();', + ' void Flush();', + ' private:', + ' struct Rep;', + ' std::unique_ptr rep_;', + '};', + 'class Builder {', + ' public:', + ' int Count();', + ' private:', + ' struct Rep;', + ' Rep* rep_;', + '};', + '} // namespace ns', + )); + write('src/table.cc', lines( + '#include "include/table.h"', + 'namespace ns {', + 'class Block {', + ' public:', + ' int size() const { return 1; }', + ' void Flush() {}', + '};', + 'struct Table::Rep {', + ' Block block;', + ' Block* index;', + '};', + // Each namesake sits where a call matched by its bare name lands on it. + 'class Columns {', + ' public:', + ' int size() const { return 0; }', + ' void Flush() {}', + '};', + 'int Table::Size() { return rep_->block.size(); }', + 'void Table::Flush() { rep_->index->Flush(); }', + '} // namespace ns', + )); + write('src/builder.cc', lines( + '#include "include/table.h"', + 'namespace ns {', + 'class Counter {', + ' public:', + ' int size() const { return 2; }', + '};', + 'struct Builder::Rep {', + ' Counter counter;', + '};', + 'class Tally {', + ' public:', + ' int size() const { return 3; }', + '};', + 'int Builder::Count() { return rep_->counter.size(); }', + '} // namespace ns', + )); + cg = await CodeGraph.init(root, { index: true }); + }, 120_000); + + afterAll(() => { + cg?.close(); + if (root) fs.rmSync(root, { recursive: true, force: true }); + }); + + /** `calls` callees of the function or method `qualifiedName`, as `qualifiedName`. */ + const calls = (qualifiedName: string): string[] => { + const caller = [...cg.getNodesByKind('function'), ...cg.getNodesByKind('method')] + .find((n: Node) => n.qualifiedName === qualifiedName); + expect(caller, qualifiedName).toBeDefined(); + return cg.getCallees(caller!.id).filter((r) => r.edge.kind === 'calls').map((r) => r.node.qualifiedName).sort(); + }; + + it("a member of the Rep a smart pointer holds, defined in the calling file", () => { + expect(calls('ns::Table::Size')).toEqual(['ns::Block::size']); + }); + + it("a pointer member of the Rep", () => { + expect(calls('ns::Table::Flush')).toEqual(['ns::Block::Flush']); + }); + + it('a raw Rep pointer', () => { + expect(calls('ns::Builder::Count')).toEqual(['ns::Counter::size']); + }); +}); diff --git a/src/resolution/cpp-supertypes.ts b/src/resolution/cpp-supertypes.ts index e3ec8e470..349831548 100644 --- a/src/resolution/cpp-supertypes.ts +++ b/src/resolution/cpp-supertypes.ts @@ -29,7 +29,11 @@ * (`fmt::detail::buffer` under fmt's FMT_BEGIN_NAMESPACE); * - a class a source file (`.cc`, `.cpp`, `.c`) defines belongs to that * translation unit, so only that file sees it (leveldb's skiplist test - * declares its own `leveldb::Comparator`); + * declares its own `leveldb::Comparator`) — and so does a name a header + * writes, read for code in that file, which includes the header: a + * pimpl's `std::unique_ptr rep_`, whose `Table::Rep` the source file + * calling through it defines (leveldb's `Table`, rocksdb's + * `BlockBasedTableBuilder`); * - a template parameter (`template class X : public Base`) or * a member of one (`public T::Base`) is whatever a template argument makes * it: no edge. @@ -76,14 +80,15 @@ function isCFamily(n: Node): boolean { } /** - * The declarations code at `site` can see (visibleFrom), where a class local - * to a function counts only in that function's lines: the index names it - * after the function, and every gtest `TEST_F` body in a namespace is - * `ns::TEST_F`, so one body's local `Listener` passed for any other's — in - * the body itself, and in the methods of a local class of its own. + * The declarations code at `site` can see (visibleFrom), read for code in + * the file `unit`, where a class local to a function counts only in that + * function's lines: the index names it after the function, and every gtest + * `TEST_F` body in a namespace is `ns::TEST_F`, so one body's local + * `Listener` passed for any other's — in the body itself, and in the methods + * of a local class of its own. */ -function seenFrom(site: CppNameSite, context: ResolutionContext): (n: Node) => boolean { - const visible = visibleFrom(site.file); +function seenFrom(site: CppNameSite, context: ResolutionContext, unit?: string): (n: Node) => boolean { + const visible = visibleFrom(site.file, unit); return (n) => { if (!visible(n)) return false; const fn = cppFunctionAround(n, context); @@ -111,9 +116,12 @@ function cppFunctionAround(cls: Node, context: ResolutionContext): Node | null { return fn; } -/** The declarations code in `file` can see: a header's, and the file's own. */ -function visibleFrom(file: string): (n: Node) => boolean { - return (n) => n.filePath === file || !SOURCE_FILE.test(n.filePath); +/** + * The declarations code in `file` can see: a header's, and the file's own — + * and, read for code in the file `unit` (one that includes `file`), `unit`'s. + */ +function visibleFrom(file: string, unit?: string): (n: Node) => boolean { + return (n) => n.filePath === file || n.filePath === unit || !SOURCE_FILE.test(n.filePath); } export function matchCppSupertype(ref: UnresolvedRef, context: ResolutionContext): ResolvedRef | null { @@ -176,13 +184,14 @@ export function cppClassWritten(written: string, site: CppNameSite, ref: Unresol const global = written.startsWith('::'); if (!global && site.parameters.has(segments[0]!)) return null; - const visible = seenFrom(site, context); + // (For code in the reference's own file: the file it is in includes the one that writes the name.) + const visible = seenFrom(site, context, ref.filePath); const scopes = cppScopesWithin(site.scope); if (site.classScope) scopes.splice(1, 0, site.classScope); const resolved = (node: Node): CppWrittenClass => ({ node, exact: true }); // The scopes around the name short of the global one, or only the global one for `::Base`. - const lexical = classNode(cppClassNamed(written, global ? [''] : scopes.slice(0, -1), ref, context, visible), site, context); + const lexical = classNode(cppClassNamed(written, global ? [''] : scopes.slice(0, -1), ref, context, visible), site, context, visible); if (lexical !== undefined) return lexical && resolved(lexical); // What the file writes before the name, the nearest first: each stands for @@ -193,7 +202,7 @@ export function cppClassWritten(written: string, site: CppNameSite, ref: Unresol const usings = usingsIn(site.file, context).filter((u) => u.line < site.line).reverse(); const lookUp = (spelled: string): Node | undefined => { const absolute = spelled.startsWith('::'); - return classNode(cppClassNamed(spelled, absolute ? [''] : scopes, ref, context, visible), site, context) ?? undefined; + return classNode(cppClassNamed(spelled, absolute ? [''] : scopes, ref, context, visible), site, context, visible) ?? undefined; }; // `::_pbi::MessageGlobalsBase` under `namespace _pbi = ::google::protobuf::internal;`. if (segments.length > 1) { @@ -216,7 +225,7 @@ export function cppClassWritten(written: string, site: CppNameSite, ref: Unresol if (hit) return resolved(hit); } // The global scope. - const outermost = classNode(cppClassNamed(written, [''], ref, context, visible), site, context); + const outermost = classNode(cppClassNamed(written, [''], ref, context, visible), site, context, visible); if (outermost !== undefined) return outermost && resolved(outermost); } // `fmt::detail::buffer`, where a macro opens `namespace fmt` around `detail::buffer`. @@ -267,10 +276,11 @@ function writtenBase(ref: UnresolvedRef, context: ResolutionContext): string { } /** - * The node of the class a lookup found, as the file that writes the name sees - * it (null: the name names no particular class; undefined: keep looking). + * The node of the class a lookup found, among the declarations the lookup can + * see (`visible`; null: the name names no particular class; undefined: keep + * looking). */ -function classNode(named: CppNamedClass, site: CppNameSite, context: ResolutionContext): Node | null | undefined { +function classNode(named: CppNamedClass, site: CppNameSite, context: ResolutionContext, visible: (n: Node) => boolean): Node | null | undefined { if (named === null || named === undefined) return named; if ('alias' in named) return named.alias; const classes = context @@ -278,8 +288,8 @@ function classNode(named: CppNamedClass, site: CppNameSite, context: ResolutionC .filter((n) => isCFamily(n) && CLASS_KINDS.has(n.kind) && n.id !== site.self?.id); // A lookup that ended on the class itself (its namespace lost) found nothing. if (classes.length === 0) return undefined; - const visible = classes.filter(seenFrom(site, context)); - return pick(visible.length > 0 ? visible : classes, site.file); + const shown = classes.filter(visible); + return pick(shown.length > 0 ? shown : classes, site.file); } /**