Perform overload resolution immediately in C++ operator lookup.

examples/interop/cpp/hello_world.carbon

@@ -2,31 +2,55 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
-import Cpp inline "#include <cstdio>";
-
-fn Run() {
-  // TODO: Requires class with virtual bases (`basic_ostream`).
-  // Cpp.std.cout << "Hello world!\n";
-
-  // TODO: Requires variadic function.
-  // Cpp.printf("Hello world!\n");
-
-  // TODO: Requires nullable pointer.
-  // Cpp.puts("Hello world!\n");
-
-  // TODO: Requires nullable void pointer.
-  // Cpp.write(1, "Hello world!\n", 13);
-
-  // TODO: Requires Core.String API.
-  // let message: str = "Hello world!\n\n";
-  // for (c: char in message) {
-  //   Cpp.putchar((c as u8) as i32);
-  // }
+import Cpp library "<cstdio>";
+import Cpp library "<iostream>";
+import Cpp library "<string_view>";
+import Cpp library "<unistd.h>";
 
+fn HelloArrayOfChars() {
   let message: array(char, 13) =
       ('H', 'e', 'l', 'l', 'o', ' ', 'w', 'o', 'r', 'l', 'd', '!', '\n');
   for (c: char in message) {
     // TODO: u8 should probably have an implicit cast to i32.
     Cpp.putchar((c as u8) as i32);
   }
 }
+
+fn HelloStringAsArrayOfChars() {
+  // TODO: Requires IndexWith support for str.
+  // let message: str = "Hello world!\n\n";
+  // for (c: char in message) {
+  //   Cpp.putchar((c as u8) as i32);
+  // }
+}
+
+fn HelloPuts() {
+  // TODO: Requires mapping from Optional(const char*) into C++.
+  // TODO: There should be a better way to interact with functions that expect a
+  // null-terminated string.
+  // Cpp.puts(Cpp.std.data("Hello world!\n\0"));
+}
+
+fn HelloWrite() {
+  // TODO: Requires mapping from Optional(const char*) into C++.
+  // let s: str = "Hello world!\n";
+  // Cpp.write(1, Cpp.std.data(s), Cpp.std.size(s));
+}
+
+fn HelloPrintf() {
+  // TODO: Requires variadic function.
+  // Cpp.printf("Hello world!\n");
+}
+
+fn HelloIostreams() {
+  Cpp.std.cout << "Hello world!\n";
+}
+
+fn Run() {
+  HelloArrayOfChars();
+  HelloStringAsArrayOfChars();
+  HelloPuts();
+  HelloWrite();
+  HelloPrintf();
+  HelloIostreams();
+}

toolchain/check/cpp/call.cpp

@@ -15,10 +15,14 @@
 namespace Carbon::Check {
 
 // Returns whether the function is an imported C++ operator member function.
-static auto IsCppOperatorMethod(Context& context, SemIR::FunctionId function_id)
-    -> bool {
+auto IsCppOperatorMethod(Context& context, SemIR::InstId inst_id) -> bool {
+  auto function_type = context.types().TryGetAs<SemIR::FunctionType>(
+      context.insts().Get(inst_id).type_id());
+  if (!function_type) {
+    return false;
+  }
   SemIR::ClangDeclId clang_decl_id =
-      context.functions().Get(function_id).clang_decl_id;
+      context.functions().Get(function_type->function_id).clang_decl_id;
   return clang_decl_id.has_value() &&
          IsCppOperatorMethodDecl(
              context.clang_decls().Get(clang_decl_id).key.decl);
@@ -41,9 +45,6 @@ auto PerformCallToCppFunction(Context& context, SemIR::LocId loc_id,
       if (self_id.has_value()) {
         // Preserve the `self` argument from the original callee.
         fn.self_id = self_id;
-      } else if (IsCppOperatorMethod(context, fn.function_id)) {
-        // Adjust `self` and args for C++ overloaded operator methods.
-        fn.self_id = arg_ids.consume_front();
       }
       return PerformCallToFunction(context, loc_id, callee_id, fn, arg_ids);
     }

toolchain/check/cpp/call.h

@@ -10,6 +10,11 @@
 
 namespace Carbon::Check {
 
+// Returns whether the specified instruction refers to a C++ overloaded operator
+// that is a method. If so, the first operand will be passed as `self` rather
+// than as the first argument.
+auto IsCppOperatorMethod(Context& context, SemIR::InstId inst_id) -> bool;
+
 // Checks and builds SemIR for a call to a C++ function in the given overload
 // set with self `self_id` and arguments `arg_ids`.
 //

toolchain/check/cpp/import.cpp

@@ -740,6 +740,13 @@ static auto GetInheritanceKind(clang::CXXRecordDecl* class_def)
     return SemIR::Class::Final;
   }
 
+  if (class_def->getNumVBases()) {
+    // TODO: We treat classes with virtual bases as final for now. We use the
+    // layout of the class including its virtual bases as its Carbon type
+    // layout, so we wouldn't behave correctly if we derived from it.
+    return SemIR::Class::Final;
+  }
+
   if (class_def->isAbstract()) {
     // If the class has any abstract members, it's abstract.
     return SemIR::Class::Abstract;
@@ -802,8 +809,15 @@ static auto ImportClassObjectRepr(Context& context, SemIR::ClassId class_id,
 
   // Import bases.
   for (const auto& base : clang_def->bases()) {
-    CARBON_CHECK(!base.isVirtual(),
-                 "Should not import definition for class with a virtual base");
+    if (base.isVirtual()) {
+      // If the base is virtual, skip it from the layout. We don't know where it
+      // will actually appear within the complete object layout, as a pointer to
+      // this class might point to a derived type that puts the vbase in a
+      // different place.
+      // TODO: Track that the virtual base existed. Support derived-to-vbase
+      // conversions by generating a clang AST fragment.
+      continue;
+    }
 
     auto [base_type_inst_id, base_type_id] =
         ImportTypeAndDependencies(context, import_ir_inst_id, base.getType());
@@ -842,6 +856,10 @@ static auto ImportClassObjectRepr(Context& context, SemIR::ClassId class_id,
       class_info.base_id = SemIR::InstId::None;
     }
 
+    // TODO: If the base class has virtual bases, the size of the type that we
+    // add to the layout here will be the full size of the class (including
+    // virtual bases), whereas the size actually occupied by this base class is
+    // only the nvsize (excluding virtual bases).
     auto base_offset = base.isVirtual()
                            ? clang_layout.getVBaseClassOffset(base_class)
                            : clang_layout.getBaseClassOffset(base_class);
@@ -2413,17 +2431,6 @@ auto ImportClassDefinitionForClangDecl(Context& context, SemIR::LocId loc_id,
     auto* class_def = class_decl->getDefinition();
     CARBON_CHECK(class_def, "Complete type has no definition");
 
-    if (class_def->getNumVBases()) {
-      // TODO: Handle virtual bases. We don't actually know where they go in the
-      // layout. We may also want to use a different size in the layout for
-      // `partial C`, excluding the virtual base. It's also not entirely safe to
-      // just skip over the virtual base, as the type we would construct would
-      // have a misleading size. For now, treat a C++ class with vbases as
-      // incomplete in Carbon.
-      context.TODO(loc_id, "class with virtual bases");
-      return false;
-    }
-
     BuildClassDefinition(context, import_ir_inst_id, class_id, class_inst_id,
                          class_def);
   } else if (auto* enum_decl = dyn_cast<clang::EnumDecl>(clang_decl)) {

toolchain/check/cpp/operators.cpp

@@ -8,6 +8,7 @@
 #include "clang/Sema/Sema.h"
 #include "toolchain/check/cpp/import.h"
 #include "toolchain/check/cpp/location.h"
+#include "toolchain/check/cpp/overload_resolution.h"
 #include "toolchain/check/cpp/type_mapping.h"
 #include "toolchain/check/inst.h"
 #include "toolchain/check/type.h"
@@ -190,31 +191,82 @@ auto LookupCppOperator(Context& context, SemIR::LocId loc_id, Operator op,
     }
   }
 
-  auto arg_exprs = InventClangArgs(context, arg_ids);
-  if (!arg_exprs.has_value()) {
+  auto maybe_arg_exprs = InventClangArgs(context, arg_ids);
+  if (!maybe_arg_exprs.has_value()) {
     return SemIR::ErrorInst::InstId;
   }
+  auto& arg_exprs = *maybe_arg_exprs;
 
   clang::SourceLocation loc = GetCppLocation(context, loc_id);
   clang::OverloadCandidateSet::OperatorRewriteInfo operator_rewrite_info(
       *op_kind, loc, /*AllowRewritten=*/true);
-  clang::UnresolvedSet<4> functions;
+  clang::UnresolvedSet<0> functions;
   clang::OverloadCandidateSet candidate_set(
       loc, clang::OverloadCandidateSet::CSK_Operator, operator_rewrite_info);
+
+  clang::Sema& sema = context.clang_sema();
+
   // This works for both unary and binary operators.
-  context.clang_sema().LookupOverloadedBinOp(candidate_set, *op_kind, functions,
-                                             *arg_exprs);
+  sema.LookupOverloadedBinOp(candidate_set, *op_kind, functions, arg_exprs);
 
-  for (auto& it : candidate_set) {
-    if (!it.Function) {
-      continue;
+  clang::OverloadCandidateSet::iterator best_viable_fn;
+  switch (candidate_set.BestViableFunction(sema, loc, best_viable_fn)) {
+    case clang::OverloadingResult::OR_Success: {
+      if (!best_viable_fn->Function) {
+        // The best viable candidate was a builtin. Let the Carbon operator
+        // machinery handle that.
+        return SemIR::InstId::None;
+      }
+      if (best_viable_fn->RewriteKind) {
+        context.TODO(
+            loc_id,
+            llvm::formatv("Rewriting operator{0} using {1} is not supported",
+                          clang::getOperatorSpelling(
+                              candidate_set.getRewriteInfo().OriginalOperator),
+                          best_viable_fn->Function->getNameAsString()));
+        return SemIR::ErrorInst::InstId;
+      }
+      sema.MarkFunctionReferenced(loc, best_viable_fn->Function);
+      auto result_id = ImportCppFunctionDecl(
+          context, loc_id, best_viable_fn->Function,
+          // If this is an operator method, the first arg will be used as self.
+          arg_ids.size() -
+              (isa<clang::CXXMethodDecl>(best_viable_fn->Function) ? 1 : 0));
+      CheckCppOverloadAccess(context, loc_id, best_viable_fn->FoundDecl,
+                             result_id);
+      return result_id;
     }
-    functions.addDecl(it.Function, it.FoundDecl.getAccess());
+    case clang::OverloadingResult::OR_No_Viable_Function: {
+      // OK, didn't find a viable C++ candidate, but this is not an error, as
+      // there might be a Carbon candidate.
+      return SemIR::InstId::None;
+    }
+    case clang::OverloadingResult::OR_Ambiguous: {
+      const char* spelling = clang::getOperatorSpelling(*op_kind);
+      candidate_set.NoteCandidates(
+          clang::PartialDiagnosticAt(
+              loc, sema.PDiag(clang::diag::err_ovl_ambiguous_oper_binary)
+                       << spelling << arg_exprs[0]->getType()
+                       << arg_exprs[1]->getType()),
+          sema, clang::OCD_AmbiguousCandidates, arg_exprs, spelling, loc);
+      return SemIR::ErrorInst::InstId;
+    }
+    case clang::OverloadingResult::OR_Deleted:
+      const char* spelling = clang::getOperatorSpelling(*op_kind);
+      auto* message = best_viable_fn->Function->getDeletedMessage();
+      // The best viable function might be a different operator if the best
+      // candidate is a rewritten candidate, so use the operator kind of the
+      // candidate itself in the diagnostic.
+      candidate_set.NoteCandidates(
+          clang::PartialDiagnosticAt(
+              loc, sema.PDiag(clang::diag::err_ovl_deleted_oper)
+                       << clang::getOperatorSpelling(
+                              best_viable_fn->Function->getOverloadedOperator())
+                       << (message != nullptr)
+                       << (message ? message->getString() : llvm::StringRef())),
+          sema, clang::OCD_AllCandidates, arg_exprs, spelling, loc);
+      return SemIR::ErrorInst::InstId;
   }
-
-  return ImportCppOverloadSet(
-      context, loc_id, SemIR::NameScopeId::None, SemIR::NameId::CppOperator,
-      /*naming_class=*/nullptr, std::move(functions), operator_rewrite_info);
 }
 
 auto IsCppOperatorMethodDecl(clang::Decl* decl) -> bool {

toolchain/check/cpp/overload_resolution.cpp

@@ -84,22 +84,31 @@ static auto AddOverloadCandidates(clang::Sema& sema,
   }
 }
 
-// Checks whether a selected overload is accessible and diagnoses if not.
-static auto CheckOverloadAccess(Context& context, SemIR::LocId loc_id,
-                                const SemIR::CppOverloadSet& overload_set,
-                                clang::DeclAccessPair overload,
-                                SemIR::InstId overload_inst_id) -> void {
+auto CheckCppOverloadAccess(Context& context, SemIR::LocId loc_id,
+                            clang::DeclAccessPair overload,
+                            SemIR::InstId overload_inst_id,
+                            SemIR::NameScopeId parent_scope_id) -> void {
   SemIR::AccessKind member_access_kind = MapCppAccess(overload);
   if (member_access_kind == SemIR::AccessKind::Public) {
     return;
   }
+  if (overload_inst_id == SemIR::ErrorInst::InstId) {
+    return;
+  }
+
+  auto function_id =
+      context.insts().GetAs<SemIR::FunctionDecl>(overload_inst_id).function_id;
+  auto& function = context.functions().Get(function_id);
+  if (!parent_scope_id.has_value()) {
+    parent_scope_id = function.parent_scope_id;
+  }
 
   auto name_scope_const_id = context.constant_values().Get(
-      context.name_scopes().Get(overload_set.parent_scope_id).inst_id());
+      context.name_scopes().Get(parent_scope_id).inst_id());
   SemIR::AccessKind allowed_access_kind =
       GetHighestAllowedAccess(context, loc_id, name_scope_const_id);
-  CheckAccess(context, loc_id, SemIR::LocId(overload_inst_id),
-              overload_set.name_id, member_access_kind,
+  CheckAccess(context, loc_id, SemIR::LocId(overload_inst_id), function.name_id,
+              member_access_kind,
               /*is_parent_access=*/false,
               {.constant_id = name_scope_const_id,
                .highest_allowed_access = allowed_access_kind});
@@ -155,25 +164,13 @@ auto PerformCppOverloadResolution(Context& context, SemIR::LocId loc_id,
 
   switch (overloading_result) {
     case clang::OverloadingResult::OR_Success: {
-      // TODO: Handle the cases when Function is null.
       CARBON_CHECK(best_viable_fn->Function);
-      if (best_viable_fn->RewriteKind) {
-        context.TODO(
-            loc_id,
-            llvm::formatv("Rewriting operator{0} using {1} is not supported",
-                          clang::getOperatorSpelling(
-                              candidate_set.getRewriteInfo().OriginalOperator),
-                          best_viable_fn->Function->getNameAsString()));
-        return SemIR::ErrorInst::InstId;
-      }
+      CARBON_CHECK(!best_viable_fn->RewriteKind);
       sema.MarkFunctionReferenced(loc, best_viable_fn->Function);
       SemIR::InstId result_id = ImportCppFunctionDecl(
-          context, loc_id, best_viable_fn->Function,
-          // If this is an operator method, the first arg will be used as self.
-          arg_exprs.size() -
-              (IsCppOperatorMethodDecl(best_viable_fn->Function) ? 1 : 0));
-      CheckOverloadAccess(context, loc_id, overload_set,
-                          best_viable_fn->FoundDecl, result_id);
+          context, loc_id, best_viable_fn->Function, arg_exprs.size());
+      CheckCppOverloadAccess(context, loc_id, best_viable_fn->FoundDecl,
+                             result_id, overload_set.parent_scope_id);
       return result_id;
     }
     case clang::OverloadingResult::OR_No_Viable_Function: {

toolchain/check/cpp/overload_resolution.h

@@ -11,6 +11,15 @@
 
 namespace Carbon::Check {
 
+// Checks whether a selected overload is accessible and diagnoses if not.
+// `parent_scope_id`, if specified, describes the scope that was named to find
+// the overload. If unspecified, we assume the overload was found in the class
+// that it is a direct member of, rather than a derived class.
+auto CheckCppOverloadAccess(
+    Context& context, SemIR::LocId loc_id, clang::DeclAccessPair overload,
+    SemIR::InstId overload_inst_id,
+    SemIR::NameScopeId parent_scope_id = SemIR::NameScopeId::None) -> void;
+
 // Resolves which function to call using Clang overloading resolution, or
 // returns an error instruction if overload resolution failed.
 //