diff --git a/CHANGELOG.md b/CHANGELOG.md index e91ee9e03..0cfc4e2e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - A file named the way Google names its tests, like `wire_format_unittest.cc` in protocolbuffers/protobuf, `logging_unittest.cc` in google/glog or a Python `foo_unittest.py`, now counts as a test, as `wire_format_test.cc` already did. Before, it was taken for production code: search and `codegraph_explore` ranked it alongside the code it tests, `codegraph affected` never listed it, and calls in production code could link to a class or type a unittest declares for itself, as google/breakpad's `stack_frame_entries_.size()` did to a unittest's `StackHelper`. Re-index projects that have such files after upgrading. - In C, C++ and Objective-C, a struct or class member declared as a pointer, reference, array or function, like `SharedState* shared;`, `jv elements[];` or `virtual Status Put(…) = 0;`, no longer makes the struct look like it inherits from the member's type. Before, `codegraph_explore` and impact listed those types as base classes, every struct holding a pointer to a type as one of its subclasses, and a method of that struct as an override of the type's method with the same name. Real base classes like `class Derived : public Base`, Objective-C superclasses and Go's embedded fields are unchanged. Re-index C, C++ and Objective-C projects after upgrading. - In C++, a call on a variable, parameter or member declared through a `typedef` or `using` alias now links to the method of the type the alias names, with the alias looked up the way C++ does it: in the calling function, its class and the classes that class inherits from, then the namespaces around it. In google/leveldb, `table_.Insert(buf)` on a `Table table_;` declared next to `typedef SkipList Table;` now reaches `SkipList::Insert` instead of the unrelated `HandleTable::Insert`, and `Table::Iterator iter(&table_);` reaches `SkipList::Iterator`'s constructor. A class that only shares the alias's name is no longer taken for it, and when the aliased type has no such method, as with an alias of `std::vector` or of a template parameter, no method is guessed from the receiver's name. Re-index C++ projects after upgrading. +- In C++, a call on a variable, parameter or member declared with template arguments nested inside its template arguments, like `autovector>` or `Striped>`, now links to the method of the class it is declared as. Before, the nested arguments hid that class: in RocksDB, `files_marked_for_compaction_.clear()` on such an `autovector` linked to an unrelated `CompactionInputFiles::clear` that only shares words with the variable's name, and `mutex_.Get(key)` on a `Striped` linked to nothing. Re-index C++ projects after upgrading. - In C++, a base class like `Message` in `class DynamicMessage final : public Message` now links to the class C++ itself would pick: the one declared in the namespaces around the class, or brought in by the file's `using` declarations and namespace aliases, and the class a `typedef` or `using` alias names. Before, a base class was matched by its name alone, so it could land on a class of the same name in another namespace or another language: in protocolbuffers/protobuf, `DynamicMessage` derived from the JSON parser's internal `Message` class, and in google/leveldb and RocksDB, iterators derived from an unrelated nested `Iterator` class such as the skip list's. The wrong base showed up in `codegraph_explore`'s type hierarchy and in impact, and tied the base's methods to the wrong overrides. A template parameter used as a base no longer links to a class that shares its name, and a base written from the global scope, like `public ::testing::Test`, now links to its class. Re-index C++ projects after upgrading. - In C and C++, a header or source file named like a test, such as protocolbuffers/protobuf's `conformance_test.h` and `test_runner.h`, now counts as part of the code that `#include`s it, directly or through other headers. Before, calls from that code into it were dropped, so protobuf's conformance suites' `suite_.ReportFailure(…)` and `RunValidInputTest(…)` calls, and its unit tests' calls to the shared `TestUtil` helpers, linked to nothing. A test file that only tests include is still kept out of reach of the rest of your code. Re-index C and C++ projects after upgrading. - In C++, a class, struct or method now keeps the namespaces and the class it is declared in when the parser misreads something earlier in the file, such as an unknown macro in front of a member or in a class header. Before, everything after the misread code could lose them, or land inside a class it isn't in: in protocolbuffers/protobuf, `FieldDescriptor` was indexed without `google::protobuf::`, and in RocksDB, the `Opts` struct declared inside a cache table class was indexed outside it, so finding a class's base classes and the methods called on its objects fell back to guessing by name. Methods that had come loose from their class are its members again, and classes the misread code had hidden, like leveldb's POSIX file and environment classes, are now indexed. Re-index C++ projects after upgrading. diff --git a/__tests__/cpp-nested-template-receiver.test.ts b/__tests__/cpp-nested-template-receiver.test.ts new file mode 100644 index 000000000..b6e3e9ac9 --- /dev/null +++ b/__tests__/cpp-nested-template-receiver.test.ts @@ -0,0 +1,278 @@ +/** + * A C++ receiver declared with template arguments nested in its template + * arguments calls its own class's method. + * + * Receiver inference reduced a declared type to its last name by cutting each + * `<…>` at its first `>`, so `autovector>` came + * out as `autovector >` and `Striped>` as + * `Striped >` — names no class has. facebook/rocksdb's + * `files_marked_for_compaction_.clear()` on such an `autovector` was then + * guessed by the receiver's words to be `CompactionInputFiles::clear`, and + * `mutex_.Get(key)` on a `Striped` got no edge at all: a capitalized type no + * class declares is taken to be from outside the project. Template arguments + * now go with the ones nested in them, so both calls reach their own class. + */ +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 { stripCppTemplateArguments } from '../src/resolution/cpp-type-aliases'; +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-nested-')); + 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 (file)`. */ +function calls(cg: CodeGraph, caller: string): string[] { + return cg + .getCallees(callable(cg, caller).id) + .filter((r) => r.edge.kind === 'calls') + .map((r) => `${r.node.qualifiedName} (${r.node.filePath})`) + .sort(); +} + +/** `calls` callees with how each was resolved: `qualifiedName resolvedBy@confidence`. */ +function resolvedCalls(cg: CodeGraph, caller: string): string[] { + return cg + .getCallees(callable(cg, caller).id) + .filter((r) => r.edge.kind === 'calls') + .map((r) => `${r.node.qualifiedName} ${String(r.edge.metadata?.resolvedBy)}@${String(r.edge.metadata?.confidence)}`) + .sort(); +} + +/** facebook/rocksdb's shapes, trimmed: the small vector, and the class whose words the receivers share. */ +const ROCKSDB = { + 'util/autovector.h': [ + 'namespace rocksdb {', + 'template ', + 'class autovector {', + ' public:', + ' size_t size() const { return 0; }', + ' void clear() {}', + ' void push_back(const T& item) {}', + '};', + '} // namespace rocksdb', + '', + ].join('\n'), + 'db/compaction/compaction.h': [ + 'namespace rocksdb {', + 'struct FileMetaData {};', + 'struct CompactionInputFiles {', + ' size_t size() const { return 0; }', + ' void clear() {}', + '};', + '} // namespace rocksdb', + '', + ].join('\n'), + 'db/version_set.h': [ + '#include "util/autovector.h"', + '#include "db/compaction/compaction.h"', + 'namespace rocksdb {', + 'class VersionEdit {};', + 'class VersionStorageInfo {', + ' public:', + ' void ComputeFilesMarkedForCompaction();', + ' size_t CountEditLists(VersionEdit* edit);', + ' private:', + ' autovector> files_marked_for_compaction_;', + '};', + '} // namespace rocksdb', + '', + ].join('\n'), + 'db/version_set.cc': [ + '#include "db/version_set.h"', + 'namespace rocksdb {', + 'void VersionStorageInfo::ComputeFilesMarkedForCompaction() {', + ' files_marked_for_compaction_.clear();', + '}', + 'size_t VersionStorageInfo::CountEditLists(VersionEdit* edit) {', + ' autovector> edit_lists;', + ' edit_lists.push_back(autovector());', + ' return edit_lists.size();', + '}', + '} // namespace rocksdb', + '', + ].join('\n'), +}; + +describe('a C++ receiver declared with nested template arguments', () => { + it('reproduction: a member of the class reaches the class\'s own method, not one named after the receiver\'s words', async () => { + const cg = await indexed(ROCKSDB); + try { + expect(calls(cg, 'rocksdb::VersionStorageInfo::ComputeFilesMarkedForCompaction')).toEqual([ + 'rocksdb::autovector::clear (util/autovector.h)', + ]); + expect(cg.getCallers(callable(cg, 'rocksdb::CompactionInputFiles::clear').id).filter((r) => r.edge.kind === 'calls')).toEqual([]); + } finally { + cg.close(); + } + }); + + it('a local of a class template nested in its own template arguments', async () => { + const cg = await indexed(ROCKSDB); + try { + // Both calls are the declared type's. `size` has two owners and the + // receiver's words name neither, so no guess reached it; the one + // `push_back` was a guess for being the only method of its name. + expect(resolvedCalls(cg, 'rocksdb::VersionStorageInfo::CountEditLists')).toEqual([ + 'rocksdb::autovector::push_back instance-method@0.9', + 'rocksdb::autovector::size instance-method@0.9', + ]); + } finally { + cg.close(); + } + }); + + it('a capitalized class template is the project\'s own, not a type from outside it', async () => { + const cg = await indexed({ + 'util/mutexlock.h': [ + 'namespace rocksdb {', + 'namespace port {', + 'class Mutex {', + ' public:', + ' void Lock() {}', + '};', + '} // namespace port', + 'template ', + 'struct CacheAlignedWrapper {', + ' T obj_;', + '};', + 'template ', + 'class Striped {', + ' public:', + ' T& Get(const Key& key) { return *data_; }', + ' private:', + ' T* data_;', + '};', + '} // namespace rocksdb', + '', + ].join('\n'), + 'db/blob/blob_file_cache.h': [ + '#include "util/mutexlock.h"', + 'namespace rocksdb {', + 'class BlobFileCache {', + ' public:', + ' void Evict(int key);', + ' private:', + ' Striped> mutex_;', + '};', + '} // namespace rocksdb', + '', + ].join('\n'), + 'db/blob/blob_file_cache.cc': [ + '#include "db/blob/blob_file_cache.h"', + 'namespace rocksdb {', + 'void BlobFileCache::Evict(int key) {', + ' mutex_.Get(key);', + '}', + '} // namespace rocksdb', + '', + ].join('\n'), + }); + try { + expect(calls(cg, 'rocksdb::BlobFileCache::Evict')).toEqual(['rocksdb::Striped::Get (util/mutexlock.h)']); + } finally { + cg.close(); + } + }); + + it('the end of a declaration begun on the line above names no type, not even its template argument\'s', async () => { + // rocksdb's blob_file_reader.h breaks `autovector>>& blob_reqs` over two lines. The second + // line alone, read as a type, is a `CachableEntry` here: the receiver's + // type is the `autovector` on the line above. + const cg = await indexed({ + ...ROCKSDB, + 'table/block_based/cachable_entry.h': [ + 'namespace rocksdb {', + 'template ', + 'class CachableEntry {', + ' public:', + ' size_t size() const { return 0; }', + '};', + '} // namespace rocksdb', + '', + ].join('\n'), + 'db/blob/blob_file_reader.cc': [ + '#include "util/autovector.h"', + '#include "table/block_based/cachable_entry.h"', + 'namespace rocksdb {', + 'struct BlobContents {};', + 'struct BlobReadRequest {};', + 'size_t MultiGetBlob(', + ' autovector>>& blob_reqs) {', + ' return blob_reqs.size();', + '}', + '} // namespace rocksdb', + '', + ].join('\n'), + }); + try { + expect(calls(cg, 'rocksdb::MultiGetBlob')).toEqual([]); + } finally { + cg.close(); + } + }); + + it('a declaration begun on the line above still hides a member of the same name', async () => { + // The parameter's type is not on the line that names it, but the + // parameter is still what the call is on: the class's `std::string + // blob_reqs` member it hides says nothing about the call. + const cg = await indexed({ + 'util/autovector.h': ROCKSDB['util/autovector.h'], + 'db/blob/blob_file_reader.cc': [ + '#include ', + '#include "util/autovector.h"', + 'namespace rocksdb {', + 'struct BlobContents {};', + 'class BlobFileReader {', + ' public:', + ' size_t MultiGetBlob(autovector>>& blob_reqs) const;', + ' private:', + ' std::string blob_reqs;', + '};', + 'size_t BlobFileReader::MultiGetBlob(autovector>>& blob_reqs) const {', + ' return blob_reqs.size();', + '}', + '} // namespace rocksdb', + '', + ].join('\n'), + }); + try { + expect(calls(cg, 'rocksdb::BlobFileReader::MultiGetBlob')).toEqual(['rocksdb::autovector::size (util/autovector.h)']); + } finally { + cg.close(); + } + }); +}); + +describe('stripCppTemplateArguments', () => { + it('drops template arguments together with the ones nested in them', () => { + expect(stripCppTemplateArguments('autovector>')).toBe('autovector'); + expect(stripCppTemplateArguments('Striped>')).toBe('Striped'); + expect(stripCppTemplateArguments('const std::map >&')).toBe('const std::map&'); + expect(stripCppTemplateArguments('SkipList>::Iterator')).toBe('SkipList::Iterator'); + }); +}); diff --git a/src/resolution/cpp-supertypes.ts b/src/resolution/cpp-supertypes.ts index ff47ebad2..7ac3c74e4 100644 --- a/src/resolution/cpp-supertypes.ts +++ b/src/resolution/cpp-supertypes.ts @@ -45,6 +45,7 @@ import { cppScopesWithin, cppTemplateParameters, cppTypeSegments, + stripCppTemplateArguments, type CppNamedClass, } from './cpp-type-aliases'; import { cppMacroNamespaceFrames, cppNamespaceAliases } from './name-matcher'; @@ -163,19 +164,7 @@ function writtenBase(ref: UnresolvedRef, context: ResolutionContext): string { } const written = text.slice(0, end).trim(); const squash = (s: string): string => s.replace(/\s+/g, ''); - return squash(stripTemplateArguments(written)) === squash(ref.referenceName) ? written : ref.referenceName.trim(); -} - -/** `Base>::Inner` → `Base::Inner`. */ -function stripTemplateArguments(text: string): string { - let out = ''; - let depth = 0; - for (const c of text) { - if (c === '<') depth++; - else if (c === '>') depth = Math.max(0, depth - 1); - else if (depth === 0) out += c; - } - return out; + return squash(stripCppTemplateArguments(written)) === squash(ref.referenceName) ? written : ref.referenceName.trim(); } /** diff --git a/src/resolution/cpp-type-aliases.ts b/src/resolution/cpp-type-aliases.ts index 374d57919..2a4cbf33e 100644 --- a/src/resolution/cpp-type-aliases.ts +++ b/src/resolution/cpp-type-aliases.ts @@ -65,7 +65,7 @@ interface TypeName { } /** `SkipList>::Iterator` → `SkipList::Iterator`. */ -function stripTemplateArguments(text: string): string { +export function stripCppTemplateArguments(text: string): string { let out = ''; let depth = 0; for (const c of text) { @@ -112,7 +112,7 @@ function cppTypeName(raw: string): TypeName | null { /** Is a written type a pointer (`Table*`, `const Foo* const`), not counting its template arguments? */ export function isCppPointerType(raw: string): boolean { - return stripTemplateArguments(raw).includes('*'); + return stripCppTemplateArguments(raw).includes('*'); } /** @@ -336,7 +336,7 @@ function collectTemplateHeader(lines: readonly string[], startLine: number, star if (!header) return; for (const item of splitTopLevel(header[1]!)) { // `typename T = int`, `template class Policy`, `typename... Ts`, `int N`. - const declared = stripTemplateArguments(item).split('=')[0]!; + const declared = stripCppTemplateArguments(item).split('=')[0]!; const name = /([A-Za-z_]\w*)\s*$/.exec(declared)?.[1]; if (name && name !== 'typename' && name !== 'class') names.add(name); } diff --git a/src/resolution/name-matcher.ts b/src/resolution/name-matcher.ts index 4e2c9e6a4..3013fb032 100644 --- a/src/resolution/name-matcher.ts +++ b/src/resolution/name-matcher.ts @@ -14,7 +14,7 @@ import { SWIFT_TYPE_PATH_CALL, resolveSwiftTypePathCall } from './swift-type-vis import { dartImportPrefixes, dartLibrarySees, dartPrefixSees, inSameDartLibrary } from './dart-libraries'; import { isDartLocallyBound } from './dart-local-scope'; import { breakVbTie, isVbMemberInScope, isVbNestedTypeInScope, isVbTypeQualifiedBy, matchVbTypedCall, preferVbProject, sameVbProject } from './vbnet-receivers'; -import { cppAliasedTypeName, cppTypeSegments, isCppPointerType, resolveCppAliasedType } from './cpp-type-aliases'; +import { cppAliasedTypeName, cppTypeSegments, isCppPointerType, resolveCppAliasedType, stripCppTemplateArguments } from './cpp-type-aliases'; import { cppIncludedFile, cppIncluders } from './cpp-includers'; import { isTestPath } from '../search/query-utils'; import { isMinifiedContent } from '../extraction/generated-detection'; @@ -8808,11 +8808,39 @@ const CPP_NON_TYPE_TOKENS = new Set([ 'sizeof', 'alignof', 'typeid', 'and', 'or', 'not', 'xor', ]); +/** + * Does a `>` in a declared C++ type close no `<`? Then the type began on an + * earlier line — rocksdb's `std::unique_ptr>>& blob_reqs` under + * `autovectorb->c`). + */ +function cppTypeBeganAbove(typeName: string): boolean { + let depth = 0; + for (const c of typeName) { + if (c === '<') depth++; + else if (c === '>' && --depth < 0) return true; + } + return false; +} + +/** + * The last name of a declared C++ type: `const std::vector>&` → `vector`, `ns::Table>` → `Table`. Null when the text + * names no type. + */ function normalizeCppTypeName(typeName: string): string | null { - const normalized = typeName - .replace(/\b(const|volatile|mutable|typename|class|struct)\b/g, ' ') - .replace(/[&*]+/g, ' ') - .replace(/<[^>]*>/g, ' ') + // Without its head, what is left of a type begun above names a template + // argument's type, not the declared one. + if (cppTypeBeganAbove(typeName)) return null; + // Template arguments go with the ones nested in them. Cut at their first + // `>`, `Table>` was `Table >`, which names no type: a call + // on it never reached Table's method, and a capitalized name no class has + // ruled out any guess as well. + const normalized = stripCppTemplateArguments( + typeName + .replace(/\b(const|volatile|mutable|typename|class|struct)\b/g, ' ') + .replace(/[&*]+/g, ' '), + ) .replace(/\s+/g, ' ') .trim(); @@ -9075,6 +9103,10 @@ function inferCppReceiverType( const inCallerScope = isCppCallersDeclaration(ref.filePath, i + 1, ref, context); noteCppDeclaration(found, declaratorMatch[1]!, inCallerScope); return cppDeclaredType(declaratorMatch[1]!, normalized, inCallerScope, ref, context, found); + } else if (found && cppTypeBeganAbove(declaratorMatch[1] ?? '')) { + // The end of a declaration begun on an earlier line, maybe the + // receiver's: one found further up may be another variable. + found.shadowed = true; } } else if (found && cppRebindsReceiver(line, escapedReceiver)) { found.shadowed = true;