typing: set warn_unreachable

This makes mypy raise an error whenever it detects code which is
statically unreachable, e.g.

    x: int
    if isinstance(x, str):
        ... # Statement is unreachable  [unreachable]

This is really neat and finds quite a few logic and typing bugs.

Sometimes the code is intentionally unreachable in terms of types, e.g.
raising TypeError when a function is given an argument with a wrong
type. In these cases a `type: ignore[unreachable]` is needed, but I
think it's a nice code hint.

setup.cfg

@@ -103,5 +103,6 @@ show_error_codes = True
 strict_equality = True
 warn_redundant_casts = True
 warn_return_any = True
+warn_unreachable = True
 warn_unused_configs = True
 no_implicit_reexport = True

src/_pytest/_code/code.py

@@ -676,7 +676,7 @@ class FormattedExcinfo:
 
     def get_source(
         self,
-        source: "Source",
+        source: Optional["Source"],
         line_index: int = -1,
         excinfo: Optional[ExceptionInfo[BaseException]] = None,
         short: bool = False,

src/_pytest/assertion/__init__.py

@@ -57,7 +57,7 @@ def register_assert_rewrite(*names: str) -> None:
     """
     for name in names:
         if not isinstance(name, str):
-            msg = "expected module names as *args, got {0} instead"
+            msg = "expected module names as *args, got {0} instead"  # type: ignore[unreachable]
             raise TypeError(msg.format(repr(names)))
     for hook in sys.meta_path:
         if isinstance(hook, rewrite.AssertionRewritingHook):

src/_pytest/assertion/rewrite.py

@@ -16,6 +16,7 @@ import types
 from typing import Callable
 from typing import Dict
 from typing import IO
+from typing import Iterable
 from typing import List
 from typing import Optional
 from typing import Sequence
@@ -455,12 +456,9 @@ def _should_repr_global_name(obj: object) -> bool:
         return True
 
 
-def _format_boolop(explanations, is_or: bool):
+def _format_boolop(explanations: Iterable[str], is_or: bool) -> str:
     explanation = "(" + (is_or and " or " or " and ").join(explanations) + ")"
-    if isinstance(explanation, str):
+    return explanation.replace("%", "%%")
-        return explanation.replace("%", "%%")
-    else:
-        return explanation.replace(b"%", b"%%")
 
 
 def _call_reprcompare(

src/_pytest/capture.py

@@ -116,7 +116,7 @@ def _py36_windowsconsoleio_workaround(stream: TextIO) -> None:
         return
 
     # Bail out if ``stream`` doesn't seem like a proper ``io`` stream (#2666).
-    if not hasattr(stream, "buffer"):
+    if not hasattr(stream, "buffer"):  # type: ignore[unreachable]
         return
 
     buffered = hasattr(stream.buffer, "raw")

src/_pytest/compat.py

@@ -176,8 +176,9 @@ def getfuncargnames(
         p.name
         for p in parameters.values()
         if (
-            p.kind is Parameter.POSITIONAL_OR_KEYWORD
+            # TODO: Remove type ignore after https://github.com/python/typeshed/pull/4383
-            or p.kind is Parameter.KEYWORD_ONLY
+            p.kind is Parameter.POSITIONAL_OR_KEYWORD  # type: ignore[unreachable]
+            or p.kind is Parameter.KEYWORD_ONLY  # type: ignore[unreachable]
         )
         and p.default is Parameter.empty
     )

src/_pytest/config/__init__.py

@@ -1390,7 +1390,7 @@ def _assertion_supported() -> bool:
     except AssertionError:
         return True
     else:
-        return False
+        return False  # type: ignore[unreachable]
 
 
 def create_terminal_writer(

src/_pytest/config/findpaths.py

@@ -107,7 +107,7 @@ def locate_config(
 
 
 def get_common_ancestor(paths: Iterable[py.path.local]) -> py.path.local:
-    common_ancestor = None
+    common_ancestor = None  # type: Optional[py.path.local]
     for path in paths:
         if not path.exists():
             continue

src/_pytest/debugging.py

@@ -7,6 +7,7 @@ from typing import Any
 from typing import Callable
 from typing import Generator
 from typing import List
+from typing import Optional
 from typing import Tuple
 from typing import Union
 
@@ -23,6 +24,8 @@ from _pytest.nodes import Node
 from _pytest.reports import BaseReport
 
 if TYPE_CHECKING:
+    from typing import Type
+
     from _pytest.capture import CaptureManager
     from _pytest.runner import CallInfo
 
@@ -92,20 +95,22 @@ def pytest_configure(config: Config) -> None:
 class pytestPDB:
     """Pseudo PDB that defers to the real pdb."""
 
-    _pluginmanager = None  # type: PytestPluginManager
+    _pluginmanager = None  # type: Optional[PytestPluginManager]
     _config = None  # type: Config
-    _saved = []  # type: List[Tuple[Callable[..., None], PytestPluginManager, Config]]
+    _saved = (
+        []
+    )  # type: List[Tuple[Callable[..., None], Optional[PytestPluginManager], Config]]
     _recursive_debug = 0
-    _wrapped_pdb_cls = None
+    _wrapped_pdb_cls = None  # type: Optional[Tuple[Type[Any], Type[Any]]]
 
     @classmethod
-    def _is_capturing(cls, capman: "CaptureManager") -> Union[str, bool]:
+    def _is_capturing(cls, capman: Optional["CaptureManager"]) -> Union[str, bool]:
         if capman:
             return capman.is_capturing()
         return False
 
     @classmethod
-    def _import_pdb_cls(cls, capman: "CaptureManager"):
+    def _import_pdb_cls(cls, capman: Optional["CaptureManager"]):
         if not cls._config:
             import pdb
 
@@ -144,7 +149,7 @@ class pytestPDB:
         return wrapped_cls
 
     @classmethod
-    def _get_pdb_wrapper_class(cls, pdb_cls, capman: "CaptureManager"):
+    def _get_pdb_wrapper_class(cls, pdb_cls, capman: Optional["CaptureManager"]):
         import _pytest.config
 
         # Type ignored because mypy doesn't support "dynamic"
@@ -176,9 +181,11 @@ class pytestPDB:
                                 "PDB continue (IO-capturing resumed for %s)"
                                 % capturing,
                             )
+                        assert capman is not None
                         capman.resume()
                     else:
                         tw.sep(">", "PDB continue")
+                assert cls._pluginmanager is not None
                 cls._pluginmanager.hook.pytest_leave_pdb(config=cls._config, pdb=self)
                 self._continued = True
                 return ret
@@ -232,10 +239,10 @@ class pytestPDB:
         """Initialize PDB debugging, dropping any IO capturing."""
         import _pytest.config
 
-        if cls._pluginmanager is not None:
+        if cls._pluginmanager is None:
-            capman = cls._pluginmanager.getplugin("capturemanager")
+            capman = None  # type: Optional[CaptureManager]
         else:
-            capman = None
+            capman = cls._pluginmanager.getplugin("capturemanager")
         if capman:
             capman.suspend(in_=True)
 

src/_pytest/doctest.py

@@ -635,8 +635,8 @@ def _init_checker_class() -> "Type[doctest.OutputChecker]":
                 return got
             offset = 0
             for w, g in zip(wants, gots):
-                fraction = w.group("fraction")
+                fraction = w.group("fraction")  # type: Optional[str]
-                exponent = w.group("exponent1")
+                exponent = w.group("exponent1")  # type: Optional[str]
                 if exponent is None:
                     exponent = w.group("exponent2")
                 if fraction is None:

src/_pytest/fixtures.py

@@ -993,7 +993,8 @@ class FixtureDef(Generic[_FixtureValue]):
         else:
             scope_ = scope
         self.scopenum = scope2index(
-            scope_ or "function",
+            # TODO: Check if the `or` here is really necessary.
+            scope_ or "function",  # type: ignore[unreachable]
             descr="Fixture '{}'".format(func.__name__),
             where=baseid,
         )
@@ -1319,7 +1320,7 @@ def fixture(  # noqa: F811
     # **kwargs and check `in`, but that obfuscates the function signature.
     if isinstance(fixture_function, str):
         # It's actually the first positional argument, scope.
-        args = (fixture_function, *args)
+        args = (fixture_function, *args)  # type: ignore[unreachable]
         fixture_function = None
     duplicated_args = []
     if len(args) > 0:

src/_pytest/junitxml.py

@@ -330,7 +330,7 @@ def _check_record_param_type(param: str, v: str) -> None:
     type."""
     __tracebackhide__ = True
     if not isinstance(v, str):
-        msg = "{param} parameter needs to be a string, but {g} given"
+        msg = "{param} parameter needs to be a string, but {g} given"  # type: ignore[unreachable]
         raise TypeError(msg.format(param=param, g=type(v).__name__))
 
 

src/_pytest/monkeypatch.py

@@ -91,7 +91,7 @@ def annotated_getattr(obj: object, name: str, ann: str) -> object:
 
 
 def derive_importpath(import_path: str, raising: bool) -> Tuple[str, object]:
-    if not isinstance(import_path, str) or "." not in import_path:
+    if not isinstance(import_path, str) or "." not in import_path:  # type: ignore[unreachable]
         raise TypeError(
             "must be absolute import path string, not {!r}".format(import_path)
         )
@@ -272,7 +272,7 @@ class MonkeyPatch:
         character.
         """
         if not isinstance(value, str):
-            warnings.warn(
+            warnings.warn(  # type: ignore[unreachable]
                 pytest.PytestWarning(
                     "Value of environment variable {name} type should be str, but got "
                     "{value!r} (type: {type}); converted to str implicitly".format(

src/_pytest/outcomes.py

@@ -28,7 +28,7 @@ class OutcomeException(BaseException):
 
     def __init__(self, msg: Optional[str] = None, pytrace: bool = True) -> None:
         if msg is not None and not isinstance(msg, str):
-            error_msg = (
+            error_msg = (  # type: ignore[unreachable]
                 "{} expected string as 'msg' parameter, got '{}' instead.\n"
                 "Perhaps you meant to use a mark?"
             )

src/_pytest/pathlib.py

@@ -367,7 +367,6 @@ def make_numbered_dir_with_cleanup(
 
 
 def resolve_from_str(input: str, root: py.path.local) -> Path:
-    assert not isinstance(input, Path), "would break on py2"
     rootpath = Path(root)
     input = expanduser(input)
     input = expandvars(input)

src/_pytest/python.py

@@ -1097,7 +1097,10 @@ class Metafunc:
             elif isinstance(id_value, (float, int, bool)):
                 new_ids.append(str(id_value))
             else:
-                msg = "In {}: ids must be list of string/float/int/bool, found: {} (type: {!r}) at index {}"
+                msg = (  # type: ignore[unreachable]
+                    "In {}: ids must be list of string/float/int/bool, "
+                    "found: {} (type: {!r}) at index {}"
+                )
                 fail(
                     msg.format(func_name, saferepr(id_value), type(id_value), idx),
                     pytrace=False,

src/_pytest/python_api.py

@@ -495,7 +495,8 @@ def approx(expected, rel=None, abs=None, nan_ok: bool = False) -> ApproxBase:
     elif (
         isinstance(expected, Iterable)
         and isinstance(expected, Sized)
-        and not isinstance(expected, STRING_TYPES)
+        # Type ignored because the error is wrong -- not unreachable.
+        and not isinstance(expected, STRING_TYPES)  # type: ignore[unreachable]
     ):
         cls = ApproxSequencelike
     else:
@@ -662,8 +663,8 @@ def raises(  # noqa: F811
     else:
         excepted_exceptions = expected_exception
     for exc in excepted_exceptions:
-        if not isinstance(exc, type) or not issubclass(exc, BaseException):
+        if not isinstance(exc, type) or not issubclass(exc, BaseException):  # type: ignore[unreachable]
-            msg = "expected exception must be a BaseException type, not {}"
+            msg = "expected exception must be a BaseException type, not {}"  # type: ignore[unreachable]
             not_a = exc.__name__ if isinstance(exc, type) else type(exc).__name__
             raise TypeError(msg.format(not_a))
 

src/_pytest/reports.py

@@ -386,7 +386,8 @@ def pytest_report_to_serializable(
         data = report._to_json()
         data["$report_type"] = report.__class__.__name__
         return data
-    return None
+    # TODO: Check if this is actually reachable.
+    return None  # type: ignore[unreachable]
 
 
 def pytest_report_from_serializable(

src/_pytest/terminal.py

@@ -522,12 +522,12 @@ class TerminalReporter:
         rep = report
         res = self.config.hook.pytest_report_teststatus(
             report=rep, config=self.config
-        )  # type: Tuple[str, str, str]
+        )  # type: Tuple[str, str, Union[str, Tuple[str, Mapping[str, bool]]]]
         category, letter, word = res
-        if isinstance(word, tuple):
+        if not isinstance(word, tuple):
-            word, markup = word
-        else:
             markup = None
+        else:
+            word, markup = word
         self._add_stats(category, [rep])
         if not letter and not word:
             # Probably passed setup/teardown.

src/_pytest/tmpdir.py

@@ -26,7 +26,7 @@ class TempPathFactory:
     """
 
     _given_basetemp = attr.ib(
-        type=Path,
+        type=Optional[Path],
         # Use os.path.abspath() to get absolute path instead of resolve() as it
         # does not work the same in all platforms (see #4427).
         # Path.absolute() exists, but it is not public (see https://bugs.python.org/issue25012).

testing/code/test_source.py

@@ -238,7 +238,7 @@ def test_getline_finally() -> None:
             c(1)  # type: ignore
         finally:
             if teardown:
-                teardown()
+                teardown()  # type: ignore[unreachable]
     source = excinfo.traceback[-1].statement
     assert str(source).strip() == "c(1)  # type: ignore"
 

testing/test_assertrewrite.py

@@ -381,7 +381,7 @@ class TestAssertionRewrite:
         )
 
         def f7() -> None:
-            assert False or x()
+            assert False or x()  # type: ignore[unreachable]
 
         assert (
             getmsg(f7, {"x": x})
@@ -416,7 +416,7 @@ class TestAssertionRewrite:
 
     def test_short_circuit_evaluation(self) -> None:
         def f1() -> None:
-            assert True or explode  # type: ignore[name-defined] # noqa: F821
+            assert True or explode  # type: ignore[name-defined,unreachable] # noqa: F821
 
         getmsg(f1, must_pass=True)
 
@@ -471,7 +471,7 @@ class TestAssertionRewrite:
         assert getmsg(f1) == "assert ((3 % 2) and False)"
 
         def f2() -> None:
-            assert False or 4 % 2
+            assert False or 4 % 2  # type: ignore[unreachable]
 
         assert getmsg(f2) == "assert (False or (4 % 2))"
 

testing/test_monkeypatch.py

@@ -360,7 +360,7 @@ def test_issue156_undo_staticmethod(Sample: "Type[Sample]") -> None:
     monkeypatch.setattr(Sample, "hello", None)
     assert Sample.hello is None
 
-    monkeypatch.undo()
+    monkeypatch.undo()  # type: ignore[unreachable]
     assert Sample.hello()
 
 

testing/test_pytester.py

@@ -23,7 +23,9 @@ def test_make_hook_recorder(testdir) -> None:
     recorder = testdir.make_hook_recorder(item.config.pluginmanager)
     assert not recorder.getfailures()
 
-    pytest.xfail("internal reportrecorder tests need refactoring")
+    # (The silly condition is to fool mypy that the code below this is reachable)
+    if 1 + 1 == 2:
+        pytest.xfail("internal reportrecorder tests need refactoring")
 
     class rep:
         excinfo = None