Perform overload resolution immediately in C++ operator lookup. (carbon-language#6416)

Don't attempt to defer overload resolution by creating a
`CppOverloadSet`; this was incorrect as we weren't saving the complete
clang::OverloadCandidateSet, resulting in template candidates not being
found. Moreover, saving the overload candidate set would be expensive,
as the representation is surprisingly large, and is unnecessary since
we're about to build a call.

In passing, improve the diagnostics for overload resolution failure to
use Clang's operator overload resolution messages rather than its call
overload resolution messages.

This fixes calls to templated operator overloads, which is the final
piece needed for us to successfully compile an iostream-based "Hello
world" program.

---------

Co-authored-by: Jon Ross-Perkins <[email protected]>

Author: zygoloid

examples/interop/cpp/hello_world.carbon

@@ -2,31 +2,54 @@
 // 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>";
 
+// Demonstrate passing `char`s to a C++ function.
+fn HelloPutchar() {
   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);
   }
+
+  // TODO: Use a loop over a string literal instead. Requires IndexWith support
+  // for str.
+  //
+  // let message: str = "Hello world!\n";
+  // for (c: char in message) {
+  //   Cpp.putchar((c as u8) as i32);
+  // }
+}
+
+// Demonstrate passing a null-terminated string to a C++ function.
+fn HelloStdio() {
+  // 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"));
+
+  // TODO: Requires variadic function support.
+  // Cpp.printf("Hello world!\n\0");
+}
+
+// Demonstrate passing a string as a void pointer to a C++ function.
+fn HelloWrite() {
+  // TODO: Requires mapping from Optional(const char*) into a C++ const void*.
+  // let s: str = "Hello world!\n";
+  // Cpp.write(1, Cpp.std.data(s), Cpp.std.size(s));
+}
+
+fn HelloIostreams() {
+  Cpp.std.cout << "Hello world!\n";
+}
+
+fn Run() {
+  HelloPutchar();
+  HelloStdio();
+  HelloWrite();
+  HelloIostreams();
 }

toolchain/check/cpp/call.cpp

@@ -14,16 +14,6 @@
 
 namespace Carbon::Check {
 
-// Returns whether the function is an imported C++ operator member function.
-static auto IsCppOperatorMethod(Context& context, SemIR::FunctionId function_id)
-    -> bool {
-  SemIR::ClangDeclId clang_decl_id =
-      context.functions().Get(function_id).clang_decl_id;
-  return clang_decl_id.has_value() &&
-         IsCppOperatorMethodDecl(
-             context.clang_decls().Get(clang_decl_id).key.decl);
-}
-
 auto PerformCallToCppFunction(Context& context, SemIR::LocId loc_id,
                               SemIR::CppOverloadSetId overload_set_id,
                               SemIR::InstId self_id,
@@ -41,9 +31,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/operators.cpp

@@ -8,11 +8,13 @@
 #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"
 #include "toolchain/check/type_completion.h"
 #include "toolchain/sem_ir/ids.h"
+#include "toolchain/sem_ir/typed_insts.h"
 
 namespace Carbon::Check {
 
@@ -190,36 +192,105 @@ 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::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, clang::UnresolvedSet<0>{},
+                             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));
+      if (auto fn_decl =
+              context.insts().TryGetAsWithId<SemIR::FunctionDecl>(result_id)) {
+        CheckCppOverloadAccess(context, loc_id, best_viable_fn->FoundDecl,
+                               fn_decl->inst_id);
+      } else {
+        CARBON_CHECK(result_id == SemIR::ErrorInst::InstId);
+      }
+      return result_id;
+    }
+    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;
     }
-    functions.addDecl(it.Function, it.FoundDecl.getAccess());
+    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 {
   auto* clang_method_decl = dyn_cast<clang::CXXMethodDecl>(decl);
   return clang_method_decl && clang_method_decl->isOverloadedOperator();
 }
 
+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_type->function_id).clang_decl_id;
+  return clang_decl_id.has_value() &&
+         IsCppOperatorMethodDecl(
+             context.clang_decls().Get(clang_decl_id).key.decl);
+}
+
 }  // namespace Carbon::Check

toolchain/check/cpp/operators.h

@@ -20,6 +20,11 @@ auto LookupCppOperator(Context& context, SemIR::LocId loc_id, Operator op,
 // Returns whether the decl is an operator member function.
 auto IsCppOperatorMethodDecl(clang::Decl* decl) -> bool;
 
+// 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;
+
 }  // namespace Carbon::Check
 
 #endif  // CARBON_TOOLCHAIN_CHECK_CPP_OPERATORS_H_

toolchain/check/cpp/overload_resolution.cpp

@@ -84,22 +84,27 @@ 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::KnownInstId<SemIR::FunctionDecl> overload_inst_id,
+    SemIR::NameScopeId parent_scope_id) -> void {
   SemIR::AccessKind member_access_kind = MapCppAccess(overload);
   if (member_access_kind == SemIR::AccessKind::Public) {
     return;
   }
 
+  auto function_id = context.insts().Get(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 +160,18 @@ 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());
+      if (auto fn_decl =
+              context.insts().TryGetAsWithId<SemIR::FunctionDecl>(result_id)) {
+        CheckCppOverloadAccess(context, loc_id, best_viable_fn->FoundDecl,
+                               fn_decl->inst_id, overload_set.parent_scope_id);
+      } else {
+        CARBON_CHECK(result_id == SemIR::ErrorInst::InstId);
+      }
       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::KnownInstId<SemIR::FunctionDecl> 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.
 //