Adding tests

[mitar]

I added tests for #24 and changed the default.

I added tests for #22, #32, #33.

[mitar]

I think this should raise an exception.

[Stewori]

I'd like to pretend that this succeeds because of recursion proof handling in type_util._issubclass_Union/type_util._issubclass_Union_rec. However I tried to out-comment the recursion-proof part and still get the same result. While this behavior is puzzling I would not investigate further unless we find a particular failure.

[mitar]

So you are saying you disable recursion checks you added and this test still succeeds?

I copied it from here and at that time it failed for sure: #22 (comment)

[Stewori]

Oh right, the exception is what you actually suggested. Do you find it important to uncover recursive type declarations this way?
I think it would be possible to let type_util._issubclass_Union throw an exception in such a case. I think I'd prefer to have such a behavior optional. Will think about it...

[mitar]

I do not have strong opinion here. I just see it as an invalid type. So it would simplify debugging. But maybe this type is not invalid and it is true that isinstance should return False, if we would be able to resolve the recursion somehow. So I do not really mind either way. It just be that it will surprise some.

[Stewori]

Awesome! Thanks a lot for writing this commit.
I will definitely accept it, just some remarks. If you like you can handle these, but I can also do by myself, it's not much work.

• typo: should be 'ellipsis' in test_tuple_elipsis
• Better use another name than 'Container' because of potential misconception with typing.Container (for human readers), e.g. better call it _Container
• in test_empty_values also check regarding typing.Iterable and typing.Container (against regressions)
• Changing defaults and adding tests should better be separate commits (don't fix this one, it's just a hint for next time)
• add a comment at the top of each test method, hosting a link to the corresponding issue and discussion in github
• the line pytypes.resolve_fw_decl(_Container) is not required I suppose
• tests for forward refs could maybe have some more crosschecks, e.g.

pytypes.is_subtype(int, _Container) #T
pytypes.is_subtype(float, _Container) #T
pytypes.is_subtype(Data, _Container) #T
pytypes.is_subtype(_Container, Data) #F

[mitar]

Updated, but tests are now failing. I added an assertion I missed but it does not return an expected value. It is a bug or a buggy test?

[Stewori]

This is where the test fails. Before the change, the result was not checked to be true, it succeeded whenever no exception was thrown. Actually List[float] is no subtype of Wrapper because of invariance of List 's type parameter. Either construct Wrapper differently or change expected outcome of this test. E.g.:

Wrapper = typing.Union[
             typing.Sequence['Data'],
         ]

Disclaimer: I din't check empirically. I hope my analysis is correct and this is the actual cause.

[mitar]

How it is not? I can get there by simply rewriting:

Wrapper -> Union[List[Data]] -> List[Data] -> List[Union[Wrapper, str, bytes, bool, float, int, dict]] -> List[float]

So List[float] should be a subtype of List[float], no?

>>> pytypes.is_subtype(typing.List[float], typing.List[float])
True

[Stewori]

List[Union[Wrapper, str, bytes, bool, float, int, dict]] -> List[float] is not valid, because Union[Wrapper, str, bytes, bool, float, int, dict] is a less restrictive type than float and List is invariant. Imagine someone passes List[float] to a function that expects List[Union[Wrapper, str, bytes, bool, float, int, dict]]. Then the method might write integers or strings into the list.
What is maybe a bad logic in deep_type is to conclude that something like [1.5, 2.3, 5.7] is List[float]. Just because is doesn't contain strings does not mean it is not intended to ever do so. This is a symptom of PEP 484 not being intended for runtime type checking. But python does not provide a way to annotate that a list object that currently contains only floats is also allowed to contain strings. (Except PEP 526 -- Syntax for Variable Annotations, but supporting PEP 526 is not a trivial enhanchement.) Some simpler solution would be needed.... Ideas?

[Stewori]

Okay, since this test is about another topic, please change it to Sequence. We can follow up on List invariance issues else where, e.g. in #21.

[mitar]

Done.

[mitar]

Now it fails here. Not sure why would this be false?

[Stewori]

Because currently Wrapper is essentially equal to Sequence[Data], which is neither subtype nor supertype nor equal to plain Data.

[mitar]

But Data is also Union[Wrapper], no?

[Stewori]

Yes, but that is not sufficient. Union[x, y] is only subtype of z if x is subtype of z and y is subtype of z. Since Data can also be float, str, etc. it is in general not subtype of Wrapper.

[Stewori]

self.assertTrue(pytypes.is_subtype(typing.List[float], Wrapper)) should work as well, i.e. changing List to Sequence was not necessary here, only in declaration of Wrapper.

[Stewori]

self.assertFalse(pytypes.is_subtype(Wrapper, Data)) is failing now. Unlike the line above, this is actually true. As participant of the Data-Union, Wrapper is indeed a subtype of it. As discussed earlier the converse is not true.

[mitar]

Fixed. :-) I think I have no clue about typing anymore. ;-)

[Stewori]

Sorry to hear that. Hope it doesn't stop you from helping to improve pytypes ;)
Feel free to ask more detals if it helps. Here I think it boils down to union semantics, so let me point out the main rules again:
Union[x, y] is subtype of z iff x is subtype of z and y is subtype of z.
z is subtype of Union[x, y] iff z is subtype of x or z is subtype of y.
This directly corresponds to union operation from set theory if x, y, z are sets and subtype means subset. There is also the intersection correspondence in type theory, but PEP 484 does not provide an intersection type (was once discussed in an issue of typing, but then drpped because rare use cases did not outweight the difficulties in implementing it for static type checking/mypy)

[Stewori]

Perfect!