bugfix: py contains raises errors when appropiate by Skylion007 · Pull Request #4209 · pybind/pybind11

Skylion007 · 2022-10-03T17:42:17Z

Description

Previously our contains functions just swallowed errors like TypeErrors for unhashable types. This is poorly defined and only really worked with dict and sets (but set the Python API in a bad state). @rwgk Could you do some global testing to see how widespread the problem is off using contains to check if a key is hashable.
There is a similar problem with PySet_Add(), but we abuse that ourselves in the STL casters, so we will need some utilities to check if an object is hashable first.

Suggested changelog entry:

* Properly raise exceptions in contains methods (like when an object in unhashable).

rwgk · 2022-10-03T18:21:32Z

Could you do some global testing to see how widespread the problem is off using contains to check if a key is hashable.

Will do.

rwgk · 2022-10-03T18:26:26Z

Hi @mattip, PyPy doesn't seem to raise TypeError as expected in the added tests, while all cpython versions do. Is that an intentional difference in behavior?

def test_unhashable_exceptions(arg, func):
class Unhashable:
__hash__ = None
with pytest.raises(TypeError) as exc_info:
func(arg, Unhashable())

mattip · 2022-10-03T20:35:24Z

If I understand correctly, func here is PySet_Contains, PyDict_Contains, class.__contains__() ? I can reproduce the PyPy problem for PyDict_Contains but not for the other two. I fixed PyDict_Contains in PyPy 99677b3cad6e.

rwgk · 2022-10-04T07:21:12Z

If I understand correctly, func here is PySet_Contains, PyDict_Contains, class.__contains__() ? I can reproduce the PyPy problem for PyDict_Contains but not for the other two. I fixed PyDict_Contains in PyPy 99677b3cad6e.

Thanks @mattip!

I downloaded and unpacked the "log archive" here, then:

$ grep 'arg =' *.txt
12______pypy-3.7_____windows-2022_____x64.txt:2022-10-03T17:50:59.4914903Z arg = set(), func = <builtin_function_or_method object at 0x000001e499692950>
13______pypy-3.8_____windows-2022_____x64.txt:2022-10-03T17:50:45.7048591Z arg = set(), func = <builtin_function_or_method object at 0x0000025a0a13fec0>
14______pypy-3.9_____windows-2022_____x64.txt:2022-10-03T17:51:14.2422408Z arg = set(), func = <builtin_function_or_method object at 0x0000020df0abc100>
19______pypy-3.7_____macos-latest_____x64.txt:2022-10-03T18:01:25.6466500Z arg = set(), func = <builtin_function_or_method object at 0x00007f866f4be480>
20______pypy-3.8_____macos-latest_____x64.txt:2022-10-03T17:59:04.6502290Z arg = set(), func = <builtin_function_or_method object at 0x00007fa078f29600>
21______pypy-3.9_____macos-latest_____x64.txt:2022-10-03T17:55:13.3994880Z arg = set(), func = <builtin_function_or_method object at 0x00007fec66e64800>
5______pypy-3.7_____ubuntu-latest_____x64.txt:2022-10-03T17:49:03.7385057Z arg = set(), func = <builtin_function_or_method object at 0x00000000021403a0>
6______pypy-3.8_____ubuntu-latest_____x64_-DPYBIND11_FINDPYTHON=ON.txt:2022-10-03T17:48:25.9971616Z arg = set(), func = <builtin_function_or_method object at 0x00000000027e54b0>
7______pypy-3.9_____ubuntu-latest_____x64.txt:2022-10-03T17:49:23.3291437Z arg = set(), func = <builtin_function_or_method object at 0x00007fa78b01bd00>

AFAICT it only fails for set(), but succeeds for dict() and CustomContains() (I'm sure pytest runs all 3 even if one of them fails), for all PyPy versions that we cover.

@Skylion007 does my understanding look right to you?

mattip · 2022-10-04T13:21:07Z

Right, I can reproduce with a nightly pypy in a repo that runs CI on popular C extensions. I will try to fix it there.

rwgk · 2022-10-04T15:50:15Z

Could you do some global testing to see how widespread the problem is off using contains to check if a key is hashable.
Will do.

I don't see any problems in the global testing results related to this PR.

Very minor caveat: results are unusually noisy this morning, but I looked pretty carefully.

mattip · 2022-10-06T01:32:29Z

There is a corner case for empty sets where PyPy does not raise. CPython2 and CPython3 raise. See https://foss.heptapod.net/pypy/pypy/-/issues/3824

rwgk · 2022-10-12T22:11:16Z

 bool object_api<D>::contains(T &&item) const {
- return attr("__contains__")(std::forward<T>(item)).template cast<bool>();
+ auto ret = attr("__contains__")(std::forward<T>(item)).template cast<bool>();
+ if (PyErr_Occurred()) {


Can this actually happen?
I was thinking the line before will either succeed or there will be an exception (error_already_set, cast_error) in flight already.

@rwgk Good catch, my test designed to test this pathway fired fine without the if statement locally.

rwgk

Thanks Aaron for plugging these holes, and the new tests!

rwgk · 2022-10-12T22:40:25Z

CentOS7 / PGI 22.3 appears to be generally broken (same failure in another PR) and the other failing test is a notorious flake. I think it's best if we ignore the CentoOS failure, merge our PRs, then one of us can look what's going on with CentOS.

rwgk · 2022-10-13T18:50:34Z

@Skylion007 I just rebased this on master, to trigger the CI, to see if the CentOS7 / PGI 22.3 problem was fixed in the meantime.

rwgk · 2022-10-13T20:23:42Z

@henryiii @ax3l Do you know what's behind the error below? First observed yesterday, still failing now.

https://github.com/pybind/pybind11/actions/runs/3244896869/jobs/5321695298

Downloading packages:
Package nvhpc-22.3-1.x86_64.rpm is not signed
--------------------------------------------------------------------------------
Total 59 MB/s | 5.9 GB 01:42 warning: /var/cache/yum/x86_64/7/nvhpc/packages/nvhpc-22-9-22.9-1.x86_64.rpm: Header V4 RSA/SHA512 Signature, key ID 264a7796: NOKEY
Retrieving key from https://developer.download.nvidia.com/hpc-sdk/rhel/RPM-GPG-KEY-NVIDIA-HPC-SDK
Importing GPG key 0x264A7796:
Userid : "NVIDIA HPC SDK"
Fingerprint: 5793 edb8 2f7b fb26 ca4b 783f 7343 6a89 264a 7796
From : https://developer.download.nvidia.com/hpc-sdk/rhel/RPM-GPG-KEY-NVIDIA-HPC-SDK
Package nvhpc-22-3-22.3-1.x86_64.rpm is not signed
Error: Process completed with exit code 1.

rwgk · 2022-10-17T22:03:29Z

Hi @Skylion007, is there a chance that you could merge this today? (Asking b/o I'm hoping to include this in the next smart_holder update.)

Skylion007 · 2022-10-17T22:45:03Z

@rwgk Wasn't sure if wanted to wait until the next major update as this is a bit of a behavior change.

Skylion007 · 2022-10-17T22:45:22Z

If we are good with it, feel free to merge it.

rwgk · 2022-10-17T23:00:47Z

If we are good with it, feel free to merge it.

@henryiii

Do you want this in 2.10.1? It looks more like a bug-fix-behavior-change than a feature-behavior-change to me.

henryiii

Yes, I’d say this is a bug fix.

rwgk · 2022-10-17T23:15:16Z

Thanks!

Skylion007 · 2022-10-17T23:15:27Z

Only concern is that set.add() is still broken, and we sort of rely on that broken behavior in the caster. So that's a bigger PR issue I haven't looked at.

rwgk · 2022-10-17T23:18:11Z

Sorry, looks like I was a bit fast.

So that's a bigger PR issue I haven't looked at.

Leaving that for later sounds like the right move, does that make sense?

Skylion007 requested review from henryiii and rwgk October 3, 2022 17:42

mattip mentioned this pull request Oct 4, 2022
test pybind11 from Skylion007:py-contains-raise-unhashable pypy/binary-testing#26
Closed

mattip reviewed Oct 11, 2022
View reviewed changes

Comment threadtests/test_pytypes.py Outdated

rwgk reviewed Oct 12, 2022
View reviewed changes

rwgk approved these changes Oct 12, 2022
View reviewed changes

Skylion007 added 9 commits October 13, 2022 11:48

bugfix: contains now throws an exception if the key is not hashable
ca9fc91

Fix tests and improve robustness
3b5b5f7

Remove todo
374205c

Workaround PyPy corner case
589f949

PyPy xfail
7d10ee1

Fix typo
9f2077c

fix xfail
85c429c

Make clang-tidy happy
ad91190

Remove redundant exc checking
129c6cf

rwgk force-pushed the py-contains-raise-unhashable branch from 294f720 to 129c6cfCompare October 13, 2022 18:49

rwgk closed this Oct 14, 2022

rwgk reopened this Oct 14, 2022

henryiii approved these changes Oct 17, 2022
View reviewed changes

rwgk merged commit b926396 into pybind:masterOct 17, 2022

github-actionsBot added the needs changelog Possibly needs a changelog entry label Oct 17, 2022

Skylion007 deleted the py-contains-raise-unhashable branch October 17, 2022 23:18

henryiii removed the needs changelog Possibly needs a changelog entry label Oct 20, 2022

henryiii mentioned this pull request Oct 21, 2022
Scratch PR for testing pybind11 v2.10.0 with Python 3.11rc2 #4271
Closed

rwgk mentioned this pull request Feb 11, 2023
FWD pybind11 google/pybind11clif#4209
Closed

Uh oh!

Conversation

Skylion007 commented Oct 3, 2022

Description

Suggested changelog entry:

Uh oh!

rwgk commented Oct 3, 2022

Uh oh!

rwgk commented Oct 3, 2022

Uh oh!

mattip commented Oct 3, 2022

Uh oh!

rwgk commented Oct 4, 2022

Uh oh!

mattip commented Oct 4, 2022

Uh oh!

rwgk commented Oct 4, 2022

Uh oh!

mattip commented Oct 6, 2022

Uh oh!

Uh oh!

rwgkOct 12, 2022

Choose a reason for hiding this comment

Uh oh!

Skylion007Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

rwgk left a comment

Choose a reason for hiding this comment

Uh oh!

rwgk commented Oct 12, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwgk commented Oct 13, 2022

Uh oh!

rwgk commented Oct 13, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwgk commented Oct 17, 2022

Uh oh!

Skylion007 commented Oct 17, 2022

Uh oh!

Skylion007 commented Oct 17, 2022

Uh oh!

rwgk commented Oct 17, 2022

Uh oh!

henryiii left a comment

Choose a reason for hiding this comment

Uh oh!

rwgk commented Oct 17, 2022

Uh oh!

Skylion007 commented Oct 17, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwgk commented Oct 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rwgk commented Oct 12, 2022•
edited
Loading

rwgk commented Oct 13, 2022•
edited
Loading

Skylion007 commented Oct 17, 2022•
edited
Loading