Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for importing a trivial global C++ function #5033

Merged
merged 18 commits into from
Mar 3, 2025
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion toolchain/check/check.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ struct Unit {
SemIR::File* sem_ir;

// The Clang AST owned by `CompileSubcommand`.
std::unique_ptr<clang::ASTUnit>* cpp_ast;
std::unique_ptr<clang::ASTUnit>* cpp_ast = nullptr;
};

// Checks a group of parse trees. This will use imports to decide the order of
Expand Down
13 changes: 10 additions & 3 deletions toolchain/check/check_unit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,16 @@ auto CheckUnit::InitPackageScopeAndImports() -> void {
ImportCurrentPackage(package_inst_id, namespace_type_id);
CARBON_CHECK(context_.scope_stack().PeekIndex() == ScopeIndex::Package);
ImportOtherPackages(namespace_type_id);
ImportCppFiles(context_, unit_and_imports_->unit->sem_ir->filename(),
unit_and_imports_->unit->cpp_ast,
unit_and_imports_->cpp_import_names, fs_);

const auto& cpp_import_names = unit_and_imports_->cpp_import_names;
if (!cpp_import_names.empty()) {
auto* cpp_ast = unit_and_imports_->unit->cpp_ast;
CARBON_CHECK(cpp_ast);
CARBON_CHECK(!cpp_ast->get());
*cpp_ast =
ImportCppFiles(context_, unit_and_imports_->unit->sem_ir->filename(),
cpp_import_names, fs_);
}
}

auto CheckUnit::CollectDirectImports(
Expand Down
88 changes: 48 additions & 40 deletions toolchain/check/import_cpp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -116,16 +116,13 @@ static auto AddNamespace(Context& context, PackageNameId cpp_package_id,
}

auto ImportCppFiles(Context& context, llvm::StringRef importing_file_path,
std::unique_ptr<clang::ASTUnit>* ast,
llvm::ArrayRef<Parse::Tree::PackagingNames> imports,
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs)
-> void {
-> std::unique_ptr<clang::ASTUnit> {
if (imports.empty()) {
return;
return nullptr;
}

CARBON_CHECK(ast);
CARBON_CHECK(!ast->get());
CARBON_CHECK(!context.sem_ir().cpp_ast());

auto [generated_ast, ast_has_error] =
Expand All @@ -141,64 +138,75 @@ auto ImportCppFiles(Context& context, llvm::StringRef importing_file_path,
name_scope.set_is_closed_import(true);
name_scope.set_is_cpp_scope(true);

*ast = std::move(generated_ast);
context.sem_ir().set_cpp_ast(ast->get());
context.sem_ir().set_cpp_ast(generated_ast.get());

if (ast_has_error) {
name_scope.set_has_error();
}

return std::move(generated_ast);
}

static auto ClangLookup(Context& context, SemIR::NameId name_id)
// Lookups the given name in the Clang AST. Returns the lookup result if lookup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Lookups the given name in the Clang AST. Returns the lookup result if lookup
// Looks up the given name in the Clang AST. Returns the lookup result if lookup

Small grammar thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// was successful.
static auto ClangLookup(Context& context, SemIR::LocId loc_id,
SemIR::NameId name_id)
-> std::optional<clang::LookupResult> {
clang::ASTUnit* ast = context.sem_ir().cpp_ast();
CARBON_CHECK(ast);
CARBON_CHECK(ast->hasSema());
clang::Sema& sema = ast->getSema();

auto name = context.names().GetAsStringIfIdentifier(name_id);
CARBON_CHECK(name);
llvm::StringRef name = context.names().GetIRBaseName(name_id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
llvm::StringRef name = context.names().GetIRBaseName(name_id);
std::optional<llvm::StringRef> name = context.names().GetAsStringIfIdentifier(name_id);
if (!name) {
// Special names never exist in C++ code.
return std::nullopt;
}

(see comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
I thought of using a dedicated error for this use case to point out this is a special name (and maybe even tell the user how to fix), but keeping as is.
See 16867c9


clang::LookupResult lookup(
sema,
clang::DeclarationNameInfo(
clang::DeclarationName(
sema.getPreprocessor().getIdentifierInfo(*name)),
sema.getPreprocessor().getIdentifierInfo(name)),
clang::SourceLocation()),
clang::Sema::LookupNameKind::LookupOrdinaryName);

if (!sema.LookupQualifiedName(
lookup, ast->getASTContext().getTranslationUnitDecl())) {
bool found = sema.LookupQualifiedName(
lookup, ast->getASTContext().getTranslationUnitDecl());

if (lookup.isClassLookup()) {
// TODO: When support class lookup, also check access.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// TODO: When support class lookup, also check access.
// TODO: To support class lookup, also return the AccessKind for storage.

To be sure, access can't be checked here; that could lead to something like a protected member being incorrectly hidden, when the caller doesn't have access. Instead the AccessKind needs to be part of the returned information so that it can be stored in the lookup table (instead of AccessKind::Public), and then used by lookup to determine access.

(also, small grammar thing -- "to support" or "when supporting")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

context.TODO(loc_id, "Unsupported: Lookup in Class");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe (given the "unsupported" string):

Suggested change
context.TODO(loc_id, "Unsupported: Lookup in Class");
context.TODO(loc_id, "Unsupported: Lookup in Class");
return std::nullopt;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

if (!found) {
return std::nullopt;
}

return lookup;
}

// Imports a function declaration from Clang to Carbon. If successful, returns
// the new Carbon function declaration `InstId`.
static auto ImportFunctionDecl(Context& context, SemIR::LocId loc_id,
SemIR::NameScopeId scope_id,
SemIR::NameId name_id,
const clang::FunctionDecl* clang_decl)
-> SemIR::InstId {
if (clang_decl->isVariadic()) {
context.TODO(loc_id, "Unsupported: Variadic function");
return SemIR::InstId::None;
return SemIR::ErrorInst::SingletonInstId;
}
if (!clang_decl->isGlobal()) {
context.TODO(loc_id, "Unsupported: Non-global function");
return SemIR::InstId::None;
return SemIR::ErrorInst::SingletonInstId;
}
if (clang_decl->getTemplatedKind() != clang::FunctionDecl::TK_NonTemplate) {
context.TODO(loc_id, "Unsupported: Template function");
return SemIR::InstId::None;
return SemIR::ErrorInst::SingletonInstId;
}
if (!clang_decl->param_empty()) {
context.TODO(loc_id, "Unsupported: Function with parameters");
return SemIR::InstId::None;
return SemIR::ErrorInst::SingletonInstId;
}
if (!clang_decl->getReturnType()->isVoidType()) {
context.TODO(loc_id, "Unsupported: Function with non-void return type");
return SemIR::InstId::None;
return SemIR::ErrorInst::SingletonInstId;
}

auto function_decl = SemIR::FunctionDecl{
Expand All @@ -207,25 +215,23 @@ static auto ImportFunctionDecl(Context& context, SemIR::LocId loc_id,
context, SemIR::LocIdAndInst(Parse::NodeId::None, function_decl));

auto function_info = SemIR::Function{
SemIR::EntityWithParamsBase{
.name_id = name_id,
.parent_scope_id = scope_id,
.generic_id = SemIR::GenericId::None,
.first_param_node_id = Parse::NodeId::None,
.last_param_node_id = Parse::NodeId::None,
.pattern_block_id = SemIR::InstBlockId::Empty,
.implicit_param_patterns_id = SemIR::InstBlockId::Empty,
.param_patterns_id = SemIR::InstBlockId::Empty,
.call_params_id = SemIR::InstBlockId::Empty,
.is_extern = false,
.extern_library_id = SemIR::LibraryNameId ::None,
.non_owning_decl_id = SemIR::InstId::None,
.first_owning_decl_id = decl_id,
.definition_id = SemIR::InstId::None},
SemIR::FunctionFields{
.return_slot_pattern_id = SemIR::InstId::None,
.virtual_modifier = SemIR::FunctionFields::VirtualModifier::None,
.self_param_id = SemIR::InstId::None}};
{.name_id = name_id,
.parent_scope_id = scope_id,
.generic_id = SemIR::GenericId::None,
.first_param_node_id = Parse::NodeId::None,
.last_param_node_id = Parse::NodeId::None,
.pattern_block_id = SemIR::InstBlockId::Empty,
.implicit_param_patterns_id = SemIR::InstBlockId::Empty,
.param_patterns_id = SemIR::InstBlockId::Empty,
.call_params_id = SemIR::InstBlockId::Empty,
.is_extern = false,
.extern_library_id = SemIR::LibraryNameId::None,
.non_owning_decl_id = SemIR::InstId::None,
.first_owning_decl_id = decl_id,
.definition_id = SemIR::InstId::None},
{.return_slot_pattern_id = SemIR::InstId::None,
.virtual_modifier = SemIR::FunctionFields::VirtualModifier::None,
.self_param_id = SemIR::InstId::None}};

function_decl.function_id = context.functions().Add(function_info);

Expand All @@ -237,6 +243,8 @@ static auto ImportFunctionDecl(Context& context, SemIR::LocId loc_id,
return decl_id;
}

// Imports a declaration from Clang to Carbon. If successful, returns the
// instruction for the new Carbon declaration.
static auto ImportNameDecl(Context& context, SemIR::LocId loc_id,
SemIR::NameScopeId scope_id, SemIR::NameId name_id,
const clang::NamedDecl* clang_decl)
Expand All @@ -256,7 +264,7 @@ static auto ImportNameDecl(Context& context, SemIR::LocId loc_id,
auto ImportNameFromCpp(Context& context, SemIR::LocId loc_id,
SemIR::NameScopeId scope_id, SemIR::NameId name_id)
-> SemIR::InstId {
auto lookup = ClangLookup(context, name_id);
auto lookup = ClangLookup(context, loc_id, name_id);
if (!lookup) {
return SemIR::InstId::None;
}
Expand All @@ -274,7 +282,7 @@ auto ImportNameFromCpp(Context& context, SemIR::LocId loc_id,
"find a single result; LookupResultKind: {0}",
lookup->getResultKind())
.str());
return SemIR::InstId::None;
return SemIR::ErrorInst::SingletonInstId;
}

return ImportNameDecl(context, loc_id, scope_id, name_id,
Expand Down
6 changes: 3 additions & 3 deletions toolchain/check/import_cpp.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ namespace Carbon::Check {

// Generates a C++ header that includes the imported cpp files, parses it,
// generates the AST from it and links `SemIR::File` to it. Report C++ errors
// and warnings. If successful, adds a `Cpp` namespace.
// and warnings. If successful, adds a `Cpp` namespace and returns the AST.
auto ImportCppFiles(Context& context, llvm::StringRef importing_file_path,
std::unique_ptr<clang::ASTUnit>* ast,
llvm::ArrayRef<Parse::Tree::PackagingNames> imports,
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs) -> void;
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs)
-> std::unique_ptr<clang::ASTUnit>;

// Looks up the given name in the Clang AST generated when importing C++ code.
// If successful, generates the instruction and returns the new `InstId`.
Expand Down
Loading