Add new checker bad-chained-comparison

[zenlyj]

Type of Changes

	Type
	🐛 Bug fix
✓	✨ New feature
	🔨 Refactoring
✓	📜 Docs

Description

As suggested by the issue, a default checker is added to warn users about potential logical mistakes from a chained comparison consisting of comparisons that belong to different semantic categories/groups. Please take a look at the referenced issue as this implementation is pretty much a 1-1 of the solution suggested there.

Any advice is greatly appreciated!

Closes #6559

[Pierre-Sassoulas]

The new message in primer seems genuine ! Amazing !

[Pierre-Sassoulas]

This looks pretty good already, thank you !

[nickdrozd]

I think it would be reasonable to enable this by default and not make it an extension. The one primer result appears to be a full-on error and my guess is that most hits would be also be errors.

I know we try to avoid false positives, but on the other hand, surfacing latent errors helps to highlight the value of the tool.

[zenlyj]

I think it would be reasonable to enable this by default and not make it an extension. The one primer result appears to be a full-on error and my guess is that most hits would be also be errors.

I know we try to avoid false positives, but on the other hand, surfacing latent errors helps to highlight the value of the tool.

I agree, I feel that most hits would most likely be errors. I don't think there are many scenarios that warrant such confusing chained comparisons. But ultimately, it depends on how tolerant we are towards false positives.

Any thoughts on this? @Pierre-Sassoulas

[codecov]

Codecov Report

Merging #7990 (05ae84a) into main (259b3d7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7990   +/-   ##
=======================================
  Coverage   95.42%   95.43%           
=======================================
  Files         176      177    +1     
  Lines       18521    18545   +24     
=======================================
+ Hits        17674    17698   +24     
  Misses        847      847           

Impacted Files	Coverage Δ
pylint/checkers/bad_chained_comparison.py	100.00% <100.00%> (ø)

[Pierre-Sassoulas]

I agree with @nickdrozd, as evidenced by the primer result this is a great check. It's not that often that we discover a real world bug in the primers with a 0% false positive rate from the start. In fact it never happened before. If we want to make this a default check we just have to move it to pylint/checker, please keep the code independent :)

I suggested some wording change and a shorter message with more details, let me know what you think. (I did not review the logic again)

[Pierre-Sassoulas]

I think it's a pretty nice checker, great job @zenlyj. Let's wait for another maintainer review, this is a default check after all, mistakes would affect a lot of users.

[Pierre-Sassoulas]

I have a really hard time to understand the pre-commit error. I can reproduce locally but I can't seem to fix it.

[zenlyj]

@Pierre-Sassoulas it seems that the pre-commit error is due to the files having Windows style line breaks. It should be fixed now, sorry for the trouble!

Regarding the documentation check fail, is this expected for new documentation pages?

[github-actions]

🤖 Effect of this PR on checked open source code: 🤖

Effect on sentry:
The following messages are now emitted:

1. bad-chained-comparison:
   Suspicious 2-part chained comparison using semantically incompatible operators ('in' and 'is not')
   https://github.com/getsentry/sentry/blob/6d6c0bd9b8125d554f966da1bb0b70fcab601c27/src/sentry/api/endpoints/project_details.py#L550

This comment was generated for commit 05ae84a

[Pierre-Sassoulas]

The documentation check is expected to fail as long as it's not merged in main and in the official doc.

[Pierre-Sassoulas]

First check that not only did not have a single primer false positives but even detected a real bug to fix in a very popular well scrutinized project. It already detected and permitted to fix a bug in sentry, which is pretty impressive ! Great checker, amazing job 👌