| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Currently blocks marked as unreachable in `reachability.py` through semanal_pass1 are ignored by the `--warn-unreachable` flag through a `block.is_unreachable` check. Namely, reachability marks the if bodies and else-body as unreachable if it is not reachable on the current platform or Python version. This works for if-statements that have an else branch but if the if-statement has a short-circuiting early return or raise on a particular platform, then `reachability.py` places an empty else block and marks it unreachable but has no effect on the fall-through code. When `checker.py` correctly determines that the if-statement is always True and has a return inside it, it correctly marks any code after the if-statement as unreachable. This fix makes it so the code after if-statements that are known to be always true is marked reachable to surpress spurious `--warn-unreachable` warnings.
for more information, see https://pre-commit.ci
|
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 |
Sorry, something went wrong.
|
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:
I think this is good behavior. Think of every sys.platform/sys.version_info check as a typing.TYPE_CHECKING check. |
Sorry, something went wrong.
Co-authored-by: A5rocks <git@helvetica.moe>
|
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. |
Sorry, something went wrong.
What you've done is totally fine. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Also, this also fixes:
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. |
Sorry, something went wrong.
|
mypy_primer results look good, we've eliminated false positives in rich when running on non-windows codebases. Edit:
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. |
Sorry, something went wrong.
| from_assignment: bool | ||
|
|
||
|
|
||
| class UnreachableType(enum.Enum): |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
@ammaraskar mind fixing the conflicts? (otherwise I would be fine to adopt this PR) |
Sorry, something went wrong.
|
I'll try to take a look at the CI issue tonight but if I don't respond in a few days feel free to adopt the PR. |
Sorry, something went wrong.
|
Diff from mypy_primer, showing the effect of this PR on open source code: core (https://github.com/home-assistant/core)
+ homeassistant/helpers/config_validation.py:363: error: Unused "type: ignore" comment [unused-ignore]
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]
|
Sorry, something went wrong.
|
The code still self-type checks, only fails in mypyc and not much has changed. I think the CI failure is a bug in mypyc. I don't have the bandwidth to look into it at the moment. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.
/cc @A5rocks