pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.29k stars 1.13k forks source link

Check for circular comparisons and other comparison improvements #5814

Open areveny opened 2 years ago

areveny commented 2 years ago

Current problem

a = 1
b = 2

if a > b and b > a:
    pass

compare.py:4:3: R1716: Simplify chained comparison between the operands (chained-comparison)

While it's nice some message that points to the error, it's not quite correct.

Desired solution

Detect circular comparisons which simplify to False.

This is a graph pattern is solved with a topological sort. The main risks are the performance overhead for building the graph, which should be small because the number of statements in any given if is usually low. There is also the complexity risk of having a graph and associated algorithms implementation embedded in the code.

Having the graph could open up the following:

Could see if we can suggest simplifying if a >= b and b <= a: to if a == b: but this could be more difficult. There might also be something we can do with or statements.

Building the graph would also allow us to pick up situations like

    elif a > 1 and a > 10:
        pass
    elif a < 100 and a < 10:
        pass

which currently do not emit.

This issue came up while thinking about #5800. It I'm not immediately sure how to suggest the refactor with the current implementation. Building this graph might more readily suggests the refactor.

We could modify chained-comparison to detect comparison cycles and do #5800. But unless there are objections I want to try the graph implementation because of the benefits described above.

areveny commented 2 years ago

Thanks for merging. I have the core logic of this assembled, I'll finish up the documentation and add a PR now that we're on 2.14.

Pierre-Sassoulas commented 2 years ago

@areveny do you still want to handle this ?

areveny commented 2 years ago

Yes! Thank you for checking. I have been sitting on this for quite a while. I will make time to get the PR in.

Pierre-Sassoulas commented 2 years ago

No rush, I was just checking if I should clear the assignment :)