fix: enable py::implicitly_convertible<py::none, ...> for py::class_-wrapped types #3059

crisluengo · 2021-06-25T06:04:47Z

Description

Enables implicit casts from None to a custom type, by allowing custom converters to run first, and doing the check for py::none at the end.
This fixes #2778.

The new test case is a nice illustration:

py::class_<NoneCastTester>(m, "NoneCastTester")
    .def(py::init<>())
    .def(py::init<int>())
    .def(py::init([](py::none const&) { return NoneCastTester{}; }));
py::implicitly_convertible<py::none, NoneCastTester>();
m.def("ok_obj_or_none", [](NoneCastTester const& foo) { return foo.answer; });

This allows in Python:

ok_obj_or_none(NoneCastTester())  # as was allowed previously
ok_obj_or_none(None)              # this is new

Suggested changelog entry:

* Enable ``py::implicitly_convertible<py::none, ...>`` for ``py::class_``-wrapped types.

crisluengo · 2021-06-25T06:28:49Z

There seems to be some issue downloading Catch on Windows. None of the failed tests actually ran.

henryiii · 2021-06-25T17:17:22Z

Known issue for the last day or two. Guessing it's a networking issue on GHA's side?

henryiii

Seems fine to me, @EricCousineau-TRI or @rwgk would you like to take a look too? Also, @rwgk should we port your CI patch for now? Maybe we can get the file some other way, instead? (It's a GitHub to GitHub download that's failing on Windows only...)

henryiii · 2021-06-25T17:21:31Z

@YannickJadoul You looked at this before, as well, along with @bstaletic, happy to take a once-over!

EricCousineau-TRI

LGTM for code!

That being said, there is an existing way to admit casting from py::none to a different representation (custom object) by using py::detail::type_caster<>.
Docs: https://pybind11.readthedocs.io/en/stable/advanced/cast/custom.html
Example (in pybind11) for std::optional<T> caster:

pybind11/include/pybind11/stl.h

Lines 281 to 283 in 484b0f0

    
           } else if (src.is_none()) { 
        
               return true;  // default-constructed value is already empty 
        
           }

Therefore, could you better qualify your commit / PR title to reflect that?
e.g.
"fix: allow casting from None to custom object when using type_caster_generic" (impl details)
or
"fix: allow casting from None to custom object for generic types"

rwgk · 2021-06-25T17:37:50Z

"fix: allow casting from None to custom object for generic types"

LGTM for code, too.

Using the word "custom" puts me on the wrong track. IIUC the feature change is for py::class_-wrapped types and ties in with py::implicitly_convertible, is that correct? Would this make sense as PR title?

Enable py::implicitly_convertible<py::none, ...> for py::class_-wrapped types.

crisluengo · 2021-06-25T19:15:02Z

@rwgk
Indeed, it is about the implicit cast to a wrapped C++ class. I have updated the PR title and the suggested change log entry.

@EricCousineau-TRI
It is maybe possible to write a py::detail::type_caster<>, I honestly have not tried this avenue. I have written type casters for C++ types that I wanted to correspond to native Python types (i.e. I didn't want to wrap the C++ classes). I don't know how py::detail::type_caster<> interacts with a wrapped class. On the other hand, why write so much code for stuff that should be generated automatically? :)

rwgk · 2021-06-25T19:26:05Z

To get full CI coverage, could you try to adopt my ci.yml patch from the smart_holder branch here in this PR?

One way to get the diff (git pull on master & smart_holder first to be up-to-date):

git diff master smart_holder -- .github/workflows/ci.yml

Ignore the first two + in the diff, apply everything else.

rwgk · 2021-06-25T20:47:27Z

The one failing CI job is safe to ignore, GHA simply ran out of macos hosts (I saw that several times before):

2021-06-25T20:11:48.6474262Z Found online and idle hosted runner(s) in the current repository's enterprise account that matches the required labels: 'macos-latest'
2021-06-25T20:11:48.6474317Z Waiting for a hosted runner in 'enterprise' to pick this job...

rwgk

@henryiii I think merging this quickly including the ci.yml patch would be great.

EricCousineau-TRI · 2021-06-25T23:06:12Z

@EricCousineau-TRI
It is maybe possible to write a py::detail::type_caster<>, I honestly have not tried this avenue. [...]

Haha, my b. I was more or less trying to say the same thing as Ralf - your fix is great and stand on its own, just needed to clarify the title. Sorry about that, and thanks!

rwgk · 2021-06-26T00:56:07Z

Thanks everybody! I'll merge this now with the added benefit of restoring our CI to be fully functioning.

…wrapped types (#3059) * Allow casting from None to a custom object, closes #2778 * ci.yml patch from the smart_holder branch for full CI coverage.

rwgk · 2021-06-30T20:27:23Z

tests/test_methods_and_attributes.py

@@ -431,6 +431,17 @@ def test_accepts_none(msg):
    assert "incompatible function arguments" in str(excinfo.value)


+def test_casts_none(msg):


Post-merge comment: msg is unused and can be removed. Looks like a copy-paste accident.
(I'll create a PR to fix.)

I did not go back to check when and why exactly this slipped in. Also removing unused `msg` (oversight in PR #3059).

…nd#3059.

v2.7.0 (Jul 16, 2021) --------------------- New features: * Enable ``py::implicitly_convertible<py::none, ...>`` for ``py::class_``-wrapped types. `#3059 <https://github.com/pybind/pybind11/pull/3059>`_ * Allow function pointer extraction from overloaded functions. `#2944 <https://github.com/pybind/pybind11/pull/2944>`_ * NumPy: added ``.char_()`` to type which gives the NumPy public ``char`` result, which also distinguishes types by bit length (unlike ``.kind()``). `#2864 <https://github.com/pybind/pybind11/pull/2864>`_ * Add ``pybind11::bytearray`` to manipulate ``bytearray`` similar to ``bytes``. `#2799 <https://github.com/pybind/pybind11/pull/2799>`_ * ``pybind11/stl/filesystem.h`` registers a type caster that, on C++17/Python 3.6+, converts ``std::filesystem::path`` to ``pathlib.Path`` and any ``os.PathLike`` to ``std::filesystem::path``. `#2730 <https://github.com/pybind/pybind11/pull/2730>`_ * A ``PYBIND11_VERSION_HEX`` define was added, similar to ``PY_VERSION_HEX``. `#3120 <https://github.com/pybind/pybind11/pull/3120>`_ Signed-off-by: Zheng Ruoqin <[email protected]> Signed-off-by: Khem Raj <[email protected]>

v2.7.0 (Jul 16, 2021) --------------------- New features: * Enable ``py::implicitly_convertible<py::none, ...>`` for ``py::class_``-wrapped types. `#3059 <https://github.com/pybind/pybind11/pull/3059>`_ * Allow function pointer extraction from overloaded functions. `#2944 <https://github.com/pybind/pybind11/pull/2944>`_ * NumPy: added ``.char_()`` to type which gives the NumPy public ``char`` result, which also distinguishes types by bit length (unlike ``.kind()``). `#2864 <https://github.com/pybind/pybind11/pull/2864>`_ * Add ``pybind11::bytearray`` to manipulate ``bytearray`` similar to ``bytes``. `#2799 <https://github.com/pybind/pybind11/pull/2799>`_ * ``pybind11/stl/filesystem.h`` registers a type caster that, on C++17/Python 3.6+, converts ``std::filesystem::path`` to ``pathlib.Path`` and any ``os.PathLike`` to ``std::filesystem::path``. `#2730 <https://github.com/pybind/pybind11/pull/2730>`_ * A ``PYBIND11_VERSION_HEX`` define was added, similar to ``PY_VERSION_HEX``. `#3120 <https://github.com/pybind/pybind11/pull/3120>`_ Signed-off-by: Zheng Ruoqin <[email protected]> Signed-off-by: Khem Raj <[email protected]> Signed-off-by: Trevor Gamblin <[email protected]>

Allow casting from None to a custom object, closes pybind#2778

944cf7f

henryiii approved these changes Jun 25, 2021

View reviewed changes

henryiii changed the title ~~Allow casting from None to a custom object~~ fix: allow casting from None to a custom object Jun 25, 2021

EricCousineau-TRI requested changes Jun 25, 2021

View reviewed changes

crisluengo changed the title ~~fix: allow casting from None to a custom object~~ fix: enable py::implicitly_convertible<py::none, ...> for py::class_-wrapped types Jun 25, 2021

ci.yml patch from the smart_holder branch for full CI coverage.

86fe43a

rwgk approved these changes Jun 25, 2021

View reviewed changes

EricCousineau-TRI approved these changes Jun 25, 2021

View reviewed changes

rwgk merged commit 93e6919 into pybind:master Jun 26, 2021

github-actions bot added the needs changelog Possibly needs a changelog entry label Jun 26, 2021

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Jun 26, 2021

Tracking change in type_caster_base.h on master (PR pybind#3059).

21828db

crisluengo mentioned this pull request Jun 26, 2021

Allow building against system libs instead of bundled dependencies DIPlib/diplib#76

Open

rwgk pushed a commit that referenced this pull request Jun 26, 2021

Tracking change in type_caster_base.h on master (PR #3059).

68b6f8c

rwgk reviewed Jun 30, 2021

View reviewed changes

rwgk pushed a commit that referenced this pull request Jun 30, 2021

Fixing merge accident related to merging PR #3059 from master.

94f0db5

I did not go back to check when and why exactly this slipped in. Also removing unused `msg` (oversight in PR #3059).

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Jun 30, 2021

Fixing very minor oversight in PR pybind#3059.

90cb135

henryiii pushed a commit that referenced this pull request Jun 30, 2021

tests: remove very minor oversight in PR #3059. (#3066)

cad79c1

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Jul 11, 2021

Rollback (experiment) of DOWNLOAD_CATCH=OFF change merged via PR pybi…

9417ffe

…nd#3059.

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Jul 11, 2021

Rollback of DOWNLOAD_CATCH=OFF change merged via PR pybind#3059.

0644e1b

rwgk pushed a commit that referenced this pull request Jul 11, 2021

Rollback of DOWNLOAD_CATCH=OFF change merged via PR #3059. (#3092)

0f4761b

henryiii removed the needs changelog Possibly needs a changelog entry label Jul 13, 2021

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3059

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: enable py::implicitly_convertible<py::none, ...> for py::class_-wrapped types #3059

fix: enable py::implicitly_convertible<py::none, ...> for py::class_-wrapped types #3059

crisluengo commented Jun 25, 2021 •

edited

Loading

crisluengo commented Jun 25, 2021

henryiii commented Jun 25, 2021

henryiii left a comment

henryiii commented Jun 25, 2021

EricCousineau-TRI left a comment

rwgk commented Jun 25, 2021

crisluengo commented Jun 25, 2021

rwgk commented Jun 25, 2021 •

edited

Loading

rwgk commented Jun 25, 2021

rwgk left a comment

EricCousineau-TRI commented Jun 25, 2021

rwgk commented Jun 26, 2021

rwgk Jun 30, 2021

	} else if (src.is_none()) {
	return true; // default-constructed value is already empty
	}

		@@ -431,6 +431,17 @@ def test_accepts_none(msg):
		assert "incompatible function arguments" in str(excinfo.value)


		def test_casts_none(msg):

fix: enable py::implicitly_convertible<py::none, ...> for py::class_-wrapped types #3059

fix: enable py::implicitly_convertible<py::none, ...> for py::class_-wrapped types #3059

Conversation

crisluengo commented Jun 25, 2021 • edited Loading

Description

Suggested changelog entry:

crisluengo commented Jun 25, 2021

henryiii commented Jun 25, 2021

henryiii left a comment

Choose a reason for hiding this comment

henryiii commented Jun 25, 2021

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

rwgk commented Jun 25, 2021

crisluengo commented Jun 25, 2021

rwgk commented Jun 25, 2021 • edited Loading

rwgk commented Jun 25, 2021

rwgk left a comment

Choose a reason for hiding this comment

EricCousineau-TRI commented Jun 25, 2021

rwgk commented Jun 26, 2021

rwgk Jun 30, 2021

Choose a reason for hiding this comment

crisluengo commented Jun 25, 2021 •

edited

Loading

rwgk commented Jun 25, 2021 •

edited

Loading