From 9ceb0918accfb752987ff502ae794be15d5ce35f Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Sun, 8 Nov 2026 07:36:16 -0500 Subject: [PATCH 1/2] Reject ill-formed template bodies in BestOverloadFunctionMatch and capture their diagnostics When the best match is a not-yet-instantiated function template specialization, instantiate its body and reject the match if the body is ill-formed. Callers use BestOverloadFunctionMatch to probe whether a valid instantiation exists, so an ill-formed body should surface as "no viable function" rather than as a hard error when a call wrapper is compiled for it later. This mirrors cling's LookupHelper::overloadFunctionSelector. The match is rejected if any error occurred during the instantiation (using a DiagnosticErrorTrap), not only if the specialization itself was marked invalid: errors such as a failing static_assert or an error in a nested instantiation leave the specialization valid, and accepting it would either run code which should not have compiled, or fail at link time for the missing nested definition. The rejected specialization is marked invalid, so that later lookups reject it as well, and InstantiateTemplate no longer returns a specialization marked invalid. The diagnostics from instantiating the body are rendered into a string as clang would print them (with source snippets and instantiation notes) instead of being printed, and are available through the new GetLastOverloadMatchDiagnostics, so that callers can report them where appropriate (e.g. the errors when no valid match is found, or warnings from a successful instantiation), while probing for a valid match stays quiet. Co-Authored-By: Claude Opus 5.5 --- lib/CppInterOp/CppInterOp.cpp | 96 +++++++++++++++++++ lib/CppInterOp/CppInterOp.td | 10 ++ .../CppInterOp/FunctionReflectionTest.cpp | 77 +++++++++++++++ 3 files changed, 183 insertions(+) diff --git a/lib/CppInterOp/CppInterOp.cpp b/lib/CppInterOp/CppInterOp.cpp index b9ece7f05..3b6f2e0a4 100644 --- a/lib/CppInterOp/CppInterOp.cpp +++ b/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,46 @@ class clangSilent { 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() { + 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 fOldOwnedClient; + std::string fText; + llvm::raw_string_ostream fStream; + clang::TextDiagnosticPrinter fPrinter; +}; } // namespace // See TClingClassInfo::IsLoaded @@ -2941,6 +2982,15 @@ bool GetClassTemplatedMethods(const std::string& name, ConstDeclRef parent, 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 +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& candidates, @@ -2950,6 +3000,7 @@ BestOverloadFunctionMatch(const std::vector& candidates, 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 +3112,46 @@ BestOverloadFunctionMatch(const std::vector& candidates, } } + // 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 +6517,11 @@ static Decl* InstantiateTemplate(TemplateDecl* TemplateD, // 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; diff --git a/lib/CppInterOp/CppInterOp.td b/lib/CppInterOp/CppInterOp.td index a17f33462..ab65e7d99 100644 --- a/lib/CppInterOp/CppInterOp.td +++ b/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). diff --git a/unittests/CppInterOp/FunctionReflectionTest.cpp b/unittests/CppInterOp/FunctionReflectionTest.cpp index 608fdfa87..56f85e7b8 100644 --- a/unittests/CppInterOp/FunctionReflectionTest.cpp +++ b/unittests/CppInterOp/FunctionReflectionTest.cpp @@ -3149,6 +3149,83 @@ 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 Decls; + std::string code = R"( + template int failing_assert(T) { + static_assert(sizeof(T) == 0, "failing_assert is ill-formed"); + return 0; + } + template struct Nested { + static int get() { return T::missing; } + }; + template int nested_error(T) { return Nested::get(); } + template struct Old { + [[deprecated("use something else")]] static int get() { return 1; } + }; + template int warns(T) { return Old::get(); } + template int fine(T) { return 1; } + )"; + GetAllTopLevelDecls(code, Decls); + + auto Candidates = [&Decls](llvm::StringRef name) { + std::vector candidates; + for (auto* D : Decls) + if (auto* FTD = llvm::dyn_cast(D); + FTD && FTD->getName() == name) + candidates.emplace_back(D); + EXPECT_EQ(candidates.size(), 1); + return candidates; + }; + + std::vector args = {Cpp::GetType("int").data}; + std::vector 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(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::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 Decls; From c1259b3baf89b0a3c56c0d7606843a88c86c9912 Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Sun, 8 Nov 2026 10:49:50 -0500 Subject: [PATCH 2/2] Make the ill-formed body test independent of the default warning flags, address clang-tidy Enable -Wdeprecated-declarations explicitly around the declarations whose instantiation is expected to warn, since cling disables it by default on Windows (where the test failed). Delete the copy and move operations of clangCapture, mark the thread-local diagnostics string as an intended non-const global, and include the headers used directly by the test. Co-Authored-By: Claude Opus 5.5 --- lib/CppInterOp/CppInterOp.cpp | 6 ++++++ unittests/CppInterOp/FunctionReflectionTest.cpp | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/lib/CppInterOp/CppInterOp.cpp b/lib/CppInterOp/CppInterOp.cpp index 3b6f2e0a4..1dade97f1 100644 --- a/lib/CppInterOp/CppInterOp.cpp +++ b/lib/CppInterOp/CppInterOp.cpp @@ -809,6 +809,11 @@ class clangCapture { 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) @@ -2984,6 +2989,7 @@ bool GetClassTemplatedMethods(const std::string& name, ConstDeclRef parent, // 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() { diff --git a/unittests/CppInterOp/FunctionReflectionTest.cpp b/unittests/CppInterOp/FunctionReflectionTest.cpp index 56f85e7b8..acd3a897f 100644 --- a/unittests/CppInterOp/FunctionReflectionTest.cpp +++ b/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 #include #include +#include #include #include @@ -3164,10 +3166,15 @@ TYPED_TEST(CPPINTEROP_TEST_MODE, static int get() { return T::missing; } }; template int nested_error(T) { return Nested::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 struct Old { [[deprecated("use something else")]] static int get() { return 1; } }; template int warns(T) { return Old::get(); } + #pragma clang diagnostic pop template int fine(T) { return 1; } )"; GetAllTopLevelDecls(code, Decls);