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.

Sign up for GitHub

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

Jump to bottom

Print key in KeyError in map.__getitem__/__delitem__ #5397

Merged
merged 11 commits into from
Oct 8, 2024
Merged

Print key in KeyError in map.__getitem__/__delitem__ #5397

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
71e8a37
Print key in map.getitem/delitem KeyError
sarlinpe Oct 6, 2024
f2d1951
Add tests
sarlinpe Oct 6, 2024
0a07d1e
Fix tests
sarlinpe Oct 6, 2024
ce7a05d
Make robust
sarlinpe Oct 6, 2024
ce0b107
Make clang-tidy happy
sarlinpe Oct 6, 2024
fcc6bde
Return a Python str
sarlinpe Oct 7, 2024
c3c9e2b
Show beginning and end of the message
sarlinpe Oct 7, 2024
917226f
Avoid implicit conversion
sarlinpe Oct 7, 2024
bd60a6a
Merge branch 'master' into sarlinpe/map-key-error-info
sarlinpe Oct 7, 2024
ef17743
Split out `format_message_key_error_key_object()` to reduce amount of…
rwgk Oct 7, 2024
f80d132
Use `"✄✄✄"` instead of `"..."`
rwgk Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/pybind11/stl_bind.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,7 @@ class_<Map, holder_type> bind_map(handle scope, const std::string &name, Args &&
[](Map &m, const KeyType &k) -> MappedType & {
auto it = m.find(k);
if (it == m.end()) {
throw key_error();
throw key_error(str(cast(k)).cast<std::string>());
Copy link
Collaborator

@rwgk rwgk Oct 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this more closely, there are a bunch of things that can go wrong:

  • cast(k) may fail,
  • str(...) may fail,
  • .cast<std::string>() may fail.

Any of those failures will be a secondary error, masking the desired KeyError. This can be extremely confusing.

Making this robust is not easy.

What's needed is something like (untested; only to give the idea):

str key_as_str;
try {
  key_as_str = str(cast(k));
}
catch (const std::exception&) {
    throw key_error();
}
set_error(PyExc_KeyError, key_as_tr);
throw error_already_set();

This would be implemented in a helper function.

Another thought: if str() fails, try repr().

Oh, there should also be a size limit. I.e. you don't want to produce a 1M numpy array as a str.

Sorry to make this difficult, but these things sure will bite, someone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwgk PTAL. I added a test for long keys. I don't know what kind of object would fail in str of cast - happy to add a test if you have a suggestion. What is the benefit of set_error + throw error_already_set() over throw key_error() ?

}
return it->second;
},
Expand All @@ -808,7 +808,7 @@ class_<Map, holder_type> bind_map(handle scope, const std::string &name, Args &&
cl.def("__delitem__", [](Map &m, const KeyType &k) {
auto it = m.find(k);
if (it == m.end()) {
throw key_error();
throw key_error(str(cast(k)).cast<std::string>());
}
m.erase(it);
});
Expand Down
8 changes: 8 additions & 0 deletions tests/test_stl_binders.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,14 @@
assert list(mm) == ["b"]
assert list(mm.items()) == [("b", 2.5)]

with pytest.raises(KeyError) as excinfo:
_ = mm["a_long_key"]
assert "a_long_key" in excinfo.value

Check failure on line 307 in tests/test_stl_binders.py

View workflow job for this annotation

GitHub Actions / 🐍 3.8 • ubuntu-20.04 • x64 -DPYBIND11_FINDPYTHON=ON -DCMAKE_CXX_FLAGS="-D_=1" 

test_map_delitem

TypeError: argument of type 'KeyError' is not iterable

Check failure on line 307 in tests/test_stl_binders.py

View workflow job for this annotation

GitHub Actions / 🐍 3.11 • ubuntu-latest • x64 

test_map_delitem

TypeError: argument of type 'KeyError' is not iterable

Check failure on line 307 in tests/test_stl_binders.py

View workflow job for this annotation

GitHub Actions / 🐍 3.13 • ubuntu-20.04 • x64 

test_map_delitem

TypeError: argument of type 'KeyError' is not iterable

Check failure on line 307 in tests/test_stl_binders.py

View workflow job for this annotation

GitHub Actions / 🐍 3.9 • ubuntu-20.04 • x64 

test_map_delitem

TypeError: argument of type 'KeyError' is not iterable

Check failure on line 307 in tests/test_stl_binders.py

View workflow job for this annotation

GitHub Actions / 🐍 3.12 • ubuntu-20.04 • x64 

test_map_delitem

TypeError: argument of type 'KeyError' is not iterable

Check failure on line 307 in tests/test_stl_binders.py

View workflow job for this annotation

GitHub Actions / 🐍 pypy-3.10 • ubuntu-20.04 • x64 

test_map_delitem

TypeError: 'KeyError' object is not iterable

Check failure on line 307 in tests/test_stl_binders.py

View workflow job for this annotation

GitHub Actions / 🐍 pypy-3.8 • ubuntu-20.04 • x64 -DPYBIND11_FINDPYTHON=ON 

test_map_delitem

TypeError: 'KeyError' object is not iterable

Check failure on line 307 in tests/test_stl_binders.py

View workflow job for this annotation

GitHub Actions / 🐍 pypy-3.9 • ubuntu-20.04 • x64 

test_map_delitem

TypeError: 'KeyError' object is not iterable

with pytest.raises(KeyError) as excinfo:
del mm["a_long_key"]
assert "a_long_key" in excinfo.value

um = m.UnorderedMapStringDouble()
um["ua"] = 1.1
um["ub"] = 2.6
Expand Down
Loading