diff --git a/toolchain/check/cpp/generate_ast.cpp b/toolchain/check/cpp/generate_ast.cpp index 2d1d62b3c9f5..a2720d7a87ca 100644 --- a/toolchain/check/cpp/generate_ast.cpp +++ b/toolchain/check/cpp/generate_ast.cpp @@ -9,6 +9,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/Decl.h" +#include "clang/AST/DeclContextInternals.h" #include "clang/AST/Mangle.h" #include "clang/Basic/DiagnosticParse.h" #include "clang/Basic/FileManager.h" @@ -271,11 +272,11 @@ auto CarbonExternalASTSource::BuildCarbonNamespace() -> void { clang::SourceLocation(), identifier, /*PrevDecl=*/nullptr, /*Nested=*/false); decl_context->addDecl(carbon_cpp_namespace); - - // We provide custom lookup results within this namespace. - carbon_cpp_namespace->setHasExternalVisibleStorage(); } + // We provide custom lookup results within this namespace. + carbon_cpp_namespace->getPrimaryContext()->setHasExternalVisibleStorage(); + // Register this file's package scope as corresponding to the `Carbon` // namespace in C++. // TODO: For mangling purposes, include the package as a sub-namespace. @@ -290,24 +291,26 @@ auto CarbonExternalASTSource::BuildCarbonNamespace() -> void { auto CarbonExternalASTSource::FindExternalVisibleDeclsByName( const clang::DeclContext* decl_context, clang::DeclarationName decl_name, const clang::DeclContext* /*OriginalDC*/) -> bool { - // Find the Carbon declaration corresponding to this Clang declaration. - auto* decl = cast( - const_cast(decl_context->getPrimaryContext())); - if (isa(decl)) { - // Functions don't meaningfully have visible decls, but bail out early since - // we can't form a `ClangDeclKey` for a function in the abstract. + auto clang_decl = GetCarbonOwnedDecl(decl_context); + if (!clang_decl) { return false; } - auto key = SemIR::ClangDeclKey::ForNonFunctionDecl(decl); - auto decl_id = context_->clang_decls().LookupId(key); - if (!decl_id.has_value()) { - return false; - } - auto clang_decl = context_->clang_decls().Get(decl_id); - if (clang_decl.is_imported) { - // This is imported from C++, presumably from a Clang AST file, so it's not - // our responsibility to provide its name lookup results. - return false; + + auto* primary_context = + const_cast(decl_context->getPrimaryContext()); + if (auto* lookup_map = primary_context->getLookupPtr()) { + if (auto it = lookup_map->find(decl_name); + it != lookup_map->end() && !it->second.isNull()) { + // We already exported declarations for this name, and may have been + // asked again after `completeVisibleDeclsMap` re-enabled external + // visible storage. Mark external lookup complete without re-exporting. + // + // Note that this only sets that there are no external decls from an AST + // file, despite its name. External visible decls already added by other + // AST sources, such as this one, are retained. + SetNoExternalVisibleDeclsForName(primary_context, decl_name); + return true; + } } llvm::SmallVector lookup_scopes; @@ -316,7 +319,7 @@ auto CarbonExternalASTSource::FindExternalVisibleDeclsByName( // here - completeness should've been checked by clang before this point. if (!AppendLookupScopesForConstant( *context_, SemIR::LocId::None, - context_->constant_values().Get(clang_decl.inst_id), + context_->constant_values().Get(clang_decl->inst_id), SemIR::ConstantId::None, /*extended_scope=*/false, &lookup_scopes)) { return false; } @@ -335,6 +338,7 @@ auto CarbonExternalASTSource::FindExternalVisibleDeclsByName( break; } default: + SetNoExternalVisibleDeclsForName(primary_context, decl_name); return false; } @@ -342,18 +346,26 @@ auto CarbonExternalASTSource::FindExternalVisibleDeclsByName( // `required=false` so Carbon doesn't diagnose a failure, let Clang diagnose // it or even SFINAE. - LookupResult result = - LookupQualifiedName(*context_, SemIR::LocId::None, name_id, lookup_scopes, - /*required=*/false); + LookupResult result = LookupQualifiedName(*context_, GetCurrentCppLocId(), + name_id, lookup_scopes, + /*required=*/false); if (!result.scope_result.is_found()) { + // Avoid caching a negative lookup, as a declaration with this name may be + // added in Carbon later. + if (auto* lookup_map = primary_context->getLookupPtr()) { + if (auto it = lookup_map->find(decl_name); + it != lookup_map->end() && it->second.isNull()) { + lookup_map->erase(it); + } + } return false; } // Map the found Carbon entity to a Clang NamedDecl. CARBON_KIND_SWITCH(MapInstIdToClangDeclOrType(result)) { - case CARBON_KIND(clang::NamedDecl* clang_decl): { - if (clang_decl) { - SetExternalVisibleDeclsForName(decl_context, decl_name, {clang_decl}); + case CARBON_KIND(clang::NamedDecl* mapped_decl): { + if (mapped_decl) { + SetExternalVisibleDeclsForName(decl_context, decl_name, {mapped_decl}); return true; } else { SetNoExternalVisibleDeclsForName(decl_context, decl_name); diff --git a/toolchain/check/testdata/interop/cpp/namespace/lookup_after_failure.carbon b/toolchain/check/testdata/interop/cpp/namespace/lookup_after_failure.carbon new file mode 100644 index 000000000000..397df727f836 --- /dev/null +++ b/toolchain/check/testdata/interop/cpp/namespace/lookup_after_failure.carbon @@ -0,0 +1,179 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/int.carbon +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/interop/cpp/namespace/lookup_after_failure.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/interop/cpp/namespace/lookup_after_failure.carbon + +// --- fail_negative_lookup_via_using_directive.carbon +library "[[@TEST_NAME]]"; + +import Cpp; + +namespace N; + +inline Cpp ''' +namespace Fallback { + int LaterFn() { return 1; } + int LaterNestedFn() { return 2; } +} +using namespace Fallback; +using namespace Carbon; +using namespace Carbon::N; + +// Unqualified lookups search `Carbon` and `Carbon::N` (failing there without +// triggering typo correction) and resolve to `Fallback`. +// CHECK:STDERR: fail_negative_lookup_via_using_directive.carbon:[[@LINE+3]]:9: error: name `LaterFn` used before it was declared [NameUseBeforeDecl] +// CHECK:STDERR: int a = LaterFn(); +// CHECK:STDERR: ^ +int a = LaterFn(); +int b = LaterNestedFn(); +'''; + +// CHECK:STDERR: fail_negative_lookup_via_using_directive.carbon:[[@LINE+7]]:4: note: declared here [NameUseBeforeDeclNote] +// CHECK:STDERR: fn LaterFn() -> i32 { return 3; } +// CHECK:STDERR: ^~~~~~~ +// CHECK:STDERR: +// CHECK:STDERR: fail_negative_lookup_via_using_directive.carbon:[[@LINE-7]]:9: error: name `LaterNestedFn` used before it was declared [NameUseBeforeDecl] +// CHECK:STDERR: int b = LaterNestedFn(); +// CHECK:STDERR: ^ +fn LaterFn() -> i32 { return 3; } +// CHECK:STDERR: fail_negative_lookup_via_using_directive.carbon:[[@LINE+4]]:6: note: declared here [NameUseBeforeDeclNote] +// CHECK:STDERR: fn N.LaterNestedFn() -> i32 { return 4; } +// CHECK:STDERR: ^~~~~~~~~~~~~ +// CHECK:STDERR: +fn N.LaterNestedFn() -> i32 { return 4; } + +inline Cpp ''' +// Subsequent qualified lookups find the newly declared Carbon functions rather +// than seeing cached negative lookup results. +int c = Carbon::LaterFn(); +int d = Carbon::N::LaterNestedFn(); +'''; + +// --- fail_typo_in_carbon_namespace.carbon +library "[[@TEST_NAME]]"; + +import Cpp; + +class ExportedType { + var x: i32; +} + +fn EarlyFn() {} + +inline Cpp ''' +Carbon::ExportedType* p1 = nullptr; + +void TestTypoInCarbon() { + // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+4]]:11: error: no member named 'NoSuchDecl' in namespace 'Carbon' [CppInteropParseError] + // CHECK:STDERR: 19 | Carbon::NoSuchDecl(); + // CHECK:STDERR: | ^~~~~~~~~~ + // CHECK:STDERR: + Carbon::NoSuchDecl(); + // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+8]]:11: error: no type named 'ExportedTyp' in namespace 'Carbon'; did you mean 'ExportedType'? [CppInteropParseError] + // CHECK:STDERR: 28 | Carbon::ExportedTyp* p_typo = p1; + // CHECK:STDERR: | ~~~~~~~~^~~~~~~~~~~ + // CHECK:STDERR: | ExportedType + // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE-19]]:20: note: 'ExportedType' declared here [CppInteropParseNote] + // CHECK:STDERR: 5 | class ExportedType { + // CHECK:STDERR: | ^ + // CHECK:STDERR: + Carbon::ExportedTyp* p_typo = p1; + // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+7]]:11: error: no member named 'LaterDeclared' in namespace 'Carbon' [CppInteropParseError] + // CHECK:STDERR: 36 | Carbon::LaterDeclared(); + // CHECK:STDERR: | ^~~~~~~~~~~~~ + // CHECK:STDERR: + // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+3]]:11: error: name `LaterDeclared` used before it was declared [NameUseBeforeDecl] + // CHECK:STDERR: Carbon::LaterDeclared(); + // CHECK:STDERR: ^ + Carbon::LaterDeclared(); + Carbon::EarlyFn(); + Carbon::ExportedType* p2 = p1; +} +'''; + +// CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+4]]:4: note: declared here [NameUseBeforeDeclNote] +// CHECK:STDERR: fn LaterDeclared() -> i32 { return 42; } +// CHECK:STDERR: ^~~~~~~~~~~~~ +// CHECK:STDERR: +fn LaterDeclared() -> i32 { return 42; } + +class LaterType { + var y: i32; +} + +inline Cpp ''' +void TestAfterTypoInCarbon() { + int v = Carbon::LaterDeclared(); + static_assert(sizeof(Carbon::ExportedType) == 4); + static_assert(sizeof(Carbon::LaterType) == 4); +} +'''; + +// --- fail_typo_in_nested_namespace.carbon +library "[[@TEST_NAME]]"; + +import Cpp; + +namespace Nested; +fn Nested.EarlyFn() {} + +inline Cpp ''' +void TestTypoInNested() { + // CHECK:STDERR: fail_typo_in_nested_namespace.carbon:[[@LINE+7]]:19: error: no member named 'LaterInNested' in namespace 'Carbon::Nested' [CppInteropParseError] + // CHECK:STDERR: 17 | Carbon::Nested::LaterInNested(); + // CHECK:STDERR: | ^~~~~~~~~~~~~ + // CHECK:STDERR: + // CHECK:STDERR: fail_typo_in_nested_namespace.carbon:[[@LINE+3]]:19: error: name `LaterInNested` used before it was declared [NameUseBeforeDecl] + // CHECK:STDERR: Carbon::Nested::LaterInNested(); + // CHECK:STDERR: ^ + Carbon::Nested::LaterInNested(); + Carbon::Nested::EarlyFn(); +} +'''; + +// CHECK:STDERR: fail_typo_in_nested_namespace.carbon:[[@LINE+4]]:11: note: declared here [NameUseBeforeDeclNote] +// CHECK:STDERR: fn Nested.LaterInNested() {} +// CHECK:STDERR: ^~~~~~~~~~~~~ +// CHECK:STDERR: +fn Nested.LaterInNested() {} + +inline Cpp ''' +void TestAfterTypoInNested() { + Carbon::Nested::LaterInNested(); +} +'''; + +// --- fail_typo_in_carbon_class.carbon +library "[[@TEST_NAME]]"; + +import Cpp; + +class MyClass { + var a: i32; + fn StaticMethod1(); + fn StaticMethod2(); + fn Method(self); +} + +inline Cpp ''' +void TestTypoInClass() { + Carbon::MyClass::StaticMethod1(); + // CHECK:STDERR: fail_typo_in_carbon_class.carbon:[[@LINE+4]]:20: error: no member named 'NoSuchMember' in 'Carbon::MyClass' [CppInteropParseError] + // CHECK:STDERR: 19 | Carbon::MyClass::NoSuchMember(); + // CHECK:STDERR: | ^~~~~~~~~~~~ + // CHECK:STDERR: + Carbon::MyClass::NoSuchMember(); + Carbon::MyClass::StaticMethod1(); + Carbon::MyClass::StaticMethod2(); + Carbon::MyClass c{1}; + c.Method(); + static_assert(sizeof(Carbon::MyClass) == 4); +} +'''; diff --git a/toolchain/sem_ir/BUILD b/toolchain/sem_ir/BUILD index 7fa4ad192ec1..bd36e4da4f33 100644 --- a/toolchain/sem_ir/BUILD +++ b/toolchain/sem_ir/BUILD @@ -470,6 +470,7 @@ cc_library( hdrs = ["read_only_ast_source.h"], deps = [ "//toolchain/sem_ir:file", + "@llvm-project//clang:ast", "@llvm-project//clang:sema", ], ) diff --git a/toolchain/sem_ir/class.cpp b/toolchain/sem_ir/class.cpp index 2dbe16ba2f6b..5ee9c93f39d6 100644 --- a/toolchain/sem_ir/class.cpp +++ b/toolchain/sem_ir/class.cpp @@ -73,9 +73,9 @@ auto Class::GetStructTypeFields(const File& sem_ir, auto GetAsCarbonOwnedClass(const File& sem_ir, const clang::TagDecl* tag_decl) -> std::optional> { - // Quickly check whether we could possibly own this class. - // TODO: Once we multiplex with the ASTReader, handle - // ASTReader::completeVisibleDeclsMap setting this to `false`. + // Quickly check whether we could possibly own this class. Carbon-owned + // classes always have external visible storage (preserved across + // `ASTReader::completeVisibleDeclsMap` by `ReadOnlyASTSource`). if (!tag_decl->hasExternalVisibleStorage()) { return std::nullopt; } diff --git a/toolchain/sem_ir/read_only_ast_source.cpp b/toolchain/sem_ir/read_only_ast_source.cpp index f7b1374b786a..1467f4170f58 100644 --- a/toolchain/sem_ir/read_only_ast_source.cpp +++ b/toolchain/sem_ir/read_only_ast_source.cpp @@ -4,10 +4,71 @@ #include "toolchain/sem_ir/read_only_ast_source.h" +#include + +#include "clang/AST/Decl.h" +#include "clang/AST/DeclBase.h" +#include "clang/AST/DeclContextInternals.h" + namespace Carbon::SemIR { char ReadOnlyASTSource::id; +auto ReadOnlyASTSource::GetCarbonOwnedDecl( + const clang::DeclContext* decl_context) const -> std::optional { + auto* decl = cast( + const_cast(decl_context->getPrimaryContext())); + if (isa(decl)) { + // Functions don't meaningfully have visible decls, but bail out early since + // we can't form a `ClangDeclKey` for a function in the abstract. + return std::nullopt; + } + auto key = SemIR::ClangDeclKey::ForNonFunctionDecl(decl); + auto decl_id = sem_ir_.clang_decls().LookupId(key); + if (!decl_id.has_value()) { + return std::nullopt; + } + auto clang_decl = sem_ir_.clang_decls().Get(decl_id); + if (clang_decl.is_imported) { + // This is imported from C++, presumably from a Clang AST file, so it's not + // our responsibility to provide its name lookup results. + return std::nullopt; + } + return clang_decl; +} + +auto ReadOnlyASTSource::completeVisibleDeclsMap( + const clang::DeclContext* decl_context) -> void { + if (!GetCarbonOwnedDecl(decl_context)) { + return; + } + + // `ASTReader::completeVisibleDeclsMap` unconditionally clears external + // visible storage, even for DeclContexts it does not own. Restore it for + // Carbon-owned DeclContexts so future lookups continue to query us. + auto* primary_context = + const_cast(decl_context->getPrimaryContext()); + primary_context->setHasExternalVisibleStorage(true); + + if (auto* lookup_map = primary_context->getLookupPtr(); + lookup_map && !lookup_map->empty()) { + // `setHasExternalVisibleStorage(true)` sets + // `NeedToReconcileExternalVisibleStorage`, which would otherwise mark all + // already-resolved entries as needing another external lookup. Trigger + // reconciliation now and clear `hasExternalDecls` on all existing entries + // so we don't re-export or re-diagnose them. + // + // Note that this only clears out external decls from an AST file, despite + // its name. External visible decls already added by other AST sources, such + // as this one, are retained. + SetNoExternalVisibleDeclsForName(primary_context, + lookup_map->begin()->first); + for (auto& [_, decls] : *lookup_map) { + decls.removeExternalDecls(); + } + } +} + // Get the field offset for each field in a class. // // Returns true on success, false if any error occurs. diff --git a/toolchain/sem_ir/read_only_ast_source.h b/toolchain/sem_ir/read_only_ast_source.h index 2a22a8b78836..f770edc909a2 100644 --- a/toolchain/sem_ir/read_only_ast_source.h +++ b/toolchain/sem_ir/read_only_ast_source.h @@ -5,6 +5,8 @@ #ifndef CARBON_TOOLCHAIN_SEM_IR_READ_ONLY_AST_SOURCE_H_ #define CARBON_TOOLCHAIN_SEM_IR_READ_ONLY_AST_SOURCE_H_ +#include + #include "clang/Sema/ExternalSemaSource.h" #include "toolchain/sem_ir/file.h" @@ -14,6 +16,9 @@ class ReadOnlyASTSource : public clang::ExternalSemaSource { public: explicit ReadOnlyASTSource(const File& sem_ir) : sem_ir_(sem_ir) {} + auto completeVisibleDeclsMap(const clang::DeclContext* decl_context) + -> void override; + auto layoutRecordType( const clang::RecordDecl* record_decl, uint64_t& size, uint64_t& alignment, llvm::DenseMap& field_offsets, @@ -29,6 +34,12 @@ class ReadOnlyASTSource : public clang::ExternalSemaSource { return s->isA(&id); } + protected: + // If `decl_context` was created by exporting a Carbon declaration to C++, + // returns its `ClangDecl`. Otherwise returns `std::nullopt`. + auto GetCarbonOwnedDecl(const clang::DeclContext* decl_context) const + -> std::optional; + private: // For LLVM RTTI. static char id;