Stewori / pytypes

Typing-toolbox for Python 3 _and_ 2.7 w.r.t. PEP 484.
Apache License 2.0
200 stars 20 forks source link

_issubclass and bound_typevars argument #24

Closed mitar closed 6 years ago

mitar commented 6 years ago
import typing
from pytypes import type_util

T = typing.TypeVar('T', covariant=True)

class L(typing.List[T]):
    pass

C = typing.TypeVar('T', bound=L)

type_util._issubclass(L[float], C)  # False
type_util._issubclass(L[float], C, bound_typevars={})  # True

Why does bound_typevar change result from False to True?

Also, why code never checks if superclass is in bound_typevars? I think it just relays on catch-all except. I think such approach might hide some bugs?

Stewori commented 6 years ago

Sorry, this is a documentation issue. bound_typevars is kind of an output parameter. If provided, it grants _issubclass the freedom to assign typevars as they occur. Only predefined mappings are checked, i.e. if the dict already contained an entry for C. As a consequence, if a typevar occurs within a type in inconsistent manner, this will error. You can observe how it populates typevar bindings:

bound_typevars={}
_issubclass(L[float], C, bound_typevars=bound_typevars)  # True
print(bound_typevars) # {~T: __main__.L[float]}
Stewori commented 6 years ago

Just looked into the doc and found out that bound_typevars is not yet documented at all. So that's a todo then...

Maybe a flag to tell _issubclass whether it is allowed to bind typevars or not would be good. The usecase for this dict was to have kind of a memory between subsequent calls, i.e. if you want to check multiple types that share one or several typevars. On first occurrence they are assigned, but later an inconsistent assignment would cause an error.

mitar commented 6 years ago

Yes, I understand this idea about the output/memory. This is necessary to type check outputs of the function for example.

But I do not understand why having this output changes the outcome. :-) I would assume that type-wise the output should be the same, no? Only that this memory is discarded?

Stewori commented 6 years ago

I didn't want unassigned typevars to be treated as Any in general (e.g. if someone overlooks that some unspecified typevar is in the game they should get an alert). Providing a dict for assignements is the way to tell _issubclass that it's okay to assign them. Maybe this behaviour should be reconsidered. At least I do see a usecase for a strict check i.e. only considering existing assignments. Maybe we could change the default value to {} rather than None.

Stewori commented 6 years ago

As of https://github.com/Stewori/pytypes/commit/74ef4b9babfe3b1d70b0d547e8e8eea575dcdd1b _issubclass has the boolean-argument bound_typevars_readonly (default: False). If set to True, _issubclass will not assign typevars, but only check against existing ones. For strict test-cases... Todo: Write test for this flag.

mitar commented 6 years ago

Hm, nice. But I am not sure how does this help my case above? :-)

>>> type_util._issubclass(L[float], C, bound_typevars={})
True
>>> type_util._issubclass(L[float], C, bound_typevars_readonly=True)
False
>>> type_util._issubclass(L[float], C, bound_typevars_readonly=True, bound_typevars={})
False

I think this is related, but not sure if it addresses the question that I think type_util._issubclass(L[float], C) should return True by default.

Stewori commented 6 years ago

Only in the sense that bound_typevars_readonly=True yields the behaviour you originally expected for bound_typevars={}. I'm considering to use bound_typevars={} as default rather than bound_typevars=None.

mitar commented 6 years ago

bound_typevars_readonly=True, bound_typevars={} as a default? This sounds reasonable.

Stewori commented 6 years ago

Hmm, I thought bound_typevars_readonly=False, bound_typevars={}. With bound_typevars_readonly=True it would be the same behavior like currently with bound_typevars=None, wouldn't?

mitar commented 6 years ago

Oh, you are right.

mitar commented 6 years ago

I think this can be closed now, no?

Stewori commented 6 years ago

I kept it open as a reminder to improve documentation. I think doc of _issubclass explains it properly now. I suppose the quick manual should mention this behavior as well. Anyway, I'm fine with closing now.