Improve diagnostics for overload resolution failure. (#6091)

Include notes listing the candidates and explaining why they didn't
work. Rather than duplicating the (substantial) logic for this, use the
Clang machinery to generate these diagnostics.

In order to support this, add a mechanism to map `SemIR::LocId`s to
`clang::SourceLocation`s. This works by creating source buffers in Clang
that refer into the Carbon source file so that `SourceLocation`s can
point into them.

---------

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

Author: zygoloid

toolchain/check/BUILD

@@ -23,6 +23,7 @@ cc_library(
         "convert.cpp",
         "cpp/custom_type_mapping.cpp",
         "cpp/import.cpp",
+        "cpp/location.cpp",
         "cpp/operators.cpp",
         "cpp/overload_resolution.cpp",
         "cpp/thunk.cpp",
@@ -73,6 +74,7 @@ cc_library(
         "convert.h",
         "cpp/custom_type_mapping.h",
         "cpp/import.h",
+        "cpp/location.h",
         "cpp/operators.h",
         "cpp/overload_resolution.h",
         "cpp/thunk.h",
@@ -148,6 +150,7 @@ cc_library(
         "//toolchain/lex:tokenized_buffer",
         "//toolchain/parse:node_kind",
         "//toolchain/parse:tree",
+        "//toolchain/sem_ir:absolute_node_id",
         "//toolchain/sem_ir:clang_decl",
         "//toolchain/sem_ir:expr_info",
         "//toolchain/sem_ir:file",

toolchain/check/context.h

@@ -157,6 +157,11 @@ class Context {
     return import_ir_constant_values_;
   }
 
+  auto cpp_carbon_file_locations()
+      -> llvm::SmallVector<clang::SourceLocation>& {
+    return cpp_carbon_file_locations_;
+  }
+
   auto definitions_required_by_decl() -> llvm::SmallVector<SemIR::InstId>& {
     return definitions_required_by_decl_;
   }
@@ -387,6 +392,10 @@ class Context {
   // Inline 0 elements because it's expected to require heap allocation.
   llvm::SmallVector<SemIR::ConstantValueStore, 0> import_ir_constant_values_;
 
+  // Per-Carbon-file start locations for corresponding Clang source buffers.
+  // Owned and managed by code in cpp/location.cpp.
+  llvm::SmallVector<clang::SourceLocation> cpp_carbon_file_locations_;
+
   // Declaration instructions of entities that should have definitions by the
   // end of the current source file.
   llvm::SmallVector<SemIR::InstId> definitions_required_by_decl_;

toolchain/check/cpp/import.cpp

@@ -1667,6 +1667,13 @@ auto ImportCppFunctionDecl(Context& context, SemIR::LocId loc_id,
 
   SemIR::Function& function_info = context.functions().Get(*function_id);
   if (IsCppThunkRequired(context, function_info)) {
+    Diagnostics::AnnotationScope annotate_diagnostics(
+        &context.emitter(), [&](auto& builder) {
+          CARBON_DIAGNOSTIC(InCppThunk, Note,
+                            "in thunk for C++ function used here");
+          builder.Note(loc_id, InCppThunk);
+        });
+
     clang::FunctionDecl* thunk_clang_decl =
         BuildCppThunk(context, function_info);
     if (thunk_clang_decl) {

toolchain/check/cpp/location.cpp

@@ -0,0 +1,82 @@
+// 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 "toolchain/check/cpp/location.h"
+
+#include "toolchain/sem_ir/absolute_node_id.h"
+#include "toolchain/sem_ir/ids.h"
+
+namespace Carbon::Check {
+
+namespace {
+struct FileInfo {
+  const SemIR::File* sem_ir;
+  clang::SourceLocation start_loc;
+};
+}  // namespace
+
+// Map a CheckIRId into information about the corresponding file in both SemIR
+// and Clang's source manager.
+static auto GetFileInfo(Context& context, SemIR::CheckIRId ir_id) -> FileInfo {
+  const SemIR::File* sem_ir = &context.sem_ir();
+  int file_index = 0;
+
+  // If the file is imported, locate it in our imports map.
+  if (ir_id != context.sem_ir().check_ir_id()) {
+    auto import_id = context.check_ir_map().Get(ir_id);
+    CARBON_CHECK(import_id.has_value());
+    file_index = import_id.index + 1;
+
+    sem_ir = context.import_irs().Get(import_id).sem_ir;
+    CARBON_CHECK(sem_ir, "Node location in nonexistent IR");
+  }
+
+  // If we've seen this file before, reuse the same FileID.
+  auto& file_start_locs = context.cpp_carbon_file_locations();
+  if (static_cast<int>(file_start_locs.size()) <= file_index) {
+    // Never valid; prepare a slot for the caching below.
+    file_start_locs.resize(file_index + 1);
+  } else if (file_start_locs[file_index].isValid()) {
+    return {.sem_ir = sem_ir, .start_loc = file_start_locs[file_index]};
+  }
+
+  // We've not seen this file before. Create a corresponding virtual file in
+  // Clang's source manager.
+  // TODO: Consider recreating the complete import path instead of only the
+  // final entry.
+  const auto& source = sem_ir->parse_tree().tokens().source();
+  auto& src_mgr = context.ast_context().getSourceManager();
+  auto file_id = src_mgr.createFileID(
+      llvm::MemoryBufferRef(source.text(), source.filename()));
+  auto file_start_loc = src_mgr.getLocForStartOfFile(file_id);
+  file_start_locs[file_index] = file_start_loc;
+  return {.sem_ir = sem_ir, .start_loc = file_start_loc};
+}
+
+auto GetCppLocation(Context& context, SemIR::LocId loc_id)
+    -> clang::SourceLocation {
+  if (!context.sem_ir().clang_ast_unit()) {
+    return clang::SourceLocation();
+  }
+
+  // Break down the `LocId` into an import path. If that ends in a C++ location,
+  // we can just return that directly.
+  llvm::SmallVector<SemIR::AbsoluteNodeId> absolute_node_ids =
+      SemIR::GetAbsoluteNodeId(&context.sem_ir(), loc_id);
+  if (absolute_node_ids.back().check_ir_id() == SemIR::CheckIRId::Cpp) {
+    return context.sem_ir().clang_source_locs().Get(
+        absolute_node_ids.back().clang_source_loc_id());
+  }
+
+  // This is a location in Carbon code; get or create a corresponding file in
+  // Clang and build a corresponding location.
+  auto absolute_node_id = absolute_node_ids.back();
+  auto [ir, start_loc] = GetFileInfo(context, absolute_node_id.check_ir_id());
+  const auto& tree = ir->parse_tree();
+  auto offset =
+      tree.tokens().GetByteOffset(tree.node_token(absolute_node_id.node_id()));
+  return start_loc.getLocWithOffset(offset);
+}
+
+}  // namespace Carbon::Check

toolchain/check/cpp/location.h

@@ -0,0 +1,19 @@
+// 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
+
+#ifndef CARBON_TOOLCHAIN_CHECK_CPP_LOCATION_H_
+#define CARBON_TOOLCHAIN_CHECK_CPP_LOCATION_H_
+
+#include "toolchain/check/context.h"
+#include "toolchain/sem_ir/ids.h"
+
+namespace Carbon::Check {
+
+// Maps a Carbon source location into an equivalent Clang source location.
+auto GetCppLocation(Context& context, SemIR::LocId loc_id)
+    -> clang::SourceLocation;
+
+}  // namespace Carbon::Check
+
+#endif  // CARBON_TOOLCHAIN_CHECK_CPP_LOCATION_H_

toolchain/check/cpp/overload_resolution.cpp

@@ -4,15 +4,27 @@
 
 #include "toolchain/check/cpp/overload_resolution.h"
 
+#include "clang/Basic/DiagnosticSema.h"
 #include "clang/Sema/Overload.h"
 #include "clang/Sema/Sema.h"
 #include "toolchain/check/cpp/import.h"
+#include "toolchain/check/cpp/location.h"
 #include "toolchain/check/cpp/type_mapping.h"
 #include "toolchain/sem_ir/ids.h"
 #include "toolchain/sem_ir/typed_insts.h"
 
 namespace Carbon::Check {
 
+// Map a Carbon name into a C++ name.
+static auto GetCppName(Context& context, SemIR::NameId name_id)
+    -> clang::DeclarationName {
+  // TODO: Some special names should probably use different formatting. In
+  // particular, NameId::CppOperator should probably map back to a
+  // CXXOperatorName.
+  auto name_str = context.names().GetFormatted(name_id);
+  return clang::DeclarationName(&context.ast_context().Idents.get(name_str));
+}
+
 // Adds the given overload candidates to the candidate set.
 static auto AddOverloadCandidataes(clang::Sema& sema,
                                    clang::OverloadCandidateSet& candidate_set,
@@ -67,13 +79,6 @@ auto PerformCppOverloadResolution(Context& context, SemIR::LocId loc_id,
                                   SemIR::InstId self_id,
                                   llvm::ArrayRef<SemIR::InstId> arg_ids)
     -> SemIR::InstId {
-  Diagnostics::AnnotationScope annotate_diagnostics(
-      &context.emitter(), [&](auto& builder) {
-        CARBON_DIAGNOSTIC(InCallToCppFunction, Note,
-                          "in call to Cpp function here");
-        builder.Note(loc_id, InCallToCppFunction);
-      });
-
   // Map Carbon call argument types to C++ types.
   clang::Expr* self_expr = nullptr;
   if (self_id.has_value()) {
@@ -82,67 +87,63 @@ auto PerformCppOverloadResolution(Context& context, SemIR::LocId loc_id,
       return SemIR::ErrorInst::InstId;
     }
   }
-  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;
 
   const SemIR::CppOverloadSet& overload_set =
       context.cpp_overload_sets().Get(overload_set_id);
 
+  clang::SourceLocation loc = GetCppLocation(context, loc_id);
+
   // Add candidate functions from the name lookup.
   clang::OverloadCandidateSet candidate_set(
-      // TODO: Add location accordingly.
-      clang::SourceLocation(),
-      clang::OverloadCandidateSet::CandidateSetKind::CSK_Normal);
+      loc, clang::OverloadCandidateSet::CandidateSetKind::CSK_Normal);
 
   clang::ASTUnit* ast = context.sem_ir().clang_ast_unit();
   CARBON_CHECK(ast);
   clang::Sema& sema = ast->getSema();
 
   AddOverloadCandidataes(sema, candidate_set, overload_set.candidate_functions,
-                         self_expr, *arg_exprs);
+                         self_expr, arg_exprs);
 
   // Find best viable function among the candidates.
   clang::OverloadCandidateSet::iterator best_viable_fn;
   clang::OverloadingResult overloading_result =
-      // TODO: Add location accordingly.
-      candidate_set.BestViableFunction(sema, clang::SourceLocation(),
-                                       best_viable_fn);
+      candidate_set.BestViableFunction(sema, loc, best_viable_fn);
 
   switch (overloading_result) {
     case clang::OverloadingResult::OR_Success: {
       // TODO: Handle the cases when Function is null.
       CARBON_CHECK(best_viable_fn->Function);
-      sema.MarkFunctionReferenced(clang::SourceLocation(),
-                                  best_viable_fn->Function);
+      sema.MarkFunctionReferenced(loc, best_viable_fn->Function);
       SemIR::InstId result =
           ImportCppFunctionDecl(context, loc_id, best_viable_fn->Function);
       return result;
     }
     case clang::OverloadingResult::OR_No_Viable_Function: {
-      // TODO: Add notes with the candidates.
-      CARBON_DIAGNOSTIC(CppOverloadingNoViableFunctionFound, Error,
-                        "no matching function for call to `{0}`",
-                        SemIR::NameId);
-      context.emitter().Emit(loc_id, CppOverloadingNoViableFunctionFound,
-                             overload_set.name_id);
+      candidate_set.NoteCandidates(
+          clang::PartialDiagnosticAt(
+              loc, sema.PDiag(clang::diag::err_ovl_no_viable_function_in_call)
+                       << GetCppName(context, overload_set.name_id)),
+          sema, clang::OCD_AllCandidates, arg_exprs);
       return SemIR::ErrorInst::InstId;
     }
     case clang::OverloadingResult::OR_Ambiguous: {
-      // TODO: Add notes with the candidates.
-      CARBON_DIAGNOSTIC(CppOverloadingAmbiguousCandidatesFound, Error,
-                        "call to `{0}` is ambiguous", SemIR::NameId);
-      context.emitter().Emit(loc_id, CppOverloadingAmbiguousCandidatesFound,
-                             overload_set.name_id);
+      candidate_set.NoteCandidates(
+          clang::PartialDiagnosticAt(
+              loc, sema.PDiag(clang::diag::err_ovl_ambiguous_call)
+                       << GetCppName(context, overload_set.name_id)),
+          sema, clang::OCD_AmbiguousCandidates, arg_exprs);
       return SemIR::ErrorInst::InstId;
     }
     case clang::OverloadingResult::OR_Deleted: {
-      // TODO: Add notes with the candidates.
-      CARBON_DIAGNOSTIC(CppOverloadingDeletedFunctionFound, Error,
-                        "call to deleted function `{0}`", SemIR::NameId);
-      context.emitter().Emit(loc_id, CppOverloadingDeletedFunctionFound,
-                             overload_set.name_id);
+      sema.DiagnoseUseOfDeletedFunction(
+          loc, clang::SourceRange(loc, loc),
+          GetCppName(context, overload_set.name_id), candidate_set,
+          best_viable_fn->Function, arg_exprs);
       return SemIR::ErrorInst::InstId;
     }
   }

toolchain/check/cpp/type_mapping.cpp

@@ -16,6 +16,7 @@
 #include "toolchain/base/value_ids.h"
 #include "toolchain/check/context.h"
 #include "toolchain/check/convert.h"
+#include "toolchain/check/cpp/location.h"
 #include "toolchain/check/literal.h"
 #include "toolchain/sem_ir/class.h"
 #include "toolchain/sem_ir/expr_info.h"
@@ -272,9 +273,9 @@ auto InventClangArg(Context& context, SemIR::InstId arg_id) -> clang::Expr* {
 
   // TODO: Avoid heap allocating more of these on every call. Either cache them
   // somewhere or put them on the stack.
-  return new (context.ast_context()) clang::OpaqueValueExpr(
-      // TODO: Add location accordingly.
-      clang::SourceLocation(), arg_cpp_type.getNonReferenceType(), value_kind);
+  return new (context.ast_context())
+      clang::OpaqueValueExpr(GetCppLocation(context, SemIR::LocId(arg_id)),
+                             arg_cpp_type.getNonReferenceType(), value_kind);
 }
 
 auto InventClangArgs(Context& context, llvm::ArrayRef<SemIR::InstId> arg_ids)