diff --git a/CHANGELOG.md b/CHANGELOG.md index b9eef3fe0..eded1e0eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -124,6 +124,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - In C++, a call whose receiver's type can't be read no longer links to a method of a class nested in the class the receiver's name suggests, or of a class whose name only contains that name. Before, such a guess took any method of the call's name whose qualified name contained the class's name: protocolbuffers/protobuf's `descriptor->file()` in a template reached `Descriptor::ExtensionRange::file`, and facebook/rocksdb's `CompositeEnv::IsInstanceOf(name)` reached `CompositeEnvWrapper::IsInstanceOf`. A call written through a class, like `PessimisticTransaction::PopSavePoint()` in an override, now links to the method that class inherits, here `TransactionBaseImpl::PopSavePoint`. Re-index C++ projects after upgrading. - In C++, an alias that names a class through a class derived from the one that declares it, like protocolbuffers/protobuf's `using ConformanceRequestSetting = BinaryAndJsonConformanceSuite::ConformanceRequestSetting;`, now leads to that class, `ConformanceTestSuite::ConformanceRequestSetting`. Before, an alias was followed only to a class declared under exactly the name it spells, so objects created through this one linked to the alias rather than the class. Re-index C++ projects after upgrading. - In C++, a name written in a googletest `TEST_F` or `TEST_P` body now finds the test fixture's nested types, those of its base classes included, before anything in the surrounding namespace, as C++ does for the member function such a body is: facebook/rocksdb's `new TestUserDefinedIndexFactory()` links to the class nested in the fixture's base. A class declared inside a test body, or inside any function, is now only found from that function, so the local classes many RocksDB tests each declare under one name, like `FakeWF` or `MyManager`, no longer stand in for one another, and a call like `fwf->SetIOError(true)` reaches the `FakeWF` its own test declares. Re-index C++ projects after upgrading. +- In C and C++ tests, a name that several files declare now links to the one the test's own includes declare, when there is one: protocolbuffers/protobuf's C# generator tests call the `TestSourceDir()` that `googletest.h` declares, not another test helper's they never include; its upb tests call `upb_Arena_New()` and the rest of upb's C API in upb's own files, not the copies bundled for PHP and Ruby, which had left those calls unlinked; and facebook/rocksdb's C API test reaches `rocksdb_open` and its siblings in `db/c.cc`. Re-index C and C++ projects after upgrading. - In C++, a declaration continued on a line that starts with `*`, like facebook/rocksdb's `const auto& el =` over `*reinterpret_cast*>(jhandle);`, is now read, so calls such as `el->OnFlushCompleted(…)` link to `EventListener`'s methods; such a line used to be skipped as part of a comment. Code commented out in a `/* … */` block, and the text of a string literal, like protocolbuffers/protobuf's `[[deprecated("Please use SpaceUsedLong() instead")]]`, are no longer read as declarations, which had sent a call to the wrong method or to none. Re-index C++ projects after upgrading. - In C++, a call with a comparison or a shift among its arguments, like protocolbuffers/protobuf's `p->Emit({{"declare", has_bit_word_index < 0 ? … : ""}, …}, "…")` or `WriteVarint(num << 3, s)`, is now read with the right number of arguments, so it links to the overload that fits rather than one taking fewer. A parameter or variable whose type has template arguments is now read even after another such parameter on the same line, like facebook/rocksdb's `const autovector& cf_ids`, or with empty ones, like fmt's `parse_context<>& ctx`: calls on it link to that class's methods, and calls on a `std::vector` declared that way no longer link to a project class's method of the same name. Re-index C++ projects after upgrading. - In C++, a call passing `nullptr` now links to the overload that takes a `std::nullptr_t`, as C++ picks it, rather than one taking a pointer, and a call no longer links to an overload declared `= delete`, which no call that compiles can reach: protocolbuffers/protobuf's `ptr.reset(nullptr)` on an `Arena::UniquePtr` reaches `reset(std::nullptr_t)`, and `field.Add("foo")` on a `RepeatedPtrField` reaches `Add(Element&&)` instead of the deleted `Add(const Element&)`. Re-index C++ projects after upgrading. diff --git a/__tests__/cpp-test-translation-unit.test.ts b/__tests__/cpp-test-translation-unit.test.ts new file mode 100644 index 000000000..b936455c0 --- /dev/null +++ b/__tests__/cpp-test-translation-unit.test.ts @@ -0,0 +1,256 @@ +/** + * A C or C++ test is a program of its own, so a name it uses is one its + * translation unit declares. Of several same-named candidates, a test suite + * the test does not include is not one of them (protobuf's csharp generator + * tests called `TestSourceDir()`, which googletest.h declares, and + * test_util2.h's `TestUtil::TestSourceDir`, included by none of them, took + * the calls), and what the unit includes goes before what it doesn't (a test + * of the hpb backend calls upb's `upb_Arena_New()` from upb/mem/arena.h, not + * the copies php-upb.h and ruby-upb.h bundle; rocksdb's histogram test sleeps + * on a `SystemClock`, not on emulated_clock.h's `EmulatedSystemClock`). A + * test whose includes don't resolve (jemalloc's tests include a generated + * header) keeps the one candidate there is, and a definition in the test's + * own file still comes first (#1079). + */ +import { describe, it, expect, afterAll, beforeAll } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { CodeGraph } from '../src'; + +let root = ''; +let cg: CodeGraph; + +beforeAll(async () => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-cpp-test-tu-')); + const files: Record = { + // googletest.h declares TestSourceDir(); googletest.cc defines it. + 'src/google/protobuf/testing/googletest.h': `#ifndef GOOGLE_PROTOBUF_TESTING_GOOGLETEST_H__ +#define GOOGLE_PROTOBUF_TESTING_GOOGLETEST_H__ + +#include + +namespace google { +namespace protobuf { + +std::string TestSourceDir(); + +} // namespace protobuf +} // namespace google + +#endif +`, + 'src/google/protobuf/testing/googletest.cc': `#include "google/protobuf/testing/googletest.h" + +namespace google { +namespace protobuf { + +std::string TestSourceDir() { return "src"; } + +} // namespace protobuf +} // namespace google +`, + // A test suite's header no csharp test includes. + 'src/google/protobuf/test_util2.h': `#ifndef GOOGLE_PROTOBUF_TEST_UTIL2_H__ +#define GOOGLE_PROTOBUF_TEST_UTIL2_H__ + +#include + +namespace google { +namespace protobuf { +namespace TestUtil { + +inline std::string TestSourceDir() { return "src/google/protobuf"; } + +} // namespace TestUtil +} // namespace protobuf +} // namespace google + +#endif +`, + 'src/google/protobuf/compiler/csharp/csharp_bootstrap_unittest.cc': `#include + +#include "google/protobuf/testing/googletest.h" + +namespace google { +namespace protobuf { +namespace compiler { +namespace csharp { + +int GeneratedDescriptorsAreUpToDate() { + std::string dir = TestSourceDir(); + return static_cast(dir.size()); +} + +} // namespace csharp +} // namespace compiler +} // namespace protobuf +} // namespace google +`, + // A receiver whose type isn't read is guessed by its name, among the + // classes the test includes: rocksdb's histogram test sleeps on its + // clock, which no emulated_clock.h include of its declares. + 'include/rocksdb/system_clock.h': `namespace rocksdb { + +class SystemClock { + public: + virtual ~SystemClock() {} + virtual void SleepForMicroseconds(int micros) = 0; +}; + +class SystemClockWrapper : public SystemClock { + public: + void SleepForMicroseconds(int micros) override { target_->SleepForMicroseconds(micros); } + + private: + SystemClock* target_; +}; + +} // namespace rocksdb +`, + 'env/emulated_clock.h': `#include "rocksdb/system_clock.h" + +namespace rocksdb { + +class EmulatedSystemClock : public SystemClockWrapper { + public: + void SleepForMicroseconds(int micros) override {} +}; + +} // namespace rocksdb +`, + 'test_util/mock_time_env.h': `#include + +#include "rocksdb/system_clock.h" + +namespace rocksdb { + +class MockSystemClock : public SystemClockWrapper { + public: + void MockSleepForMicroseconds(int micros) {} +}; + +} // namespace rocksdb +`, + 'monitoring/histogram_test.cc': `#include + +#include "rocksdb/system_clock.h" +#include "test_util/mock_time_env.h" + +namespace rocksdb { + +namespace { +std::shared_ptr clock = std::make_shared(); +} // namespace + +void PopulateHistogram(int low, int high) { + for (int i = low; i <= high; i++) { + clock->SleepForMicroseconds(i); + } +} + +} // namespace rocksdb +`, + // A test that declares the class it calls on: its own file comes first, + // whatever it includes (#1079). + 'src/util/collector.h': `#include + +class Collector { + public: + std::string last_error() const { return error_; } + + private: + std::string error_; +}; +`, + // upb's arena, and the copy a bundled amalgamation carries. + 'php/ext/google/protobuf/php-upb.h': `typedef struct upb_Arena upb_Arena; + +static inline upb_Arena* upb_Arena_New(void) { return 0; } +`, + 'upb/mem/arena.h': `typedef struct upb_Arena upb_Arena; + +static inline upb_Arena* upb_Arena_New(void) { return 0; } +`, + 'hpb/backend/upb/interop_test.cc': `#include "upb/mem/arena.h" + +int ArenaInterop() { + upb_Arena* arena = upb_Arena_New(); + return arena == 0; +} +`, + 'src/util/own_collector_unittest.cc': `#include "util/collector.h" + +namespace { +class Collector { + public: + int last_error() const { return 0; } +}; +} // namespace + +int OwnCollector() { + Collector collector; + return collector.last_error(); +} +`, + // jemalloc's tests include a header the build generates, so nothing + // shows the test includes its helpers: the one helper there is stays. + 'deps/jemalloc/test/src/SFMT.c': `unsigned gen_rand32(void* ctx) { + return 4; +} +`, + 'deps/jemalloc/test/unit/SFMT.c': `#include "test/jemalloc_test.h" + +unsigned test_gen_rand32(void* ctx) { + return gen_rand32(ctx); +} +`, + }; + 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); + } + cg = await CodeGraph.init(root, { index: true }); +}); + +afterAll(() => { + cg?.close(); + if (root) fs.rmSync(root, { recursive: true, force: true }); +}); + +/** `qualifiedName (file)` of every call the file's symbols make. */ +const callsFrom = (file: string): string[] => { + const ids = cg.getNodesInFile(file).map((n) => n.id); + return cg.getOutgoingEdgesFrom(ids).filter((e) => e.kind === 'calls').map((e) => { + const target = cg.getNode(e.target)!; + return `${target.qualifiedName} (${target.filePath})`; + }); +}; + +describe('a C/C++ test links what its translation unit declares', () => { + it('passes over a test suite it does not include for the declaration it does', () => { + const calls = callsFrom('src/google/protobuf/compiler/csharp/csharp_bootstrap_unittest.cc'); + expect(calls).toContain('google::protobuf::TestSourceDir (src/google/protobuf/testing/googletest.cc)'); + expect(calls.some((c) => c.includes('test_util2.h'))).toBe(false); + }); + + it('guesses a receiver among the classes its includes declare', () => { + const calls = callsFrom('monitoring/histogram_test.cc'); + expect(calls).toContain('rocksdb::SystemClock::SleepForMicroseconds (include/rocksdb/system_clock.h)'); + expect(calls.some((c) => c.includes('emulated_clock.h'))).toBe(false); + }); + + it('calls the copy of a function its includes declare', () => { + expect(callsFrom('hpb/backend/upb/interop_test.cc')).toContain('upb_Arena_New (upb/mem/arena.h)'); + }); + + it('still prefers a declaration in the test file itself', () => { + const calls = callsFrom('src/util/own_collector_unittest.cc'); + expect(calls.some((c) => c.endsWith('(src/util/own_collector_unittest.cc)') && c.includes('last_error'))).toBe(true); + expect(calls.some((c) => c.includes('(src/util/collector.h)'))).toBe(false); + }); + + it('keeps the only candidate when its includes do not show the unit', () => { + expect(callsFrom('deps/jemalloc/test/unit/SFMT.c')).toContain('gen_rand32 (deps/jemalloc/test/src/SFMT.c)'); + }); +}); diff --git a/src/resolution/name-matcher.ts b/src/resolution/name-matcher.ts index c39382abf..8fa8b991b 100644 --- a/src/resolution/name-matcher.ts +++ b/src/resolution/name-matcher.ts @@ -1608,14 +1608,122 @@ const fileStem = (filePath: string): string => { function isIncludedCppTestSuite(candidate: Node, ref: UnresolvedRef, context: ResolutionContext): boolean { if ((candidate.language !== 'c' && candidate.language !== 'cpp') || (ref.language !== 'c' && ref.language !== 'cpp')) return false; if (cppIncluders(candidate.filePath, context).has(ref.filePath)) return true; - const stem = fileStem(candidate.filePath); - for (const include of context.getNodesInFile(candidate.filePath)) { - if (include.kind !== 'import' || fileStem(include.name) !== stem) continue; - const header = cppIncludedFile(include, context); - if (header && header !== candidate.filePath && fileStem(header) === stem && isTestSuitePath(header) && - cppIncluders(header, context).has(ref.filePath)) return true; + return cppSameStemHeaders(candidate.filePath, context) + .some((header) => isTestSuiteFile(header, context) && cppIncluders(header, context).has(ref.filePath)); +} + +const CPP_STEM_HEADERS = new WeakMap>(); + +/** + * The headers a C or C++ file includes under its own name + * (`conformance_test.cc` → `conformance_test.h`): the declarations its + * definitions implement. + */ +function cppSameStemHeaders(filePath: string, context: ResolutionContext): string[] { + let memo = CPP_STEM_HEADERS.get(context); + if (!memo) CPP_STEM_HEADERS.set(context, (memo = new Map())); + let headers = memo.get(filePath); + if (!headers) { + headers = []; + const stem = fileStem(filePath); + for (const include of context.getNodesInFile(filePath)) { + if (include.kind !== 'import' || fileStem(include.name) !== stem) continue; + const header = cppIncludedFile(include, context); + if (header && header !== filePath && fileStem(header) === stem) headers.push(header); + } + memo.set(filePath, headers); } - return false; + return headers; +} + +const TEST_FILES = new WeakMap>(); +const TEST_SUITE_FILES = new WeakMap>(); + +/** isTestPath, memoized: it is asked of every reference with an ambiguous C or C++ name. */ +function isTestFile(filePath: string, context: ResolutionContext): boolean { + let memo = TEST_FILES.get(context); + if (!memo) TEST_FILES.set(context, (memo = new Map())); + let hit = memo.get(filePath); + if (hit === undefined) memo.set(filePath, (hit = isTestPath(filePath))); + return hit; +} + +/** isTestSuitePath, memoized: it is asked of every candidate of such a name. */ +function isTestSuiteFile(filePath: string, context: ResolutionContext): boolean { + let memo = TEST_SUITE_FILES.get(context); + if (!memo) TEST_SUITE_FILES.set(context, (memo = new Map())); + let hit = memo.get(filePath); + if (hit === undefined) memo.set(filePath, (hit = isTestSuitePath(filePath))); + return hit; +} + +/** + * Whether a C or C++ definition is in `ref`'s translation unit, as its + * includes show it: in the reference's own file, in a file the reference's + * file includes, directly or through other includes, or in the source file + * that implements such a header — the one named like it that includes it + * (`googletest.cc` for `googletest.h`) — unless the source keeps it to + * itself (a `static`, or what isCppTranslationUnitLocal finds). A test + * suite's source implements only a test suite's header, as in + * isIncludedCppTestSuite. + */ +function isInCppTranslationUnit(candidate: Node, ref: UnresolvedRef, context: ResolutionContext): boolean { + if (candidate.filePath === ref.filePath) return true; + if (candidate.language !== 'c' && candidate.language !== 'cpp') return false; + if (cppIncluders(candidate.filePath, context).has(ref.filePath)) return true; + if (!C_SOURCE_EXT.test(candidate.filePath) || isCppTranslationUnitLocal(candidate, ref, context) || + ((candidate.kind === 'function' || candidate.kind === 'variable' || candidate.kind === 'constant') && + isStaticCFunction(candidate, context))) return false; + const suite = isTestSuiteFile(candidate.filePath, context); + return cppSameStemHeaders(candidate.filePath, context) + .some((header) => (!suite || isTestSuiteFile(header, context)) && cppIncluders(header, context).has(ref.filePath)); +} + +/** + * What a C or C++ name written alone can mean outside a class, by its kind: + * a call reaches a function, or a type and its constructor (`Foo(…)`); a + * construction makes a type (`std::string buffer(n, c)` is no enum's + * `string`); any other reference names anything but a member. Nothing a + * class declares is in reach — its methods, its nested types, their + * constructors: only an object, a qualified name or the class's own code + * reaches them. gmock's `Value(result, …)` matcher is no + * `RepeatedFieldAccessor::Value`, breakpad's `EXPECT_CALL(handler, Entry(…))` + * no `CallFrameInfo::Entry` struct. + */ +function isCppNameAloneTarget(n: Node, ref: UnresolvedRef, context: ResolutionContext): boolean { + const constructor = n.kind === 'method' && n.qualifiedName.endsWith(`${n.name}::${n.name}`); + const fits = ref.referenceKind === 'calls' ? n.kind === 'function' || constructor || CPP_TYPE_KINDS.has(n.kind) + : ref.referenceKind === 'instantiates' ? constructor || CPP_TYPE_KINDS.has(n.kind) + : constructor || (n.kind !== 'method' && n.kind !== 'field' && n.kind !== 'property'); + if (!fits) return false; + const scope = cppParentScope(constructor ? cppParentScope(n.qualifiedName) : n.qualifiedName); + return !cppTypeDecls(scope, context).some((d) => CPP_CLASS_KINDS.has(d.kind)); +} + +/** + * Same-named candidates for a C or C++ reference in a test, narrowed to the + * ones its translation unit declares, when there are any. A test is a + * program of its own, so the name it uses is the one its includes declare: + * protobuf's csharp generator tests call `TestSourceDir()`, which + * googletest.h, an include of theirs, declares, and test_util2.h's + * `TestUtil::TestSourceDir`, a test suite none of them includes, took the + * calls; a test of the hpb backend calls upb's `upb_Arena_New()` from + * upb/mem/arena.h, not the copies php-upb.h and ruby-upb.h bundle. For a + * name the reference writes alone (`byName`), only what the name alone can + * mean counts (isCppNameAloneTarget), and a call the extractor recorded + * without the receiver it is written on (`columns()[0].value()`) is left to + * the name strategies. When the unit declares none of them — a test whose + * includes don't resolve — the candidates stay as they are. Not for an + * `#include`, which names a file, nor for production code, which + * isVisibleAcrossFiles keeps from test suites. + */ +function inCppTestTranslationUnit(candidates: Node[], ref: UnresolvedRef, context: ResolutionContext, byName = false): Node[] { + if (candidates.length < 2 || (ref.language !== 'c' && ref.language !== 'cpp') || + ref.referenceKind === 'imports' || !isTestFile(ref.filePath, context)) return candidates; + if (byName && ref.referenceKind === 'calls' && cppImplicitReceiver(ref, context) !== 'bare') return candidates; + const within = candidates.filter((n) => isInCppTranslationUnit(n, ref, context) && + (!byName || n.filePath === ref.filePath || isCppNameAloneTarget(n, ref, context))); + return within.length > 0 ? within : candidates; } const MINIFIED_SCRIPTS = new WeakMap>(); @@ -8033,10 +8141,12 @@ export function matchByExactName( // file's symbol of that name, so a bare call has no cross-file candidate. !(bareJs && n.filePath !== ref.filePath && isLocallyBoundJsName(ref.referenceName, ref.filePath, context)) ); - const candidates = dartBare ? nearestDartMembers(preferOwnDartLibrary(filtered, ref, context), ref, context) + const ranked = dartBare ? nearestDartMembers(preferOwnDartLibrary(filtered, ref, context), ref, context) : dartTypeRef && dartPrefix === null ? preferOwnDartLibrary(filtered, ref, context) : swiftShape && swiftShape.shape !== 'chained' ? nearestSwiftMembers(filtered, ref, context) : kotlinBare ? lexicalKotlinMembers(filtered, ref, context) : cppSpecializationPool(filtered, ref, context); + // (Above the ceiling below nothing is picked, so no translation unit is read.) + const candidates = ranked.length <= AMBIGUOUS_NAME_CEILING ? inCppTestTranslationUnit(ranked, ref, context, true) : ranked; if (candidates.length === 0) { return null; @@ -9130,7 +9240,7 @@ export function resolveMethodOnType( // intentionally in ANOTHER file (#314) — is unaffected: those blocks return // early whenever an import pins the class. if (ref.referenceKind === 'function_ref' && matches.length !== 1) return null; - const ordered = preferCallSiteFile(cppVisibleFirst(matches, ref, context), ref.filePath); + const ordered = preferCallSiteFile(inCppTestTranslationUnit(cppVisibleFirst(matches, ref, context), ref, context), ref.filePath); return { original: ref, targetNodeId: ordered[0]!.id, @@ -13074,6 +13184,9 @@ export function clearNameMatcherMemos(context: ResolutionContext): void { clearCppNamespaceMemos(context); clearCppRawStringMemos(context); CPP_CLASSES_IN_FILE.delete(context); + CPP_STEM_HEADERS.delete(context); + TEST_FILES.delete(context); + TEST_SUITE_FILES.delete(context); CPP_BASE_SPECS.delete(context); CPP_USING_MEMBERS.delete(context); CPP_MEMBER_MACRO_ARGS.delete(context); @@ -14671,7 +14784,7 @@ export function matchMethodCall( // resolves to `a/`'s method (#1079). const strat1 = nmTimedT('mc-class', ref, (): ResolvedRef | null => { let classCandidates = preferCallSiteFile( - context.getNodesByName(objectOrClass!).filter(isMethodOwnerKind), + inCppTestTranslationUnit(context.getNodesByName(objectOrClass!).filter(isMethodOwnerKind), ref, context), ref.filePath, ); // A C# class the call's namespaces can see before one they can't: @@ -14774,7 +14887,7 @@ export function matchMethodCall( if (capitalizedReceiver !== objectOrClass) { const strat2 = nmTimedT('mc-capital', ref, (): ResolvedRef | null => { const fuzzyClassCandidates = preferCallSiteFile( - context.getNodesByName(capitalizedReceiver).filter(isMethodOwnerKind), + inCppTestTranslationUnit(context.getNodesByName(capitalizedReceiver).filter(isMethodOwnerKind), ref, context), ref.filePath, ); for (const classNode of fuzzyClassCandidates) { @@ -14909,7 +15022,7 @@ export function matchMethodCall( // the first seen) resolves to the call site's own file rather than the // first-indexed duplicate (#1079). const std = isStdMethodName(ref.language, methodName!) && !/^(?:self|Self|this|base)$/.test(objectOrClass!); - for (const method of preferCallSiteFile(targetMethods, ref.filePath)) { + for (const method of preferCallSiteFile(inCppTestTranslationUnit(targetMethods, ref, context), ref.filePath)) { if (std && !receiverNamesOwner(receiverLink(objectOrClass!), method, context)) continue; // The owner type's own name — not its namespace (`eShop.ClientApp…` // shares `Client` with every `httpClient`) nor the method's.