Fix --warn-unreachable fall-through from ALWAYS_TRUE ifs

[ammaraskar]

This is a quick swing at trying to fix #10773

Lots of details in my comment on the original issue and the commit message. This was a quick first approach that I came up with, it's not perfect. Since this doesn't tackle handling early returns in the semanal_pass1.py, conditional imports etc caused before the short-circuiting return are not considered.

• Fixes if MYPY guarding an always-return block v. unreachable #11380
• Fixes Platform check is not remembered after return #14940
• Fixes (🐞) Artificially literal bools not considered when performing reachability checks where branch terminates #12365

/cc @A5rocks

[ammaraskar]

From the mypy_primer checks, I guess marking the subsequent code as reachable is not a good approach since the code may have platform specific code such as the poetry example: https://github.com/python-poetry/poetry/blob/603646c64601eb49886a97f6e930aba2cc520475/src/poetry/utils/helpers.py#L294-L314

def _get_win_folder_from_registry(csidl_name: str) -> str:
    if sys.platform != "win32":
        raise RuntimeError("Method can only be called on Windows.")

    import winreg as _winreg
    ...
    _winreg.HKEY_CURRENT_USER,

Gonna switch it to suppress_unreachable_warnings() instead, though this may cause us to ignore some valid unreachable code such as with, unless you are running on fictional platform:

def foo() -> int:
    if sys.platform != 'fictional':
        return 1
    return 0
    return 0 + 1  # <-- Does not get marked unreachable

[A5rocks]

I'm not convinced about this approach because it feels excessively ad-hoc (surely I should be able to have a else: pass and it has the same behavior, etc.)... I tried the approach I recommended and it's a bit different than what I thought but it seems to work. The main idea is that if there's a frame that travelled to the current frame that was marked as unreachable by semanal pass1, then we could potentially be travelling from that frame to this frame, so this is unreachable by semanal pass1 (unless there's a reachable frame coming this way).

The above probably doesn't really make sense but here's the relevant diff (I could make a PR if you want):

diff

diff --git a/mypy/binder.py b/mypy/binder.py
index 3d833153d..da31d0e2f 100644
--- a/mypy/binder.py
+++ b/mypy/binder.py
@@ -1,5 +1,5 @@
 from __future__ import annotations
-
+import enum
 from collections import defaultdict
 from collections.abc import Iterator
 from contextlib import contextmanager
@@ -35,6 +35,9 @@ class CurrentType(NamedTuple):
     type: Type
     from_assignment: bool

+class UnreachableType(enum.Enum):
+    BINDER_UNREACHABLE = enum.auto()
+    SEMANAL_UNREACHABLE = enum.auto()

 class Frame:
     """A Frame represents a specific point in the execution of a program.
@@ -51,7 +54,7 @@ class Frame:
     def __init__(self, id: int, conditional_frame: bool = False) -> None:
         self.id = id
         self.types: dict[Key, CurrentType] = {}
-        self.unreachable = False
+        self.unreachable: UnreachableType | None = None
         self.conditional_frame = conditional_frame
         self.suppress_unreachable_warnings = False

@@ -161,8 +164,8 @@ class ConditionalTypeBinder:
             self._add_dependencies(key)
         self._put(key, typ, from_assignment)

-    def unreachable(self) -> None:
-        self.frames[-1].unreachable = True
+    def unreachable(self, from_semanal: bool = False) -> None:
+        self.frames[-1].unreachable = UnreachableType.SEMANAL_UNREACHABLE if from_semanal else UnreachableType.BINDER_UNREACHABLE

     def suppress_unreachable_warnings(self) -> None:
         self.frames[-1].suppress_unreachable_warnings = True
@@ -175,13 +178,20 @@ class ConditionalTypeBinder:
             return None
         return found.type

-    def is_unreachable(self) -> bool:
+    def is_unreachable(self) -> UnreachableType | None:
         # TODO: Copy the value of unreachable into new frames to avoid
         # this traversal on every statement?
-        return any(f.unreachable for f in self.frames)
+        result = None
+        for f in self.frames:
+            if f.unreachable and not result:
+                result = f.unreachable
+            elif f.unreachable == UnreachableType.SEMANAL_UNREACHABLE:
+                result = f.unreachable
+
+        return result

     def is_unreachable_warning_suppressed(self) -> bool:
-        return any(f.suppress_unreachable_warnings for f in self.frames)
+        return any(f.suppress_unreachable_warnings for f in self.frames) or self.is_unreachable() == UnreachableType.SEMANAL_UNREACHABLE

     def cleanse(self, expr: Expression) -> None:
         """Remove all references to a Node from the binder."""
@@ -202,6 +212,9 @@ class ConditionalTypeBinder:
         If a key is declared as AnyType, only update it if all the
         options are the same.
         """
+        if all(f.unreachable for f in frames):
+            self.unreachable(from_semanal=any(f.unreachable == UnreachableType.SEMANAL_UNREACHABLE for f in frames))
+
         all_reachable = all(not f.unreachable for f in frames)
         frames = [f for f in frames if not f.unreachable]
         changed = False
@@ -262,8 +275,6 @@ class ConditionalTypeBinder:
                 self._put(key, type, from_assignment=True)
                 changed = True

-        self.frames[-1].unreachable = not frames
-
         return changed

     def pop_frame(self, can_skip: bool, fall_through: int) -> Frame:
@@ -411,7 +422,7 @@ class ConditionalTypeBinder:
         for f in self.frames[index + 1 :]:
             frame.types.update(f.types)
             if f.unreachable:
-                frame.unreachable = True
+                frame.unreachable = f.unreachable
         self.options_on_return[index].append(frame)

     def handle_break(self) -> None:
diff --git a/mypy/checker.py b/mypy/checker.py
index 3734f3170..0e9b8987c 100644
--- a/mypy/checker.py
+++ b/mypy/checker.py
@@ -3021,7 +3021,7 @@ class TypeChecker(NodeVisitor[None], CheckerPluginInterface):
             # 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()
+            self.binder.unreachable(from_semanal=True)
             return
         for s in b.body:
             if self.binder.is_unreachable():

---

However:

Gonna switch it to suppress_unreachable_warnings() instead, though this may cause us to ignore some valid unreachable code such as with, unless you are running on fictional platform:

I think this is good behavior. Think of every sys.platform/sys.version_info check as a typing.TYPE_CHECKING check.

[ammaraskar]

I agree, your approach makes way more sense. It is actually what I was initially trying to do but I wasn't sure how the frame ancestor accessibility stuff worked in binder.

I've pushed your changes in and added you as a Co-Author, please let me know if that's fine or I can close this and you can make your own PR.

[A5rocks]

I've pushed your changes in and added you as a Co-Author, please let me know if that's fine or I can close this and you can make your own PR.

What you've done is totally fine.

[github-actions]

Diff from mypy_primer, showing the effect of this PR on open source code:

rich (https://github.com/Textualize/rich)
- rich/_win32_console.py:16: error: Statement is unreachable  [unreachable]
- rich/_windows.py:25: error: Statement is unreachable  [unreachable]
- rich/_windows.py:40: error: Statement is unreachable  [unreachable]

[A5rocks]

I assume the reason why the unused type ignore doesn't get a mention is because the code doesn't get checked because it's unreachable? I wonder why it worked before.

[A5rocks]

Also, this also fixes:

• if MYPY guarding an always-return block v. unreachable #11380
• (🐞) Artificially literal bools not considered when performing reachability checks where branch terminates #12365
• Platform check is not remembered after return #14940

And the prior aiohttp change in mypy primer didn't actually make sense it seems. Here's the code from aiohttp where it was saying the # type: ignore[unreachable] was unnecessary.

if ssl is not None:
    SSL_ALLOWED_TYPES = (ssl.SSLContext, bool, Fingerprint)
else:  # pragma: no cover
    SSL_ALLOWED_TYPES = (bool,)  # type: ignore[unreachable]

so it's good that it's gone.

[ammaraskar]

mypy_primer results look good, we've eliminated false positives in rich when running on non-windows codebases.

Edit:

Also, this also fixes:

Thanks, I went through each of those and made sure it works. Added fixes in the PR description so they get auto-closed. I don't think they need explicit unit tests since they're all seminal level checks which we already cover.

[A5rocks]

As part of #18707 I've been thinking about related concepts, and I think it's possible to do this without an enum actually: simply mark skipped blocks as suppressed (currently they're only marked as unreachable) and if all the blocks flowing into a block are unreachable, check if any of them are suppressed -- if so, then the current block is suppressed.