warn-unreachable behaves differently on else-return than on bare return

[sirosen]

Bug Report

mypy --warn-unreachable will error on an unreachable return after an if block but not a semantically equivalent else: return ... after an if block. This shows up in platform checking logic, as an example of "unreachable code" which mypy understands and allows.

Based on what I found in existing warn-unreachable issues, there's no report of this same issue.

To Reproduce

Run mypy --warn-unreachable on the two following samples.

This passes:

def foo():
    if sys.platform != "win32":
        return 1
    else:
        return 0

This fails (on Linux/macOS. Use == "win32" to observe on Windows):

def foo():
    if sys.platform != "win32":
        return 1
    return 0

I also put together a small example dir of this in a github repo, if that makes it easier:
https://github.com/sirosen/repro/tree/2a56d3b/mypy-issues

Expected Behavior

mypy should pass on both of these, as they are the same.

Your Environment

• Mypy version used: 0.910

[hi-ogawa]

EDIT: Sorry, I noticed that the unit test example in the "original comment" below isn't type-checked since it doesn't have annotations. I can reproduce on master with this example.

• test.py

import sys

def f() -> int:
    if sys.platform != "win32":
        return 1
    return 0


def g() -> int:
    if sys.platform == "win32":
        return 1
    return 0

• command

$ mypy --warn-unreachable test.py
test.py:6: error: Statement is unreachable
Found 1 error in 1 file (checked 1 source file)

---

(Original comment)

I cannot reproduce this on master, so possibly it is already fixed by this PR #10560, which fixed some crash from if-branch with sys.platform.
I'm using this unit test locally:

[case testUnreachableBranchBySys1]
# flags: --warn-unreachable
import sys

def f():
    if sys.platform != "win32":
        return 1
    return 0
[builtins fixtures/dict.pyi]

[case testUnreachableBranchBySys2]
# flags: --warn-unreachable
import sys

def f():
    if sys.platform == "win32":
        return 1
    return 0
[builtins fixtures/dict.pyi]

[intgr]

I experienced this issue as well, mypy 1.1.1 with --warn-unreachable

Unreachable error: (Playground link)

    if sys.platform == "linux":
        return
    print("This is reachable on other OS!")

No unreachable error: (Playground link)

    if sys.platform == "linux":
        return
    else:
        print("This is reachable on other OS!")

[ammaraskar]

#5964 is an old bug that is this issue plus a little more functionality.

Currently the way that sys.version_info and sys.platform checks end up not getting flagged by --warn-unreachable is this code block:

mypy/mypy/checker.py

Lines 3020 to 3025 in 1eb9d4c

	if b.is_unreachable:
	# This block was marked as being unreachable during semantic analysis.
	# It turns out any blocks marked in this way are *intentionally* marked
	# as unreachable -- so we don't display an error.
	self.binder.unreachable()
	return

When reachability.py and its consider_sys_version_info and consider_sys_platform functions get invoked in the first mypy pass semanal_pass1.py, they set this b.is_unreachable property:

mypy/mypy/reachability.py

Lines 327 to 329 in 1eb9d4c

	def mark_block_unreachable(block: Block) -> None:
	block.is_unreachable = True
	block.accept(MarkImportsUnreachableVisitor())

---

The is_unreachable property is defined at the Block level meaning that the block for those functions looks like:

Block(
  body=[
    IfStmt(
      expr=ComparisonExpr("!=", ...),
      body=Block(body=[ReturnStmt(...)]),
      else_body=...
    ),

    ReturnStmt()
  ]
)

Currently the if/else based check works because unreachable is able to mark the else_body or body as unreachable. However, for this feature we would specifically want to mark the ReturnStmt in the outer block as unreachable and then somehow pick that up in checker.py.

I'm gonna try to see if I can come up with a PR to solve this but that's the root cause and problem of the issue.

[A5rocks]

However, for this feature we would specifically want to mark the ReturnStmt as unreachable and then somehow pick that up in checker.py.

I was thinking it might be easier to have some sort of unreachability kind that frames get marked as (rather than just generally marking them as unreachable) for the binder to check... but I'm not too sure.