prevent tuple subclasses from being interpreted as generic
#3768
Conversation
| # 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) | ||
|
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
On line 395 above it drops things which are exactly type tuple, maybe we could expand that to drop any subtype of tuple too?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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))There was a problem hiding this comment.
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
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.
tybug
changed the title
namedtuple from being interpreted as sequencestuple subclasses from being interpreted as generic
| # 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) | ||
|
|
There was a problem hiding this comment.
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))
Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>
| pass | ||
|
|
||
|
|
||
| def test_only_tuple_subclasses_in_typing_type(): |
There was a problem hiding this comment.
I'd just skip this on Python 3.8, since it's using later typing features.
If we could also get a subclassing-based test working that'd make me more confident that future changes won't accidentally break something, but I'd be happy to merge without that too.
There was a problem hiding this comment.
I slept on it, but I'm drawing a blank on a subclassing-based test (as opposed to using Protocol) where the only valid types are tuple types. I'd be happy to add it if someone else can come up with one, though.
|
Thanks Zac! Happy to land my first contribution in Hypothesis, and looking forward to contributing more in the future! |
closes #3767.
Hypothesis is actually doing the "right" thing here.
class Thing(namedtuple("Thing", []))is a subclass oftuplewhich is both generic and a sequence. So providing concrete instantiations ofThingto egst.from_type(Sequence[int])type checks.But, this seems to me like undesirable behavior: users treat namedtuples more like a struct or dataclass and not as either generic or a collection.
I've special-cased an exception against namedtuples here - but let me know if you prefer a different approach.After discussion, this PR instead prevents any tuple subclasses from being interpreted as generic.