Skip to content

mypy skips type checking if it sees assertion inconsistent with param signature #8349

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

Closed
gjcarneiro opened this issue Jan 31, 2020 · 4 comments

Comments

@gjcarneiro
Copy link

Given this test file:

def foo(foo: int, bar: int) -> int:
    assert isinstance(foo, str)
    return bar + "1"

Mypy is thrown off the scent by the inconsistent assert assert isinstance(foo, str), and then it skips type checking the entire function, without any hint of a warning:

(mypy) 11:42:57 (master) ~/Downloads/mypy$ mypy --version
mypy 0.770+dev.ea3c65cc377fcb6e810dec9ed6314b25113dcda2
(mypy) 11:43:04 (master) ~/Downloads/mypy$ mypy /tmp/test.py 
Success: no issues found in 1 source file

If I remove the assert, it does the type checking correctly:

def foo(foo: int, bar: int) -> int:
    # assert isinstance(foo, str)
    return bar + "1"
(mypy) 11:43:11 (master) ~/Downloads/mypy$ mypy /tmp/test.py 
/tmp/test.py:4: error: Unsupported operand types for + ("int" and "str")
Found 1 error in 1 file (checked 1 source file)
@gjcarneiro
Copy link
Author

Also tried versions 0.720 and 0.761, same issue.

@Michael0x2a
Copy link
Collaborator

Having isinstance(...) narrow types/sometimes mark branches as unreachable is more or less by design.

You can have mypy report an error when this happens by using the --warn-unreachable flag. That will make mypy report a "Statement is unreachable" error on the return after the assert.

(And mypy master will also report start reporting an error on the isinstance check itself warning you that a subclass of int and str cannot exist.)

@gjcarneiro
Copy link
Author

Well, I think that:

  1. the type is not being narrowed, we are assert it's a completely different type, unrelated to the previously declared type: re-declaring a variable as str, when it used to be int, is not type narrowing, AFAIK;
  2. but, most of all, the code is not really unreachable: if you run with python3 -O, the assert simply disappears, and then you have a bug due to the bad bet type usage.

So, this is weird:

(mypy) 04:24:43 (master) ~/Downloads/mypy$ mypy --warn-unreachable /tmp/test.py 
/tmp/test.py:3: error: Subclass of "int" and "str" cannot exist: would have incompatible method signatures
/tmp/test.py:4: error: Statement is unreachable
Found 2 errors in 1 file (checked 1 source file)
(mypy) 04:24:54 (master) ~/Downloads/mypy$ mypy  /tmp/test.py 
Success: no issues found in 1 source file
(mypy) 04:25:11 (master) ~/Downloads/mypy$ 

Yes, error: Subclass of "int" and "str" cannot exist: would have incompatible method signatures is exactly the sort of error I was expecting. But why does it only show up with the --warn-unreachable option? This has nothing to do with code being reachable or not. Can you please turn this on unconditionally? Pretty please? 🙏

@Michael0x2a
Copy link
Collaborator

the type is not being narrowed, we are assert it's a completely different type, unrelated to the previously declared type: re-declaring a variable as str, when it used to be int, is not type narrowing, AFAIK;

It's type narrowing because mypy is assuming your function type signature is accurate and that you really are providing an int/a subclass of int. So when you do isinstance(foo, str), that must mean foo must be a subclass of both int and str.

This is type narrowing because the set of types that subclass both int and str (in this case, the empty set) is smaller than the set that subclasses just int.

If you want to go further and say a variable is a new, completely unrelated type, I'd use a cast.

If you're worried about foo not being what it's declared type is, I recommend widening the type hint to be more accurate or maybe experimenting with the disallow unreachable family of flags to find cases where dynamically-typed code is passing values into statically-typed code.

but, most of all, the code is not really unreachable: if you run with python3 -O, the assert simply disappears, and then you have a bug due to the bad bet type usage.

There's some discussion about this in #7467.

But basically, mypy trusts you are accurately encoding out-of-band information about the type of something when you use an assert. Note that you need this same degree of trust to use the -O flag: if you're not confident your asserts are always true, you wouldn't be using -O in prod/would have replaced the assert with an if statement.

Traditionally, the way you'd validate your asserts is by writing a bunch of unit tests -- mypy helps this process along a bit by helping you detect some always-false asserts using --warn-unreachable.

Changing how mypy interprets asserts would also likely not be feasible due to backwards compatibility concerns: it's pretty common to use asserts as a way of narrowing types or marking branches as unreachable in cases where mypy's type inference was too conservative.

Yes, error: Subclass of "int" and "str" cannot exist: would have incompatible method signatures is exactly the sort of error I was expecting. But why does it only show up with the --warn-unreachable option? This has nothing to do with code being reachable or not.

The --warn-unreachable flag is designed to report the reason why something was determined to be unreachable whenever possible. In this case, since the subclass cannot exist, the assert is always false. So, the "subclass cannot exist" error is the reason for why the return is inferred to be unreachable.

Can you please turn this on unconditionally?

I think this goes against the general philosophy of mypy, which is to have the default settings be pretty lenient and ask users to opt-in to stricter checks. This makes it easier to adopt mypy on large, pre-existing untyped codebases: strictness can be layered on incrementally instead of all at once.

If you want to enable many of the existing strictness options, I recommend adding a mypy config file to your project and enabling things like warn-unreachable there. Then, you will always see unreachable-based warnings when you run mypy my-project.

You can also track this issue for adding --warn-unreachable to the --strict flag once the former has matured a bit more.

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

No branches or pull requests

2 participants