[mypyc] Don't coerce types checked with isinstance (#811) #10245
Conversation
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.
mypyc/irbuild/specialize.py
Outdated
| # 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) |
There was a problem hiding this comment.
This could be moved to within the if statement, so that if somehow there aren't any arguments, we won't get an IndexError.
| [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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Similar to below -- to test that the bug fixed, can you switch the order of the isinstance checks?
Prevent IndexErrors at compilation time if the there are no arguments given to isinstance().
mypyc/test-data/run-integers.test
Outdated
| @@ -157,6 +157,15 @@ def check_bitwise(x: int, y: int) -> None: | |||
| check_or(ll, rr) | |||
| check_xor(ll, rr) | |||
|
|
|||
| [case testIsinstanceNotBoolAndIsInt] | |||
| def test_isinstance_not_bool_and_is_int(value: object) -> bool: | |||
| return not isinstance(value, bool) and isinstance(value, int) | |||
There was a problem hiding this comment.
Actually this already seems to work correctly. Can you switch the checks so that it will reproduce the incorrect old behavior? I.e., like this:
return isinstance(value, int) and not isinstance(value, bool)| [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) |
There was a problem hiding this comment.
Similar to below -- to test that the bug fixed, can you switch the order of the isinstance checks?
|
Oops, good catch. |
Reverse the order of comparison to check a use-case which was actually formerly buggy.
| return r6 | ||
|
|
||
| [case testIsinstanceIntAndNotBool] | ||
| # This test is to ensure that 'value' doesn't get coerced into a bool when we need it to be an int |
There was a problem hiding this comment.
Hmm the comment is a bit unclear. Maybe something like "ensure that 'value' doesn't get coerced to int when we are checking if it's a bool, since an int can never be an instance of bool"?
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.