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

_issubclass and bound_typevars argument #24

Closed
mitar opened this issue Dec 19, 2017 · 12 comments
Closed

_issubclass and bound_typevars argument #24

mitar opened this issue Dec 19, 2017 · 12 comments
Labels

Comments

@mitar
Copy link
Collaborator

mitar commented Dec 19, 2017

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
Copy link
Owner

Stewori commented Dec 19, 2017

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
Copy link
Owner

Stewori commented Dec 19, 2017

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
Copy link
Collaborator Author

mitar commented Dec 19, 2017

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
Copy link
Owner

Stewori commented Dec 19, 2017

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 added a commit that referenced this issue Jan 16, 2018
…d_typevars, c.f. #24. Improved doc of _issubclass and _isinstance.
@Stewori
Copy link
Owner

Stewori commented Jan 16, 2018

As of 74ef4b9 _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
Copy link
Collaborator Author

mitar commented Jan 17, 2018

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
Copy link
Owner

Stewori commented Jan 17, 2018

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
Copy link
Collaborator Author

mitar commented Jan 17, 2018

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

@Stewori
Copy link
Owner

Stewori commented Jan 17, 2018

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
Copy link
Collaborator Author

mitar commented Jan 17, 2018

Oh, you are right.

@mitar mitar mentioned this issue Apr 5, 2018
@mitar
Copy link
Collaborator Author

mitar commented Apr 18, 2018

I think this can be closed now, no?

@Stewori
Copy link
Owner

Stewori commented Apr 18, 2018

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.

@Stewori Stewori closed this as completed Apr 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants