Skip to content
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

[BUG]: std::vector<std::optional<std::string>> behaviour has changed in 2.2.0 #747

Closed
daveah opened this issue Oct 4, 2024 · 2 comments
Closed

Comments

@daveah
Copy link

daveah commented Oct 4, 2024

Problem description

When binding a std::vector<std::optional<std::string>> Nones in python become empty strings in C++ in 2.2.0; whereas in 2.1.0 Nones become empty optionals. I suspect this is related to #675

Reproducible example code

C++:

#include <nanobind/nanobind.h>
#include <nanobind/stl/optional.h>
#include <nanobind/stl/string.h>
#include <nanobind/stl/vector.h>

namespace nb = nanobind;

using namespace nb::literals;

std::string vector_optional_string(std::vector<std::optional<std::string>> strs) {
    std::string result;
    for (const auto& str : strs) {
        if (!str.has_value()) {
            result += "NONE, ";
        } else {
            result += str.value() + ", ";
        }
    }
    return result;
}

NB_MODULE(nanobind_example_ext, m) {
    m.def("vector_optional_string", &vector_optional_string);
}

Python3:

from .nanobind_example_ext import vector_optional_string
vector_optional_string(["foo", "bar", "baz", None])

In 2.1.0 we get:
'foo, bar, baz, NONE, '

In 2.2.0 we get:
'foo, bar, baz, , '

@daveah
Copy link
Author

daveah commented Oct 4, 2024

To reproduce this, I downloaded nanobind_example from github, hacked it for the above functions, compiled in Python 3.11; nanobind 2.1.0 and nanobind 2.2.0

@wojdyr
Copy link
Contributor

wojdyr commented Oct 4, 2024

Ooops. I was thinking that we don't need to explicitly set nullopt in from_python. Here is a patch with a fix:

--- a/include/nanobind/stl/detail/nb_optional.h
+++ b/include/nanobind/stl/detail/nb_optional.h
@@ -23,5 +23,6 @@ struct optional_caster {
     bool from_python(handle src, uint8_t flags, cleanup_list* cleanup) noexcept {
-        if (src.is_none())
-            // default-constructed value is already empty
+        if (src.is_none()) {
+            value.reset();
             return true;
+        }

I'll add a test and make a PR.

EDIT:
actually I copied the comment about default-constructed value being empty from pybind11:
https://github.com/pybind/pybind11/blob/7e418f49243bb7d13fa92cf2634af1eeac386465/include/pybind11/stl.h#L523-L529
Is it that resetting the value is not needed in pybind11, but is needed in nanobind?

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

2 participants