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

prevent tuple subclasses from being interpreted as generic #3768

Merged
merged 10 commits into from
Oct 15, 2023
Merged

prevent tuple subclasses from being interpreted as generic #3768

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c237600
prevent namedtuples from being interpreted as sequences
tybug Oct 13, 2023
65fec1d
use typing alias to support pthon 3.8
tybug Oct 13, 2023
8a5bfca
reject all subclasses of tuple as generic
tybug Oct 14, 2023
eec6a07
update release note
tybug Oct 14, 2023
f523db1
reformat
tybug Oct 14, 2023
fc15290
fix release notes wording
tybug Oct 14, 2023
4a65e5a
add test for a generic protocol whose only concrete subtypes are tuples
tybug Oct 14, 2023
e398b28
Merge branch 'namedtuple-generic' of https://github.com/tybug/hypothe…
tybug Oct 14, 2023
7c7375c
move tuple subclass test to py39
tybug Oct 15, 2023
0d0ff88
Merge branch 'master' into namedtuple-generic
Zac-HD Oct 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ their individual contributions.
* `Lampros Mountrakis <https://www.github.com/lmount>`_
* `Lea Provenzano <https://github.com/leaprovenzano>`_
* `Lee Begg <https://www.github.com/llnz2>`_
* `Liam DeVoe <https://github.com/tybug>`_
* `Libor Martínek <https://github.com/bibajz>`_
* `Lisa Goeller <https://www.github.com/lgoeller>`_
* `Louis Taylor <https://github.com/kragniz>`_
Expand Down
3 changes: 3 additions & 0 deletions hypothesis-python/RELEASE.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
RELEASE_TYPE: patch

This patch improves `st.register_type_strategy` when used with `namedtuple`, by preventing namedtuples from being interpreted as a sequence and provided to strategies like `st.from_type(Sequence[int])`.
16 changes: 16 additions & 0 deletions hypothesis-python/src/hypothesis/strategies/_internal/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,22 @@ def from_typing_type(thing):
# the ghostwriter than it's worth, via undefined names in the repr.
mapping.pop(collections.deque, None)
mapping.pop(typing.Deque, None)
# namedtuples are, strictly speaking, generic. However, users treat them as
# dataclasses or structs, not as a generically typed collection, so we
# don't want to provide generic instantiations of these.
#
# Namedtuples aren't an actual type, so we'll check for internal attributes
# set by collections.namedtuple and hope nobody sets these on their own
# tuple subclass.
for t in sorted(mapping, key=type_sorting_key):
if (
isinstance(t, type)
and issubclass(t, tuple)
and hasattr(t, "_fields")
and hasattr(t, "_asdict")
):
mapping.pop(t)

Copy link
Member Author

@tybug tybug Oct 13, 2023 •
edited

Choose a reason for hiding this comment

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

I'm not sure where this should appear in the function order-wise. There's a portion lower down where mapping is iterated over in the same way - maybe this check belongs there?

Copy link
Contributor

Choose a reason for hiding this comment

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

On line 395 above it drops things which are exactly type tuple, maybe we could expand that to drop any subtype of tuple too?

Copy link
Member

@Zac-HD Zac-HD Oct 13, 2023 •
edited

Choose a reason for hiding this comment

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

Seems reasonable, yeah. I think we'd want to discard os.Environ first, and then if there are non-tuple types discard all the tuple types. It's important that if you're resolving a namedtuple, or union of several, that works without discarding all or all-but-one!

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated as you described: if there is at least one other type, drop all tuple (sub)classes.

I admit I'm not sure what input to st.from_type would cause mapping to be filled with only tuple subclasses. This might be relevant if the cover tests yell at me for not providing an input which fails the new conditional branch 🙂

Copy link
Member

Choose a reason for hiding this comment

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

I think we need something along the lines of:

import abc

class AbstractStruct(abc.ABC, tuple):
    @abstractmethod
    def foo(self): ...

class ConcreteA(AbstractStruct):
    def foo(self): pass
class ConcreteB(AbstractStruct):
    def foo(self): pass

def test_handles odd tuple_subclasses():
    s = st.from_type(AbstractStruct)
    assert_all_examples(s, lambda x: isinstance(x, AbstractStruct))
    find_any(s, lambda x: isinstance(x, ConcreteA))
    find_any(s, lambda x: isinstance(x, ConcreteB))

Copy link
Member Author

@tybug tybug Oct 14, 2023 •
edited

Choose a reason for hiding this comment

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

hmm, I don't think this works, because AbstractStruct doesn't pass this conditional:

hypothesis/hypothesis-python/src/hypothesis/strategies/_internal/core.py

Lines 1364 to 1369 in 9606972

if isinstance(thing, types.typing_root_type) or (
sys.version_info[:2] >= (3, 9)
and isinstance(get_origin(thing), type)
and get_args(thing)
):
return types.from_typing_type(thing)

I managed to come up with an alternative test involving a custom protocol whose only concrete instantiations are tuple subtypes - let me know what you think.

if len(mapping) > 1:
# issubclass treats bytestring as a kind of sequence, which it is,
# but treating it as such breaks everything else when it is presumed
Expand Down
18 changes: 18 additions & 0 deletions hypothesis-python/tests/cover/test_lookup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1053,3 +1053,21 @@ def f(x: int):
msg = "@no_type_check decorator prevented Hypothesis from inferring a strategy"
with pytest.raises(TypeError, match=msg):
st.builds(f).example()


class NotAnIntSequence(collections.namedtuple("NotAnIntSequence", [])):
pass


def test_namedtuple_is_not_a_sequence():
# namedtuples are a subclass of tuple and are technically a sequence, but
# users don't expect this behavior.
# see https://github.com/HypothesisWorks/hypothesis/issues/3767.
with temp_registered(NotAnIntSequence, st.builds(NotAnIntSequence)):
seq_type = collections.abc.Sequence[int]

@given(st.from_type(seq_type))
def f(val):
assert type(val) is not NotAnIntSequence

f()