[WIP] Improved error handling for template instantiation

[bendavid]

Two substantive changes:

1. Explicitly catch errors in pyroot when the wrapper function fails to compile (this is actually an expanded version of a partial fix which is already upstream in cppyy: https://github.com/wlav/cppyy-backend/commit/8de6ed5ffcabaeba52fba5b8471149c6bb1fe71d)
2. Make sure template instantiation fails by catching clang errors within LookupHelper and rolling back the transaction where appropriate (still not entirely sure this is exactly the right fix, @Axel-Naumann @jalopezg-git please take a look)
3. Implement a mechanism for redirecting cling diagnostics to a user provided ostream and use this in cppyy to capture the diagnostic output and append it to the python exceptions or warnings as appropriate

This PR fixes https://github.com/root-project/root/issues/11854

There are still some remaining problems with the transaction rollback, however template instantiation from cppyy now behaves the same as calling TInterpreter::Declare in this respect. This is likely related to the issues described by @jalopezg-git in https://github.com/root-project/root/pull/12449#issuecomment-1467860880 and can be fixed in a future PR.

Consider the following test case:

test.h:

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();
}

test.py

import ROOT

ret = ROOT.gInterpreter.Declare('#include "test.h"')
print("declare ret", ret)

print("creating helper")
helper = ROOT.Helper[ROOT.std.vector["double"]]()

print("calling helper")

for i in range(2):
   print(f"call attempt {i}")
   try:
      res = ROOT.call_helper(helper)
      print("helper call succeeded:", res)
   except Exception as e:
      print("helper call failed")
      print(e)

The output below is now close to optimal for the first instantiation attempt. On the second instantiation attempt the error message is different/less useful because of the imperfect transaction rollback already noted. (but the same happens instantiating the template through TInterpreter::Declare as said)

declare ret True
creating helper
calling helper
call attempt 0
helper call failed
Template method resolution failed:
  Failed to instantiate "call_helper(Helper<vector<double> >&)"
    In file included from input_line_52:1:
/home/b/bendavid/pyrootdebug6/test.h: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();
    ~~~ ^
/home/b/bendavid/pyrootdebug6/test.h:18:10: note: in instantiation of member function 'Helper<std::vector<double, std::allocator<double> > >::operator()' requested here
  return helper();
         ^
note: in instantiation of function template specialization 'call_helper<Helper<std::vector<double, std::allocator<double> > > >' requested here
/home/b/bendavid/pyrootdebug6/test.h:9:23: note: variable 'res' declared const here
    const std::size_t res = 0;
    ~~~~~~~~~~~~~~~~~~^~~~~~~

  Failed to instantiate "call_helper(Helper<vector<double> >*)"
    error: called object type 'Helper<std::vector<double, std::allocator<double> > > *' is not a function or function pointer
note: in instantiation of function template specialization 'call_helper<Helper<std::vector<double, std::allocator<double> > > *>' requested here

  Failed to instantiate "call_helper(Helper<vector<double> >)"
    error: type 'const Helper<std::vector<double, std::allocator<double> > >' does not provide a call operator
note: in instantiation of function template specialization 'call_helper<Helper<std::vector<double, std::allocator<double> > > >' requested here

call attempt 1
helper call failed
Template method resolution failed:
  Failed to instantiate "call_helper(Helper<vector<double> >&)"
    error: type 'const Helper<std::vector<double, std::allocator<double> > >' does not provide a call operator
note: in instantiation of function template specialization 'call_helper<Helper<std::vector<double, std::allocator<double> > > >' requested here

  Failed to instantiate "call_helper(Helper<vector<double> >*)"
    error: called object type 'Helper<std::vector<double, std::allocator<double> > > *' is not a function or function pointer
note: in instantiation of function template specialization 'call_helper<Helper<std::vector<double, std::allocator<double> > > *>' requested here

  Failed to instantiate "call_helper(Helper<vector<double> >)"
    error: type 'const Helper<std::vector<double, std::allocator<double> > >' does not provide a call operator
note: in instantiation of function template specialization 'call_helper<Helper<std::vector<double, std::allocator<double> > > >' requested here

(on the console the output also has the nice highlighting and colors one would normally get from clang diagnostic printing, see screenshot below)

Needless to say, taken together this constitutes a major improvement when trying to use complex templated code with pyroot/cppyy

[phsft-bot]

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14 How to customize builds

[Axel-Naumann]

The failure to unload broken declarations (@jalopezg-git FYI), does that still happen after this PR, or is this addressed by the PR? I'm not sure I understand how much of the PR description describes this PR vs what's left to be done?

[bendavid]

All of the code example/output in the PR description corresponds to with this PR included.

Repeating the lookup without diagnostic suppression doesn't give the correct error message again (this corresponds to the "call attempt 1" case in the output from test.py in the PR description)

ie with superfluous debug output snipped out:

declare ret True
creating helper
calling helper
call attempt 0
In file included from input_line_52:1:
/home/b/bendavid/pyrootdebug3/test.h: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();
    ~~~ ^
/home/b/bendavid/pyrootdebug3/test.h:18:10: note: in instantiation of member function 'Helper<std::vector<double, std::allocator<double> > >::operator()' requested here
  return helper();
         ^
note: in instantiation of function template specialization 'call_helper<Helper<std::vector<double, std::allocator<double> > > >' requested here
/home/b/bendavid/pyrootdebug3/test.h:9:23: note: variable 'res' declared const here
    const std::size_t res = 0;
    ~~~~~~~~~~~~~~~~~~^~~~~~~
/home/b/bendavid/pyrootdebug3/test.h:18:10: error: called object type 'Helper<std::vector<double, std::allocator<double> > > *' is not a function or function pointer
  return helper();
         ^~~~~~
note: in instantiation of function template specialization 'call_helper<Helper<std::vector<double, std::allocator<double> > > *>' requested here
helper call failed
Template method resolution failed:
  Failed to instantiate "call_helper(Helper<vector<double> >&)"
  Failed to instantiate "call_helper(Helper<vector<double> >*)"
  Failed to instantiate "call_helper(Helper<vector<double> >)"
call attempt 1
/home/b/bendavid/pyrootdebug3/test.h:18:10: error: called object type 'Helper<std::vector<double, std::allocator<double> > > *' is not a function or function pointer
  return helper();
         ^~~~~~
note: in instantiation of function template specialization 'call_helper<Helper<std::vector<double, std::allocator<double> > > *>' requested here
helper call failed
Template method resolution failed:
  Failed to instantiate "call_helper(Helper<vector<double> >&)"
  Failed to instantiate "call_helper(Helper<vector<double> >*)"
  Failed to instantiate "call_helper(Helper<vector<double> >)"

So the relevant error message error: cannot assign to variable 'res' with const-qualified type 'const std::size_t' (aka 'const unsigned long') only appears in the first attempt (and is only printed because I've set gDebug=6 here)

[bendavid]

Failure for ubuntu20 build looks possibly unrelated to this PR.

[bendavid]

So in fact there is already a problem with rolling back the transaction even when just using TInterpreter::Declare:

test.h

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();
}

testdeclare.py

import ROOT

ret = ROOT.gInterpreter.Declare('#include "test.h"')
print("header include ret", ret)

print("creating helper")
helper = ROOT.Helper[ROOT.std.vector["double"]]()

bad_template = "template std::size_t call_helper<Helper<std::vector<double>>>(const Helper<std::vector<double>>&);"

for i in range(2):
    print(f"declare attempt {i}")
    ret = ROOT.gInterpreter.Declare(bad_template)
    print("ret", ret)

output:

header include ret True
creating helper
declare attempt 0
In file included from input_line_52:1:
/home/b/bendavid/pyrootdebug3/test.h: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();
    ~~~ ^
/home/b/bendavid/pyrootdebug3/test.h:18:10: note: in instantiation of member function 'Helper<std::vector<double, std::allocator<double> > >::operator()' requested here
  return helper();
         ^
/home/b/bendavid/pyrootdebug3/test.h:9:23: note: variable 'res' declared const here
    const std::size_t res = 0;
    ~~~~~~~~~~~~~~~~~~^~~~~~~
ret False
declare attempt 1
/home/b/bendavid/pyrootdebug3/test.h:18:10: error: type 'const Helper<std::vector<double, std::allocator<double> > >' does not provide a call operator
  return helper();
         ^~~~~~
input_line_55:1:22: note: in instantiation of function template specialization 'call_helper<Helper<std::vector<double, std::allocator<double> > > >' requested here
template std::size_t call_helper<Helper<std::vector<double>>>(const Helper<std::vector<double>>&);
                     ^
ret False

So again the error message is different/more obscure on the second attempt at explicit template instantiation.

[jalopezg-git]

The failure to unload broken declarations (@jalopezg-git FYI), does that still happen after this PR, or is this addressed by the PR? I'm not sure I understand how much of the PR description describes this PR vs what's left to be done?

I don't know how this PR relates to the unloading issues in cling. What I saw in the past is that DeclUnloader is buggy; specifically, it always removes declarations from the AST when that's not always appropriate. One case in which this fails is for members of a templated class (which clang initially marks as "pending instantiation"). If those are instantiated implicitly as part of a transaction that fails, DeclUnloader removes the member declaration. This prevents the decl from being re-emitted when needed. Instead, it should be left in the previous state and marked as "pending instantiation" again. I have some code that should fix this (which coincides with most if not all the reported unloading issues). I will clean it and open a PR as soon as I finish the current on-going RNTuple work. :slightly_smiling_face:

[bendavid]

After modifying the logic to catch the error and fail the transaction rather than unloading the decl directly, repeated attempts at template instantiation from pyroot now behaves similarly to with TInterpreter::Declare. The remaining problems with incomplete rollback are almost certainly related to the issue which @jalopezg-git referred to, and can be fixed by his forthcoming PR.

Still TODO for this PR: Capture and print the relevant errors and warnings during template instantiation.

[github-actions[bot]]

Test Results

       2 files         2 suites   14h 55m 0s :stopwatch: 2 416 tests 2 408 :heavy_check_mark: 0 :zzz: 8 :x: 4 834 runs  4 821 :heavy_check_mark: 4 :zzz: 9 :x:

For more details on these failures, see this check.

Results for commit fce62a9a.

:recycle: This comment has been updated with latest results.

[bendavid]

The failure in tutorial-roofit-rf408_RDataFrameToRooFit-py is actually a real error which wasn't being caught before (a rather subtle SFINAE problem)

[bendavid]

PR and description updated addressing also the diagnostic capture and printing.

I'm still not totally sure about how the catching of errors and rollback of the transaction is handled in LookupHelper as said in the description.

[bendavid]

Any ideas on the remaining windows failure would also be welcome (it doesn't happen on linux and I don't have a windows setup to test with at the moment)

[phsft-bot]

Build failed on windows10/cxx14. Running on null:C:\build\workspace\root-pullrequests-build See console output.

Failing tests:

• projectroot.roottest.cling.dict.roottest_cling_dict_assertTmpltDefArgCtor

[jalopezg-git]

I think this PR stalled for a while; perhaps we can give it a push and finally merge in the coming weeks?

[bendavid]

Yes ok I can come back to this next week.

[jalopezg-git]

There are still some remaining problems with the transaction rollback, however template instantiation from cppyy now behaves the same as calling TInterpreter::Declare in this respect. This is likely related to the issues described by @jalopezg-git in #12449 (comment) and can be fixed in a future PR.

FYI, https://github.com/root-project/root/pull/13565 should fix the issues with unloading that I mentioned before in this PR! I still need to look at two test failures, but it's mostly there :slightly_smiling_face:!

[jalopezg-git]

There are still some remaining problems with the transaction rollback, however template instantiation from cppyy now behaves the same as calling TInterpreter::Declare in this respect. This is likely related to the issues described by @jalopezg-git in #12449 (comment) and can be fixed in a future PR.

FYI, #13565 should fix the issues with unloading that I mentioned before in this PR! I still need to look at two test failures, but it's mostly there 🙂!

@bendavid #13565 was recently merged into master. Provided that you have the time, you could rebase this PR and see how it works now.