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

Implicit conversion of ints (and char const*) to handles? #1309

Open
anntzer opened this issue Mar 6, 2018 · 13 comments · May be fixed by #4004
Open

Implicit conversion of ints (and char const*) to handles? #1309

anntzer opened this issue Mar 6, 2018 · 13 comments · May be fixed by #4004

Comments

@anntzer
Copy link
Contributor

anntzer commented Mar 6, 2018

Issue description

Is there a reason why handles are not implicitly constructible from ints (I may well have missed something obvious...)? e.g. something like

py::object func() {
    py::object tpl = py::make_tuple(1, 2);
    return tpl[1];
}

seems to have a clear meaning but fails to compile (error: invalid conversion from ‘int’ to ‘const char*’ (there are two overloads for operator[], one taking a handle and the other a const char*)).

If char const* was also implicitly convertible to handle then the second overload would not be necessary either.

For additional confusion, 0 is implicitly convertible to a handle (an invalid one: the null pointer).

Asked on gitter (a more specific version of this) but got no reply.

Reproducible example code

Include the above-mentioned code in python_example (replacing the add function there, and the m.def(... line).

@jagerman
Copy link
Member

jagerman commented Mar 6, 2018

I don't think there's any particularly good reason why it's not there--other than that no one has proposed/implemented it yet. (As a side note, for tuples in particular, you're better off keeping the object as a py::tuple, which does work as you expect; it also bypasses the heavier PyObject_GetItem in favour of the lighter PyTuple_GetItem. Likewise for py::list).

For the more generic py::object, I suppose we could accept any arbitrary argument type with a cast to Python. Then strings, floats, custom types, etc. would all work as arguments to obj[]. ISTM that this could replace both the handle and char *key versions, since casting either of those does the right thing (i.e. nothing for the former, and making a python string for the latter).

@anntzer
Copy link
Contributor Author

anntzer commented Mar 7, 2018

OK, I really thought that was such an obvious thing to do that I must have had missed a good reason :-) For example, is there a way to ensure that the literal 0 becomes the integer 0 rather than a null pointer (as it does now)?
Tuple indexing is only an example, of course; there are plenty of other places where this could be used (e.g., py::getattr(foo, "bar", 42 <- this argument)).

@jagerman
Copy link
Member

jagerman commented Mar 7, 2018

Yup. Meanwhile there are other places that already do accept arbitrary arguments--the arguments to make_tuple, for example. So you're right that's it's not a stretch (and indeed expected) to get automatic Python casting.

@cielavenir
Copy link
Contributor

@jagerman @anntzer do you think woodychow@98c9f77 works?

/cc @woodychow

@anntzer
Copy link
Contributor Author

anntzer commented Jun 10, 2022

I guess so? Haven't tried, though.

@woodychow
Copy link

cielavenir, hope you are doing well. Well, let me know if you want me to create a pull request. I don't really remember the context of this anymore.

@cielavenir
Copy link
Contributor

cielavenir commented Jun 13, 2022

@woodychow

context

py::object::operator[] does not accept int (i.e. subscripting by index).

as pybind11 2.10 dropped python2, perhaps we should make MR to v2.9 branch? could you? I resolved conflict in https://github.com/cielavenir/pybind11/commits/v2.9_item_accessor_T .

By the way I was able to test our system on https://github.com/cielavenir/pybind11/commits/v2.9_ty .

@woodychow
Copy link

Well, it's time to upgrade to Python 3 buddy.

I guess I can CP to v2.9 if this MR is accepted, and v2.9 is still open to new MRs...

@Skylion007
Copy link
Collaborator

What's the motivation? For pytypes that have int or char* as a valid subscript type, pytypes already extends that as can be found here. int to pointer conversion is not desirable for a lot of reasons, including hurting the compilers' optimization performance.

Any explicit conversions to handles or objects etc can already be handled by pybind11::cast() function so I am unsure what the motivation is here.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 14, 2022

To be clear, I was originally only proposing this for operator[], where the following asymmetry is a bit jarring:

#include <pybind11/pybind11.h>
namespace py = pybind11;
PYBIND11_MODULE(python_example, m) {
    py::object d = py::dict();
    d["foo"] = "foo"; py::print(d["foo"]);  // OK.
    // d[1] = 1; py::print(d[1]);  // Not OK.
}

@Skylion007
Copy link
Collaborator

@anntzer It seems like the real issue is that 0 is implicitly convertible to a pyobject* which is implicitly convertible to a handle. It's not intended behavior for ints to be implicitly convertible to a handle (and bugprone behavior).

I would argue that this implicit behavior makes things a bit harder to debug especially for more complex casts than primitives. Thoughts @rwgk @henryiii ?

@Skylion007
Copy link
Collaborator

Alternative solution: seems like it may have encountered an issue in our test suite though: #4006

@cielavenir
Copy link
Contributor

@woodychow sorry - could you add me as a collaborator of your pybimd11 repository?

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 a pull request may close this issue.

5 participants