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

Mark function type caster as optional #910

Closed
wants to merge 1 commit into from
Closed

Conversation

tjstum
Copy link
Contributor

@tjstum tjstum commented Feb 4, 2025

At runtime, the type caster will convert between Python None and an empty std::function object. However, the emitted stubs do not reflect this fact

At runtime, the type caster will convert between Python `None` and an
empty `std::function` object. However, the emitted stubs do not
reflect this fact
@wjakob
Copy link
Owner

wjakob commented Feb 4, 2025

Hi @tjstum,

if what you say is correct, it looks to me like an undocumented omission/bug. The only nanobind argument types that can take None are wrappers (e.g. nb:object), and class bindings. Anything to do with type casters normally should normally not be nullable -- the user has to wrap them explicitly using std::optional.

@wojdyr
Copy link
Contributor

wojdyr commented Feb 5, 2025

It can take None only if .none() is specified.
If stubgen can't check cast_flags::accepts_none, I think it's OK that it reflects the default (no none) case. The user can overwrite it with nb::sig.

I find handling None to be a convenient feature rather than a bug.
Let's say we have a C++ function in class A:

void set_logger(std::function<void(std::string)> f)

As it is now, the user explicitly adds .none() to make it handle None:

.def("set_logger", &A::set_logger, nb::arg().none())

If wrapping in std::optional was necessary, the equivalent would be:

#include <nanobind/stl/optional.h>

.def("set_logger", [](A& self, std::optional<std::function<void(std::string)>> f) {
    self.set_logger(f ? *f : nullptr);
}, nb::arg().none())

IMO adding .none() is explicit enough. The latter is less readable.

The documentation is not accurate here – I tried to bring it up in #683.
std::variant and ndarray also handle None (and adding const char* to this list, as it is in pybind11, would be nice).

@tjstum
Copy link
Contributor Author

tjstum commented Feb 5, 2025

Yeah, this actually first showed up in properties (e.g. def_ro and def_prop_ro). If those are/return a std::function, the type_caster for std::function could actually return Python None (if the std::function is empty). So it's not (only) about accepting None.

@wjakob I'm not entirely sure I understand what you are saying about wrapping with std::optional. If you have a std::function member attribute (that's exposed using .def_ro), where would you wrap it?

@tjstum
Copy link
Contributor Author

tjstum commented Feb 11, 2025

I talked to @oremanj offline about this for a while.

Another downside of this approach is that, if you have a std::function attribute bound with def_rw, you need to add nb::arg().none() to the def_rw binding so that the setter will accept None. However, if all we do is change the type caster, it will appear (from static type information) as though the setter accepts None (but it won't without the argument annotation).

So I'll give this some more thought and see if we can come up with a different proposal. But it's sort of a tricky situation, with a type caster being willing to accept/return None but functions needing the extra annotation as well.

@tjstum tjstum closed this Feb 11, 2025
@tjstum tjstum deleted the funcopt branch February 11, 2025 23:00
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

Successfully merging this pull request may close these issues.

3 participants