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

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

Merged
merged 11 commits into from
Oct 8, 2024

Conversation

sarlinpe
Copy link
Contributor

@sarlinpe sarlinpe commented Oct 6, 2024

Description

When a C++ container is bound by bind_map and one accesses or deletes an element that does not exist, pybind11 currently raises an empty, uninformative message: KeyError: ''. This PR prints the key, which is the behavior of Python's dict.

Suggested changelog entry:

When getting or deleting an element in a container bound by ``bind_map``, print the key in ``KeyError`` if it does not exist.

@rwgk
Copy link
Collaborator

rwgk commented Oct 6, 2024

Looks useful, thanks. Could you please add tests? (Please tag me when it's ready for review with tests.)

@sarlinpe
Copy link
Contributor Author

sarlinpe commented Oct 6, 2024

@rwgk Tests added, ready for review.

@@ -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

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() ?

include/pybind11/stl_bind.h Outdated Show resolved Hide resolved
include/pybind11/stl_bind.h Outdated Show resolved Hide resolved
@rwgk
Copy link
Collaborator

rwgk commented Oct 7, 2024

I added commit ef17743 to reduce the amount of templated code.

Waiting for GitHub Actions to finish.

@sarlinpe
Copy link
Contributor Author

sarlinpe commented Oct 8, 2024

LGTM, thank you!

Also rename variable to `cut_length`, to not get into even/odd issues with the meaning of "half".
@rwgk rwgk merged commit 56e69a2 into pybind:master Oct 8, 2024
80 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changelog Possibly needs a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants