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 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..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); + 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 b05d2ea61f039..e68db1b79ee86 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,14 @@ PyObject* TemplateProxy::Instantiate(const std::string& fname, pos = proto.find("initializer_list", pos + 6); } - Cppyy::TCppMethod_t m2 = Cppyy::GetMethodTemplate(scope, fname, proto); + 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); + // 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); } } @@ -226,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; } @@ -296,10 +304,19 @@ PyObject* TemplateProxy::Instantiate(const std::string& fname, PyObject* pymeth = CPPOverload_Type.tp_descr_get(pyol, bNeedsRebind ? fSelf : nullptr, (PyObject*)&CPPOverload_Type); Py_DECREF(pyol); + // 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; } - 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; } @@ -801,6 +818,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; @@ -818,12 +837,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; @@ -854,8 +873,8 @@ 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; @@ -863,11 +882,22 @@ static PyObject* tpp_overload(TemplateProxy* pytmpl, PyObject* args) // else attempt instantiation if (!cppmeth) { - return nullptr; + 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()); + 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)) { 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 6343d2bc16b72..488fe8221e7d0 100644 --- a/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx +++ b/bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx @@ -1088,9 +1088,11 @@ 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); + } return cstr; } @@ -2101,8 +2103,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::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 @@ -2110,46 +2112,71 @@ 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); + + const std::string diagnosticsold = diagnostics.str(); + 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 - 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) { @@ -2165,15 +2192,24 @@ 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); + 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 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; + } + } } + diagnostics << diagnostics2.str(); } } @@ -2636,5 +2672,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..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); + TCppMethod_t GetMethodTemplate(TCppScope_t scope, const std::string &name, const std::string &proto, + std::ostringstream &diagnostics); RPY_EXPORTED TCppIndex_t GetGlobalOperator( 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() 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/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; 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 diff --git a/interpreter/cling/lib/Interpreter/LookupHelper.cpp b/interpreter/cling/lib/Interpreter/LookupHelper.cpp index 66fed6c29fa10..5462ce85ff13b 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" @@ -1112,23 +1113,6 @@ namespace cling { ObjExprClassification = ObjExpr.Classify(Context); } - // - // Tell the diagnostic engine to ignore all diagnostics. - // - 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); - // // Construct the overload candidate set. // @@ -1216,8 +1200,11 @@ namespace cling { S.InstantiateFunctionDefinition(SourceLocation(), TheDecl, true /*recursive instantiation*/); } - if (TheDecl->isInvalidDecl()) { - // if the decl is invalid try to clean up + 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; } @@ -1538,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 @@ -1587,10 +1590,8 @@ 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); + return findFunction(foundDC, funcName, GivenArgs, objectIsConst, Context, + Interp, functionSelector, diagOnOff); } struct NoParse { @@ -1824,8 +1825,11 @@ namespace cling { if (!fdecl->isDefined()) S.InstantiateFunctionDefinition(loc, fdecl, true /*recursive instantiation*/); - if (fdecl->isInvalidDecl()) { - // if the decl is invalid try to clean up + 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; } 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: