[Feature Request] Tautological comparison warning

[fleimgruber]

Consider a specimen of the form

def truism(a: str, b: str) -> bool:
    b = b + "abc"
    return a.replace(" ", "").lower() == a.replace(" ", "").lower()

which slipped code review, it should read: a.replace(" ", "").lower() == b.replace(" ", "").lower(). The general case of return a == a for built-in types is what C++ compilers call "tautological-compare" warning. Of course this is trivial to catch in a unit-test, but could it be similarly trivial to catch it in beartype?

[leycec]

Fascinating... Type-checking failures on noop self-comparisons, huh? That's an intriguing use case you've rustled up. To be sure, you have my attention.

In theory, all things are possible through @beartype. In practice, @beartype has yet to do this thing. Moreover, doing this thing could prove non-trivial. I've never even contemplated it, honestly; to my knowledge, neither has anybody else. I'm unclear whether pure-static linters like ruff and pylint or pure-static type-checkers like mypy and pyright even detect this. They... probably don't. They probably should, though. That's decidedly an error. We can all agree here.

Cogitate and Listen: @leycec Back with a Brand New Vision

Back to tedious theory. As a hybrid runtime-static type-checker, ^{...in beartype_this_package() mode, anyway} @beartype is theoretically capable of type-checking pretty much anything. In this case, this can probably be statically type-checked during the abstract syntax tree (AST) transformations applied by @beartype.

Having briefly researched this for a hot five minutes just now, I strongly suspect that @beartype can do this without too much intestinal grief on my part. Notably, as instructions to my future self five years down the roadmap (where I am even balder and more addled with chronic self-doubt), @beartype should:

• In our existing private beartype.roar._roarwarn submodule:
  • Define a new public BeartypeClawAstTautologicalCompareWarning subclass.
• In our existing private beartype._util.ast.utilasttest submodule:
  • Define a new is_nodes_equal() utility function strongly inspired by this StackOverflow post. Interestingly, detecting whether two AST nodes semantically represent the same Python logic is non-trivial (but feasible) via the terrifying power of recursion:

from ast import AST
from itertools import zip_longest
from beartype.typing import (
    List,
    Union,
)

Node = Union[AST, List[AST]]

def is_nodes_equal(node1: Node, node2: Node) -> bool:
    if type(node1) is not type(node2):
        return False

    if isinstance(node1, AST):
        for k, v in vars(node1).items():
            if k in {"lineno", "end_lineno", "col_offset", "end_col_offset", "ctx"}:
                continue
            if not is_nodes_equal(v, getattr(node2, k)):
                return False
        return True
    elif isinstance(node1, list) and isinstance(node2, list):
        return all(is_nodes_equal(n1, n2) for n1, n2 in zip_longest(node1, node2))

    return node1 == node2

• In our existing private beartype.claw._ast.clawastmain.BeartypeNodeTransformer subclass:
  • Define a new visit_Compare() method resembling:

from beartype.roar import BeartypeClawAstTautologicalCompareWarning
from beartype._util.ast.utilasttest import is_nodes_equal
from warnings import warn
...

class BeartypeNodeTransformer(NodeTransformer):
    ...

    def visit_Compare(self, node: Compare) -> Compare:
        node_curr = node.left

        for node_next in node.comparators:
            if is_nodes_equal(node_curr, node_next):
                warn(
                    #FIXME: Improve warning to also emit a human-readable
                    #description of the current line number and line, please.
                    #Please, Gods! We beg thee! Somebody do all this for us!
                    (
                        f'Tautological (i.e., useless) comparison between '
                        f'objects {repr(node_curr)} and {repr(node_next)} '
                        f'detected. It is bad. We feel sad. So should you.'
                    ),
                    BeartypeClawAstTautologicalCompareWarning,
                )

            node_curr = node_next

Pretty sure that should suffice. Probably? Possibly? Maybe? Okay, I have no idea whatsoever. Let's pretend that works and call it a Tuesday, gentlemen!

Thanks so much for the intriguing thought experiment. You even tagged this as a [Feature Request] in the title and leveraged classic CompSci jargon like "tautology." I wish I saw that word more than I do. It fills my cat emoji with sparkly goodness. 😻

^{P.S.: I dutifully ping @langfield, who goes in for this sort of hardcore type theory. Masochists, unite.}