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

Surprising issubclass result #21

Closed
mitar opened this issue Dec 18, 2017 · 27 comments
Closed

Surprising issubclass result #21

mitar opened this issue Dec 18, 2017 · 27 comments
Labels

Comments

@mitar
Copy link
Collaborator

mitar commented Dec 18, 2017

Why the following returns false?

import typing
from pytypes import type_util

type_util._issubclass(typing.List[float], typing.List)

Isn't this the same as type_util._issubclass(typing.List[float], typing.List[typing.Any])? And then float is an instance of typing.Any? But this is false as well:

type_util._issubclass(typing.List[float], typing.List[typing.Any])
@mitar
Copy link
Collaborator Author

mitar commented Dec 18, 2017

Now, the following return true:

T = typing.TypeVar('T', covariant=True)
L = typing.List[T]
type_util._issubclass(L[float], L[typing.Any])

But this is false again:

type_util._issubclass(L[float], L)

This is surprising in some ways. First, L should really be the same as L[typing.Any]. That's how it is defined in Python's typing.

But the other thing is that covariance should matter when we are talking about return types from a function, but for a stand-alone type comparison it should not, no?

@Stewori
Copy link
Owner

Stewori commented Dec 18, 2017

List is unfortunately invariant as of PEP 484, which means that nothing is a subclass of List[Any] but List[Any] itself. Reason for this is that list is writable. I suppose Sequence[Any] would do what you want.

T = typing.TypeVar('T', covariant=True)
L = typing.List[T]
type_util._issubclass(L[float], L[typing.Any])

Using a covariant typevar as value for an invariant type parameter seems invalid to me. This is currently undefined behavior, but I suppose pytypes should throw an error in this case. That said, it seems to accept it, but you're right the observed behaviour is not very pleasant.

type_util._issubclass(L[float], L)

should behave like the Any version. So labelling this as a bug...

@Stewori Stewori added the bug label Dec 18, 2017
@mitar
Copy link
Collaborator Author

mitar commented Dec 18, 2017

This is currently undefined behavior, but I suppose pytypes should throw an error in this case.

Please don't. :-) This is a workaround I am currently using. The issue with Sequence is that it is an abstract type, you cannot really create it. Maybe we should be using typing.Tuple there instead, but currently we are using List which we in practice use as immutable. So this works for us, but it is not enforced by type checking, true.

Moreover, one other reason why this should not be an error is because you might want to signal that a particular function is not changing the List and that it can accept then types in a covariant manner. This is in fact an example from PEP 484, exactly about List. So it seems that one should be able to declare that a list acts covariantly, even if it is mutable.

@Stewori
Copy link
Owner

Stewori commented Dec 18, 2017

Please don't. :-)

No worries, I want pytypes to be convenient. It is already less strict than PEP 484 in case of mutable maps, see pytypes.covariant_Mapping

I still think it's fine to use Seqence as type annotation even though no corresponding type exists in Python. AFAIK it is the official way to denote that something accepts lists and tuples and other sequence-like objects.

This is in fact an example from PEP 484, exactly about List.

Hmmm I don't find an example there where they simply use List[T_co]. As I understood it one has to define a new subclass of Generic: class ImmutableList(Generic[T_co]): ...

Anyway, for me it seems okay to let pytypes accept your version as well.

@mitar
Copy link
Collaborator Author

mitar commented Dec 18, 2017

I was thinking about this section:

Consider a class Employee with a subclass Manager. Now suppose we have a function with an argument annotated with List[Employee]. Should we be allowed to call this function with a variable of type List[Manager] as its argument? Many people would answer "yes, of course" without even considering the consequences. But unless we know more about the function, a type checker should reject such a call: the function might append an Employee instance to the list, which would violate the variable's type in the caller.

It turns out such an argument acts contravariantly, whereas the intuitive answer (which is correct in case the function doesn't mutate its argument!) requires the argument to act covariantly.

So if function does not mutate the argument, argument can act covariantly.

But you are right that in 483 they discuss how one should create an immutable container and how:

Note, that although the variance is defined via type variables, it is not a property of type variables, but a property of generic types.

@mitar
Copy link
Collaborator Author

mitar commented Dec 18, 2017

The following works though:

import typing
from pytypes import type_util

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

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

type_util._issubclass(L[float], L)

@Stewori
Copy link
Owner

Stewori commented Dec 19, 2017

Yes, that's exactly the variant I'd consider valid and I think I took care to support. There are also tests for this.

@mitar
Copy link
Collaborator Author

mitar commented Jan 25, 2018

Playing with this a bit more.

import typing
from pytypes import type_util

class Foo:
    pass

type_util._issubclass(typing.Tuple[Foo], typing.Tuple[object, ...])  # False?
type_util._issubclass(typing.Tuple[Foo], typing.Tuple[typing.Any, ...])  # False?

I would assume those to be true?

The issue comes from type_util._isinstance((Foo(),), typing.Tuple[typing.Any, ...]), this should probably be true. Not to even mention type_util._isinstance((Foo(),), typing.Tuple[Foo, ...]).

@Stewori
Copy link
Owner

Stewori commented Jan 25, 2018

Maybe the ellipsis notation for tuple is not yet properly supported. (Maybe not at all, I'd have to look into the code...)

@Stewori
Copy link
Owner

Stewori commented Jan 27, 2018

It would be very helpful if you could write your experiments in a form that can be used as a test.
We could ideally add it to the test suite, e.g. with something like @skip("Not yet supported, see issue ..."). Lack of tests, also for already fixed issues, is a significant source of delay for the next release.

@mitar
Copy link
Collaborator Author

mitar commented Jan 30, 2018

Oh, sorry. Those examples above are self-contained. You can just copy-paste them into a Python interpreter and they should exercise the error. I thought this is enough to wrap them into tests. What would you prefer instead? A PR adding this as a test? Or some other syntax here in the issue?

@Stewori
Copy link
Owner

Stewori commented Jan 30, 2018

Yes, ideally a PR. Turning experiments into test often involves some subtle additional work, yet usually rather trivial. E.g. finding the right place in the messy test suite (yes we need a better test system,help welcome), choosing the right name, add eventually both a Python 2 and 3 version. While turning it into a test one sometimes finds that some additional cases should be covered as well.
Some help on this front would greatly help me to focus on solutions and would speed up the next release.

@Stewori
Copy link
Owner

Stewori commented Jan 30, 2018

I just noticed that is_subtype(Foo, object) is false in Python 2, unless we explicitly declare class Foo(object): pass. I guess this behavior reflects ordinary behavior of issubclass, which is fine for me. However this is one example of the pitfalls one might encounter in test-writing.

@Stewori
Copy link
Owner

Stewori commented Feb 10, 2018

I think the ellipsis-related issues are fixed now as of 774e4be. Can you confirm? Still, a tests-PR would be nice.

@mitar
Copy link
Collaborator Author

mitar commented Apr 13, 2018

So I think one issue here is that Python standard issubclass works here differently:

>>> type_util._issubclass(typing.List[float], typing.List)
False
>>> issubclass(typing.List[float], typing.List)
True

@Stewori
Copy link
Owner

Stewori commented Apr 13, 2018

Hmm, this is due to the invariance of List's type parameter. If parmeterless List is treated as List[Any] this behavior is okay. Wanted to point out there are options:

check_unbound_types : bool
	If true, treat missing parameters as unknown.
	Default: True
	Tells pytypes to actually attempt typechecking of unbound types, e.g
	things like is_subtype(List[Any], list).
	If false such checks are prohibited.
	If true, missing parameters are treated as unknown, which in turn is
	treated according to strict_unknown_check flag.

strict_unknown_check : bool
	Controls the meaning of unknown parameters.
	Default: False
	If false, treat unknown parameters somewhat like Any.
	If true (i.e. strict mode), treat unknown parameters
	somewhat like 'nothing', because no assumptions can be made.

However these can only make checking even stricter, e.g. disallow parameterless .

@mitar
Copy link
Collaborator Author

mitar commented Apr 13, 2018

Yes, I understand where is coming from. It is just surprising. I was hoping to just replace all isinstance with pytypes. It seems I might want to do or between both. :-) If any says true, then it is an instance. :-)

Not sure what could be changed here to make this different, while keeping the proper type checking. It is just in practice surprising when you just want to see that something is a subclass or instance. Maybe we could differentiate between explicit List[Any] and List? To me that would be the best. But it is against the PEP, I think.

@Stewori
Copy link
Owner

Stewori commented Apr 14, 2018

Hmm, I actually differentiated between List[Any] and List, but not the way you suggest (i.e. treat it like List[unknown]). Partly, all this is why I aliased _isinstance with is_of_type for public API - to underline that it is not fully equal to isinstance. (Btw. replacing all isinstance is not a good idea performance-wise, as pytypes is much slower than plain isinstance e.g. because it scans for stubfiles and type comments). Then there is also list. I think that should be working like List. Maybe we could add one global parameter to tell pytypes to handle List specially as covariant, much like pytypes.covariant_Mapping works for dictionaries (and other mappings). So far I avoided this because using typing.Sequence solves most issues related with this.

@Stewori
Copy link
Owner

Stewori commented Apr 14, 2018

IIRC regarding list and other collection types, PEP484 defines the behavior like check_unbound_types = False would yield.

@Stewori Stewori mentioned this issue Apr 14, 2018
@Stewori
Copy link
Owner

Stewori commented Apr 14, 2018

Also see #34 (comment).
I admit that deep_type inferring List[float] e.g. from [1.2, 3.4, 2.5] in invariant sense is maybe too restrictive. List being invariant makes sense for static typechecking, but not so much for runtime typechecking. Maybe the semantic of Empty could be extended to cover this case (given that it is already solving a related issue). (Along with renaming it to Incomplete or Sampled or so. Then it is a marker that more types are fine for writing.

@mitar
Copy link
Collaborator Author

mitar commented Apr 14, 2018

A very good point and interesting point about runtime vs. static time checking. At runtime you can really know precisely what the value is. So some constraints might not be reasonable. Thanks for this insight.

@Stewori
Copy link
Owner

Stewori commented Apr 14, 2018

Skimmed through PEP 484 again and found this note:

Note: Dict, DefaultDict, List, Set and FrozenSet are mainly useful for annotating return values. For arguments, prefer the abstract collection types defined below, e.g. Mapping, Sequence or AbstractSet.

However this does not answer how to handle the case that a function really intends typesafe write-access to a list object passed in as parameter. Maybe the typechecked-decorator should offer some feature to configure this per-function additional to a global flag.

@mitar
Copy link
Collaborator Author

mitar commented Oct 30, 2018

We got hit again by this. Really hard to debug.

Hmm, this is due to the invariance of List's type parameter. If parmeterless List is treated as List[Any] this behavior is okay. Wanted to point out there are options:

There are not options to configure that type_util._issubclass(typing.List[float], typing.List) should return True, no?

@Stewori
Copy link
Owner

Stewori commented Oct 31, 2018

The only viable option at this moment is to type it using Sequence. If you need a writable type maybe Union[List, Sequence] could work...?
I cannot tell when I can provide a proper fix for this (PR welcome).

@mitar
Copy link
Collaborator Author

mitar commented Oct 31, 2018

What exactly do you think PR should do? How you imagine a change to support this?

@Stewori
Copy link
Owner

Stewori commented Nov 1, 2018

You could implement a special case for list in similar fashion as is done for dict already.
Better solution and proper solution for long term would be to widen the use of empty to cover list and dict. In that case empty should be renamed to sampled and serve as a marker for types that were guessed via deep_type.

@Stewori
Copy link
Owner

Stewori commented Aug 30, 2019

The List being invariant issue is now tracked explicitly in #76. So I'll close this one in favor of #76.

@Stewori Stewori closed this as completed Aug 30, 2019
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