wlav / cppyy

Other
387 stars 39 forks source link

`explicit` keyword is ignored for constructors #165

Open yashnator opened 1 year ago

yashnator commented 1 year ago

Hi,

I used the explicit keyword for the constructor to prevent undesired/meaningless type conversions. However, cppyy seems to ignore this restriction and type-casts the object anyway.

Here is a simple reproducer for the issue -

import cppyy

cppyy.cppdef("""
class MyClass {
public:
   MyClass() = default;
   explicit MyClass(std::string const& str) {}
};

void foo(MyClass const& myObject) {
    std::cout << "Calling foo(MyClass const&)" << std::endl;
}
"""
)

print(cppyy.gbl.foo("This should not work"))

It's mentioned in the documentation that a second iteration is done by cppyy to make the necessary conversions, and using the explicit keyword doesn't stop this iteration.

Any help would be appreciated,

Thanks

wlav commented 1 year ago

Yes, the explicit keyword is not exposed by Cling and thus ignored by cppyy. A pythonization that checks the type and rejects it, is the only workaround I can think of.

The point about a "second round" is that implicit conversion isn't a thing in Python, as everything is pass-by-reference. Hence implicit conversions are ignored initially, and only if no conversion matches, those that could have an implicit conversion are retried. (Implicit conversions are also very inefficient as the concept of a "temporary" isn't a Python thing either, so handling the temporary comes with lots of overhead.)

guitargeek commented 1 year ago

Okay, that is a bit problematic for some code in root-project/root. We often use the explicit keyword there to prevent undesired and meaningless type conversions.

In particular, this causes problems when you have patterns were cppyy attempts to "recover" from an exception by doing an illegal conversion in the second iteration. This can lead to PyROOT-exclusive bugs that are completely weird to the user.

Here I extended the reproducer by @yashnator to demonstrate this pattern:

import ROOT

ROOT.gInterpreter.Declare(
"""

class MyClass {
public:
   MyClass() = default;
   explicit MyClass(std::string const& str) {}
};

void foo(MyClass const& myObject) {
    std::cout << "Calling foo(MyClass const&)" << std::endl;
}

void foo(std::string str) {
    throw std::runtime_error{"This should not be called!"};
}
"""
)

ROOT.foo("test")

I understand that if explicit is not exposed by Cling, it can be difficult to fix this. Maybe cling should know about this? I invite also @vepadulano and @Axel-Naumann (the code owners of PyROOT) to read this and comment.

hahnjo commented 1 year ago

I just checked, Cling and ROOT meta expose this via the kIsExplicit bit in TClingMethodInfo::Property - this should be available in cppyy, no?

wlav commented 1 year ago

Indeed, missed that. Regardless, if this is for ROOT, you'll have to submit a bug report on the ROOT repo, not here, as the ROOT project contains a fork of cppyy, so any fixes here won't propagate.

guitargeek commented 1 year ago

Sure, we'll do that too!