Skip to content

pybind11/stl.h converts string to vector<string> #1258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zpz opened this issue Jan 19, 2018 · 2 comments
Closed

pybind11/stl.h converts string to vector<string> #1258

zpz opened this issue Jan 19, 2018 · 2 comments

Comments

@zpz
Copy link

zpz commented Jan 19, 2018

#include <pybind11/pybind11.h>
#include <pybind11/stl.h>
#include <iostream>
#include <string>
#include <vector>

namespace py = pybind11;


class My {
    private:
        std::string _name;
        std::vector<std::string> _names;

    public:
        My(std::string name) : _name(name) {}
        My(std::vector<std::string> names) : _names(names) {}

        void print() const {
            if (_name.length() > 0) {
                std::cout << _name << std::endl;
            }
            if (_names.size() > 0) {
                for (auto const & v : _names) {
                    std::cout << "-" << v << "-";
                }
                std::cout << std::endl;
            }
        }
};


PYBIND11_MODULE(example, m) {
    py::class_<My>(m, "My")
        .def(py::init<std::string>())
        .def(py::init<std::vector<std::string>>())
        .def("print", &My::print);
}

In Python,

>>> from example import My
>>> My('myname').print()
myname
>>> 

Now change the binding code to list the vector<string> constructor before the string constructor. Then in Python,

>>> from example import My
>>> My('myname').print()
-m--y--n--a--m--e-

If I do not include stl.h, then this does not happen---the vector<string> constructor wouldn't take a string parameter. Clearly the behavior listed above is automatic conversion by stl.h.

However, in my opinion a str should never be auto converted to a vector<string>. Because on the Python side it's trivial to convert a str to a list, the user should explicitly do this conversion if they wants to use a vector binding.

Because stl.h is often useful, the chance of the above situation happening is not low.

@bmerry
Copy link
Contributor

bmerry commented Jan 19, 2018

My 2c: I think the fundamental issue is that str should not be treated as a sequence, even though it technically is, because it is fairly rare to pass it in places that expect a general sequence and doing so more likely indicates a bug. There are a few ways one could address this e.g.

  1. Make noconvert vector loader stricter about what sequence types it accepts. It could be a whitelist (list, tuple) or a blacklist (str). That will make the vector<string> overload less preferred since it will be converting. Given the limited power of the converting/non-converting overload resolution though, I suspect there may still be other problems.
  2. Explicitly check for and reject str in the vector loader. While str is technically a sequence, it's relative rare to want to pass a str to a function that expects a sequence. There may be a case for allowing strvector<char> though.

A related question is what should happen with bytes in Python 3, which I'm guessing would be accepted by a vector<int> loader.

@jagerman
Copy link
Member

I agree that this seems like a bug. I think blacklisting str is the right away to go here. I don't even think it should be accepted in the converting pass (even if it were a std::vector<char>) because it just doesn't seem useful or intuitive. In the very rare case that someone wants that behaviour, they are still perfectly free to just overload the method with versions taking std::vector<char>/std::vector<std::string>/std::string/py::str arguments instead.

wjakob pushed a commit that referenced this issue Oct 11, 2018
* Fix for Issue #1258

list_caster::load method will now check for a Python string and prevent its automatic conversion to a list.
This should fix the issue "pybind11/stl.h converts string to vector<string> #1258" (#1258)

* Added tests for fix of issue #1258

* Changelog: stl string auto-conversion
@wjakob wjakob closed this as completed Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants