Emit redundant-expr warnings for if statements that are always true or always false.

[tyralla]

Fixes #19728

And thereby implements the two XXX comments of TypeChecker.visit_if_stmt.

The only surprise I encountered is that we must take care of while statements here, too, because while statements are transformed to if statements during type checking. It seems right to me to only emit warnings for while False but not for while True.

@A5rocks: Are you aware of other, uncovered special cases?

[tyralla]

Oh, 57 errors for Mypy itself, e.g. if TYPE_CHECKING. I will rethink this tomorrow...

[A5rocks]

We already kind of have a distinction between things that are true at compile time and run time for unreachability reporting, maybe that has a nice way to do that here? Probably not though.

(It's been too long for me to remember any special cases...)

[tyralla]

A few looks into the primer results (from bottom to top; without obvious repetitions):

nox

Mypy issues (in this case already under discussion in #11764).

x: str | None,
if callable(x):  # error: If condition is always false
    ...  # type: ignore[unreachable]

boostedblob

Additional runtime type checking.

def f(x: str | int) -> None:
    if not isinstance(x, str | int):  # error: If condition is always false
        raise ValueError

def g(x: str | int) -> None:
    if isinstance(x, str):
        return
    if isinstance(x, int):  # error: If condition is always true
       return
    raise AssertionError

scikit-build-core

Really redundant code (or at least one that needs explanations):

def f(env: dict[str, str] | None):
    if env:
        for key, value in env.items():
            if isinstance(value, bool):  # error: If condition is always false
                ...

import dataclasses

@dataclasses.dataclass
class Settings1:
    cmake: bool = False

@dataclasses.dataclass
class Settings2:
    cmake: bool = False

def f(s1: Settings1, s2: Settings2) -> None:
    if not s1.cmake or s2.cmake:
        ...
    if s2.cmake:  # error: If condition is always false
        ...

Based on these few examples, this PR seems correct. However, it somehow collides with exhaustiveness checking.

[tyralla]

I tried to filter out warnings for redundant if statements that obviously serve for dynamic type or exhaustiveness checking. The logic is very similar to the one implemented in TypeChecker.is_noop_for_reachability (there is currently even some code duplication). After revisiting some of the primer results, there seem to be two missed cases:

Assertion or raise statement not in else body:

def f(x: int | str) -> None:
    if isinstance(x, int):
        ...
    elif isinstance(x, str):
        ...
    assert False

Custom error handling method:

def f(x: int) -> None:
    if not isinstance(x, int):
        print("type error!")
        return None
    ....

Assertion or raise statement not in the first line of the if or else body:

def f(x: int) -> None:
    if not isinstance(x, int):
        msg = "type error!"
        raise TypeError(msg)
    ....

Missing assertion or raise statement:

def f(x: Literal[1, 2]) -> None:
    if x == 1:
        ...
    elif x == 2:
        ...
    return None

Return NotImplemented:

    def __add__(self, x: float) -> float:
        if not isinstance(name, float):
            return NotImplemented  # type: ignore[unreachable]
        ...

The current state of this PR also causes two unreachable warnings for reasons I have not yet investigated.

[tyralla]

@A5rocks: Thanks for the remarks! I adjusted everything as suggested.

I think the only big open case left (for this PR) is:

def f(x: int | str) -> None:
    if isinstance(x, int):
        ...
    elif isinstance(x, str):
        ...
    assert False

I tend to make the block after the last if or elif statement available as a surrogate else block, but I am not sure if I can implement this cleanly. Or maybe we could check that the last if block returns None (in this case), but this seems too complicated/fragile. Alternatively, we could document that users must explicitly write the else block or use type: ignore if this is not what they want (possibly not a very good alternative):

def f(x: int | str) -> None:
    if isinstance(x, int):
        ...
    elif isinstance(x, str):
        ...
    else:
        assert False

[A5rocks]

If I'm understanding your case right (that you don't want to trigger redundant-expr for elif isinstance(x, str):), then I agree synthesizing an else makes sense. But I think you can probably do something better: check if the statement is unreachable. We want this to warn:

def blah(x: int | str) -> None:
  if isinstance(x, int):
    return
  elif isinstance(x, str):
    pass
  more_things()

and we want this to pass:

def blah(x: int | str) -> None:
  if isinstance(x, int):
    return
  elif isinstance(x, str):
    return
  more_things()

Specifically, the number of reachable paths that can lead to the more_things() including a synthesized else: has to be 0... which I believe is exactly what unreachability checks. I'm honestly not sure this case is worth this much effort but I think that might work?

(We'll still need the synthesized else to check whether to actually warn or not)

FYI I can't resolve conversations, so feel free to.

[github-actions]

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

aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/client_exceptions.py:323:4: error: If condition is always true  [redundant-expr]
+ aiohttp/client_exceptions.py:323:4: note: See https://mypy.rtfd.io/en/stable/_refs.html#code-redundant-expr for more info
+ aiohttp/helpers.py:267:8: error: If condition is always false  [redundant-expr]
+ aiohttp/helpers.py:877:14: error: If condition is always true  [redundant-expr]
+ aiohttp/cookiejar.py:297:12: error: If condition is always false  [redundant-expr]
+ aiohttp/cookiejar.py:485:15: error: While condition is always false  [redundant-expr]
+ aiohttp/multipart.py:306:15: error: While condition is always true  [redundant-expr]
+ aiohttp/multipart.py:447:15: error: While condition is always true  [redundant-expr]
+ aiohttp/multipart.py:660:12: error: If condition is always false  [redundant-expr]
+ aiohttp/connector.py:159:12: error: If condition is always true  [redundant-expr]
+ aiohttp/connector.py:812:8: error: If condition is always false  [redundant-expr]
+ aiohttp/client_reqrep.py:169:4: error: If condition is always true  [redundant-expr]
+ aiohttp/client_reqrep.py:275:12: error: If condition is always true  [redundant-expr]
+ aiohttp/client.py:948:16: error: If condition is always true  [redundant-expr]
+ aiohttp/web_response.py:267:14: error: If condition is always true  [redundant-expr]
+ aiohttp/web_fileresponse.py:328:16: error: If condition is always true  [redundant-expr]
+ aiohttp/web_protocol.py:715:16: error: If condition is always false  [redundant-expr]
+ aiohttp/web_runner.py:263:20: error: If condition is always true  [redundant-expr]

mypy (https://github.com/python/mypy)
+ mypy/nodes.py:4533: error: If condition is always true  [redundant-expr]
+ mypy/nodes.py:4533: note: See https://mypy.rtfd.io/en/stable/_refs.html#code-redundant-expr for more info
+ mypy/nodes.py:4582: error: If condition is always true  [redundant-expr]
+ mypy/nodes.py:4626: error: If condition is always true  [redundant-expr]
+ mypy/nodes.py:4831: error: If condition is always true  [redundant-expr]
+ mypy/traverser.py:126: error: If condition is always true  [redundant-expr]
+ mypy/traverser.py:195: error: If condition is always true  [redundant-expr]
+ mypy/traverser.py:201: error: If condition is always true  [redundant-expr]
+ mypy/server/astmerge.py:448: error: If condition is always true  [redundant-expr]
+ mypy/server/astmerge.py:460: error: If condition is always true  [redundant-expr]
+ mypy/server/astmerge.py:477: error: If condition is always true  [redundant-expr]
+ mypy/fixup.py:280: error: If condition is always true  [redundant-expr]
+ mypy/fixup.py:282: error: If condition is always true  [redundant-expr]
+ mypy/fixup.py:315: error: If condition is always true  [redundant-expr]
+ mypy/fixup.py:322: error: If condition is always true  [redundant-expr]
+ mypy/fixup.py:362: error: If condition is always true  [redundant-expr]
+ mypy/constraints.py:133: error: If condition is always false  [redundant-expr]
+ mypy/constraints.py:133: note: Error code "redundant-expr" not covered by "type: ignore" comment
+ mypy/messages.py:1254: error: If condition is always true  [redundant-expr]
+ mypyc/analysis/ircheck.py:307: error: If condition is always true  [redundant-expr]
+ mypy/server/astdiff.py:259: error: If condition is always true  [redundant-expr]
+ mypy/plugins/dataclasses.py:140: error: If condition is always true  [redundant-expr]
+ mypy/semanal.py:1003: error: If condition is always true  [redundant-expr]
+ mypy/semanal.py:1403: error: If condition is always true  [redundant-expr]
+ mypy/semanal.py:3874: error: If condition is always true  [redundant-expr]
+ mypy/semanal.py:5819: error: If condition is always true  [redundant-expr]
+ mypy/checker.py:2508: error: If condition is always true  [redundant-expr]
+ mypy/checker.py:8671: error: If condition is always true  [redundant-expr]
+ mypy/checker.py:8672: error: If condition is always true  [redundant-expr]
+ mypy/build.py:1077: error: If condition is always false  [redundant-expr]
+ mypy/build.py:1330: error: If condition is always false  [redundant-expr]
+ mypyc/irbuild/prepare.py:263: error: If condition is always true  [redundant-expr]
+ mypy/stubtest.py:349: error: If condition is always false  [redundant-expr]
+ mypy/stubtest.py:632: error: If condition is always false  [redundant-expr]
+ mypyc/irbuild/expression.py:919: error: If condition is always true  [redundant-expr]

pyproject-metadata (https://github.com/pypa/pyproject-metadata)
+ pyproject_metadata/pyproject.py:86: error: If condition is always false  [redundant-expr]
+ pyproject_metadata/pyproject.py:100: error: If condition is always false  [redundant-expr]
+ pyproject_metadata/pyproject.py:105: error: If condition is always false  [redundant-expr]
+ pyproject_metadata/pyproject.py:168: error: If condition is always true  [redundant-expr]
+ pyproject_metadata/pyproject.py:249: error: If condition is always true  [redundant-expr]
+ pyproject_metadata/pyproject.py:333: error: If condition is always false  [redundant-expr]
+ pyproject_metadata/pyproject.py:341: error: If condition is always false  [redundant-expr]
+ pyproject_metadata/pyproject.py:351: error: If condition is always false  [redundant-expr]
+ pyproject_metadata/pyproject.py:383: error: If condition is always false  [redundant-expr]
+ pyproject_metadata/pyproject.py:396: error: If condition is always false  [redundant-expr]
+ pyproject_metadata/pyproject.py:408: error: If condition is always false  [redundant-expr]
+ pyproject_metadata/__init__.py:369: error: If condition is always false  [redundant-expr]

rich (https://github.com/Textualize/rich)
+ rich/text.py:990: error: If condition is always true  [redundant-expr]
+ rich/text.py:1241: error: If condition is always true  [redundant-expr]
+ rich/rule.py:96: error: If condition is always true  [redundant-expr]
+ rich/align.py:178: error: If condition is always true  [redundant-expr]

pyodide (https://github.com/pyodide/pyodide)
+ pyodide-build/pyodide_build/vendor/loky.py:166: error: If condition is always false  [redundant-expr]
+ pyodide-build/pyodide_build/common.py:268: error: If condition is always true  [redundant-expr]
+ src/py/pyodide/http/pyxhr.py:188: error: If condition is always true  [redundant-expr]

packaging (https://github.com/pypa/packaging)
+ src/packaging/_manylinux.py:142: error: If condition is always false  [redundant-expr]
+ src/packaging/markers.py:344: error: If condition is always false  [redundant-expr]
+ src/packaging/metadata.py:386: error: If condition is always false  [redundant-expr]

poetry (https://github.com/python-poetry/poetry)
+ src/poetry/utils/_compat.py:39: error: If condition is always false  [redundant-expr]
+ src/poetry/pyproject/toml.py:48: error: If condition is always false  [redundant-expr]
+ src/poetry/utils/authenticator.py:164: error: If condition is always true  [redundant-expr]
+ src/poetry/config/source.py:23: error: If condition is always false  [redundant-expr]
+ src/poetry/utils/env/env_manager.py:196: error: If condition is always false  [redundant-expr]
+ src/poetry/utils/env/env_manager.py:377: error: If condition is always false  [redundant-expr]
+ src/poetry/installation/executor.py:819: error: If condition is always false  [redundant-expr]
+ src/poetry/console/commands/remove.py:70: error: If condition is always false  [redundant-expr]
+ tests/console/commands/test_install.py:155: error: If condition is always false  [redundant-expr]
+ tests/console/commands/test_build.py:184: error: If condition is always false  [redundant-expr]
+ src/poetry/console/commands/add.py:329: error: If condition is always true  [redundant-expr]

archinstall (https://github.com/archlinux/archinstall)
+ archinstall/lib/models/authentication.py:47: error: If condition is always false  [redundant-expr]
+ archinstall/lib/models/mirrors.py:100: error: If condition is always true  [redundant-expr]
+ archinstall/lib/installer.py:840: error: If condition is always true  [redundant-expr]
+ archinstall/lib/installer.py:946: error: If condition is always true  [redundant-expr]
+ archinstall/lib/authentication/authentication_menu.py:120: error: If condition is always true  [redundant-expr]
+ archinstall/lib/profile/profile_menu.py:142: error: If condition is always true  [redundant-expr]

build (https://github.com/pypa/build)
+ src/build/__main__.py:187: error: If condition is always true  [redundant-expr]

xarray (https://github.com/pydata/xarray)
+ xarray/compat/npcompat.py: note: In function "isdtype":
+ xarray/compat/npcompat.py:72: error: If condition is always false  [redundant-expr]
+ xarray/core/utils.py: note: In function "equivalent":
+ xarray/core/utils.py:266: error: If condition is always false  [redundant-expr]
+ xarray/backends/locks.py: note: In function "combine_locks":
+ xarray/backends/locks.py:267: error: If condition is always true  [redundant-expr]
+ xarray/core/indexing.py: note: In function "group_indexers_by_index":
+ xarray/core/indexing.py:147: error: If condition is always true  [redundant-expr]
+ xarray/core/indexing.py: note: In function "map_index_queries":
+ xarray/core/indexing.py:197: error: If condition is always false  [redundant-expr]
+ xarray/core/indexing.py: note: In function "_decompose_outer_indexer":
+ xarray/core/indexing.py:1398: error: If condition is always false  [redundant-expr]
+ xarray/core/indexing.py: note: At top level:
+ xarray/core/indexes.py: note: In member "from_variables" of class "PandasIndex":
+ xarray/core/indexes.py:722: error: If condition is always false  [redundant-expr]
+ xarray/core/indexes.py: note: In member "to_pandas_indexes" of class "Indexes":
+ xarray/core/indexes.py:1853: error: If condition is always false  [redundant-expr]
+ xarray/core/indexes.py:1853: error: If condition is always true  [redundant-expr]
+ xarray/core/indexes.py:1855: error: If condition is always true  [redundant-expr]
+ xarray/core/indexes.py: note: In member "copy_indexes" of class "Indexes":
+ xarray/core/indexes.py:1881: error: If condition is always false  [redundant-expr]
+ xarray/core/indexes.py:1881: error: If condition is always true  [redundant-expr]
+ xarray/plot/facetgrid.py: note: In member "set_axis_labels" of class "FacetGrid":
+ xarray/plot/facetgrid.py:852: error: If condition is always false  [redundant-expr]
+ xarray/plot/facetgrid.py: note: At top level:
+ xarray/core/variable.py: note: In member "_pad_options_dim_to_index" of class "Variable":
+ xarray/core/variable.py:1225: error: If condition is always false  [redundant-expr]
+ xarray/core/variable.py: note: At top level:
+ xarray/structure/merge.py: note: In function "coerce_pandas_values":
+ xarray/structure/merge.py:530: error: If condition is always false  [redundant-expr]
+ xarray/core/coordinates.py: note: In member "equals" of class "Coordinates":
+ xarray/core/coordinates.py:477: error: If condition is always false  [redundant-expr]
+ xarray/core/coordinates.py: note: In member "identical" of class "Coordinates":
+ xarray/core/coordinates.py:488: error: If condition is always false  [redundant-expr]
+ xarray/conventions.py: note: In function "ensure_not_multiindex":
+ xarray/conventions.py:56: error: If condition is always false  [redundant-expr]
+ xarray/plot/dataarray_plot.py: note: In function "line":
+ xarray/plot/dataarray_plot.py:521: error: If condition is always true  [redundant-expr]
+ xarray/plot/dataarray_plot.py:524: error: If condition is always true  [redundant-expr]
+ xarray/plot/dataarray_plot.py: note: At top level:
+ xarray/plot/dataarray_plot.py: note: In function "_add_labels":
+ xarray/plot/dataarray_plot.py:1110: error: If condition is always true  [redundant-expr]
+ xarray/core/dataset.py: note: In member "expand_dims" of class "Dataset":
+ xarray/core/dataset.py:4576: error: If condition is always true  [redundant-expr]
+ xarray/core/dataset.py: note: In member "transpose" of class "Dataset":
+ xarray/core/dataset.py:6237: error: If condition is always false  [redundant-expr]
+ xarray/core/dataset.py: note: In member "dropna" of class "Dataset":
+ xarray/core/dataset.py:6378: error: If condition is always true  [redundant-expr]
+ xarray/core/dataset.py: note: In member "sortby" of class "Dataset":
+ xarray/core/dataset.py:8102: error: If condition is always true  [redundant-expr]
+ xarray/indexes/nd_point_index.py: note: In member "from_variables" of class "NDPointIndex":
+ xarray/indexes/nd_point_index.py:270: error: If condition is always false  [redundant-expr]
+ xarray/core/parallel.py: note: In function "check_result_variables":
+ xarray/core/parallel.py:50: error: If condition is always true  [redundant-expr]
+ xarray/core/parallel.py: note: At top level:
+ xarray/testing/assertions.py: note: In function "_assert_dataarray_invariants":
+ xarray/testing/assertions.py:431: error: If condition is always true  [redundant-expr]
+ xarray/testing/assertions.py: note: In function "_assert_dataset_invariants":
+ xarray/testing/assertions.py:466: error: If condition is always true  [redundant-expr]
+ xarray/testing/assertions.py: note: In function "_assert_internal_invariants":
+ xarray/testing/assertions.py:490: error: If condition is always true  [redundant-expr]
+ xarray/backends/common.py: note: In function "ensure_dtype_not_object":
+ xarray/backends/common.py:682: error: If condition is always false  [redundant-expr]
+ xarray/computation/rolling.py: note: In member "construct" of class "Coarsen":
+ xarray/computation/rolling.py:1191: error: If condition is always true  [redundant-expr]
+ xarray/computation/rolling.py:1191: error: If condition is always false  [redundant-expr]
+ xarray/computation/rolling.py:1215: error: If condition is always false  [redundant-expr]
+ xarray/computation/fit.py: note: In function "polyfit":
+ xarray/computation/fit.py:186: error: If condition is always true  [redundant-expr]
+ xarray/tests/test_indexing.py: note: In member "test_group_indexers_by_index" of class "TestIndexers":
+ xarray/tests/test_indexing.py:104: error: If condition is always false  [redundant-expr]
+ xarray/tests/test_indexing.py: note: At top level:
+ xarray/tests/test_backends.py: note: In member "check_dtypes_roundtripped" of class "DatasetIOBase":
+ xarray/tests/test_backends.py:521: error: If condition is always false  [redundant-expr]
+ xarray/tests/test_backends.py: note: At top level:
+ xarray/tests/test_backends_datatree.py: note: In function "test_to_zarr_compute_false":
+ xarray/tests/test_backends_datatree.py:787: error: If condition is always true  [redundant-expr]

scikit-build-core (https://github.com/scikit-build/scikit-build-core)
+ src/scikit_build_core/settings/skbuild_overrides.py:219: error: If condition is always false  [redundant-expr]
+ src/scikit_build_core/_vendor/pyproject_metadata/pyproject.py:86: error: If condition is always false  [redundant-expr]
+ src/scikit_build_core/_vendor/pyproject_metadata/pyproject.py:100: error: If condition is always false  [redundant-expr]
+ src/scikit_build_core/_vendor/pyproject_metadata/pyproject.py:105: error: If condition is always false  [redundant-expr]
+ src/scikit_build_core/_vendor/pyproject_metadata/pyproject.py:168: error: If condition is always true  [redundant-expr]
+ src/scikit_build_core/_vendor/pyproject_metadata/pyproject.py:249: error: If condition is always true  [redundant-expr]
+ src/scikit_build_core/_vendor/pyproject_metadata/pyproject.py:333: error: If condition is always false  [redundant-expr]
+ src/scikit_build_core/_vendor/pyproject_metadata/pyproject.py:341: error: If condition is always false  [redundant-expr]
+ src/scikit_build_core/_vendor/pyproject_metadata/pyproject.py:351: error: If condition is always false  [redundant-expr]
+ src/scikit_build_core/_vendor/pyproject_metadata/pyproject.py:383: error: If condition is always false  [redundant-expr]
+ src/scikit_build_core/_vendor/pyproject_metadata/pyproject.py:396: error: If condition is always false  [redundant-expr]
+ src/scikit_build_core/_vendor/pyproject_metadata/pyproject.py:408: error: If condition is always false  [redundant-expr]
+ src/scikit_build_core/_vendor/pyproject_metadata/__init__.py:369: error: If condition is always false  [redundant-expr]
+ src/scikit_build_core/setuptools/build_cmake.py:250: error: If condition is always false  [redundant-expr]
+ src/scikit_build_core/hatch/plugin.py:73: error: If condition is always false  [redundant-expr]
+ src/scikit_build_core/build/__main__.py:45: error: If condition is always true  [redundant-expr]

nox (https://github.com/wntrblm/nox)
+ nox/sessions.py:617: error: If condition is always false  [redundant-expr]

[tyralla]

Ah, yes, I now check for the reachability of the if body's last frame instead to search for a return statement, so that this test now also passes:

def f5(x: Literal[1, 2]) -> int:
    if x == 1:
        return 1
    if x == 2:
        if bool():
            return 2
        else:
            return 3

I think we have now reached a good compromise between detecting real problems and allowing dynamic type and exhaustion checking. Personally, I would prefer more strictness, but the number of primer results is even so very high. (An additional option that provides more strictness, like --redundant-expr-strict, would be too much, wouldn't it?)

---

I looked at a few cases of the various primer hits for xarray, and they all seemed justified.

Two examples related to numpy specialities (cc @jorenham):

def isdtype(dtype: np.dtype[Any] | type[Any]) -> None:
    if isinstance(dtype, np.generic):  #  error: If condition is always false
        ...

def equivalent(first: object, last: object) -> bool:
    result = first == last
    if not isinstance(result, bool):  #  error: If condition is always false
        ...

One example of the many occurrences of non-overlapping None checks (this is typical for the Mypy code base, too):

def combine_locks(locks: Sequence[Lock]) -> Lock:
    """Combine a sequence of locks into a single lock."""
    all_locks: list[Lock] = []
    for lock in locks:
        if isinstance(lock, CombinedLock):
            all_locks.extend(lock.locks)
        elif lock is not None:  #  error: If condition is always true
            all_locks.append(lock)

[jorenham]

Two examples related to numpy specialities (cc @jorenham):

def isdtype(dtype: np.dtype[Any] | type[Any]) -> None:
    if isinstance(dtype, np.generic):  #  error: If condition is always false
        ...

Yea, this is a classic example of confusion about dtypes (numpy.dtype instances) vs scalar types (instances of a concrete subclass of numpy.generic, e.g. numpy.dtype.type()). We hope to be able to eventually get rid of scalar types altogether, but at this point that's more of a wish than it is a plan.

But note that things like np.dtype(float) == np.float64 and np.dtype("float64") == float evaluate to True in numpy (I'm not a fan of this, but at this point it'd probably be too painful to remove). See also https://github.com/numpy/numpy/blob/75e1f93049fd0e418e2bfc31aed61e65a10924ea/numpy/__init__.pyi#L1572-L1576

[tyralla]

Thanks for the feedback, Joren!

I tried to reproduce the strange reachability warnings that occurred in the meantime, but could not. Therefore, nothing suggests that we would introduce a bug when merging this PR. However, we would have to turn off the redundant-expr check for Mypy or adjust the codebase. Most reports I checked are due to non-overlaps with None, and some are more exhaustiveness checking-like and would possibly just require an else: assert_never statement. In principle, fixing them would be straightforward. However, some of these checks even date back to Alore, and it is unclear to me whether they are relevant in extraordinary situations. So, it's time to ask the maintainers for their opinion...

A side note: It would possibly not be too hard to provide a separate PR that helps to avoid redundancy and reachability warnings in such (simple) cases:

def f(x: int) -> None:
    if not isinstance(x, int):
        msg = "type error!"
        raise TypeError(msg)
    ....