From d77d9906de40420c81cdaaa82d2bc47033fb1811 Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Wed, 8 Mar 2023 04:18:46 +0100 Subject: [PATCH 01/15] explicitly throw errors when WrapperCall fails --- .../cppyy-backend/clingwrapper/src/clingwrapper.cxx | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx index 6343d2bc16b72..3ae8849ff427d 100644 --- a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx +++ b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx @@ -1045,6 +1045,7 @@ T CallT(Cppyy::TCppMethod_t method, Cppyy::TCppObject_t self, size_t nargs, void T t{}; if (WrapperCall(method, nargs, args, (void*)self, &t)) return t; + throw std::runtime_error("failed to resolve function"); return (T)-1; } @@ -1075,6 +1076,7 @@ void* Cppyy::CallR(TCppMethod_t method, TCppObject_t self, size_t nargs, void* a void* r = nullptr; if (WrapperCall(method, nargs, args, (void*)self, &r)) return r; + throw std::runtime_error("failed to resolve function"); return nullptr; } @@ -1088,9 +1090,12 @@ char* Cppyy::CallS( cstr = cppstring_to_cstring(*cppresult); *length = cppresult->size(); cppresult->std::string::~basic_string(); - } else - *length = 0; - free((void*)cppresult); + free((void *)cppresult); + } else { + *length = 0; + free((void *)cppresult); + throw std::runtime_error("failed to resolve function"); + } return cstr; } @@ -1100,6 +1105,7 @@ Cppyy::TCppObject_t Cppyy::CallConstructor( void* obj = nullptr; if (WrapperCall(method, nargs, args, nullptr, &obj)) return (TCppObject_t)obj; + throw std::runtime_error("failed to resolve function"); return (TCppObject_t)0; } @@ -1117,6 +1123,7 @@ Cppyy::TCppObject_t Cppyy::CallO(TCppMethod_t method, if (WrapperCall(method, nargs, args, self, obj)) return (TCppObject_t)obj; ::operator delete(obj); + throw std::runtime_error("failed to resolve function"); return (TCppObject_t)0; } From b8ea44fc9e61ee20fa66797e8bc252a19ffe0a8a Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Wed, 8 Mar 2023 04:19:19 +0100 Subject: [PATCH 02/15] improve error message when function call fails --- bindings/pyroot/cppyy/CPyCppyy/src/CPPMethod.cxx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/CPPMethod.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/CPPMethod.cxx index df1dbef0567a4..e66e2fb0e8de7 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/CPPMethod.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/CPPMethod.cxx @@ -321,15 +321,15 @@ void CPyCppyy::CPPMethod::SetPyError_(PyObject* msg) if (!isCppExc) { // this is the case where no Python error has occurred yet, and we set a new // error with context info - PyErr_Format(errtype, "%s =>\n %s: %s", cdoc, cname, cmsg ? cmsg : ""); + PyErr_Format(errtype, "Failed to call \"%s\" =>\n %s: %s", cdoc, cname, cmsg ? cmsg : ""); } else { // augment the top message with context information PyObject *&topMessage = ((CPPExcInstance*)evalue)->fTopMessage; Py_XDECREF(topMessage); if (msg) { - topMessage = CPyCppyy_PyText_FromFormat("%s =>\n %s: %s | ", cdoc, cname, cmsg); + topMessage = CPyCppyy_PyText_FromFormat("Failed to call \"%s\" =>\n %s: %s | ", cdoc, cname, cmsg); } else { - topMessage = CPyCppyy_PyText_FromFormat("%s =>\n %s: ", cdoc, cname); + topMessage = CPyCppyy_PyText_FromFormat("Failed to call \"%s\" =>\n %s: ", cdoc, cname); } // restore the updated error #if PY_VERSION_HEX >= 0x030c0000 From 93bb21a0366210743b1624355be9545ee91ee764 Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Wed, 8 Mar 2023 04:19:37 +0100 Subject: [PATCH 03/15] catch errors during template instantiation --- .../cling/lib/Interpreter/LookupHelper.cpp | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/interpreter/cling/lib/Interpreter/LookupHelper.cpp b/interpreter/cling/lib/Interpreter/LookupHelper.cpp index 66fed6c29fa10..1a052b8244994 100644 --- a/interpreter/cling/lib/Interpreter/LookupHelper.cpp +++ b/interpreter/cling/lib/Interpreter/LookupHelper.cpp @@ -13,6 +13,7 @@ #include "DeclUnloader.h" #include "cling/Interpreter/Interpreter.h" #include "cling/Utils/AST.h" +#include "cling/Utils/Diagnostics.h" #include "cling/Utils/ParserStateRAII.h" #include "clang/AST/ASTContext.h" @@ -1113,21 +1114,14 @@ namespace cling { } // - // Tell the diagnostic engine to ignore all diagnostics. + // Suppress printing of diagnostics (we cannot simply ignore them + // since then it is not possible the check for errors programmatically + // either) // - bool OldSuppressAllDiagnostics - = S.getDiagnostics().getSuppressAllDiagnostics(); - S.getDiagnostics().setSuppressAllDiagnostics( - diagOnOff == LookupHelper::NoDiagnostics); - - struct ResetDiagSuppression { - bool _Old; - Sema& _S; - ResetDiagSuppression(Sema &S, bool Old): _Old(Old), _S(S) {} - ~ResetDiagSuppression() { - _S.getDiagnostics().setSuppressAllDiagnostics(_Old); - } - } DiagSuppressionRAII(S, OldSuppressAllDiagnostics); + + utils::DiagnosticsStore ds(S.getDiagnostics(), false, /*Own*/ + diagOnOff == + LookupHelper::WithDiagnostics /*Report*/); // // Construct the overload candidate set. @@ -1216,6 +1210,9 @@ namespace cling { S.InstantiateFunctionDefinition(SourceLocation(), TheDecl, true /*recursive instantiation*/); } + if (S.getDiagnostics().hasErrorOccurred()) { + TheDecl->setInvalidDecl(); + } if (TheDecl->isInvalidDecl()) { // if the decl is invalid try to clean up UnloadDecl(&S, const_cast(TheDecl)); @@ -1824,6 +1821,9 @@ namespace cling { if (!fdecl->isDefined()) S.InstantiateFunctionDefinition(loc, fdecl, true /*recursive instantiation*/); + if (S.getDiagnostics().hasErrorOccurred()) { + fdecl->setInvalidDecl(); + } if (fdecl->isInvalidDecl()) { // if the decl is invalid try to clean up UnloadDecl(&S, fdecl); From 2e1fbf38e1da44052b0df2e9473935487e479bb3 Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Sat, 18 Mar 2023 16:05:09 +0100 Subject: [PATCH 04/15] handle failures through the transaction rather than directly unloading the decl --- .../cling/lib/Interpreter/LookupHelper.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/interpreter/cling/lib/Interpreter/LookupHelper.cpp b/interpreter/cling/lib/Interpreter/LookupHelper.cpp index 1a052b8244994..3c051122a12e9 100644 --- a/interpreter/cling/lib/Interpreter/LookupHelper.cpp +++ b/interpreter/cling/lib/Interpreter/LookupHelper.cpp @@ -1214,8 +1214,7 @@ namespace cling { TheDecl->setInvalidDecl(); } if (TheDecl->isInvalidDecl()) { - // if the decl is invalid try to clean up - UnloadDecl(&S, const_cast(TheDecl)); + // don't unload the decl, it will be done by failing the transaction return 0; } } @@ -1584,10 +1583,13 @@ namespace cling { if (!inputEval(GivenArgs,funcArgs,diagOnOff,P,Interp,LH)) return 0; Interpreter::PushTransactionRAII pushedT(Interp); - return findFunction(foundDC, - funcName, GivenArgs, objectIsConst, - Context, Interp, functionSelector, - diagOnOff); + auto res = findFunction(foundDC, funcName, GivenArgs, objectIsConst, + Context, Interp, functionSelector, diagOnOff); + if (!res) { + auto* T = const_cast(Interp->getCurrentTransaction()); + T->setIssuedDiags(Transaction::kErrors); + } + return res; } struct NoParse { @@ -1825,8 +1827,7 @@ namespace cling { fdecl->setInvalidDecl(); } if (fdecl->isInvalidDecl()) { - // if the decl is invalid try to clean up - UnloadDecl(&S, fdecl); + // don't unload the decl, it will be done by failing the transaction return 0; } return fdecl; From 5bf70b50381138764c5b241645217e59a8cbb63c Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Tue, 21 Mar 2023 00:58:22 +0100 Subject: [PATCH 05/15] fix handling of failures via the transaction --- .../cling/lib/Interpreter/LookupHelper.cpp | 67 ++++++++++--------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/interpreter/cling/lib/Interpreter/LookupHelper.cpp b/interpreter/cling/lib/Interpreter/LookupHelper.cpp index 3c051122a12e9..5462ce85ff13b 100644 --- a/interpreter/cling/lib/Interpreter/LookupHelper.cpp +++ b/interpreter/cling/lib/Interpreter/LookupHelper.cpp @@ -1113,16 +1113,6 @@ namespace cling { ObjExprClassification = ObjExpr.Classify(Context); } - // - // Suppress printing of diagnostics (we cannot simply ignore them - // since then it is not possible the check for errors programmatically - // either) - // - - utils::DiagnosticsStore ds(S.getDiagnostics(), false, /*Own*/ - diagOnOff == - LookupHelper::WithDiagnostics /*Report*/); - // // Construct the overload candidate set. // @@ -1210,11 +1200,12 @@ namespace cling { S.InstantiateFunctionDefinition(SourceLocation(), TheDecl, true /*recursive instantiation*/); } - if (S.getDiagnostics().hasErrorOccurred()) { - TheDecl->setInvalidDecl(); - } - if (TheDecl->isInvalidDecl()) { - // don't unload the decl, it will be done by failing the transaction + if (TheDecl->isInvalidDecl() && + !S.getDiagnostics().hasErrorOccurred()) { + // if the decl is invalid try to clean up (unless an error occurred, + // in which case this will be handled instead by the failed + // transaction) + UnloadDecl(&S, const_cast(TheDecl)); return 0; } } @@ -1534,11 +1525,27 @@ namespace cling { // Lookup failed. return 0; } - return functionSelector(foundDC,objectIsConst,GivenArgs, - Result, - FuncNameInfo, - FuncTemplateArgs, - Context, P, S, diagOnOff); + + // Suppress printing of diagnostics (we cannot simply ignore them + // since then it is not possible the check for errors programmatically + // either) + + utils::DiagnosticsStore ds(S.getDiagnostics(), false, /*Own*/ + diagOnOff == + LookupHelper::WithDiagnostics /*Report*/); + + auto res = functionSelector(foundDC, objectIsConst, GivenArgs, Result, + FuncNameInfo, FuncTemplateArgs, Context, P, S, + diagOnOff); + + if (S.getDiagnostics().hasErrorOccurred()) { + auto* transaction = + const_cast(Interp->getCurrentTransaction()); + transaction->setIssuedDiags(Transaction::kErrors); + return 0; + } + + return res; } template @@ -1583,13 +1590,8 @@ namespace cling { if (!inputEval(GivenArgs,funcArgs,diagOnOff,P,Interp,LH)) return 0; Interpreter::PushTransactionRAII pushedT(Interp); - auto res = findFunction(foundDC, funcName, GivenArgs, objectIsConst, - Context, Interp, functionSelector, diagOnOff); - if (!res) { - auto* T = const_cast(Interp->getCurrentTransaction()); - T->setIssuedDiags(Transaction::kErrors); - } - return res; + return findFunction(foundDC, funcName, GivenArgs, objectIsConst, Context, + Interp, functionSelector, diagOnOff); } struct NoParse { @@ -1823,11 +1825,12 @@ namespace cling { if (!fdecl->isDefined()) S.InstantiateFunctionDefinition(loc, fdecl, true /*recursive instantiation*/); - if (S.getDiagnostics().hasErrorOccurred()) { - fdecl->setInvalidDecl(); - } - if (fdecl->isInvalidDecl()) { - // don't unload the decl, it will be done by failing the transaction + if (fdecl->isInvalidDecl() && + !S.getDiagnostics().hasErrorOccurred()) { + // if the decl is invalid try to clean up (unless an error occurred, + // in which case this will be handled instead by the failed + // transaction) + UnloadDecl(&S, fdecl); return 0; } return fdecl; From 81dd6207d2babaa78f7534d6dda97e4d0f0698a2 Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Tue, 21 Mar 2023 14:28:51 +0100 Subject: [PATCH 06/15] implement redirection of interpreter diagonstic output to user-provided ostream --- core/meta/inc/TInterpreter.h | 34 ++++++++++++++++++++++++ core/metacling/src/TCling.cxx | 30 +++++++++++++++++++++ core/metacling/src/TCling.h | 2 ++ core/metacling/src/TClingDiagnostics.cxx | 11 ++++++++ core/metacling/src/TClingDiagnostics.h | 15 +++++++++++ 5 files changed, 92 insertions(+) diff --git a/core/meta/inc/TInterpreter.h b/core/meta/inc/TInterpreter.h index 59f2a09b91039..e09ec35b01ad4 100644 --- a/core/meta/inc/TInterpreter.h +++ b/core/meta/inc/TInterpreter.h @@ -64,6 +64,7 @@ class TInterpreter : public TNamed { virtual Bool_t SetSuspendAutoParsing(Bool_t value) = 0; friend class SuspendAutoParsing; + friend class RedirectDiagnostics; public: // See as in TSchemaType.h. @@ -129,6 +130,39 @@ class TInterpreter : public TNamed { ~SuspendAutoLoadingRAII() { fInterp->SetClassAutoLoading(fOldValue); } }; +private: + bool fIsRedirectingDiagnostics = false; + +public: + class DiagnosticsRAII { + public: + virtual ~DiagnosticsRAII() {}; + }; + + class RedirectDiagnostics { + TInterpreter *fInterp; + bool fOldValue; + std::unique_ptr fRedirect; + + public: + RedirectDiagnostics(TInterpreter *where, std::ostream &os, bool enableColors = false, unsigned int indent = 0) + : fInterp(where), + fOldValue(fInterp->fIsRedirectingDiagnostics), + fRedirect(fInterp->MakeRedirectDiagnosticsRAII(os, enableColors, indent)) + { + fInterp->fIsRedirectingDiagnostics = true; + } + + ~RedirectDiagnostics() { fInterp->fIsRedirectingDiagnostics = fOldValue; } + }; + + bool IsRedirectingDiagnostics() const { return fIsRedirectingDiagnostics; } + +protected: + virtual std::unique_ptr + MakeRedirectDiagnosticsRAII(std::ostream &os, bool enableColors = false, unsigned int indent = 0) = 0; + +public: typedef int (*AutoLoadCallBack_t)(const char*); typedef std::vector > FwdDeclArgsToKeepCollection_t; diff --git a/core/metacling/src/TCling.cxx b/core/metacling/src/TCling.cxx index 30ca417874c32..10e4304f76506 100644 --- a/core/metacling/src/TCling.cxx +++ b/core/metacling/src/TCling.cxx @@ -111,6 +111,7 @@ clang/LLVM technology. #include "cling/Interpreter/Transaction.h" #include "cling/MetaProcessor/MetaProcessor.h" #include "cling/Utils/AST.h" +#include "cling/Utils/Diagnostics.h" #include "cling/Utils/ParserStateRAII.h" #include "cling/Utils/SourceNormalization.h" #include "cling/Interpreter/Exception.h" @@ -399,6 +400,19 @@ extern "C" void TCling__UnlockCompilationDuringUserCodeExecution(void* /*state*/ } } +namespace { +class TClingDiagnosticsRAII : public TInterpreter::DiagnosticsRAII { + cling::utils::ReplaceDiagnostics fReplace; + std::unique_ptr fConsumer; + +public: + TClingDiagnosticsRAII(clang::DiagnosticsEngine &Diags, clang::DiagnosticConsumer *Replace) + : fReplace(Diags, *Replace, false), fConsumer(Replace) + { + } +}; +} // namespace + //////////////////////////////////////////////////////////////////////////////// /// Update TClingClassInfo for a class (e.g. upon seeing a definition). @@ -7591,6 +7605,22 @@ Bool_t TCling::LoadText(const char* text) const return (fInterpreter->declare(text) == cling::Interpreter::kSuccess); } +//////////////////////////////////////////////////////////////////////////////// +/// Make RAII for redirecting diagnostic messages to an ostream +std::unique_ptr +TCling::MakeRedirectDiagnosticsRAII(std::ostream &os, bool enableColors, unsigned int indent) +{ + + auto *consumer = new TClingRedirectDiagnosticPrinter(os, &fInterpreter->getDiagnostics().getDiagnosticOptions(), + fInterpreter->getCI()->getLangOpts(), enableColors, indent); + + // redirect owns consumer + auto *redirect = new TClingDiagnosticsRAII(fInterpreter->getDiagnostics(), consumer); + + // returned unique_ptr owns redirect + return std::unique_ptr(redirect); +} + //////////////////////////////////////////////////////////////////////////////// /// Interface to cling function diff --git a/core/metacling/src/TCling.h b/core/metacling/src/TCling.h index 1f0aa189fd89c..9b4c6da1d37e2 100644 --- a/core/metacling/src/TCling.h +++ b/core/metacling/src/TCling.h @@ -188,6 +188,8 @@ class TCling final : public TInterpreter { protected: Bool_t SetSuspendAutoParsing(Bool_t value) final; + std::unique_ptr + MakeRedirectDiagnosticsRAII(std::ostream &os, bool enableColors = false, unsigned int indent = 0) final; public: // Public Interface diff --git a/core/metacling/src/TClingDiagnostics.cxx b/core/metacling/src/TClingDiagnostics.cxx index 511b4c1f3735d..7a3c45eb1941d 100644 --- a/core/metacling/src/TClingDiagnostics.cxx +++ b/core/metacling/src/TClingDiagnostics.cxx @@ -27,3 +27,14 @@ TClingDelegateDiagnosticPrinter::HandleDiagnostic(clang::DiagnosticsEngine::Leve TextDiagnosticPrinter::HandleDiagnostic(Level, Info); fHandler(Level, fOS.str()); } + +TClingRedirectDiagnosticPrinter::TClingRedirectDiagnosticPrinter(std::ostream &os, clang::DiagnosticOptions *DiagOpts, + clang::LangOptions &LangOpts, bool enableColors, + unsigned int indent) + : TextDiagnosticPrinter(fOS, DiagOpts), fOS_out(os), fOS(fOS_out) +{ + // Required to initialize the internal `clang::TextDiagnostic` instance. + TextDiagnosticPrinter::BeginSourceFile(LangOpts, nullptr); + fOS.enable_colors(enableColors); + fOS.indent(indent); +} diff --git a/core/metacling/src/TClingDiagnostics.h b/core/metacling/src/TClingDiagnostics.h index 62680f0f1ce85..a02bbb10a62a2 100644 --- a/core/metacling/src/TClingDiagnostics.h +++ b/core/metacling/src/TClingDiagnostics.h @@ -14,6 +14,7 @@ #include "clang/Frontend/TextDiagnosticPrinter.h" #include "llvm/Support/raw_ostream.h" +#include "llvm/Support/raw_os_ostream.h" #include @@ -42,4 +43,18 @@ class TClingDelegateDiagnosticPrinter : public clang::TextDiagnosticPrinter { const clang::Diagnostic &Info) override; }; +/// \brief Uses `clang::TextDiagnosticPrinter` to format diagnostics, which +/// are then passed to a user-provided output stream +/// +class TClingRedirectDiagnosticPrinter : public clang::TextDiagnosticPrinter { +private: + std::ostream &fOS_out; + llvm::raw_os_ostream fOS; + +public: + TClingRedirectDiagnosticPrinter(std::ostream &os, clang::DiagnosticOptions *DiagOpts, clang::LangOptions &LangOpts, + bool enableColors = false, unsigned int indent = 0); + ~TClingRedirectDiagnosticPrinter() override = default; +}; + #endif // ROOT_TClingDiagnostics From b206bcf584eef0958213820d5a7637fd01b5cb4b Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Tue, 21 Mar 2023 14:29:31 +0100 Subject: [PATCH 07/15] don't suppress lookup diagnostics when interpreter output is being redirected --- core/metacling/src/TClingClassInfo.cxx | 80 +++++++++----------------- core/metacling/src/TClingClassInfo.h | 3 + 2 files changed, 30 insertions(+), 53 deletions(-) diff --git a/core/metacling/src/TClingClassInfo.cxx b/core/metacling/src/TClingClassInfo.cxx index 005c17ffcaeaa..2e2159a7978b7 100644 --- a/core/metacling/src/TClingClassInfo.cxx +++ b/core/metacling/src/TClingClassInfo.cxx @@ -29,10 +29,10 @@ but the class metadata comes from the Clang C++ compiler, not CINT. #include "TClingTypeInfo.h" #include "TError.h" #include "TClingUtils.h" +#include "TClingDiagnostics.h" #include "ThreadLocalStorage.h" #include "cling/Interpreter/Interpreter.h" -#include "cling/Interpreter/LookupHelper.h" #include "cling/Utils/AST.h" #include "clang/AST/ASTContext.h" @@ -84,17 +84,11 @@ TClingClassInfo::TClingClassInfo(cling::Interpreter *interp, const char *name, b { const cling::LookupHelper& lh = fInterp->getLookupHelper(); const Type *type = nullptr; - const Decl *decl = lh.findScope(name, - gDebug > 5 ? cling::LookupHelper::WithDiagnostics - : cling::LookupHelper::NoDiagnostics, - &type, intantiateTemplate); + const Decl *decl = lh.findScope(name, LookupDiagnostics(), &type, intantiateTemplate); if (!decl) { std::string buf = TClassEdit::InsertStd(name); if (buf != name) { - decl = lh.findScope(buf, - gDebug > 5 ? cling::LookupHelper::WithDiagnostics - : cling::LookupHelper::NoDiagnostics, - &type, intantiateTemplate); + decl = lh.findScope(buf, LookupDiagnostics(), &type, intantiateTemplate); } } if (!decl && type) { @@ -271,10 +265,7 @@ const FunctionTemplateDecl *TClingClassInfo::GetFunctionTemplate(const char *fna } } const cling::LookupHelper &lh = fInterp->getLookupHelper(); - const FunctionTemplateDecl *fd - = lh.findFunctionTemplate(GetDecl(), fname, - gDebug > 5 ? cling::LookupHelper::WithDiagnostics - : cling::LookupHelper::NoDiagnostics, false); + const FunctionTemplateDecl *fd = lh.findFunctionTemplate(GetDecl(), fname, LookupDiagnostics(), false); if (fd) return fd->getCanonicalDecl(); return nullptr; } @@ -285,10 +276,7 @@ const clang::ValueDecl *TClingClassInfo::GetDataMember(const char *name) const // the given name declared in this scope. const cling::LookupHelper &lh = fInterp->getLookupHelper(); - const ValueDecl *vd - = lh.findDataMember(GetDecl(), name, - gDebug > 5 ? cling::LookupHelper::WithDiagnostics - : cling::LookupHelper::NoDiagnostics); + const ValueDecl *vd = lh.findDataMember(GetDecl(), name, LookupDiagnostics()); if (vd) return llvm::dyn_cast(vd->getCanonicalDecl()); else return nullptr; } @@ -318,11 +306,7 @@ TClingMethodInfo TClingClassInfo::GetMethod(const char *fname) const } } const cling::LookupHelper &lh = fInterp->getLookupHelper(); - const FunctionDecl *fd - = lh.findAnyFunction(GetDecl(), fname, - gDebug > 5 ? cling::LookupHelper::WithDiagnostics - : cling::LookupHelper::NoDiagnostics, - false); + const FunctionDecl *fd = lh.findAnyFunction(GetDecl(), fname, LookupDiagnostics(), false); if (!fd) { // Function not found. TClingMethodInfo tmi(fInterp); @@ -374,15 +358,9 @@ TClingMethodInfo TClingClassInfo::GetMethod(const char *fname, const cling::LookupHelper& lh = fInterp->getLookupHelper(); const FunctionDecl *fd; if (mode == kConversionMatch) { - fd = lh.findFunctionProto(GetDecl(), fname, proto, - gDebug > 5 ? cling::LookupHelper::WithDiagnostics - : cling::LookupHelper::NoDiagnostics, - objectIsConst); + fd = lh.findFunctionProto(GetDecl(), fname, proto, LookupDiagnostics(), objectIsConst); } else if (mode == kExactMatch) { - fd = lh.matchFunctionProto(GetDecl(), fname, proto, - gDebug > 5 ? cling::LookupHelper::WithDiagnostics - : cling::LookupHelper::NoDiagnostics, - objectIsConst); + fd = lh.matchFunctionProto(GetDecl(), fname, proto, LookupDiagnostics(), objectIsConst); } else { Error("TClingClassInfo::GetMethod", "The MatchMode %d is not supported.", mode); @@ -467,15 +445,9 @@ TClingMethodInfo TClingClassInfo::GetMethod(const char *fname, const cling::LookupHelper& lh = fInterp->getLookupHelper(); const FunctionDecl *fd; if (mode == kConversionMatch) { - fd = lh.findFunctionProto(GetDecl(), fname, proto, - gDebug > 5 ? cling::LookupHelper::WithDiagnostics - : cling::LookupHelper::NoDiagnostics, - objectIsConst); + fd = lh.findFunctionProto(GetDecl(), fname, proto, LookupDiagnostics(), objectIsConst); } else if (mode == kExactMatch) { - fd = lh.matchFunctionProto(GetDecl(), fname, proto, - gDebug > 5 ? cling::LookupHelper::WithDiagnostics - : cling::LookupHelper::NoDiagnostics, - objectIsConst); + fd = lh.matchFunctionProto(GetDecl(), fname, proto, LookupDiagnostics(), objectIsConst); } else { Error("TClingClassInfo::GetMethod", "The MatchMode %d is not supported.", mode); @@ -543,11 +515,7 @@ TClingMethodInfo TClingClassInfo::GetMethodWithArgs(const char *fname, arglist = ""; } const cling::LookupHelper &lh = fInterp->getLookupHelper(); - const FunctionDecl *fd - = lh.findFunctionArgs(GetDecl(), fname, arglist, - gDebug > 5 ? cling::LookupHelper::WithDiagnostics - : cling::LookupHelper::NoDiagnostics, - objectIsConst); + const FunctionDecl *fd = lh.findFunctionArgs(GetDecl(), fname, arglist, LookupDiagnostics(), objectIsConst); if (!fd) { // Function not found. TClingMethodInfo tmi(fInterp); @@ -715,10 +683,7 @@ bool TClingClassInfo::HasMethod(const char *name) const { R__LOCKGUARD(gInterpreterMutex); if (IsLoaded() && !llvm::isa(GetDecl())) { - return fInterp->getLookupHelper() - .hasFunction(GetDecl(), name, - gDebug > 5 ? cling::LookupHelper::WithDiagnostics - : cling::LookupHelper::NoDiagnostics); + return fInterp->getLookupHelper().hasFunction(GetDecl(), name, LookupDiagnostics()); } return false; } @@ -733,15 +698,11 @@ void TClingClassInfo::Init(const char *name) fType = nullptr; fIterStack.clear(); const cling::LookupHelper& lh = fInterp->getLookupHelper(); - SetDecl(lh.findScope(name, gDebug > 5 ? cling::LookupHelper::WithDiagnostics - : cling::LookupHelper::NoDiagnostics, - &fType, /* intantiateTemplate= */ true )); + SetDecl(lh.findScope(name, LookupDiagnostics(), &fType, /* intantiateTemplate= */ true)); if (!GetDecl()) { std::string buf = TClassEdit::InsertStd(name); if (buf != name) { - SetDecl(lh.findScope(buf, gDebug > 5 ? cling::LookupHelper::WithDiagnostics - : cling::LookupHelper::NoDiagnostics, - &fType, /* intantiateTemplate= */ true )); + SetDecl(lh.findScope(buf, LookupDiagnostics(), &fType, /* intantiateTemplate= */ true)); } } if (!GetDecl() && fType) { @@ -930,6 +891,19 @@ bool TClingClassInfo::IsValidMethod(const char *method, const char *proto, return mi.IsValid(); } +cling::LookupHelper::DiagSetting TClingClassInfo::LookupDiagnostics() const +{ + + // if interpreter is already redirecting its diagnostics then assume + // that handling should be controlled upstream + + if (gDebug > 5 || gInterpreter->IsRedirectingDiagnostics()) { + return cling::LookupHelper::WithDiagnostics; + } + + return cling::LookupHelper::NoDiagnostics; +} + int TClingClassInfo::InternalNext() { R__LOCKGUARD(gInterpreterMutex); diff --git a/core/metacling/src/TClingClassInfo.h b/core/metacling/src/TClingClassInfo.h index c08fa1bdae1c9..ebfda873b50ec 100644 --- a/core/metacling/src/TClingClassInfo.h +++ b/core/metacling/src/TClingClassInfo.h @@ -36,6 +36,8 @@ #include #include +#include "cling/Interpreter/LookupHelper.h" + #include "llvm/ADT/DenseMap.h" namespace cling { @@ -173,6 +175,7 @@ class TClingClassInfo final : public TClingDeclInfo { bool IsLoaded() const; bool IsValidMethod(const char *method, const char *proto, Bool_t objectIsConst, Longptr_t *offset, ROOT::EFunctionMatchMode mode = ROOT::kConversionMatch) const; int InternalNext(); + cling::LookupHelper::DiagSetting LookupDiagnostics() const; int Next(); void *New(const ROOT::TMetaUtils::TNormalizedCtxt &normCtxt) const; void *New(int n, const ROOT::TMetaUtils::TNormalizedCtxt &normCtxt) const; From bb7f3ef9a2f3cf4666c013c0cac47106310ab09a Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Tue, 21 Mar 2023 14:30:18 +0100 Subject: [PATCH 08/15] use redirection of interpreter diagnostics to capture and print compiler errors and warnings during template instantiation --- .../pyroot/cppyy/CPyCppyy/src/Converters.cxx | 3 +- bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h | 4 +- .../cppyy/CPyCppyy/src/TemplateProxy.cxx | 26 +++++--- .../pyroot/cppyy/CPyCppyy/src/Utility.cxx | 3 +- .../clingwrapper/src/clingwrapper.cxx | 62 +++++++++++-------- .../clingwrapper/src/cpp_cppyy.h | 4 +- 6 files changed, 62 insertions(+), 40 deletions(-) diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx index 639d08c3a3d2e..dd4c111ed4055 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx @@ -2688,7 +2688,8 @@ static void* PyFunction_AsCPointer(PyObject* pyobject, if (pytmpl->fTemplateArgs) fullname += CPyCppyy_PyText_AsString(pytmpl->fTemplateArgs); Cppyy::TCppScope_t scope = ((CPPClass*)pytmpl->fTI->fPyClass)->fCppType; - Cppyy::TCppMethod_t cppmeth = Cppyy::GetMethodTemplate(scope, fullname, signature); + std::ostringstream diagnostics; + Cppyy::TCppMethod_t cppmeth = Cppyy::GetMethodTemplate(scope, fullname, signature, diagnostics); if (cppmeth) { void* fptr = (void*)Cppyy::GetFunctionAddress(cppmeth, false); if (fptr) return fptr; diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h b/bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h index cc1340033bbaf..3c03eb42d5215 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h +++ b/bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h @@ -229,8 +229,8 @@ namespace Cppyy { CPPYY_IMPORT bool IsMethodTemplate(TCppScope_t scope, TCppIndex_t imeth); CPPYY_IMPORT - TCppMethod_t GetMethodTemplate( - TCppScope_t scope, const std::string& name, const std::string& proto); + TCppMethod_t + GetMethodTemplate(TCppScope_t scope, const std::string &name, const std::string &proto, std::ostream &diagnostics); CPPYY_IMPORT TCppIndex_t GetGlobalOperator( diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx index b05d2ea61f039..2e56efc050bb7 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx @@ -12,7 +12,7 @@ // Standard #include - +#include namespace CPyCppyy { @@ -177,9 +177,11 @@ PyObject* TemplateProxy::Instantiate(const std::string& fname, proto = name_v1.substr(1, name_v1.size()-2); } -// the following causes instantiation as necessary + std::ostringstream diagnostics; + + // the following causes instantiation as necessary Cppyy::TCppScope_t scope = ((CPPClass*)fTI->fPyClass)->fCppType; - Cppyy::TCppMethod_t cppmeth = Cppyy::GetMethodTemplate(scope, fname, proto); + Cppyy::TCppMethod_t cppmeth = Cppyy::GetMethodTemplate(scope, fname, proto, diagnostics); if (cppmeth) { // overload stops here // A successful instantiation needs to be cached to pre-empt future instantiations. There // are two names involved, the original asked (which may be partial) and the received. @@ -203,12 +205,12 @@ PyObject* TemplateProxy::Instantiate(const std::string& fname, pos = proto.find("initializer_list", pos + 6); } - Cppyy::TCppMethod_t m2 = Cppyy::GetMethodTemplate(scope, fname, proto); + Cppyy::TCppMethod_t m2 = Cppyy::GetMethodTemplate(scope, fname, proto, diagnostics); if (m2 && m2 != cppmeth) { - // replace if the new method with vector was found; otherwise just continue - // with the previously found method with initializer_list. - cppmeth = m2; - resname = Cppyy::GetMethodFullName(cppmeth); + // replace if the new method with vector was found; otherwise just continue + // with the previously found method with initializer_list. + cppmeth = m2; + resname = Cppyy::GetMethodFullName(cppmeth); } } @@ -296,10 +298,16 @@ PyObject* TemplateProxy::Instantiate(const std::string& fname, PyObject* pymeth = CPPOverload_Type.tp_descr_get(pyol, bNeedsRebind ? fSelf : nullptr, (PyObject*)&CPPOverload_Type); Py_DECREF(pyol); + bool empty = diagnostics.str().find_first_not_of(' ') == diagnostics.str().npos; + if (!empty) { + PyErr_WarnFormat(PyExc_Warning, 1, "Compiler warnings during instantiation of \"%s(%s)\"\n%s", fname.c_str(), + proto.c_str(), diagnostics.str().c_str()); + } return pymeth; } - PyErr_Format(PyExc_TypeError, "Failed to instantiate \"%s(%s)\"", fname.c_str(), proto.c_str()); + PyErr_Format(PyExc_TypeError, "Failed to instantiate \"%s(%s)\"\n%s", fname.c_str(), proto.c_str(), + diagnostics.str().c_str()); return nullptr; } diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/Utility.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/Utility.cxx index 41df97f1da24f..47c77b9c0a94e 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/Utility.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/Utility.cxx @@ -378,7 +378,8 @@ CPyCppyy::PyCallable* CPyCppyy::Utility::FindBinaryOperator( else { fname << "not_implemented<"; } fname << lcname << ", " << rcname << ">"; proto << "const " << lcname << "&, const " << rcname; - Cppyy::TCppMethod_t method = Cppyy::GetMethodTemplate(s_intern, fname.str(), proto.str()); + std::ostringstream diagnostics; + Cppyy::TCppMethod_t method = Cppyy::GetMethodTemplate(s_intern, fname.str(), proto.str(), diagnostics); if (method) pyfunc = new CPPFunction(s_intern, method); } } diff --git a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx index 3ae8849ff427d..2f2b4a84cb41a 100644 --- a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx +++ b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx @@ -2108,8 +2108,8 @@ bool template_compare(std::string n1, std::string n2) { return n2.compare(0, n1.size(), n1) == 0; } -Cppyy::TCppMethod_t Cppyy::GetMethodTemplate( - TCppScope_t scope, const std::string& name, const std::string& proto) +Cppyy::TCppMethod_t Cppyy::GetMethodTemplate(TCppScope_t scope, const std::string &name, const std::string &proto, + std::ostream &diagnostics) { // There is currently no clean way of extracting a templated method out of ROOT/meta // for a variety of reasons, none of them fundamental. The game played below is to @@ -2117,34 +2117,48 @@ Cppyy::TCppMethod_t Cppyy::GetMethodTemplate( // to do an explicit lookup that ignores the prototype (i.e. the full name should be // enough), and finally to ignore the template arguments part of the name as this fails // in cling if there are default parameters. - TFunction* func = nullptr; ClassInfo_t* cl = nullptr; + +// It would be possible to get the prototype from the created functions and use that to +// do a new lookup, after which ROOT/meta will manage the function. However, neither +// TFunction::GetPrototype() nor TFunction::GetSignature() is of the proper form, so +// we'll/ manage the new TFunctions instead and will assume that they are cached on the +// calling side to prevent multiple creations. + +// redirect diagnostics, taking the lock to make sure no other calls pollute the results + R__WRITE_LOCKGUARD(ROOT::gCoreMutex); + + TInterpreter::RedirectDiagnostics redirectRAII(gInterpreter, diagnostics, /*enableColors*/ true, /*indent*/ 4); + + TFunction *func = nullptr; + ClassInfo_t *cl = nullptr; if (scope == (TCppScope_t)GLOBAL_HANDLE) { - func = gROOT->GetGlobalFunctionWithPrototype(name.c_str(), proto.c_str()); - if (func && name.back() == '>') { - // make sure that all template parameters match (more are okay, e.g. defaults or - // ones derived from the arguments or variadic templates) - if (!template_compare(name, func->GetName())) - func = nullptr; // happens if implicit conversion matches the overload - } + func = gROOT->GetGlobalFunctionWithPrototype(name.c_str(), proto.c_str()); + if (func && name.back() == '>') { + // make sure that all template parameters match (more are okay, e.g. defaults or + // ones derived from the arguments or variadic templates) + if (!template_compare(name, func->GetName())) + func = nullptr; // happens if implicit conversion matches the overload + } } else { - TClassRef& cr = type_from_handle(scope); - if (cr.GetClass()) { - func = cr->GetMethodWithPrototype(name.c_str(), proto.c_str()); - if (!func) { - cl = cr->GetClassInfo(); + TClassRef &cr = type_from_handle(scope); + if (cr.GetClass()) { + func = cr->GetMethodWithPrototype(name.c_str(), proto.c_str()); + if (!func) { + cl = cr->GetClassInfo(); // try base classes to cover a common 'using' case (TODO: this is stupid and misses // out on base classes; fix that with improved access to Cling) - TCppIndex_t nbases = GetNumBases(scope); - for (TCppIndex_t i = 0; i < nbases; ++i) { - TClassRef& base = type_from_handle(GetScope(GetBaseName(scope, i))); - if (base.GetClass()) { - func = base->GetMethodWithPrototype(name.c_str(), proto.c_str()); - if (func) break; - } + TCppIndex_t nbases = GetNumBases(scope); + for (TCppIndex_t i = 0; i < nbases; ++i) { + TClassRef &base = type_from_handle(GetScope(GetBaseName(scope, i))); + if (base.GetClass()) { + func = base->GetMethodWithPrototype(name.c_str(), proto.c_str()); + if (func) + break; } } } } + } if (!func && name.back() == '>' && (cl || scope == (TCppScope_t)GLOBAL_HANDLE)) { // try again, ignoring proto in case full name is complete template @@ -2172,7 +2186,7 @@ Cppyy::TCppMethod_t Cppyy::GetMethodTemplate( if (name.back() == '>') { auto pos = name.find('<'); if (pos != std::string::npos) { - TCppMethod_t cppmeth = GetMethodTemplate(scope, name.substr(0, pos), proto); + TCppMethod_t cppmeth = GetMethodTemplate(scope, name.substr(0, pos), proto, diagnostics); if (cppmeth) { // allow if requested template names match up to the result const std::string& alt = GetMethodFullName(cppmeth); @@ -2643,5 +2657,3 @@ long long Cppyy::GetEnumDataValue(TCppEnum_t etype, TCppIndex_t idata) TEnumConstant* ecst = (TEnumConstant*)((TEnum*)etype)->GetConstants()->At((int)idata); return (long long)ecst->GetValue(); } - - diff --git a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/cpp_cppyy.h b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/cpp_cppyy.h index 44056f8b71d95..7a90365e0b295 100644 --- a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/cpp_cppyy.h +++ b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/cpp_cppyy.h @@ -226,8 +226,8 @@ namespace Cppyy { RPY_EXPORTED bool IsMethodTemplate(TCppScope_t scope, TCppIndex_t imeth); RPY_EXPORTED - TCppMethod_t GetMethodTemplate( - TCppScope_t scope, const std::string& name, const std::string& proto); + TCppMethod_t + GetMethodTemplate(TCppScope_t scope, const std::string &name, const std::string &proto, std::ostream &diagnostics); RPY_EXPORTED TCppIndex_t GetGlobalOperator( From a230a277ee87023b16668af8d26cb60f75462a41 Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Tue, 21 Mar 2023 18:21:12 +0100 Subject: [PATCH 09/15] use PyErr_WarnEx instead of PyErr_WarnFormat since the latter is not available on all build platforms --- bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx index 2e56efc050bb7..f63f2a1e9158e 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx @@ -298,10 +298,13 @@ PyObject* TemplateProxy::Instantiate(const std::string& fname, PyObject* pymeth = CPPOverload_Type.tp_descr_get(pyol, bNeedsRebind ? fSelf : nullptr, (PyObject*)&CPPOverload_Type); Py_DECREF(pyol); - bool empty = diagnostics.str().find_first_not_of(' ') == diagnostics.str().npos; - if (!empty) { - PyErr_WarnFormat(PyExc_Warning, 1, "Compiler warnings during instantiation of \"%s(%s)\"\n%s", fname.c_str(), - proto.c_str(), diagnostics.str().c_str()); + // check if diagnostics contains only spaces since this can happen as a result of clang indenting + const bool emptydiag = diagnostics.str().find_first_not_of(' ') == diagnostics.str().npos; + if (!emptydiag) { + std::ostringstream warnmsg; + warnmsg << "Compiler warnings during instantiation of \"" << fname << "(" << proto << ")\"\n" + << diagnostics.str(); + PyErr_WarnEx(PyExc_Warning, warnmsg.str().c_str(), 1); } return pymeth; } From 7756ea845095248c202929f3016e337eaa12ddf4 Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Tue, 1 Oct 2024 05:40:49 -0400 Subject: [PATCH 10/15] add improved error handling to tpp_overload as well --- .../cppyy/CPyCppyy/src/TemplateProxy.cxx | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx index f63f2a1e9158e..5a473e5717ff9 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx @@ -812,6 +812,8 @@ static PyObject* tpp_overload(TemplateProxy* pytmpl, PyObject* args) Cppyy::TCppMethod_t cppmeth = (Cppyy::TCppMethod_t) 0; std::string proto; + std::ostringstream diagnostics; + if (PyArg_ParseTuple(args, const_cast("s|i:__overload__"), &sigarg, &want_const)) { want_const = PyTuple_GET_SIZE(args) == 1 ? -1 : want_const; @@ -829,12 +831,12 @@ static PyObject* tpp_overload(TemplateProxy* pytmpl, PyObject* args) scope = ((CPPClass*)pytmpl->fTI->fPyClass)->fCppType; cppmeth = Cppyy::GetMethodTemplate( - scope, pytmpl->fTI->fCppName, proto.substr(1, proto.size()-2)); + scope, pytmpl->fTI->fCppName, proto.substr(1, proto.size()-2), diagnostics); } else if (PyArg_ParseTuple(args, const_cast("ss:__overload__"), &sigarg, &tmplarg)) { scope = ((CPPClass*)pytmpl->fTI->fPyClass)->fCppType; std::string full_name = std::string(pytmpl->fTI->fCppName) + "<" + tmplarg + ">"; - cppmeth = Cppyy::GetMethodTemplate(scope, full_name, sigarg); + cppmeth = Cppyy::GetMethodTemplate(scope, full_name, sigarg, diagnostics); } else if (PyArg_ParseTuple(args, const_cast("O|i:__overload__"), &sigarg_tuple, &want_const)) { PyErr_Clear(); want_const = PyTuple_GET_SIZE(args) == 1 ? -1 : want_const; @@ -865,16 +867,27 @@ static PyObject* tpp_overload(TemplateProxy* pytmpl, PyObject* args) proto.push_back('>'); scope = ((CPPClass*)pytmpl->fTI->fPyClass)->fCppType; - cppmeth = Cppyy::GetMethodTemplate( - scope, pytmpl->fTI->fCppName, proto.substr(1, proto.size()-2)); + cppmeth = + Cppyy::GetMethodTemplate(scope, pytmpl->fTI->fCppName, proto.substr(1, proto.size() - 2), diagnostics); } else { PyErr_Format(PyExc_TypeError, "Unexpected arguments to __overload__"); return nullptr; } + const bool emptydiag = diagnostics.str().find_first_not_of(' ') == diagnostics.str().npos; + if (!emptydiag) { + std::ostringstream warnmsg; + warnmsg << "Compiler warnings during instantiation of \"" << pytmpl->fTI->fCppName << "(" << proto << ")\"\n" + << diagnostics.str(); + PyErr_WarnEx(PyExc_Warning, warnmsg.str().c_str(), 1); + } + // else attempt instantiation if (!cppmeth) { - return nullptr; + PyErr_Format(PyExc_TypeError, "Failed to instantiate \"%s(%s)\"\n%s", pytmpl->fTI->fCppName.c_str(), + proto.c_str(), diagnostics.str().c_str()); + PyErr_Restore(pytype, pyvalue, pytrace); + return nullptr; } PyErr_Clear(); From 629664b59454ffac99945b7f04530433bf846537 Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Tue, 1 Oct 2024 06:53:14 -0400 Subject: [PATCH 11/15] improve error handling in tpp_overload --- .../cppyy/CPyCppyy/src/TemplateProxy.cxx | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx index 5a473e5717ff9..d15b9cff4567d 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx @@ -874,24 +874,25 @@ static PyObject* tpp_overload(TemplateProxy* pytmpl, PyObject* args) return nullptr; } - const bool emptydiag = diagnostics.str().find_first_not_of(' ') == diagnostics.str().npos; - if (!emptydiag) { - std::ostringstream warnmsg; - warnmsg << "Compiler warnings during instantiation of \"" << pytmpl->fTI->fCppName << "(" << proto << ")\"\n" - << diagnostics.str(); - PyErr_WarnEx(PyExc_Warning, warnmsg.str().c_str(), 1); - } - // else attempt instantiation if (!cppmeth) { - PyErr_Format(PyExc_TypeError, "Failed to instantiate \"%s(%s)\"\n%s", pytmpl->fTI->fCppName.c_str(), - proto.c_str(), diagnostics.str().c_str()); + PyErr_Format(PyExc_TypeError, "Failed in template overload resolution \"%s(%s)\"\n%s", + pytmpl->fTI->fCppName.c_str(), proto.c_str(), diagnostics.str().c_str()); PyErr_Restore(pytype, pyvalue, pytrace); return nullptr; } PyErr_Clear(); + const bool emptydiag = diagnostics.str().find_first_not_of(' ') == diagnostics.str().npos; + if (!emptydiag) { + std::ostringstream warnmsg; + warnmsg << "Compiler warnings during template overload resolution of \"" << pytmpl->fTI->fCppName << "(" << proto + << ")\"\n" + << diagnostics.str(); + PyErr_WarnEx(PyExc_Warning, warnmsg.str().c_str(), 1); + } + // TODO: the next step should be consolidated with Instantiate() PyCallable* meth = nullptr; if (Cppyy::IsNamespace(scope)) { From d9e2515396e0024d181359811981e6f220d912a3 Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Tue, 1 Oct 2024 13:09:32 -0400 Subject: [PATCH 12/15] suppress warnings and errors when lookup succeeds on a subsequent try --- bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h | 4 +- .../cppyy/CPyCppyy/src/TemplateProxy.cxx | 8 +++- .../clingwrapper/src/clingwrapper.cxx | 40 ++++++++++++++----- .../clingwrapper/src/cpp_cppyy.h | 4 +- 4 files changed, 40 insertions(+), 16 deletions(-) diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h b/bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h index 3c03eb42d5215..03c62901e6cb0 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h +++ b/bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h @@ -229,8 +229,8 @@ namespace Cppyy { CPPYY_IMPORT bool IsMethodTemplate(TCppScope_t scope, TCppIndex_t imeth); CPPYY_IMPORT - TCppMethod_t - GetMethodTemplate(TCppScope_t scope, const std::string &name, const std::string &proto, std::ostream &diagnostics); + TCppMethod_t GetMethodTemplate(TCppScope_t scope, const std::string &name, const std::string &proto, + std::ostringstream &diagnostics); CPPYY_IMPORT TCppIndex_t GetGlobalOperator( diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx index d15b9cff4567d..c0c8ca5a1c0b7 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx @@ -205,12 +205,14 @@ PyObject* TemplateProxy::Instantiate(const std::string& fname, pos = proto.find("initializer_list", pos + 6); } - Cppyy::TCppMethod_t m2 = Cppyy::GetMethodTemplate(scope, fname, proto, diagnostics); + std::ostringstream diagnostics2; + Cppyy::TCppMethod_t m2 = Cppyy::GetMethodTemplate(scope, fname, proto, diagnostics2); if (m2 && m2 != cppmeth) { // replace if the new method with vector was found; otherwise just continue // with the previously found method with initializer_list. cppmeth = m2; resname = Cppyy::GetMethodFullName(cppmeth); + diagnostics = std::move(diagnostics2); } } @@ -228,6 +230,10 @@ PyObject* TemplateProxy::Instantiate(const std::string& fname, Py_DECREF(pyol); Py_DECREF(pycachename); Py_DECREF(dct); + + PyErr_Format(PyExc_TypeError, "Failed to instantiate \"%s(%s)\"\n%s", fname.c_str(), proto.c_str(), + diagnostics.str().c_str()); + return nullptr; } diff --git a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx index 2f2b4a84cb41a..b0e2be46d5a87 100644 --- a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx +++ b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx @@ -2109,7 +2109,7 @@ bool template_compare(std::string n1, std::string n2) { } Cppyy::TCppMethod_t Cppyy::GetMethodTemplate(TCppScope_t scope, const std::string &name, const std::string &proto, - std::ostream &diagnostics) + std::ostringstream &diagnostics) { // There is currently no clean way of extracting a templated method out of ROOT/meta // for a variety of reasons, none of them fundamental. The game played below is to @@ -2127,6 +2127,7 @@ Cppyy::TCppMethod_t Cppyy::GetMethodTemplate(TCppScope_t scope, const std::strin // redirect diagnostics, taking the lock to make sure no other calls pollute the results R__WRITE_LOCKGUARD(ROOT::gCoreMutex); + const std::string diagnosticsold = diagnostics.str(); TInterpreter::RedirectDiagnostics redirectRAII(gInterpreter, diagnostics, /*enableColors*/ true, /*indent*/ 4); TFunction *func = nullptr; @@ -2162,15 +2163,25 @@ Cppyy::TCppMethod_t Cppyy::GetMethodTemplate(TCppScope_t scope, const std::strin if (!func && name.back() == '>' && (cl || scope == (TCppScope_t)GLOBAL_HANDLE)) { // try again, ignoring proto in case full name is complete template - auto declid = gInterpreter->GetFunction(cl, name.c_str()); - if (declid) { - auto existing = gMethodTemplates.find(declid); - if (existing == gMethodTemplates.end()) { - auto cw = new_CallWrapper(declid, name); - existing = gMethodTemplates.insert(std::make_pair(declid, cw)).first; - } - return (TCppMethod_t)existing->second; + std::ostringstream diagnostics2; + TInterpreter::RedirectDiagnostics redirectRAII2(gInterpreter, diagnostics2, /*enableColors*/ true, /*indent*/ 4); + + auto declid = gInterpreter->GetFunction(cl, name.c_str()); + if (declid) { + auto existing = gMethodTemplates.find(declid); + if (existing == gMethodTemplates.end()) { + auto cw = new_CallWrapper(declid, name); + existing = gMethodTemplates.insert(std::make_pair(declid, cw)).first; + } + // replace diagnostics to suppress spurious errors or warnings from the previous + // failed lookup + diagnostics = std::ostringstream(); + diagnostics << diagnosticsold; + diagnostics << diagnostics2.str(); + + return (TCppMethod_t)existing->second; } + diagnostics << diagnostics2.str(); } if (func) { @@ -2186,13 +2197,20 @@ Cppyy::TCppMethod_t Cppyy::GetMethodTemplate(TCppScope_t scope, const std::strin if (name.back() == '>') { auto pos = name.find('<'); if (pos != std::string::npos) { - TCppMethod_t cppmeth = GetMethodTemplate(scope, name.substr(0, pos), proto, diagnostics); + TCppMethod_t cppmeth = GetMethodTemplate(scope, name.substr(0, pos), proto, diagnostics2); if (cppmeth) { // allow if requested template names match up to the result const std::string& alt = GetMethodFullName(cppmeth); if (name.size() < alt.size() && alt.find('<') == pos) { - if (template_compare(name, alt)) + if (template_compare(name, alt)){ + // replace diagnostics to suppress spurious errors or warnings + // from the previous failed lookup + diagnostics = std::ostringstream(); + diagnostics << diagnosticsold; + diagnostics << diagnostics2.str(); return cppmeth; + } + } } } diff --git a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/cpp_cppyy.h b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/cpp_cppyy.h index 7a90365e0b295..ff8be902b2acf 100644 --- a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/cpp_cppyy.h +++ b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/cpp_cppyy.h @@ -226,8 +226,8 @@ namespace Cppyy { RPY_EXPORTED bool IsMethodTemplate(TCppScope_t scope, TCppIndex_t imeth); RPY_EXPORTED - TCppMethod_t - GetMethodTemplate(TCppScope_t scope, const std::string &name, const std::string &proto, std::ostream &diagnostics); + TCppMethod_t GetMethodTemplate(TCppScope_t scope, const std::string &name, const std::string &proto, + std::ostringstream &diagnostics); RPY_EXPORTED TCppIndex_t GetGlobalOperator( From 24f783b5a0bb592b0f6fa9a6fc9dc69717768f2c Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Sat, 29 Nov 2025 14:08:46 +0100 Subject: [PATCH 13/15] Adapt patches to master --- bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx | 1 - .../cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx | 7 ++----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx index c0c8ca5a1c0b7..e68db1b79ee86 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/TemplateProxy.cxx @@ -884,7 +884,6 @@ static PyObject* tpp_overload(TemplateProxy* pytmpl, PyObject* args) if (!cppmeth) { PyErr_Format(PyExc_TypeError, "Failed in template overload resolution \"%s(%s)\"\n%s", pytmpl->fTI->fCppName.c_str(), proto.c_str(), diagnostics.str().c_str()); - PyErr_Restore(pytype, pyvalue, pytrace); return nullptr; } diff --git a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx index b0e2be46d5a87..488fe8221e7d0 100644 --- a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx +++ b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx @@ -1045,7 +1045,6 @@ T CallT(Cppyy::TCppMethod_t method, Cppyy::TCppObject_t self, size_t nargs, void T t{}; if (WrapperCall(method, nargs, args, (void*)self, &t)) return t; - throw std::runtime_error("failed to resolve function"); return (T)-1; } @@ -1076,7 +1075,6 @@ void* Cppyy::CallR(TCppMethod_t method, TCppObject_t self, size_t nargs, void* a void* r = nullptr; if (WrapperCall(method, nargs, args, (void*)self, &r)) return r; - throw std::runtime_error("failed to resolve function"); return nullptr; } @@ -1094,7 +1092,6 @@ char* Cppyy::CallS( } else { *length = 0; free((void *)cppresult); - throw std::runtime_error("failed to resolve function"); } return cstr; } @@ -1105,7 +1102,6 @@ Cppyy::TCppObject_t Cppyy::CallConstructor( void* obj = nullptr; if (WrapperCall(method, nargs, args, nullptr, &obj)) return (TCppObject_t)obj; - throw std::runtime_error("failed to resolve function"); return (TCppObject_t)0; } @@ -1123,7 +1119,6 @@ Cppyy::TCppObject_t Cppyy::CallO(TCppMethod_t method, if (WrapperCall(method, nargs, args, self, obj)) return (TCppObject_t)obj; ::operator delete(obj); - throw std::runtime_error("failed to resolve function"); return (TCppObject_t)0; } @@ -2197,6 +2192,7 @@ Cppyy::TCppMethod_t Cppyy::GetMethodTemplate(TCppScope_t scope, const std::strin if (name.back() == '>') { auto pos = name.find('<'); if (pos != std::string::npos) { + std::ostringstream diagnostics2; TCppMethod_t cppmeth = GetMethodTemplate(scope, name.substr(0, pos), proto, diagnostics2); if (cppmeth) { // allow if requested template names match up to the result @@ -2213,6 +2209,7 @@ Cppyy::TCppMethod_t Cppyy::GetMethodTemplate(TCppScope_t scope, const std::strin } } + diagnostics << diagnostics2.str(); } } From cda3147d9d88fb3a7ed7ef8db887c73c47de1545 Mon Sep 17 00:00:00 2001 From: Josh Bendavid Date: Tue, 21 Mar 2023 20:46:58 +0100 Subject: [PATCH 14/15] explicitly handle 0 column case to avoid instantiation errors when used with jitted version of Book --- roofit/roofitcore/inc/RooAbsDataHelper.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/roofit/roofitcore/inc/RooAbsDataHelper.h b/roofit/roofitcore/inc/RooAbsDataHelper.h index 5347b3b862fb0..2852bb5f7d9a5 100644 --- a/roofit/roofitcore/inc/RooAbsDataHelper.h +++ b/roofit/roofitcore/inc/RooAbsDataHelper.h @@ -150,6 +150,15 @@ class RooAbsDataHelper : public RooFit::Detail::RooAbsDataFiller, ExecImpl(sizeof...(values), vector); } + // 0 column version + void Exec(unsigned int) + { + if (_eventSize) { + throw std::invalid_argument(std::string("RooDataSet can hold ") + std::to_string(_eventSize) + + " variables per event, but RDataFrame passed 0 columns."); + } + } + DataSet_t &GetAbsData() override { return *_dataset; } private: From dee2fb76aba525b313416063ee5c41e5dc3b936b Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Sat, 29 Nov 2025 14:35:48 +0100 Subject: [PATCH 15/15] [python] Add regression test for #11854 --- .../pyroot/pythonizations/test/CMakeLists.txt | 2 + .../pyroot/pythonizations/test/gh_11854.py | 40 +++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 bindings/pyroot/pythonizations/test/gh_11854.py diff --git a/bindings/pyroot/pythonizations/test/CMakeLists.txt b/bindings/pyroot/pythonizations/test/CMakeLists.txt index ad4dd92ca3794..42f6599b09c80 100644 --- a/bindings/pyroot/pythonizations/test/CMakeLists.txt +++ b/bindings/pyroot/pythonizations/test/CMakeLists.txt @@ -191,3 +191,5 @@ if(NOT MSVC OR CMAKE_SIZEOF_VOID_P EQUAL 8 OR win_broken_tests) # https://github.com/root-project/root/issues/9809 ROOT_ADD_PYUNITTEST(pyroot_array_numpy_views array_conversions.py PYTHON_DEPS numpy) endif() + +ROOT_ADD_PYUNITTEST(gh_11854 gh_11854.py) diff --git a/bindings/pyroot/pythonizations/test/gh_11854.py b/bindings/pyroot/pythonizations/test/gh_11854.py new file mode 100644 index 0000000000000..de2a31721ae48 --- /dev/null +++ b/bindings/pyroot/pythonizations/test/gh_11854.py @@ -0,0 +1,40 @@ +import unittest + +import ROOT + + +class GH11854(unittest.TestCase): + """Regression test for https://github.com/root-project/root/issues/11854""" + + def test_gh_11854(self): + + ROOT.gInterpreter.Declare(r""" + #ifndef GH11854_HELPER + #define GH11854_HELPER + + template + struct GH11854Helper { + + std::size_t operator() () const { + const std::size_t res = 0; + res = T{0, 0}.size(); + return res; + } + + }; + + template + std::size_t call_gh11854_helper(const H &helper) { + return helper(); + } + + #endif // GH11854_HELPER + """) + + helper = ROOT.GH11854Helper[ROOT.std.vector["double"]]() + with self.assertRaisesRegex(TypeError, "cannot assign to variable 'res' with const-qualified type 'const std::size_t'"): + ROOT.call_gh11854_helper(helper) + + +if __name__ == '__main__': + unittest.main()