Skip to content

assert isinstance check for a non-subtype confuses mypy #2776

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
axch opened this issue Jan 30, 2017 · 7 comments
Closed

assert isinstance check for a non-subtype confuses mypy #2776

axch opened this issue Jan 30, 2017 · 7 comments

Comments

@axch
Copy link

axch commented Jan 30, 2017

Consider a coding pattern where one chooses (for some reason) to type a function as accepting a broad argument type, but in the body to assert isinstance a narrower type, like this:

class A(object): pass

class B(A): pass

def foo(x):
    # type: (A) -> None
    reveal_type(x)
    assert isinstance(x, B)
    reveal_type(x)

Mypy recognizes the assert and narrows the type of x for the second reveal_type, as expected:

$ mypy --python-version 2.7 mypytest.py
mypytest.py:7: error: Revealed type is 'mypytest.A'
mypytest.py:9: error: Revealed type is 'mypytest.B'

But, suppose one makes a mistake, and either uses the wrong class, or forgets to inherit from A, like this:

class A(object): pass

class C(object): pass

def bar(x):
    # type: (A) -> None
    reveal_type(x)
    assert isinstance(x, C)
    reveal_type(x)

Now, mypy appears to just ignore the assert and the second reveal_type:

$ mypy --python-version 2.7 mypytest2.py
mypytest2.py:7: error: Revealed type is 'mypytest2.A'

I recognize that multiple inheritance in Python means that mypy would need intersection types in order to type x precisely in the second example, but I am very surprised that it just completely ignores the reveal_type.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 31, 2017

@axch Mypy currently believes that the assert will always raise, so the following statements will never be reached. I agree that this is pretty surprising behavior. To generally fix this we'd need intersection types, as you correctly point out. However, in some cases inferring an intersection type would not be the right thing. Consider this code:

def f(x: str) -> None:
    assert isinstance(x, bytes)
    reveal_type(x)

Now the revealed type should probably be bytes, since you can't have multiple inheritance from str and bytes.

I'm not sure what we should do in the short term -- intersection types are probably a lot of work to implement, so I doubt that's feasible right now.

Here are some ideas:

  • Give x the type C after the isinstance check. This wouldn't always do the right thing, but it would probably be less surprising than what we have right now.
  • Synthesize a TypeInfo that fakes an intersection type through multiple inheritance. This would be easier than full intersection type support, but still non-trivial. We'd still need to figure out what to do in the str / bytes case.
  • Give a warning about the assert to avoid accidentally having code that is not being type checked. We'd probably give x the type Any after the assert so that we could do some type checking (or perhaps C).
  • Silently give x the type Any. This would arguably be less bad than what we currently have, though still not great.

@axch
Copy link
Author

axch commented Jan 31, 2017

In my case, warning about the assert would have been ideal, since that's where my mistake actually was. Or, at least, warning about an "unreachable" reveal_type call.

It occurs to me that what the "right thing" to do in general is depends on a question about mypy's philosophy, which I have asked in the separate ticket #2780.

@axch
Copy link
Author

axch commented Feb 1, 2017

Another oddity: The HTML report generated by --html-report classifies all lines after the assert as "line-empty", which causes them to appear visually as covered by type checking even though they are not. I think it would be much more helpful if the coverage report claimed that those lines were not covered.

@JukkaL
Copy link
Collaborator

JukkaL commented Feb 1, 2017

@axch Can you file a separate issue for the HTML report problem?

@axch
Copy link
Author

axch commented Feb 1, 2017

Done.

@JukkaL
Copy link
Collaborator

JukkaL commented Feb 1, 2017

Thanks!

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 28, 2020

Looks like this was fixed by #8305.

@JukkaL JukkaL closed this as completed Jan 28, 2020
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