wlav / cppyy

Other
387 stars 39 forks source link

No implicit const conversion of iterators when used with emplace #152

Open agijsberts opened 1 year ago

agijsberts commented 1 year ago

Hello,

While writing a custom vector-like container I noticed strange behavior when passing non-const iterators to the emplace method of my class. I can replicate the same behavior also with std::list, as demonstrated in the following minimal example:

import cppyy

v = cppyy.gbl.std.vector[int]([1, 2, 3])
v.emplace(v.begin(), 2)

l = cppyy.gbl.std.list[int]([1, 2, 3])
l.insert(l.begin(), 2)
l.emplace(l.begin(), 2)

This fails with the error trace:

Traceback (most recent call last):
  File "<stdin>", line 8, in <module>
TypeError: Template method resolution failed:
  std::_List_iterator<int> std::list<int>::emplace(std::list<int>::const_iterator __position, int&& __args) =>
    TypeError: could not convert argument 1
  Failed to instantiate "emplace(std::_List_iterator<int>*,int)"
  std::_List_iterator<int> std::list<int>::emplace(std::list<int>::const_iterator __position, int&& __args) =>
    TypeError: could not convert argument 1

I can work around the error by replacing begin() with cbegin(), but if I am not mistaken the iterator should implicitly be converted to const_iterator; the equivalent C++ code works without problems. I am not familiar with Cppyy's codebase, but the error leads me to believe it might have something to do with the fact that emplace is a template function, which would also explain why insert works without problem despite also using const_iterator as first argument. This does not explain, however, why vector.emplace works without problems...

Tested with cppyy 3.0.0 (git head), cppyy_backend 1.14.11, cppyy_cling 6.28.0, CPyCppyy 1.12.13.

wlav commented 1 year ago

it might have something to do with the fact that emplace is a template function

Probably, as the implicit conversion works fine per se:

>>> import cppyy
>>> v = cppyy.gbl.std.vector[int]
>>> v.const_iterator(v.iterator())
<cppyy.gbl.std.__wrap_iter<const int*> object at 0x12ee849a0>
>>>

There's a bit of a problem in allowing implicit conversion with templated functions as e.g. type promotions are supposed to be disallowed. IIRC, implicit conversion is therefore disabled altogether for templates.

wlav commented 1 year ago

It's actually a bit smarter: implicit conversion is allowed for already instantiated templates, but not for templates currently being instantiated. If I enable allowing implicit conversion in all cases, it runs fine.

I'll have to think a bit on how to refine that.

agijsberts commented 1 year ago

Out of curiosity, any reason why it works currently for std::vector and not for std::list? The only mention I found of vector is the special treatment of stl sequence types at https://github.com/wlav/cppyy/blob/master/python/cppyy/_cpython_cppyy.py#L58, but this seemed unrelated and identical for both container types anyways.

wlav commented 1 year ago

Sorry, what works for std::vector but not for std::list? In the above code, the issue for both is the same: not allowing implicit conversions for templated methods. If I enable that, the code as written above, both vector and list works.

wlav commented 1 year ago

And yes, that piece of code is unrelated. It exists so that you can write:

>>> import cppyy
>>> cppyy.gbl.std.vector([1,2,3])
<cppyy.gbl.std.vector<int> object at 0x13182b740>
>>>

Note the lack of template argument.

agijsberts commented 1 year ago

Strange, to be sure I just triple checked and I only get an error on the list operation

l.emplace(l.begin(), 2)

and not on the equivalent vector operation

v.emplace(l.begin(), 2)

The code is exactly the one from my initial post. So if I remove the last line, it runs without problem.

wlav commented 1 year ago

I was running on Mac. There both emplace calls fail, and both insert calls succeed, b/c of the reasons described above. But I've found it now, there's this old workaround in the converters that is specific to STL iterators:

        // CLING WORKAROUND -- special case for STL iterators
            if (realType.rfind("__gnu_cxx::__normal_iterator", 0) /* vector */ == 0
#ifdef __APPLE__
                || realType.rfind("__wrap_iter", 0) == 0
#endif
                // TODO: Windows?
               ) {
                static STLIteratorConverter c;
                result = &c;
            } else
       // -- CLING WORKAROUND

This converter basically disables type checking on iterators that match, and per the commit message, it was b/c the cleaned type from Cling wasn't correct if it included *const&. For std::list, however, the type of the iterator for list is std::_List_const_iterator, so it doesn't use this converter.

agijsberts commented 1 year ago

Thanks a lot for getting to the bottom of this. And sorry, I should've specified that I was running this on Linux.

It's actually a bit smarter: implicit conversion is allowed for already instantiated templates, but not for templates currently being instantiated. If I enable allowing implicit conversion in all cases, it runs fine.

I'll have to think a bit on how to refine that.

I'll leave the issue open for now, as you suggest that generally allowing implicit conversions may actually fix this if it doesn't have other unwanted side effects.