Two potential conflicting type checks and dead code

[rc4typecheck]

Potential conflicting type checks and dead code in

When statically analyzing and manually reviewing the code, I noticed a potential logic conflicting in /mypyc/analysis/ircheck.py as follows:

elif isinstance(op.value, int):  # L292
    expected_type = "builtins.int"
elif isinstance(op.value, str):
    expected_type = "builtins.str"
elif isinstance(op.value, bytes):
    expected_type = "builtins.bytes"
elif isinstance(op.value, bool):  # L298 dead code
    expected_type = "builtins.object"

The condition 'isinstance(op.value, bool)' at line 298 will never be True because 'bool' is a subtype of 'int' and the previous condition 'isinstance(op.value, int)' at line 292 would have already caught all boolean values (since bools are ints in Python). This creates a logical conflict in the type checking flow.

Another issue is in /mypyc/test/test_serialization.py:

elif isinstance(x, dict):  # L53
    assert len(x.keys()) == len(y.keys()), f"Keys mismatch at {trail}"
    for (xk, xv), (yk, yv) in zip(x.items(), y.items()):
        assert_blobs_same(xk, yk, trail + ("keys",))
        assert_blobs_same(xv, yv, trail + (xk,))
elif isinstance(x, dict):  # L58 dead code
    assert x.keys() == y.keys(), f"Keys mismatch at {trail}"
    for k in x.keys():
        assert_blobs_same(x[k], y[k], trail + (k,))

The condition 'isinstance(x, dict)' at line 58 is unreachable because the same condition was already checked at line 53. If the first condition (line 53) is not satisfied, the second condition (line 58) will also not be satisfied, making it a conflicting branch type check. This appears to be a logical error in the code where the same type check is duplicated in consecutive elif branches.

Please verify if these are intentional or they are issues warranting a refactoring or fixing.

[A5rocks]

Interesting, we should have warned about these. We even enable unreachable checks! 🤔

[A5rocks]

OK it turns out --warn-unreachable does not in fact catch things like that. That's an oversight. It's kinda irrelevant for this specific issue, but I think we should fix that.