[vepadulano]

This is the first part of #12449

I'm still trying to understand how to create a test. I used the original reproducer

template <typename T>
class Helper {

public:

  Helper() {}

  std::size_t operator() () const {
    const std::size_t res = 0;
    res = T{0, 0}.size();
    return res;
  }

};

template <typename H>
std::size_t call_helper(const H &helper) {
  return helper();
}

And ran it with root, but I see the same errors printed both after this changes and with ROOT master

Processing t.C...
In file included from input_line_8:1:
/Users/vpadulan/Programs/rootproject/t.C:10:9: error: cannot assign to variable 'res' with const-qualified type 'const std::size_t' (aka 'const unsigned long')
    res = T{0, 0}.size();
    ~~~ ^
/Users/vpadulan/Programs/rootproject/t.C:18:10: note: in instantiation of member function 'Helper<std::vector<double, std::allocator<double> > >::operator()' requested here
  return helper();
         ^
/Users/vpadulan/Programs/rootproject/t.C:24:5: note: in instantiation of function template specialization 'call_helper<Helper<std::vector<double, std::allocator<double> > > >' requested here
    call_helper(h);
    ^
/Users/vpadulan/Programs/rootproject/t.C:9:23: note: variable 'res' declared const here
    const std::size_t res = 0;
    ~~~~~~~~~~~~~~~~~~^~~~~~~
Assertion failed: (!m_Unloading && "Must not nest within unloading transaction"), function addNestedTransaction, file Transaction.cpp, line 98.

[vepadulano]

This PR is currently blocked by #20952

[github-actions]

Test Results

    22 files      22 suites   3d 23h 39m 25s ⏱️
 3 812 tests  3 809 ✅ 0 💤  3 ❌
76 720 runs  76 701 ✅ 7 💤 12 ❌

For more details on these failures, see this check.

Results for commit 25e1e2f.

[bendavid]

Following up on the question of a C++-only test. Here's a reproducer for the class of problems from #12449 that uses only C++ through cling.

The t.C attempt above doesn't show the problem because it calls call_helper(h) in code that cling compiles directly, and that already reports the error correctly. The problem is in the other way of getting a template instantiated: looking it up by name and argument types through ROOT's reflection, which goes through cling::LookupHelper. That's what the cppyy-based Python bindings did, via gROOT->GetGlobalFunctionWithPrototype, TClass::GetMethodWithPrototype and gInterpreter->GetFunction. TMethodCall and TClass::GetListOfMethods, through TClingCXXRecMethIter::InstantiateTemplateWithDefaults, do the same. The macro below uses those APIs and names the templates only in strings. Run it with root -l -b -q repro.C:

repro.C

// C++-only reproducer for the class of problems in root-project/root#12449: templates instantiated
// through ROOT's reflection (TROOT/TClass/TInterpreter lookups, i.e. cling::LookupHelper), as the
// cppyy-based Python bindings did, with function bodies that are ill-formed.
// Run with: root -l -b -q repro.C
// Control: the two ProcessLine calls at the end, run alone in a fresh session, do report both errors.
// The templates are only named in strings here, never in this macro's own code, since compiling
// e.g. call_helper(h) directly reports the error as expected.
#include "TROOT.h"
#include "TInterpreter.h"
#include "TClass.h"
#include "TFunction.h"
#include "TMethod.h"
#include "TMethodCall.h"
#include "TList.h"
#include <cstdio>

void declare_templates()
{
   gInterpreter->Declare(R"CODE(
#include <vector>
#include <cstddef>

// (A) the original #12449/#11854 case: operator() is ill-formed for every T (assigns to a const)
template <typename T>
struct Helper {
   std::size_t operator()() const {
      const std::size_t res = 0;
      res = T{0, 0}.size();
      return res;
   }
};
template <typename H>
std::size_t call_helper(const H &helper) { return helper(); }
Helper<std::vector<double>> gHelper;

// (B) a body that fails a static_assert: the declaration itself stays valid
template <typename T>
int needs_big(T) { static_assert(sizeof(T) > 4, "needs_big requires a type larger than 4 bytes"); return 1; }

// (C) a member template with defaults, instantiated when ROOT lists the methods of the class
//     (TClingCXXRecMethIter::InstantiateTemplateWithDefaults, the path of #20952)
struct WithDefaults {
   template <typename T = int>
   int get() const { static_assert(sizeof(T) > 4, "get<T> requires a type larger than 4 bytes"); return 0; }
};
)CODE");
}

void report(const char *what, TFunction *f)
{
   printf("  %-62s -> %s\n", what, f ? (std::string("FOUND: ") + f->GetPrototype()).c_str() : "not found");
}

void repro()
{
   declare_templates();

   printf("\n=== lookups by name and argument types (as the cppyy bindings did)\n");
   report("gROOT->GetGlobalFunctionWithPrototype(call_helper, Helper&)",
          gROOT->GetGlobalFunctionWithPrototype("call_helper", "const Helper<std::vector<double> >&", true));
   report("gROOT->GetGlobalFunctionWithPrototype(needs_big, char)",
          gROOT->GetGlobalFunctionWithPrototype("needs_big", "char", true));
   printf("  %-62s -> %s\n", "gInterpreter->GetFunction(0, \"needs_big<char>\")",
          gInterpreter->GetFunction(nullptr, "needs_big<char>") ? "FOUND" : "not found");

   printf("\n=== listing the methods of WithDefaults (instantiates get<int> with its default)\n");
   for (auto *m : *TClass::GetClass("WithDefaults")->GetListOfMethods())
      printf("  method: %s\n", static_cast<TMethod *>(m)->GetPrototype());

   printf("\n=== calling what the lookups returned\n");
   Longptr_t helperAddr = (Longptr_t)gInterpreter->Calc("&gHelper");
   {
      TMethodCall mc;
      mc.InitWithPrototype("call_helper", "const Helper<std::vector<double> >&");
      printf("  TMethodCall(call_helper) IsValid: %d\n", mc.IsValid());
      if (mc.IsValid()) {
         mc.ResetParam();
         mc.SetParam(helperAddr);
         Longptr_t ret = -1;
         mc.Execute(ret);
         printf("  TMethodCall(call_helper) returned %ld\n", (long)ret);
      }
   }
   {
      TMethodCall mc;
      mc.InitWithPrototype("needs_big", "char");
      printf("  TMethodCall(needs_big) IsValid: %d\n", mc.IsValid());
      if (mc.IsValid()) {
         mc.ResetParam();
         mc.SetParam((Long_t)'x');
         Longptr_t ret = -1;
         mc.Execute(ret);
         printf("  TMethodCall(needs_big) returned %ld\n", (long)ret);
      }
   }

   printf("\n=== for comparison: the same calls compiled directly\n");
   printf("  ProcessLine(call_helper(gHelper)):\n");
   gROOT->ProcessLine("call_helper(gHelper);");
   printf("  ProcessLine(needs_big('x')):\n");
   gROOT->ProcessLine("needs_big('x');");
   printf("\nREPRO FINISHED\n");
}

Results with a RelWithDebInfo build without assertions, on 6.40.04 and on master (8d21bba), and on master plus only the LookupHelper.cpp part of this PR. Its RooFit part no longer applies to master.

step	6.40.04 / master	master + this PR
GetGlobalFunctionWithPrototype("call_helper", ...)	❌ found	✅ not found
GetGlobalFunctionWithPrototype("needs_big", "char")	❌ found	✅ not found
GetListOfMethods() of WithDefaults	❌ lists get<int>()	✅ not listed
gInterpreter->GetFunction(0, "needs_big<char>")	❌ found	❌ found, also in a fresh session
TMethodCall for call_helper, executed	❌ valid; the wrapper fails to link (operator() unresolved), the compiler error is never shown, and it returns 0	✅ invalid
TMethodCall for needs_big('x'), executed	❌ runs and returns 1 despite the failing static_assert	❌ same after the GetFunction call; ✅ invalid in a fresh session
afterwards, ProcessLine("call_helper(gHelper);")	❌ no compile error, only a JIT link failure	❌ no compile error, only a link failure
afterwards, ProcessLine("needs_big('x');")	❌ silent	❌ silent
control: the same two ProcessLines in a fresh session	✅ both errors reported	✅ both errors reported
GetFunction("needs_big<char>") with gDebug = 6 (lookup with diagnostics)	error printed, function still returned	❌ segfault in IncrementalParser::commitTransaction

So on master these lookups accept instantiations whose bodies are ill-formed and never show the errors. The failing function can even run, as needs_big does. The broken instantiations also persist, so later ordinary compiled code that uses them no longer reports the errors either. The fresh-session control shows those errors normally appear.

This PR fixes the first three cases, but the reproducer shows some gaps:

1. Errors are missed when the function name contains template arguments, as in GetFunction("needs_big<char>"). Parsing such a name in ParseWithShortcuts calls setSuppressAllDiagnostics(diagOnOff == NoDiagnostics), and the ParserStateRAII only restores that at the end of findFunction. So suppression is still on while the body is instantiated. clang doesn't set ErrorOccurred for suppressed diagnostics; it only increments the trap counters. So hasErrorOccurred() can't see the error. Plain names such as call_helper skip that parsing, which is why they're caught.
2. When such an error is detected, failing the transaction can crash. With diagnostics on (gDebug > 5), the same lookup is detected, the current transaction (the lookup's PushTransactionRAII one) is marked failed, and popping it segfaults in commitTransaction. Plain master just prints the error there.
3. The rollback doesn't remove the failed instantiations. Later lookups and compiled code reuse them without re-checking, as in the ProcessLine rows above. This is probably the "imperfect transaction rollback" mentioned in [WIP] Improved error handling for template instantiation #12449.
4. Rolling back the whole transaction can also unload valid instantiations created as side effects, and crash later. That's cling failure when accessing std::map elements from Python #20952; the full explanation is in this comment.

Unlike the earlier cases, these concern C++ users of ROOT's reflection, not Python: with the cppjit-based bindings, Python no longer goes through LookupHelper at all (#24053 handles that side in CppInterOp). If this gets fixed in cling, I'd suggest following the same approach as compiler-research/CppInterOp#1179: detect errors only for the one instantiation, with a DiagnosticErrorTrap, which also counts suppressed errors. Then mark that specialization invalid and reject it, instead of checking the global error flag and failing the whole transaction.

---

This comment was generated by Claude Opus 5.5.

[bendavid]

To state the conclusion from the previous comment explicitly: as it stands, this PR introduces crashes through its transaction rollbacks that master doesn't have.

• cling failure when accessing std::map elements from Python #20952 (6.40.04 built as C++17, with the cppyy bindings): rolling back the whole transaction after the failed lookup unloads valid instantiations created as side effects of the failing chain, and the next lookup crashes inside clang. Without this PR the error is swallowed and nothing crashes.
• C++ only, on master built as C++20: gInterpreter->GetFunction(0, "needs_big<char>") with gDebug > 5 segfaults in IncrementalParser::commitTransaction when the failed nested transaction is popped. Without this PR the error is just printed.
• In addition, the rollback leaves the failed specialization behind, so later lookups and compiled code reuse it without reporting the error.

The common cause seems to be the approach rather than one bug. The PR decides based on the global error flag and then fails the whole current transaction. But cling's unloading of template instantiations is incomplete in both directions: it removes valid declarations that other code still refers to, and it leaves failed ones behind. So I don't think the PR is safe to merge as is. Scoping detection and rejection to the single instantiation, as in compiler-research/CppInterOp#1179, avoids rolling back anything.

Limits of what I tested: all builds were RelWithDebInfo without assertions; the #20952 case needs a C++17 build, since in C++20 libstdc++'s pair constructors aren't templates with default arguments; I applied only the LookupHelper.cpp part of the PR. The #20952 crash happens in TClass::GetListOfMethods, which C++ code can call too, so it most likely also affects C++ callers, but I've only reproduced it from Python on 6.40. With the cppjit-based bindings, Python no longer goes through LookupHelper, so on master these problems concern C++ users of ROOT's reflection.

---

This comment was generated by Claude Opus 5.5.