Function call breaks with std::string_view

[aaronj0]

We ran into failures in root in the following call:

df["data"] = ROOT.RDataFrame("mini", (os.path.join(path, "GamGam/Data/data_{}.GamGam.root".format(x)) 
                                                                     for x in ("A", "B", "C", "D")))

to the RDataFrame constructor:

   RDataFrame(std::string_view treename, const std::vector<std::string> &filenames, 
            const ColumnNames_t &defaultColumns = {});

This currently works on ROOT master. However we unearthed this failure while reverting some patches to CPyCppyy(https://github.com/root-project/root/pull/16212/files) and discovered a minimal example also fails on cppyy master.

On trials with the cases it failed in, a minimal reproduces looks something like this:

cppyy.cppdef('''void processMessage(std::string_view A, std::vector<int> messages) {

                 for (const auto& msg : messages) {
                    std::cout << "Message2: " << msg << std::endl;
                 }} ''')

cppyy.gbl.processMessage("a", (x for x in [1, 2, 3]))

This fails when:

• A generator expression is used ( cppyy.gbl.processMessage("a", [1, 2, 3]) works)
• We add a string_view in the beginning of the function signature:

cppyy.cppdef('''void processMessage(std::vector<int> messages) {

                 for (const auto& msg : messages) {
                    std::cout << "Message2: " << msg << std::endl;
                 }} ''')

cppyy.gbl.processMessage((x for x in [1, 2, 3]))

so the above also works.

• The type of the std::vector does not matter

cc @guitargeek

[aaronj0]

More info:

The PR that reverts the patches in CPyCppyy, removes patches that revert 3 commits upstream.

So I assume the bug was induced by one of the 3 commits upstream, since the example works on ROOT master that currently reverts these commits:

In order:

1. https://github.com/wlav/CPyCppyy/commit/fce87d5e0125bb9e84ea3472dae6643faa5b8aed
2. https://github.com/wlav/CPyCppyy/commit/c06170389ea6e37fc6f25f9f0e44940fb1035625
3. https://github.com/wlav/CPyCppyy/commit/b62b2561322b6c1fdc37ac525077524ea5b02fa0

[wlav]

Those patches came from ROOT in the first place. Make up your mind. :)

[aaronj0]

Those patches came from ROOT in the first place. Make up your mind. :)

Yeah I see it's from Jonas's fixes upstream. I found that the second commit is what caused the bug. @guitargeek any idea how we can fix it?

[guitargeek]

Ok I can look into it, but if what you're saying is true (that the regression comes from c061703), then this is not a patch that comes from ROOT as far as I can tell. Anyway it's a small scoped change that caused the regression, so we should be able to sort it out fast!

[wlav]

Maybe; the problem introduced there is probably along the lines of having 2 implicit converters called and one blocking the other with their flags.

[aaronj0]

@wlav, the issue does come from https://github.com/wlav/CPyCppyy/commit/c06170389ea6e37fc6f25f9f0e44940fb1035625, specifically making this CallContext noimp call in the string_view converter:

bool CPyCppyy::STLStringViewConverter::SetArg(
    PyObject* pyobject, Parameter& para, CallContext* ctxt)
{

 // normal instance convertion (eg. string_view object passed)
   if (!PyInt_Check(pyobject) && !PyLong_Check(pyobject)) {
+       CallContextRAII<CallContext::kNoImplicit> noimp(ctxt);
        if (InstanceConverter::SetArg(pyobject, para, ctxt)) {

if I remove this line and allow it to be implicit then this issue no longer happens. How do you think we should proceed?

[wlav]

That call needs to be protected against implicit conversions; that part is correct. Looks to me that the logic in the RAII object is wrong, though.