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

Type inference over variable assignment: temp = isinstance(var, type) #6670

Open
cipherboy opened this issue Apr 12, 2019 · 2 comments
Open

Comments

@cipherboy
Copy link

cipherboy commented Apr 12, 2019

I'm filing this as a bug, but feel free to call it a feature request. :)

Lets say you have a basic container class:

class Container:
    value: int = 0

    def __init__(self, value: int = 0):
        self.value = value

    def __and__(self, other: 'Container') -> 'Container':
        return add_gate_to_cnf('and', self.value, other.value)

    def __str__(self) -> str:
        return str(self.value)

In my case, I'm adding typing to cmsh. I have a Variable class which is essentially a bool except it is backed by a variable in the CNF passed to the SAT solver. Its value isn't available until the model is solved. I'm wanting to use Variable and bool interchangeably, so building a CNF model looks like building a complex boolean expression (except you have to use bitwise operators, e.g., & instead of and). This means that if you specify some previously unspecified portion of your inputs, it gets evaluated in Python (and you get native types back), rather than having to wait until the Solve() call. Ultimately, this gives you a smaller model.

Anyhow, so the basic and implementation that supports both Container and bool works with isinstance:

def i_and(left: Union[bool, Container], right: Union[bool, Container]) -> Union[bool, Container]:
    if isinstance(left, bool) and isinstance(right, bool):
        return left and right

    if isinstance(left, bool):
        if left:
            return right
        return False

    if isinstance(right, bool):
        if right:
            return left
        return False

    return left & right

This passes mypy fine in strict mode.

The problem with this is that there are double the isinstance calls than you need: you only need two if you assign them to variables. According to cProfile on some of my code which uses cmsh, isinstance is the most called method because I'm constructing fairly large CNF models.

When you assign the isinstance calls to variables, mypy quits inferring the types of left and right, resulting in errors:

def v_and(left: Union[bool, Container], right: Union[bool, Container]) -> Union[bool, Container]:
    lbool = isinstance(left, bool)
    rbool = isinstance(right, bool)

    if lbool and rbool:
        return left and right

    if lbool:
        if left:
            return right
        return False

    if rbool:
        if right:
            return left
        return False

    return left & right

Which when type checked gives:

$ mypy --strict ./main.py 
main.py:51: error: No overload variant of "__and__" of "bool" matches argument type "Container"
main.py:51: note: Possible overload variants:
main.py:51: note:     def __and__(self, bool) -> bool
main.py:51: note:     def __and__(self, int) -> int
main.py:51: error: Unsupported operand types for & ("Container" and "bool")
main.py:51: note: Both left and right operands are unions

(Line 51 is the return left & right line from the v_and call. It thinks one and/or both of the arguments can be bool; however, they can only be Container.)

I'm running:

$ mypy --version
mypy 0.700
$ python3 --version
Python 3.7.2

Is there a different or better way of doing this? Thanks!

@ilevkivskyi
Copy link
Member

We can potentially support this assuming you annotate lbool and rbool as Final. But this is tricky and therefore low priority.

Also, in almost all Python programs (except heavy number crunching), isinstance() is one of the most called functions. This is because of how Python works (for example it is called after creation of every instance to determine whether __init__() should be called on what is returned by __new__()) so I wouldn't be too worried about this.

Finally, there is # type: ignore.

@cipherboy
Copy link
Author

Oh, definitely agree, isinstance() will be high up there. A lot of these instances though are from my code, since my main dependency (pycryptosat) is a C API. And a number of them are redundant (like this case). It'd be nice to have both typing and speed, but I'll get there with more work. Just thought this was a case for later improvement.

And yeah, no rush. :) Thanks for taking a look at this! I appreciate all the work that's gone into this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants