mirror of
https://github.com/colbymchenry/codegraph.git
synced 2026-10-24 11:24:49 +08:00
fix(cpp): -> on a value of one of the project's classes follows its operator->, like rocksdb's FileSystemPtr and MultiGetContext::Range::Iterator (#2735)
C++ reads `x->m` on a class value as `x.operator->()->m`, and applies `->` again to a class it returns. Only a library holder's `->` and a container iterator's (`std::vector<T>::iterator`) were read. A call through a project iterator or smart-pointer wrapper was guessed by the method's name: - rocksdb's `fs_->NewWritableFile(…)` on a `FileSystemPtr`, whose `operator->` returns a `std::shared_ptr<FileSystem>`, reached `FaultInjectionTestFS`; - `writable_file_->Truncate(…)` on an `FSWritableFilePtr` reached the legacy `WritableFile::Truncate`; - `iter->lkey->user_key()` through `MultiGetContext::Range::Iterator`, whose `KeyContext* operator->()` is a `KeyContext*`, reached `BlobCountingIterator::user_key`. Both a member chain's `->` and a `->` call on a declared receiver end in cppIteratorTarget. For a type that is no container's iterator, it now falls back to cppOperatorTarget: - what the class's `operator->` returns, through the pointer it is, or with `->` applied again to a class it is: a library holder, or another of these; - else what its `operator*` returns; - the return type is read where the class declares it, and the class's own template parameters are bound to the type's written arguments (cppTemplateBound): `UnownedPtr<DB>`'s `T* operator->()` is a `DB*`; - a parameter the type doesn't bind names no particular class. A log-only census on mainbdf288a7found 304 rocksdb and 42 protobuf dereferences of a project class with an `operator->` that gave no element. Among the rocksdb calls, 77 were bare-name guesses and 56 were unlinked. Kit A/B againstbdf288a7: | repo | removed | added | retargeted | same target, now typed | |---|---|---|---|---| | rocksdb | 2 | 49 | 158 | 89 | | protobuf | 0 | 0 | 7 | 3 | | leveldb, fmt, redis | identical | | | | - Retargeted: - FaultInjectionTestFS → FileSystem (57), WritableFile → FSWritableFile (32), and TruncatedRangeDelIterator through UnownedPtr (16); - LookupKey and GetContext through MultiGetContext's iterator; - SequentialFile and RandomAccessFile → FSSequentialFile and FSRandomAccessFile; - protobuf's `map_field_->` calls from WeakRepeatedPtrField to the map field bases; - overloads (DB::Put, PinnableSlice) chosen on the now-known class. - Removed: `iter->timestamp->assign(…)` on a `std::string*`, which had reached `autovector::assign`. - Added: Status::PermitUncheckedError, FileSystem::OptimizeFor…, Cleanable::RegisterCleanup, and gtest's ParamGeneratorInterface through linked_ptr. - About 40 changed sites read by hand were all right. Mutations: dropping the fallback, the operator* fallback, the second `->` or the template binding each fail the new tests. Accepting an inexact class, found only as the sole one of its name, survives; the guard stays, as in the member-chain matcher. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Colby Mchenry
co-authored by
Claude Opus 5.5
Claude Opus 5.5
parent
4e35e0cb78
commit
0c28480576
@@ -132,6 +132,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
|
||||
- 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<bool> 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> 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 `->` on a value of one of your own classes now follows that class's `operator->`, or its `operator*`, as C++ does, so calls through your iterators and smart-pointer wrappers link to the right classes. facebook/rocksdb's `fs_->NewWritableFile(…)` on a `FileSystemPtr` now reaches `FileSystem::NewWritableFile` instead of a test file system's, and `iter->lkey->user_key()` through a `MultiGetContext::Range::Iterator` reaches `LookupKey::user_key`. A class template's `T* operator->()` is read for the type it's given, like `UnownedPtr<DB>`'s. 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<Iterator*()>`, 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<Any>`, 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.
|
||||
|
||||
@@ -0,0 +1,156 @@
|
||||
/**
|
||||
* A C++ `->` on a value of one of the project's classes calls that class's
|
||||
* `operator->`, and the member is looked up on what it returns: rocksdb's
|
||||
* `MultiGetContext::Range::Iterator` declares `KeyContext* operator->()`, its
|
||||
* `UnownedPtr<T>` a `T* operator->()`, and its `FileSystemPtr` returns a
|
||||
* `std::shared_ptr<FileSystem>`, which `->` is applied to again. Only a
|
||||
* library holder's or container iterator's `->` was read, so such calls were
|
||||
* guessed by the method's name: `iter->lkey->user_key()` reached
|
||||
* `BlobCountingIterator::user_key`.
|
||||
*/
|
||||
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++ `->` through a project class's operator->", () => {
|
||||
let root: string;
|
||||
let cg: CodeGraph;
|
||||
|
||||
beforeAll(async () => {
|
||||
root = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-cpp-operator-arrow-'));
|
||||
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('src/types.h', lines(
|
||||
'#include <memory>',
|
||||
'namespace ns {',
|
||||
'class LookupKey {',
|
||||
' public:',
|
||||
' int user_key() const { return 0; }',
|
||||
'};',
|
||||
'struct KeyContext {',
|
||||
' LookupKey* k;',
|
||||
'};',
|
||||
'class Range {',
|
||||
' public:',
|
||||
' class Iterator {',
|
||||
' public:',
|
||||
' KeyContext* operator->() const { return ctx_; }',
|
||||
' KeyContext& operator*() const { return *ctx_; }',
|
||||
' private:',
|
||||
' KeyContext* ctx_;',
|
||||
' };',
|
||||
' Iterator begin() const { return Iterator(); }',
|
||||
'};',
|
||||
'class Cursor {',
|
||||
' public:',
|
||||
' KeyContext& operator*() const { return *ctx_; }',
|
||||
' private:',
|
||||
' KeyContext* ctx_;',
|
||||
'};',
|
||||
'template <typename T>',
|
||||
'class UnownedPtr {',
|
||||
' public:',
|
||||
' T* operator->() const { return p_; }',
|
||||
' private:',
|
||||
' T* p_;',
|
||||
'};',
|
||||
'class DB {',
|
||||
' public:',
|
||||
' void Flush() {}',
|
||||
'};',
|
||||
'class Handle {',
|
||||
' public:',
|
||||
' UnownedPtr<DB> operator->() const { return p_; }',
|
||||
' private:',
|
||||
' UnownedPtr<DB> p_;',
|
||||
'};',
|
||||
'class FileSystem {',
|
||||
' public:',
|
||||
' void Sync() {}',
|
||||
'};',
|
||||
'class FileSystemPtr {',
|
||||
' public:',
|
||||
' const std::shared_ptr<FileSystem>& operator->() const { return fs_; }',
|
||||
' private:',
|
||||
' std::shared_ptr<FileSystem> fs_;',
|
||||
'};',
|
||||
'} // namespace ns',
|
||||
));
|
||||
write('src/use.cc', lines(
|
||||
'#include "src/types.h"',
|
||||
'namespace ns {',
|
||||
// Each namesake sits where a call matched by its bare name lands on it.
|
||||
'class KeyNamesake {',
|
||||
' public:',
|
||||
' int user_key() const { return 1; }',
|
||||
'};',
|
||||
'class FlushNamesake {',
|
||||
' public:',
|
||||
' void Flush() {}',
|
||||
'};',
|
||||
'class SyncNamesake {',
|
||||
' public:',
|
||||
' void Sync() {}',
|
||||
'};',
|
||||
'int Chain(const Range& range) {',
|
||||
' auto i = range.begin();',
|
||||
' return i->k->user_key();',
|
||||
'}',
|
||||
'int Star(const Cursor& c) {',
|
||||
' auto& kc = *c;',
|
||||
' return kc.k->user_key();',
|
||||
'}',
|
||||
'void Template(UnownedPtr<DB> h) { h->Flush(); }',
|
||||
'void Holder(FileSystemPtr h) { h->Sync(); }',
|
||||
'void Twice(Handle h) { h->Flush(); }',
|
||||
'template <typename T>',
|
||||
'void Unbound(UnownedPtr<T> p) { p->Flush(); }',
|
||||
'} // 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 `qualifiedName`, as `qualifiedName`. */
|
||||
const calls = (qualifiedName: string): string[] => {
|
||||
const caller = cg.getNodesByKind('function').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)
|
||||
.filter((q) => !q.endsWith('::begin')).sort();
|
||||
};
|
||||
|
||||
it("a chain through an iterator class's operator->", () => {
|
||||
expect(calls('ns::Chain')).toEqual(['ns::LookupKey::user_key']);
|
||||
});
|
||||
|
||||
it("a chain through a class's operator*, when it has no operator->", () => {
|
||||
expect(calls('ns::Star')).toEqual(['ns::LookupKey::user_key']);
|
||||
});
|
||||
|
||||
it("a class template's operator-> returning its parameter, bound by the written argument", () => {
|
||||
expect(calls('ns::Template')).toEqual(['ns::DB::Flush']);
|
||||
});
|
||||
|
||||
it('an operator-> returning a library holder is applied again', () => {
|
||||
expect(calls('ns::Holder')).toEqual(['ns::FileSystem::Sync']);
|
||||
});
|
||||
|
||||
it('a class an operator-> returns has its own operator-> applied', () => {
|
||||
expect(calls('ns::Twice')).toEqual(['ns::DB::Flush']);
|
||||
});
|
||||
|
||||
it("a parameter the written type doesn't bind names no particular class", () => {
|
||||
expect(calls('ns::Unbound')).toEqual([]);
|
||||
});
|
||||
});
|
||||
@@ -14610,16 +14610,70 @@ function cppArrowTarget(
|
||||
|
||||
/**
|
||||
* The element a container's iterator of the type `iterator` points at, read
|
||||
* from the container's template arguments (cppArrowTarget). Null for any
|
||||
* other type, and for an element a class template's own `begin()` or
|
||||
* from the container's template arguments (cppArrowTarget), or what `->` on
|
||||
* a value of one of the project's classes reaches (cppOperatorTarget). Null
|
||||
* for any other type, and for an element a class template's own `begin()` or
|
||||
* `operator*` writes with its template parameter.
|
||||
*/
|
||||
function cppIteratorTarget(iterator: CppWrittenType, at: UnresolvedRef, context: ResolutionContext): CppWrittenType | null {
|
||||
const container = CPP_ITERATOR_OF.exec(cppWithoutTypeKeywords(iterator.text))?.[1];
|
||||
const element = container ? cppRangeElementOf({ ...iterator, text: container }, at, context, 1) : null;
|
||||
const element = container ? cppRangeElementOf({ ...iterator, text: container }, at, context, 1) : cppOperatorTarget(iterator, at, context);
|
||||
return element && (element.scope === iterator.scope || !isCppTemplateParameterType(element)) ? element : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* What `->` on a value of one of the project's classes of the type `type`
|
||||
* reaches, as C++ reads `x->m`: what the class's `operator->` returns,
|
||||
* through the pointer it is, or with `->` applied again to a class it is (a
|
||||
* library holder's held type, or another of these); else what its
|
||||
* `operator*` returns. The return type is read where the class declares it,
|
||||
* with the class's own template parameters bound to the arguments `type` is
|
||||
* written with (cppTemplateBound). rocksdb's `MultiGetContext::Range::Iterator`
|
||||
* declares `KeyContext* operator->()`, its `UnownedPtr<DB>` a `T*
|
||||
* operator->()`, a `DB*`, and its `FileSystemPtr` returns a
|
||||
* `std::shared_ptr<FileSystem>`. Null when the class declares neither, or
|
||||
* what it returns can't be read.
|
||||
*/
|
||||
function cppOperatorTarget(type: CppWrittenType, at: UnresolvedRef, context: ResolutionContext, hops = 0): CppWrittenType | null {
|
||||
const written = cppWithoutTypeKeywords(type.text);
|
||||
if (hops > 2 || !written || isCppPointerType(written) || isCppLibraryType(written, context)) return null;
|
||||
const found = cppClassWritten(written, type, at, context);
|
||||
if (!found?.exact || !CPP_CLASS_KINDS.has(found.node.kind)) return null;
|
||||
const arrow = cppMethodsOf(found.node, 'operator->', at, context);
|
||||
const returned = cppReturnTypeOf(arrow.length > 0 ? arrow : cppMethodsOf(found.node, 'operator*', at, context), context);
|
||||
const bound = returned && cppTemplateBound(returned, found.node, { ...type, text: written }, at, context);
|
||||
if (!bound) return null;
|
||||
if (arrow.length === 0) return { ...bound, text: bound.text.replace(/&+\s*$/, '').trim() };
|
||||
return cppPointee(bound) ?? cppOperatorTarget(bound, at, context, hops + 1);
|
||||
}
|
||||
|
||||
/**
|
||||
* The type `returned`, which the class template `cls` writes, with `cls`'s
|
||||
* own template parameters bound to the arguments of `type`, a specialization
|
||||
* of it: the `T*` of `UnownedPtr<T>` is a `DB*` for an `UnownedPtr<DB>`. An
|
||||
* alias the class declares (`pointer`) is followed first, and an argument is
|
||||
* looked up where `type` is written. A type of a class that is no template
|
||||
* comes back as written; null for a parameter `type` doesn't bind.
|
||||
*/
|
||||
function cppTemplateBound(returned: CppWrittenType, cls: Node, type: CppWrittenType, at: UnresolvedRef, context: ResolutionContext): CppWrittenType | null {
|
||||
const params = cppOwnTemplateParameters(cls, context);
|
||||
if (params.length === 0) return returned;
|
||||
const args = cppTemplateArguments(type.text) ?? [];
|
||||
const followed = cppFollowWrittenAliases(returned, at, context) ?? returned;
|
||||
const core = followed.text.replace(/\b(?:const|volatile|typename)\b|[*&]/g, ' ').trim();
|
||||
const index = params.indexOf(core);
|
||||
if (index >= 0) {
|
||||
const arg = args[index]?.trim();
|
||||
return arg ? { ...type, text: followed.text.replace(new RegExp(`\\b${core}\\b`), arg) } : null;
|
||||
}
|
||||
let text = followed.text;
|
||||
params.forEach((param, i) => {
|
||||
const arg = args[i]?.trim();
|
||||
if (param && arg) text = text.replace(new RegExp(`\\b${param}\\b`, 'g'), arg);
|
||||
});
|
||||
return { ...followed, text };
|
||||
}
|
||||
|
||||
/**
|
||||
* What dereferencing a value of the type `type` gives, for `*x` and `x->`:
|
||||
* an array's first element, what a pointer points at or a holder holds
|
||||
|
||||
Reference in New Issue
Block a user