Fix for #1698

[dmoisset]

This is an implementation of my proposed fix for #1698 ( #1698 (comment) )

Relevant differences from the description there:

• After the suggestion from @ddfisher I used class attributes in Type instead of wrapping Type objects. When I want to restrict a type I make a copy and override the can_be_true or can_be_false with instance attributes
• I didn't implement the rules for not after realising that I couldn't find a relevant example where it helped. It's easy to add later if we discover otherwise but I didn't want to bloat an already large PR
• I noticed that the deductive power was good enough to start detecting more dead code in unrelated test cases (see modifications in check-statements, check-modules and check-generics).

There are some other improvements that I saw that could be made but I decided to stop here; if you think any of these are necessary I can add them:

• Covering the "not" operator, as mentioned above
• Renaming "find_isinstance_check" to something like "types_specialized_by_condition" which is more descriptive of its current purpose.
• I used copy.copy() followed by an update to create restricted version of types, perhaps there's a better/safer way to copy those (and even cache the restricted versions to reduce memory usage)

Let me know what you think and if it's necessary to make further changes

[ddfisher]

Thanks for the PR! I'll take a detailed look at this early next week.

[ddfisher]

I think you meant "truthiness_restriction".

[ddfisher]

Sorry for the later-than-promised review. Overall, this looks great!

That said, it introduces a weird bug:

x = ""
if not x:
    pass
if x:
    pass
else:
    1 + ""  # no error, indicating this branch is not typechecked/mypy thinks it can never occur

I spent a little while looking into it, but it wasn't obvious where the problem was. I suspect it's related to some interaction with the ConditionalTypeBinder, though.

[dmoisset]

Thanks for the thorough review, I just see it (I was on holidays). I'll try
to address your comments and check the reported bug and see if I can make a
new version

On Sat, Aug 13, 2016 at 1:35 AM, David Fisher notifications@github.com
wrote:

Sorry for the later-than-promised review. Overall, this looks great!

That said, it introduces a weird bug:

x = ""if not x:
passif x:
passelse:
1 + "" # no error, indicating this branch is not typechecked/mypy thinks it can never occur

I spent a little while looking into it, but it wasn't obvious where the
problem was. I suspect it's related to some interaction with the
ConditionalTypeBinder, though.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1989 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAMQHaT8vwlAkFuuz-A50SOiBBIX_EXgks5qfRE9gaJpZM4Jd5oe
.

Daniel F. Moisset - UK Country Manager
www.machinalis.com
Skype: @dmoisset

[dmoisset]

I've added a few minor changes to fix the bug (it was a problem on join_simple over restricted types) and fix what you mentioned in comments

[ddfisher]

We probably need this for join_types also, right?

[dmoisset]

I tried to test and couldn't make a failing case with join_type; I think
that one always creates new instances and new instances have always
undetermined boolean values. The problem with join_simple is that it was
reusing the existing types

[ddfisher]

join_types also reuses existing values in some cases. I'd expect the same problem to exist, but just be pretty hard to actually repro.

[ddfisher]

This is looking great! Just had a couple more small comments.

[dmoisset]

OK, I found a reproduction for join_types and added a test and a fix. (and your change to true_only)

[ddfisher]

Thanks!