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

Raise if register_random is passed unreferenced object #3509

Merged
merged 22 commits into from Nov 19, 2022
Merged

Conversation

rsokl
Copy link
Contributor

@rsokl rsokl commented Nov 17, 2022

A pattern like

from hypothesis import register_random

class TorchRandomWrapper:
    # stuff

register_random(TorchRandomWrapper())

will lead to unexpected behavior: Hypothesis will instantly "forget" that it was ever passed an instance of TorchRandomWrapper. This is due to the fact that register_random uses a weakref-valued dict, and here we passed it an unreferenced object, which immediately gets garbage collected after being passed to register_random.

This PR will modify register_random to raise under these circumstances and to emit a warning when it looks like the input was referenced only within a temporary scope. It also fixes an overly-narrow type annotation for register_random so that it now permits structural subtypes of random.Random.

This is the new behavior:

from hypothesis import register_random

class TorchRandomWrapper:
    # stuff

register_random(TorchRandomWrapper())  # raises  ReferenceError

r = TorchRandomWrapper()
register_random(r)  # OK

def f():
    register_random(TorchRandomWrapper())

f()  # raises  ReferenceError


def g():
    r = TorchRandomWrapper()
    register_random(r)

g()  # throws UserWarning

@rsokl rsokl added the bug something is clearly wrong here label Nov 17, 2022
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Very nice! Thanks for taking this one 😁

hypothesis-python/RELEASE.rst Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/internal/entropy.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/internal/entropy.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/internal/entropy.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/internal/entropy.py Outdated Show resolved Hide resolved
@rsokl
Copy link
Contributor Author

rsokl commented Nov 18, 2022

@Zac-HD this looks like it is good to go. https://github.com/HypothesisWorks/hypothesis/actions/runs/3500864463/jobs/5863954158#step:6:539 is an unrelated flaky fail case.

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

🤩 :shipit:

@rsokl rsokl merged commit 41b420a into master Nov 19, 2022
@Zac-HD Zac-HD deleted the register-random branch November 19, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something is clearly wrong here
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants