ToPyObject for HashMap<K, V> panics if ToPyObject for K is not Python hashable

[jakelishman]

Bug Description

The conversion traits into Python for hashmaps (both std::collections and hashbrown) fail with PanicException if the natural conversion type of the key is not hashable in Python. Of course, some sort of failure in general is completely sensible, but a runtime TypeError might be more appropriate, since I think there's currently no sensible way of determining at Rust compile-time whether the converted type will be hashable in Python space.

Steps to Reproduce

Given a PyO3 project that looks like

use std::collections::HashMap;

use pyo3::prelude::*;
use pyo3::Python;

#[pyfunction]
fn make_hashmap(capacity: usize) -> HashMap<[usize; 2], f64> {
    let mut out = HashMap::with_capacity(capacity);
    out.insert([0, 1], 0.0);
    out
}

#[pymodule]
fn core(_py: Python<'_>, module: &PyModule) -> PyResult<()> {
    module.add_function(wrap_pyfunction!(make_hashmap, module)?)?;
    Ok(())
}

This will build in Rust, and install in Python space without issue. From Python space, I can call this function as

>>> from repro.core import make_hashmap
>>> make_hashmap(1)
thread '<unnamed>' panicked at 'Failed to set_item on dict: PyErr { type: <class 'TypeError'>, value: TypeError("unhashable type: 'list'"), traceback: None }', /Users/jake/.cargo/registry/src/github.com-1ecc6299db9ec823/pyo3-0.17.3/src/types/dict.rs:423:18
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
pyo3_runtime.PanicException: Failed to set_item on dict: PyErr { type: <class 'TypeError'>, value: TypeError("unhashable type: 'list'"), traceback: None }

For comparison, if I pass an invalid type (like a float) for conversion to the capacity argument, I get the TypeError I'd expect.

Backtrace

The backtrace from the example above:

thread '<unnamed>' panicked at 'Failed to set_item on dict: PyErr { type: <class 'TypeError'>, value: TypeError("unhashable type: 'list'"), traceback: None }', /Users/jake/.cargo/registry/src/jackfan.us.kg-1ecc6299db9ec823/pyo3-0.17.3/src/types/dict.rs:423:18
stack backtrace:
   0:        0x104292c58 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h7d80e14dd19335c9
   1:        0x10425e6cb - core::fmt::write::h1709d0255080e28a
   2:        0x104296b70 - std::panicking::default_hook::{{closure}}::had4d1cd22a173020
   3:        0x10429756f - std::panicking::rust_panic_with_hook::h2b231e816574a23a
   4:        0x1042970c4 - std::panicking::begin_panic_handler::{{closure}}::h9da8d88b7a4c9d5e
   5:        0x104297039 - std::sys_common::backtrace::__rust_end_short_backtrace::h10dbf1377dfaf877
   6:        0x104296ff5 - _rust_begin_unwind
   7:        0x1042985c3 - core::panicking::panic_fmt::hde1544b10dc8b4d3
   8:        0x1042987b5 - core::result::unwrap_failed::h0a3d20f5459baffe
   9:        0x10425afc0 - repro::_::__pyfunction_make_hashmap::h0b96359cfa6d4455
  10:        0x10383eab2 - _cfunction_vectorcall_FASTCALL_KEYWORDS
  11:        0x103922ffd - _call_function
  12:        0x1038fa9b8 - __PyEval_EvalFrameDefault
  13:        0x1038ef215 - __PyEval_Vector
  14:        0x103988212 - _run_mod
  15:        0x103988589 - _PyRun_InteractiveOneObjectEx
  16:        0x1039878e9 - __PyRun_InteractiveLoopObject
  17:        0x10398747f - __PyRun_AnyFileObject
  18:        0x10398976a - _PyRun_AnyFileExFlags
  19:        0x1039a8d5e - _pymain_run_stdin
  20:        0x1039a84ec - _Py_RunMain
  21:        0x10377fcca - _main
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
pyo3_runtime.PanicException: Failed to set_item on dict: PyErr { type: <class 'TypeError'>, value: TypeError("unhashable type: 'list'"), traceback: None }

Your operating system and version

macOS 12.6

Your Python version (python --version)

3.10.6

Your Rust version (rustc --version)

rustc 1.64.0 (a55dd71d5 2022-09-19)

Your PyO3 version

0.17.3

How did you install python? Did you use a virtualenv?

Built from source, executing from within a venv made with python -m venv in this case.

Additional Info

Given Python-space tuple[int, int] can be converted into the Rust-space [usize; 2], it could be a useful feature if there were some impls of HashMap<K, V> and HashSet<K> where K can be converted to a Python tuple instead of list - the conversion of these types back to Rust would already work as expected. I'd be interested in trying to contribute that, if it would fit in PyO3.

[birkenfeld]

Ugh, this is ugly. ToPyObject/IntoPy doesn't allow for failure, and of course the impl for HashMap cannot have a "must be hashable" bound. (I'm pretty sure we don't want to add another couple of traits like ToHashablePyObject.)

I don't see a good solution here, except for making all of the conversion traits fallible.

[jakelishman]

I wouldn't want to ask for overhead on every other usage of the library for something that seems like a pretty niche edge case. For my purposes, I'd be happy with specific impls for HashMap<K, V> and HashSet<K> for the cases where K is [T; N] and its ilk to make that intermediate object a tuple rather than a list in this specific case. Both possibilities naturally work in the other direction - given a dict[tuple[int, int], float] in Python-space, it can already coerce safely to HashMap<[usize; 2], f64>, so the special-case ToPyObject would maintain symmetry in the type.

If that's a possibility, and doesn't sound like too much of a dangerous special case, I'd be happy to have a go at implementing it. If not, we have a workaround in place on our side - it's not in a performance-critical place, so it's ok for us to just collect the [usize; 2]-keyed hashmap into a new (usize, usize) one at the conversion boundary.

The PanicException isn't ideal, but might be a pragmatic choice to leave it if compile-time checking isn't reasonably possible without API pain. If the special case I mentioned above is in place, I don't think there's anything else in PyO3's default conversions where a hashable Rust object has a natural conversion to an unhashable Python one, so the chance of it coming up again should be pretty low.

[davidhewitt]

I think the need for fallible conversion traits is desirable for reasons like this. We've spoken about them previously but just not fleshed out a design.

I think adding overriding implementations of ToPyObject here isn't possible without specialization, so maybe that would be viable in a future Rust.

In the meanwhile you can always write your own conversion helper at the boundary. Sounds like you are already doing this.

[jakelishman]

Thanks for the response - I'd misunderstood how concrete/generic impl blocks in Rust interact (I'm a fairly new user), and I can see now that my proposal isn't implementable in the language yet.

[davidhewitt]

Solved with the new IntoPyObject trait; #4595