Reject ill-formed template bodies in BestOverloadFunctionMatch and capture their diagnostics

lib/CppInterOp/CppInterOp.cpp

@@ -71,6 +71,7 @@
 #include "clang/Basic/Specifiers.h"
 #include "clang/Basic/Version.h"
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Interpreter/Interpreter.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Overload.h"
@@ -790,6 +791,51 @@
   clang::IgnoringDiagConsumer fIgnoringClient;
 #endif
 };
+
+/// Renders the diagnostics emitted during its lifetime into a string, formatted
+/// as clang prints them (with source snippets and instantiation notes), instead
+/// of passing them to the installed consumer. Like clangSilent this keeps
+/// probing operations quiet, but the diagnostics remain available to report
+/// where appropriate.
+class clangCapture {
+public:
+  clangCapture(clang::DiagnosticsEngine& diag, const clang::LangOptions& LO,
+               const clang::Preprocessor* PP)
+      : fDiagEngine(diag), fOldClient(diag.getClient()),
+        fOldOwnedClient(diag.takeClient()), fStream(fText),
+        fPrinter(fStream, diag.getDiagnosticOptions()) {
+    // required to initialize the printer's internal clang::TextDiagnostic
+    fPrinter.BeginSourceFile(LO, PP);
+    fDiagEngine.setClient(&fPrinter, /*ShouldOwnClient=*/false);
+  }
+
+  clangCapture(const clangCapture&) = delete;
+  clangCapture& operator=(const clangCapture&) = delete;
+  clangCapture(clangCapture&&) = delete;
+  clangCapture& operator=(clangCapture&&) = delete;
+
+  ~clangCapture() {
+    fPrinter.EndSourceFile();
+    if (fOldOwnedClient)
+      fDiagEngine.setClient(fOldOwnedClient.release(),
+                            /*ShouldOwnClient=*/true);
+    else
+      fDiagEngine.setClient(fOldClient, /*ShouldOwnClient=*/false);
+  }
+
+  const std::string& str() {
+    fStream.flush();
+    return fText;
+  }
+
+private:
+  clang::DiagnosticsEngine& fDiagEngine;
+  clang::DiagnosticConsumer* fOldClient;
+  std::unique_ptr<clang::DiagnosticConsumer> fOldOwnedClient;
+  std::string fText;
+  llvm::raw_string_ostream fStream;
+  clang::TextDiagnosticPrinter fPrinter;
+};
 } // namespace
 
 // See TClingClassInfo::IsLoaded
@@ -2941,6 +2987,16 @@
   return INTEROP_RETURN(!funcs.empty());
 }
 
+// Diagnostics from the instantiation of the body of the best match in the last
+// call to BestOverloadFunctionMatch on this thread
+// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables)
+static thread_local std::string LastOverloadMatchDiagnostics;
+
+std::string GetLastOverloadMatchDiagnostics() {
+  INTEROP_TRACE();
+  return INTEROP_RETURN(LastOverloadMatchDiagnostics);
+}
+
 // Adapted from inner workings of Sema::BuildCallExpr
 FuncRef
 BestOverloadFunctionMatch(const std::vector<FuncRef>& candidates,
@@ -2950,6 +3006,7 @@
                           bool is_operator /*= false*/) {
   INTEROP_TRACE(candidates, explicit_types, arg_types,
                 INTEROP_OUT(ambiguous_candidates), is_operator);
+  LastOverloadMatchDiagnostics.clear();
   auto& S = getSema();
   auto& C = S.getASTContext();
 
@@ -3061,6 +3118,46 @@
     }
   }
 
+  // If the winner is a not-yet-instantiated template specialization, force
+  // the instantiation of its body now, and reject the match if the body turns
+  // out to be ill-formed. Callers use this function to probe whether a valid
+  // instantiation exists, so an ill-formed body must surface as "no viable
+  // function" here rather than as a hard error when a call wrapper is compiled
+  // for it later. This mirrors cling's LookupHelper::overloadFunctionSelector.
+  // Note that probing with an expression-SFINAE helper instead is not
+  // equivalent: C++20 rewritten (reversed) operator candidates make
+  // comparisons like a member operator==(const Base&) ambiguous, which is a
+  // hard substitution failure in a SFINAE context but only a warning in a
+  // function body. DefinitionRequired stays false: a pattern without a body
+  // (defined in a library) is still a valid match.
+  // The body is ill-formed if any error occurred during its instantiation
+  // (including those of the templates it instantiates in turn), not only if
+  // the specialization itself was marked invalid: e.g. a failing static_assert
+  // or an error in a nested instantiation leaves the specialization valid, and
+  // accepting it would either run code that should not have compiled, or fail
+  // at link time for the missing nested definition. The diagnostics are
+  // captured rather than printed, and can be retrieved with
+  // GetLastOverloadMatchDiagnostics.
+  if (Result && Result->isTemplateInstantiation() && !Result->isDefined()) {
+    DiagnosticsEngine& Diags = S.getDiagnostics();
+    bool Failed;
+    {
+      clangCapture Capture(Diags, S.getLangOpts(), &S.getPreprocessor());
+      clang::DiagnosticErrorTrap ErrorTrap(Diags);
+      S.InstantiateFunctionDefinition(Loc, Result, /*Recursive=*/true);
+      Failed = Result->isInvalidDecl() || ErrorTrap.hasErrorOccurred();
+      LastOverloadMatchDiagnostics = Capture.str();
+    }
+    if (Failed) {
+      // Mark the specialization invalid, so that later lookups reject it as a
+      // substitution failure instead of finding it already defined.
+      Result->setInvalidDecl();
+      Diags.Reset(/*soft=*/true);
+      Diags.getClient()->clear();
+      Result = nullptr;
+    }
+  }
+
   delete[] Exprs;
   return INTEROP_RETURN(Result);
 }
@@ -6426,6 +6523,11 @@
       // FIXME: Diagnose what happened.
       (void)Result;
     }
+    // Never hand out a specialization whose (attempted) instantiation turned
+    // out to be ill-formed, e.g. one rejected by BestOverloadFunctionMatch:
+    // any use of it, such as compiling a call wrapper, can only fail.
+    if (Specialization && Specialization->isInvalidDecl())
+      return nullptr;
     if (instantiate_body)
       InstantiateFunctionDefinition(Specialization);
     return Specialization;

lib/CppInterOp/CppInterOp.td

@@ -1248,6 +1248,16 @@ ambiguous, the best viable candidates are appended to
   ];
 }
 
+def GetLastOverloadMatchDiagnostics : CppInterOpAPI {
+  let Doc = [{\returns the compiler diagnostics (errors and warnings, formatted as
+clang prints them) from the instantiation of the body of the best match in the
+last call to BestOverloadFunctionMatch on this thread, or an empty string if no
+body was instantiated. They are not printed, since overload resolution may be
+used to probe for a valid match, and a match is rejected if its body turns out
+to be ill-formed.}];
+  let ReturnType = "std::string";
+}
+
 def CodeComplete : CppInterOpAPI {
   let Doc = [{Append all code completion suggestions to Results.
 \param[out] Results CC suggestions for code fragment (appended).

unittests/CppInterOp/FunctionReflectionTest.cpp

@@ -4,6 +4,7 @@
 #include "../../lib/CppInterOp/Unwrap.h"
 
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/Basic/Version.h"
 #include "clang/Frontend/CompilerInstance.h"
@@ -12,6 +13,7 @@
 #include <CppInterOp/CppInterOpTypes.h>
 #include <cstdint>
 #include <llvm/ADT/ArrayRef.h>
+#include <llvm/ADT/StringRef.h>
 #include <llvm/Support/Casting.h>
 #include <llvm/Support/raw_ostream.h>
 
@@ -3149,6 +3151,88 @@ TYPED_TEST(CPPINTEROP_TEST_MODE,
             "void consume(B0)");
 }
 
+TYPED_TEST(CPPINTEROP_TEST_MODE,
+           FunctionReflection_BestOverloadFunctionMatchIllFormedBody) {
+  // A match whose body turns out to be ill-formed when instantiated is
+  // rejected, and the diagnostics from instantiating the body are available
+  // from GetLastOverloadMatchDiagnostics rather than printed.
+  std::vector<Decl*> Decls;
+  std::string code = R"(
+  template <typename T> int failing_assert(T) {
+    static_assert(sizeof(T) == 0, "failing_assert is ill-formed");
+    return 0;
+  }
+  template <typename T> struct Nested {
+    static int get() { return T::missing; }
+  };
+  template <typename T> int nested_error(T) { return Nested<T>::get(); }
+  // the warning is enabled explicitly, since it is disabled by default in
+  // some configurations (e.g. cling on Windows)
+  #pragma clang diagnostic push
+  #pragma clang diagnostic warning "-Wdeprecated-declarations"
+  template <typename T> struct Old {
+    [[deprecated("use something else")]] static int get() { return 1; }
+  };
+  template <typename T> int warns(T) { return Old<T>::get(); }
+  #pragma clang diagnostic pop
+  template <typename T> int fine(T) { return 1; }
+  )";
+  GetAllTopLevelDecls(code, Decls);
+
+  auto Candidates = [&Decls](llvm::StringRef name) {
+    std::vector<Cpp::FuncRef> candidates;
+    for (auto* D : Decls)
+      if (auto* FTD = llvm::dyn_cast<FunctionTemplateDecl>(D);
+          FTD && FTD->getName() == name)
+        candidates.emplace_back(D);
+    EXPECT_EQ(candidates.size(), 1);
+    return candidates;
+  };
+
+  std::vector<Cpp::TemplateArgInfo> args = {Cpp::GetType("int").data};
+  std::vector<Cpp::FuncRef> ambiguous_candidates;
+
+  // a failing static_assert leaves the specialization valid, but the body is
+  // ill-formed
+  auto failing_assert = Candidates("failing_assert");
+  EXPECT_FALSE(Cpp::BestOverloadFunctionMatch(failing_assert, {}, args,
+                                              ambiguous_candidates));
+  EXPECT_TRUE(ambiguous_candidates.empty());
+  std::string diags = Cpp::GetLastOverloadMatchDiagnostics();
+  EXPECT_NE(diags.find("error"), std::string::npos) << diags;
+  EXPECT_NE(diags.find("failing_assert is ill-formed"), std::string::npos)
+      << diags;
+  // the rejected specialization stays rejected
+  EXPECT_FALSE(Cpp::BestOverloadFunctionMatch(failing_assert, {}, args,
+                                              ambiguous_candidates));
+  EXPECT_FALSE(
+      Cpp::InstantiateTemplate(Cpp::unwrap<Decl>(failing_assert[0]), args)
+          .data);
+
+  // an error in a nested instantiation
+  EXPECT_FALSE(Cpp::BestOverloadFunctionMatch(Candidates("nested_error"), {},
+                                              args, ambiguous_candidates));
+  diags = Cpp::GetLastOverloadMatchDiagnostics();
+  EXPECT_NE(diags.find("error"), std::string::npos) << diags;
+  EXPECT_NE(diags.find("Nested<int>::get"), std::string::npos) << diags;
+
+  // warnings do not reject the match, and are reported as well
+  auto warns = Candidates("warns");
+  EXPECT_TRUE(
+      Cpp::BestOverloadFunctionMatch(warns, {}, args, ambiguous_candidates));
+  diags = Cpp::GetLastOverloadMatchDiagnostics();
+  EXPECT_NE(diags.find("warning"), std::string::npos) << diags;
+  EXPECT_NE(diags.find("use something else"), std::string::npos) << diags;
+  // no body is instantiated for an already defined match
+  EXPECT_TRUE(
+      Cpp::BestOverloadFunctionMatch(warns, {}, args, ambiguous_candidates));
+  EXPECT_EQ(Cpp::GetLastOverloadMatchDiagnostics(), "");
+
+  EXPECT_TRUE(Cpp::BestOverloadFunctionMatch(Candidates("fine"), {}, args,
+                                             ambiguous_candidates));
+  EXPECT_EQ(Cpp::GetLastOverloadMatchDiagnostics(), "");
+}
+
 TYPED_TEST(CPPINTEROP_TEST_MODE,
            FunctionReflection_BestOverloadFunctionMatch_CandidateKinds) {
   std::vector<Decl*> Decls;