Repository: python/mypy
Pull request: [mypyc] Don't coerce types checked with isinstance (#811)
Prevent the coercion of things whose types are checked with isinstance
to other types - such coercion is not necessary and can lead to bugs.

Also, add test for avoiding coercion of isinstance() comparators.

Fixes mypyc/mypyc#811.


## Test Plan

Ensured via compiling by hand and running the example given in mypyc/mypyc#811 has been fixed; added a test for the new code path that is generated.

Problem statement:
Incorrect behavior for 'isinstance(x, int) and not isinstance(x, bool)'
Take this Python module, which has a function to see if a value is an int (but not a bool):

```python
# file: example.py
def is_int(value):
    return isinstance(value, int)

def is_not_bool(value):
    return not isinstance(value, bool)

def is_int_and_not_bool(value):
    return isinstance(value, int) and not isinstance(value, bool)
```
In pure Python, this acts as I would expect - "True" is a bool, so `is_int_and_not_bool(True)` returns `False`:
```sh

-> % python -c "import example; print(example.is_int(True))"
True

-> % python -c "import example; print(example.is_not_bool(True))"
False

-> % python -c "import example; print(example.is_int_and_not_bool(True))"
False
```
If I compile it with `mypyc` though, the behavior changes. Now, `is_int_and_not_bool(True)` returns `True`!

```sh
-> % /home/swnelson/code/3p/mypy/scripts/mypyc ./example.py
running build_ext
building 'example' extension
creating build/temp.linux-x86_64-3.9
creating build/temp.linux-x86_64-3.9/build
x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -fstack-protector-strong -Wformat -Werror=format-security -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/home/swnelson/code/3p/mypy/mypyc/lib-rt -I/home/swnelson/code/personal/avroc/virtualenv3.9/include -I/usr/include/python3.9 -c build/__native.c -o build/temp.linux-x86_64-3.9/build/__native.o -O3 -Werror -Wno-unused-function -Wno-unused-label -Wno-unreachable-code -Wno-unused-variable -Wno-unused-command-line-argument -Wno-unknown-warning-option -Wno-unused-but-set-variable
x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-Bsymbolic-functions -Wl,-z,relro -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 build/temp.linux-x86_64-3.9/build/__native.o -o /tmp/tmp.IkpLWyng8w/example.cpython-39-x86_64-linux-gnu.so

-> % python -c "import example; print(example.is_int(True))"
1

-> % python -c "import example; print(example.is_not_bool(True))"
False

-> % python -c "import example; print(example.is_int_and_not_bool(True))"
True
```

Hints from issue discussion:
Thanks for reporting! This indeed looks like a bug
In case it helps, I have discovered a few more things. First, if I constrain the types with a Union over `int` and `bool`, the function works fine with `mypyc`:
```python
from typing import Union

def is_int_and_not_bool(value: Union[int, bool]) -> bool:
    return isinstance(value, int) and not isinstance(value, bool)
```

```sh
# (after mypyc)
-> % python -c "import example; print(example.is_int_and_not_bool(True))"
False
```

If I just use `Any`, though, it doesn't work:
```python
from typing import Any

def is_int_and_not_bool(value: Any) -> bool:
    return isinstance(value, int) and not isinstance(value, bool)
```
```sh
# (after mypyc)
-> % python -c "import example; print(example.is_int_and_not_bool(True))"
True
```
Interesting. Another potential useful check is to compile the two pieces of code you write (one in `Any` and the other in `Union`) and do a side-by-side comparison on the generated C code to see what's different. Some editor will highlight the difference like VSCode
Mypyc converts `bool` values to `int` when the target has type `int` (this is necessary to support unboxed integers). In this case this is probably what (effectively) happens in compiled code:

```py
def is_int_and_not_bool(value: Any) -> bool:
    return isinstance(value, int) and not isinstance(int(value), bool)
```

After `isinstance(value, int)` mypyc implicitly coerces the value to `int`, which loses the fact that it's actually a boolean. This behavior is kind of logically consistent but not the right thing here.

A potential fix would be to not perform any implicit coercions for the first argument of `isinstance`. This would also improve performance in some cases, as the coercion is redundant anyway.
Note that if the function is annotated as `value: int`, it will be impossible to make it work, since the caller will coerce the boolean value to an integer, and the function has no knowledge of the original type.
Boolean as a subclass of integer was a "nice" hack in 2002, and it still keeps giving. :(

Commit message: [mypyc] Don't coerce types checked with isinstance (#811)

Prevent the coercion of things whose types are checked with isinstance
to other types - such coercion is not necessary and can lead to bugs.

Also, add test for avoiding coercion of isinstance() comparators.

Fixes mypyc/mypyc#811.

Patch to review:
diff --git a/mypyc/irbuild/specialize.py b/mypyc/irbuild/specialize.py
index 1ae21e5c5e99..c69988fea6c8 100644
--- a/mypyc/irbuild/specialize.py
+++ b/mypyc/irbuild/specialize.py
@@ -248,7 +248,11 @@ def gen_inner_stmts() -> None:
 
 @specialize_function('builtins.isinstance')
 def translate_isinstance(builder: IRBuilder, expr: CallExpr, callee: RefExpr) -> Optional[Value]:
-    # Special case builtins.isinstance
+    # Special case for builtins.isinstance
+    # Prevent coercions on the thing we are checking the instance of - there is no need to coerce
+    # something to a new type before checking what type it is, and the coercion could lead to bugs.
+    builder.types[expr.args[0]] = AnyType(TypeOfAny.from_error)
+
     if (len(expr.args) == 2
             and expr.arg_kinds == [ARG_POS, ARG_POS]
             and isinstance(expr.args[1], (RefExpr, TupleExpr))):
diff --git a/mypyc/test-data/irbuild-isinstance.test b/mypyc/test-data/irbuild-isinstance.test
new file mode 100644
index 000000000000..820cf894f415
--- /dev/null
+++ b/mypyc/test-data/irbuild-isinstance.test
@@ -0,0 +1,97 @@
+[case testIsinstanceInt]
+def is_int(value):
+    return isinstance(value, int)
+
+[out]
+def is_int(value):
+    value, r0 :: object
+    r1 :: int32
+    r2 :: bit
+    r3 :: bool
+    r4 :: object
+L0:
+    r0 = load_address PyLong_Type
+    r1 = PyObject_IsInstance(value, r0)
+    r2 = r1 >= 0 :: signed
+    r3 = truncate r1: int32 to builtins.bool
+    r4 = box(int32, r1)
+    return r4
+
+[case testIsinstanceNotBool1]
+def is_not_bool(value):
+    return not isinstance(value, bool)
+
+[out]
+def is_not_bool(value):
+    value, r0 :: object
+    r1 :: str
+    r2 :: object
+    r3 :: int32
+    r4 :: bit
+    r5 :: bool
+    r6 :: object
+    r7 :: int32
+    r8 :: bit
+    r9 :: bool
+    r10 :: object
+L0:
+    r0 = builtins :: module
+    r1 = 'bool'
+    r2 = CPyObject_GetAttr(r0, r1)
+    r3 = PyObject_IsInstance(value, r2)
+    r4 = r3 >= 0 :: signed
+    r5 = truncate r3: int32 to builtins.bool
+    r6 = box(int32, r3)
+    r7 = PyObject_Not(r6)
+    r8 = r7 >= 0 :: signed
+    r9 = truncate r7: int32 to builtins.bool
+    r10 = box(bool, r9)
+    return r10
+
+[case testIsinstanceNotBoolAndIsInt]
+# This test is to ensure that 'value' doesn't get coerced into a bool when we need it to be an int
+def is_not_bool_and_is_int(value):
+    return not isinstance(value, bool) and isinstance(value, int)
+
+[out]
+def is_not_bool_and_is_int(value):
+    value, r0 :: object
+    r1 :: str
+    r2 :: object
+    r3 :: int32
+    r4 :: bit
+    r5 :: bool
+    r6 :: object
+    r7 :: int32
+    r8 :: bit
+    r9 :: bool
+    r10, r11, r12 :: object
+    r13 :: int32
+    r14 :: bit
+    r15 :: bool
+    r16 :: object
+L0:
+    r0 = builtins :: module
+    r1 = 'bool'
+    r2 = CPyObject_GetAttr(r0, r1)
+    r3 = PyObject_IsInstance(value, r2)
+    r4 = r3 >= 0 :: signed
+    r5 = truncate r3: int32 to builtins.bool
+    r6 = box(int32, r3)
+    r7 = PyObject_Not(r6)
+    r8 = r7 >= 0 :: signed
+    r9 = truncate r7: int32 to builtins.bool
+    if r9 goto L2 else goto L1 :: bool
+L1:
+    r10 = box(bool, r9)
+    r11 = r10
+    goto L3
+L2:
+    r12 = load_address PyLong_Type
+    r13 = PyObject_IsInstance(value, r12)
+    r14 = r13 >= 0 :: signed
+    r15 = truncate r13: int32 to builtins.bool
+    r16 = box(int32, r13)
+    r11 = r16
+L3:
+    return r11
diff --git a/mypyc/test/test_irbuild.py b/mypyc/test/test_irbuild.py
index 0d1e6ce72675..3bc3fb52d28b 100644
--- a/mypyc/test/test_irbuild.py
+++ b/mypyc/test/test_irbuild.py
@@ -32,6 +32,7 @@
     'irbuild-int.test',
     'irbuild-vectorcall.test',
     'irbuild-unreachable.test',
+    'irbuild-isinstance.test',
 ]
 
 


Code review comments:
diff_hunk: @@ -0,0 +1,97 @@
+[case testIsinstanceInt]
+def is_int(value):
+    return isinstance(value, int)
+
+[out]
+def is_int(value):
+    value, r0 :: object
+    r1 :: int32
+    r2 :: bit
+    r3 :: bool
+    r4 :: object
+L0:
+    r0 = load_address PyLong_Type
+    r1 = PyObject_IsInstance(value, r0)
+    r2 = r1 >= 0 :: signed
+    r3 = truncate r1: int32 to builtins.bool
+    r4 = box(int32, r1)
+    return r4
+
+[case testIsinstanceNotBool1]
+def is_not_bool(value):
+    return not isinstance(value, bool)
+
+[out]
+def is_not_bool(value):
+    value, r0 :: object
+    r1 :: str
+    r2 :: object
+    r3 :: int32
+    r4 :: bit
+    r5 :: bool
+    r6 :: object
+    r7 :: int32
+    r8 :: bit
+    r9 :: bool
+    r10 :: object
+L0:
+    r0 = builtins :: module
+    r1 = 'bool'
+    r2 = CPyObject_GetAttr(r0, r1)
+    r3 = PyObject_IsInstance(value, r2)
+    r4 = r3 >= 0 :: signed
+    r5 = truncate r3: int32 to builtins.bool
+    r6 = box(int32, r3)
+    r7 = PyObject_Not(r6)
+    r8 = r7 >= 0 :: signed
+    r9 = truncate r7: int32 to builtins.bool
+    r10 = box(bool, r9)
+    return r10
+
+[case testIsinstanceNotBoolAndIsInt]
+# This test is to ensure that 'value' doesn't get coerced into a bool when we need it to be an int
+def is_not_bool_and_is_int(value):
+    return not isinstance(value, bool) and isinstance(value, int)
path: mypyc/test-data/irbuild-isinstance.test
text: @user1:
Similar to above, can you try adding an annotation (even simple one such as `Any` -> `bool`) in case it simplifies the IR?

Also, it would be good to have a run test case for this. This could go into `mypyc/test-data/run-integers.test`. No need to do this for the other test cases above.

@user1:
Similar to below -- to test that the bug fixed, can you switch the order of the `isinstance` checks?

diff_hunk: @@ -248,7 +248,11 @@ def gen_inner_stmts() -> None:
 
 @specialize_function('builtins.isinstance')
 def translate_isinstance(builder: IRBuilder, expr: CallExpr, callee: RefExpr) -> Optional[Value]:
-    # Special case builtins.isinstance
+    # Special case for builtins.isinstance
+    # Prevent coercions on the thing we are checking the instance of - there is no need to coerce
+    # something to a new type before checking what type it is, and the coercion could lead to bugs.
+    builder.types[expr.args[0]] = AnyType(TypeOfAny.from_error)
path: mypyc/irbuild/specialize.py
text: @user1:
This could be moved to within the if statement, so that if somehow there aren't any arguments, we won't get an `IndexError`.