Add `comparison-of-constants` checker #6413

omarandlorraine · 2022-04-20T12:51:54Z

Type of Changes

	Type
✓	🐛 Bug fix

Description

checker for catching pointless comparisons between two literals

Closes #6076

The Issue mentions tautological and logical errors. But this check is about comparisons between literals, which is a subset of all possible logical errors.

omarandlorraine · 2022-04-20T12:52:50Z

There are so many stupid commits saying "Merge branch 'PyCQA:main' into main". Does github automagically squash these away when/if the branch eventually gets merged?

omarandlorraine · 2022-04-20T12:53:37Z

I notice that my check only fires once for lines like if "abc" == "abc" != None == True:. Is this a problem?

coveralls · 2022-04-20T13:01:10Z

Pull Request Test Coverage Report for Build 2272482650

11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
10 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.01%) to 95.188%

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/stdlib.py	10	94.4%

Totals
Change from base Build 2266524332:	0.01%
Covered Lines:	15866
Relevant Lines:	16668

💛 - Coveralls

Pierre-Sassoulas · 2022-04-20T14:58:32Z

There are so many stupid commits saying "Merge branch 'PyCQA:main' into main". Does github automagically squash these away when/if the branch eventually gets merged?

Well yes, we can squash all the commit when we merge. But you can also rebase on the latest origin/main to remove the merge commit and cleanup your commits (it's better to not do that once there was a review so reviewer can check the latest commit only). pylint is a very active code base so new commits land on the main branch very often.

omarandlorraine · 2022-04-20T15:27:20Z

The remaining failing tests appear to be because of this kind of thing:

 OutputLine(symbol='comparison-with-itself', lineno=6, column=0, end_lineno=None, end_column=None, object='foo', msg='Redundant comparison - arg == arg', confidence='UNDEFINED')

In particular, I'm concerned about end_line=None and end_column=None. Maybe I broke something, because this affects existing tests as well.

Pierre-Sassoulas

I think this need some functional tests, this could be by removing disable in existing "comparison" functional files, but creating a new file would be nice too.

tests/functional/ext/comparison_placement/misplaced_comparison_constant.py

Pierre-Sassoulas · 2022-04-20T17:39:59Z

The test failing right now needs to add the expected result in functional py file with # [comparison-of-two-literals] on the line where the message should be raised.

omarandlorraine · 2022-04-21T09:36:22Z

I think this need some functional tests, this could be by removing disable in existing "comparison" functional files, but creating a new file would be nice too.

My opinion is it's covered by a3f3e61, what do you think? Is there something missing?

DanielNoord · 2022-04-21T14:28:15Z

I think this need some functional tests, this could be by removing disable in existing "comparison" functional files, but creating a new file would be nice too.

My opinion is it's covered by a3f3e61, what do you think? Is there something missing?

It would be good to have a separate file as well. That makes it easy to see what a message is testing by quickly looking up the message name in the test directory.

We will also need some documentation as discussed in #5953.

Pierre-Sassoulas

Looks good already, it's a very nice first step toward catching all constant comparison !

pylint/checkers/base/comparison_checker.py

tests/functional/a/access/access_to_protected_members.py

tests/functional/c/comparison_of_two_literals.py

omarandlorraine · 2022-04-25T08:40:44Z

As for commit 864c1c1.

The tests are failing because the literal-comparison lint is no longer firing on certain lines in tests/functional/c/comparison_of_constants.py. So I have adjusted the [symbolic-name-for-lint] annotations for this test, but I don't understand why. Does anyone know why this is?

omarandlorraine · 2022-04-25T12:22:56Z

Can anyone here see why "Checks / documentation" has failed?

Pierre-Sassoulas · 2022-04-25T13:04:27Z

@omarandlorraine the bad.py file in the documentation does not raise comparison-of-two-literals

doc/data/messages/c/comparison-of-two-literals/bad.py

jacobtylerwalls · 2022-04-25T13:16:10Z

Was there any thought given to reusing using-constant-test for this?

omarandlorraine · 2022-04-25T13:18:39Z

Was there any thought given to reusing using-constant-test for this?

No, I wasn't even aware of it.

omarandlorraine · 2022-04-25T14:27:15Z

Okay, the tests have passed, and I think it is ready to be merged in. Do you agree?

Pierre-Sassoulas · 2022-04-25T14:37:19Z

I'm sorry @omarandlorraine I did not remember that we already had a very close warnings for that kind of problem. Thank you Jacob for noticing.

I think creating a new message makes sense because the old one is a constant as in if 1: and the new one is a little different with 1 == 2 with two constants. We could merge them or normalize the name, maybe using-constant-comparison-tests ? We have to keep in mind that the idea is to maybe extends the check later to infer that a check is between two constants so we need to anticipate that. What do you think @DanielNoord @jacobtylerwalls ?

jacobtylerwalls · 2022-04-25T15:25:30Z

I was just wondering, but now after thinking a bit, I take it back. I see now that using-constant-test doesn't inspect a comparison, and if it did someday, it would need to do exactly what this check is doing, so maybe the current approach is fine.

tests/functional/u/use/use_implicit_booleaness_not_len.py

pylint/checkers/base/comparison_checker.py

tests/functional/u/use/use_implicit_booleaness_not_len.py

doc/data/messages/c/comparison-of-constants/bad.py

doc/data/messages/c/comparison-of-constants/good.py

Pierre-Sassoulas

Looking good. Sorry for the message change, it will require an upgrade of the result of the functional tests. Let's hear about what other think of it before changing it again. This is user facing, the clearer it is the better.

pylint/checkers/base/comparison_checker.py

Pierre-Sassoulas · 2022-05-03T17:31:12Z

what can I do to unconflict?

Generally in the changelog you should keep both entry (inserting your own changelog randomly help, as conflicts happens a lot more for the first entry and last entry). Regarding the other conflict it's easier to resolve with a global vision of the other MR so I did it :) The change came from #6492

Pierre-Sassoulas · 2022-05-03T17:34:18Z

Generally to avoid the problem you could use git fetch -a to have the latest main branch from upstream after your rebase, and do the rebase again (it's easier once you fixed the conflict for the first rebase).

Pierre-Sassoulas

The code itself looks good,great even, thank you for your contribution @omarandlorraine.

Regarding the message description and name and its consistency with other pylint message, I think there's two possible solutions:

We suppress comparison-of-constants if a less generic one would be raised 🚀
We raise two messages, after all one fix will remove both... 👀

I think suppression message in some context would bring complexity. What happens if the less generic message is disabled for example, should we still suppress comparison-of-constants ?

Let me know what you think of the two options and if something else could be done, then I think we should vote on the best solution to merge this before yet another change in main create a conflict 😄

tests/functional/c/comparison_of_constants.py

DanielNoord

Thanks for fixing the merge @omarandlorraine!

Sorry if my comments seem a bit much, that sort of thing happens naturally with larger PRs like these. (152 lines is quite a lot for our project).

To explain two seemingly nitpicky recurring comments:

Adding disables at the top of the file is helpful for later contributors as they don't need to add in-line disables for the same problem. Besides, they tend to keep the testable code clean.
Adding double spaces between comments and codes helps contributors who have set up their IDE to auto-format on save. My IDE auto-formats 95% of the code to this project's style. Normally such deviations would be caught by pre-commit, but some of the test files are excluded. However, when I (or somebody else) ever revisits these tests they won't be able to easily save without reformatting the complete file. Sticking to the project's style in tests even without pre-commit is therefore useful for many contributors/reviewers and increases their productivity. It's a bit nitpicky now, but would be much appreciated 😄

doc/whatsnew/2.14.rst

ChangeLog

DanielNoord · 2022-05-04T07:08:44Z

pylint/checkers/base/comparison_checker.py

@@ -58,6 +59,13 @@ class ComparisonChecker(_BasicChecker):
            "comparison-with-itself",
            "Used when something is compared against itself.",
        ),
+        "R0125": (
+            "Comparison between constants: '%s %s %s' is also a constant, remove the comparison and consider a refactor",


Suggested change

"Comparison between constants: '%s %s %s' is also a constant, remove the comparison and consider a refactor",

"Comparison between constants: '%s %s %s' has a constant value",

The R in R0125 implies that the code should be refactored so I think we can leave that out of the message here.

DanielNoord · 2022-05-04T07:09:22Z

pylint/checkers/base/comparison_checker.py

+            "When two literals are compared with each other the result is a constant, "
+            "and using the constant directly is both easier to read and more performant."
+            " Initializing 'True' and 'False' this way is not required since python 2.3",


Suggested change

"When two literals are compared with each other the result is a constant, "

"and using the constant directly is both easier to read and more performant."

" Initializing 'True' and 'False' this way is not required since python 2.3",

"When two literals are compared with each other the result is a constant. "

"Using the constant directly is both easier to read and more performant. "

"Initializing 'True' and 'False' this way is not required since Python 2.3.",

pylint/checkers/base/comparison_checker.py

DanielNoord · 2022-05-04T07:16:35Z

tests/functional/l/logical_tautology.py

@@ -11,21 +11,21 @@ def foo():
        return True
    elif arg <= arg: # [comparison-with-itself]
        return True
-    elif None == None: # [comparison-with-itself]
+    elif None == None: # [comparison-of-constants, comparison-with-itself]


As we're updating these comments as well, could you add the double spaces here?

I'll try using the autoformatter locally on my machine.

Did this work?

tests/functional/s/superfluous_parens.py

tests/functional/u/use/use_implicit_booleaness_not_len.py

tests/functional/u/using_constant_test.py

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

omarandlorraine · 2022-05-04T07:30:56Z

Thanks for fixing the merge @omarandlorraine!

No problem, thanks also to you @DanielNoord for the technical help with git. Thanks also to @Pierre-Sassoulas for cleaning up the fiddly bits

omarandlorraine · 2022-05-04T07:35:53Z

The code itself looks good,great even, thank you for your contribution @omarandlorraine.

thanks for your kind words.

* We raise two messages, after all one fix will remove both... 👀
Let me know what you think of the two options and if something else could be done, then I think we should vote on the best solution to merge this before yet another change in main create a conflict 😄

I voted eyeballs because I don't see a problem with raising two messages for one bad line of code. But I might change my vote if someone can tell my why it could be a problem (maybe it could be a UX concern, I don't know?)

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

omarandlorraine · 2022-05-04T07:47:15Z

Oh pants! You see we've got two commits each called "Whitespace" and "period at the end of changelog entry". I tried to squash those into 31327ef and
8a363d7.

But it seems to have gone wrong

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

DanielNoord · 2022-05-04T07:49:48Z

Oh pants! You see we've got two commits each called "Whitespace" and "period at the end of changelog entry". I tried to squash those into 31327ef and 8a363d7.

But it seems to have gone wrong

What you might want to do is commit batch. If you go to the Files Changes tab you can add all comments with suggestions to one batch and then commit at once. Since the comments are made by reviewers I doesn't matter too much if the commit name is not descriptive. We'll likely squash merge anyway.
Another benefit is that this will only run the CI once instead of for each commit which will save us some valuable CI runner time 😄

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

omarandlorraine · 2022-05-04T09:12:59Z

I've stuffed up again 😱

Now some tests are failing in CI, even though (some of) these pass locally.

I don't think whatever code the CI task runs is always the same as what I've got.

Pierre-Sassoulas · 2022-05-04T09:17:51Z

Now some tests are failing in CI, even though (some of) these pass locally.

The information for the line/column in functional tests come from the AST module that we do not control. We're also testing for python 3.8 to 3.10 (The AST got more stable in 3.8). If there's a discrepancy between interpreter for the line/column information coming from the AST we need to examine closely what interpreter changed and take a decision to exclude some interpreter from the check in a generic file or to separate the check by interpreter.

DanielNoord · 2022-05-04T09:18:14Z

It seems the expected output for the comparison_of_constants test is incorrect. That should be easily fixable!

DanielNoord

Some last comments about spacing, most have now been fixed. Note that there are also some open comments in pylint/checkers/base/comparison_checker.py

doc/data/messages/c/comparison-of-constants/bad.py

DanielNoord · 2022-05-04T12:31:11Z

tests/functional/a/assert_on_tuple.py

@@ -1,6 +1,6 @@
 '''Assert check example'''

-# pylint: disable=comparison-with-itself
+# pylint: disable=comparison-with-itself comparison-of-constants


Suggested change

# pylint: disable=comparison-with-itself comparison-of-constants

# pylint: disable=comparison-with-itself, comparison-of-constants

DanielNoord · 2022-05-04T12:31:34Z

tests/functional/c/comparison_of_constants.py

+# pylint: disable=missing-docstring, comparison-with-itself
+
+
+if 2 is 2: # [literal-comparison, comparison-of-constants]


Note that this file still needs to be updated for spaces.

DanielNoord · 2022-05-04T12:31:46Z

tests/functional/l/logical_tautology.py

@@ -11,21 +11,21 @@ def foo():
        return True
    elif arg <= arg: # [comparison-with-itself]
        return True
-    elif None == None: # [comparison-with-itself]
+    elif None == None: # [comparison-of-constants, comparison-with-itself]


Did this work?

DanielNoord · 2022-05-04T12:31:57Z

tests/functional/u/useless/useless_else_on_loop.py

@@ -62,9 +62,9 @@ def test_break_in_orelse_deep():
    """no false positive for break in else deeply nested
    """
    for _ in range(10):
-        if 1 < 2:
+        if 1 < 2: # pylint: disable=comparison-of-constants


This file is also missing double spacing.

black doesn't seem to mind about this issue; even if I do pre-commit run --all-files, the double space doesn't get put in. Does this depend on some kind of configuration, or?

No, pre-commit sadly won't fix this for us. We explicitly exclude these test files from pre-commit: for example, sometimes we want to test that even if there are mistakes our syntactic errors in code pylint exits gracefully instead of crashing. We don't want pre-commit to warn us about such "mistakes".

Thus, keeping style in these files is a little harder. Many of the contributors and reviewers have set up their IDE to also run some formatters whenever they save a file. For example, I run black on save together with an import sorter. This helps keep the style in our test files somewhat similar. However, when people with such "on-save-formatters" look at these test files and make a change their formatters will also change stuff unrelated to the issue they are working on, as the formatter might start sorting imports or formatting lines. So, keeping the general style in our test files not only is a stylistic choice but also helps keep some of our contributors more productive. It's a bit annoying that we need to do this by hand, especially for larger PRs such as this one, but in the long run it tends to make us all more productive 😄

Oh I get it now. pre-commit won't do this, by deliberate design. But of course we I could invoke black on those files ourselves.

Anyway I've done it manually

Those file are sometime not black formatted by design, if we run black everywhere we won't have any tests for single quote strings for example. I think we should go easy on the normalization, we want to also catch bugs for users that do not use black or isort or do not respect pep8 to the letter, it's a good thing if the functional tests are somewhat messy.

Pierre-Sassoulas

LGTM, once the point raised by Daniel are resolved ! Thank you for taking a lof of remarks into account already @omarandlorraine.

DanielNoord · 2022-05-04T21:54:14Z

I added the last changes myself. Thanks for your work here @omarandlorraine and congratulations on writing your first new checker! 😄

jacobtylerwalls · 2022-05-04T22:19:46Z

Yes thanks very much @omarandlorraine !

omarandlorraine · 2022-05-05T05:31:37Z

Oh cool!

Thanks also to everyone for making this a good experience for me.

Pierre-Sassoulas added this to the 2.15.0 milestone Apr 20, 2022

Pierre-Sassoulas requested changes Apr 20, 2022

View reviewed changes

tests/functional/ext/comparison_placement/misplaced_comparison_constant.py Outdated Show resolved Hide resolved

Pierre-Sassoulas added Enhancement ✨ Improvement to a component Checkers Related to a checker labels Apr 20, 2022

omarandlorraine requested a review from Pierre-Sassoulas April 22, 2022 15:17

Pierre-Sassoulas requested changes Apr 22, 2022

View reviewed changes

Pierre-Sassoulas reviewed Apr 25, 2022

View reviewed changes

doc/data/messages/c/comparison-of-two-literals/bad.py Outdated Show resolved Hide resolved

jacobtylerwalls reviewed Apr 25, 2022

View reviewed changes

tests/functional/u/use/use_implicit_booleaness_not_len.py Outdated Show resolved Hide resolved

Pierre-Sassoulas requested changes Apr 25, 2022

View reviewed changes

pylint/checkers/base/comparison_checker.py Show resolved Hide resolved

pylint/checkers/base/comparison_checker.py Outdated Show resolved Hide resolved

tests/functional/u/use/use_implicit_booleaness_not_len.py Outdated Show resolved Hide resolved

Pierre-Sassoulas reviewed Apr 26, 2022

View reviewed changes

doc/data/messages/c/comparison-of-constants/bad.py Outdated Show resolved Hide resolved

doc/data/messages/c/comparison-of-constants/good.py Outdated Show resolved Hide resolved

Pierre-Sassoulas requested changes Apr 27, 2022

View reviewed changes

pylint/checkers/base/comparison_checker.py Outdated Show resolved Hide resolved

Pierre-Sassoulas modified the milestones: 2.15.0, 2.14.0 Apr 27, 2022

jacobtylerwalls changed the title ~~Logic checks~~ Add comparison-of-constants checker Apr 27, 2022

Pierre-Sassoulas reviewed May 3, 2022

View reviewed changes

tests/functional/c/comparison_of_constants.py Outdated Show resolved Hide resolved

tests/functional/c/comparison_of_constants.py Outdated Show resolved Hide resolved

DanielNoord requested changes May 4, 2022

View reviewed changes

Performance adjustment

72730a5

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

omarandlorraine and others added 2 commits May 4, 2022 08:44

Whitespace

31327ef

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

period at the end of changelog entry

8a363d7

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

omarandlorraine force-pushed the logic_checks branch from c1afcbc to 8a363d7 Compare May 4, 2022 07:44

Call it R0133, not R0125

c8500f0

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

omarandlorraine and others added 3 commits May 4, 2022 09:11

Apply whitespace suggestions

1ae78f4

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

Apply suggestions from code review

4da21c9

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

correct expected output for some tests

23abb00

disable invalid-name lint for comparison_of_constants check

336f8c3

DanielNoord requested changes May 4, 2022

View reviewed changes

Sam M W added 2 commits May 4, 2022 15:14

spaces before comments

b0d4fa7

spaces before comments

a24f94a

Pierre-Sassoulas approved these changes May 4, 2022

View reviewed changes

Last fixes

72a9e20

DanielNoord approved these changes May 4, 2022

View reviewed changes

DanielNoord merged commit 6486514 into pylint-dev:main May 4, 2022

	"Comparison between constants: '%s %s %s' is also a constant, remove the comparison and consider a refactor",
	"Comparison between constants: '%s %s %s' has a constant value",

	# pylint: disable=comparison-with-itself comparison-of-constants
	# pylint: disable=comparison-with-itself, comparison-of-constants

		# pylint: disable=missing-docstring, comparison-with-itself


		if 2 is 2: # [literal-comparison, comparison-of-constants]

Add comparison-of-constants checker #6413

Add comparison-of-constants checker #6413

Conversation

omarandlorraine commented Apr 20, 2022

Type of Changes

Description

omarandlorraine commented Apr 20, 2022 • edited

omarandlorraine commented Apr 20, 2022

coveralls commented Apr 20, 2022 • edited

Pull Request Test Coverage Report for Build 2272482650

💛 - Coveralls

Pierre-Sassoulas commented Apr 20, 2022

omarandlorraine commented Apr 20, 2022

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas commented Apr 20, 2022

omarandlorraine commented Apr 21, 2022

DanielNoord commented Apr 21, 2022 • edited

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

omarandlorraine commented Apr 25, 2022

omarandlorraine commented Apr 25, 2022

Pierre-Sassoulas commented Apr 25, 2022

jacobtylerwalls commented Apr 25, 2022

omarandlorraine commented Apr 25, 2022

omarandlorraine commented Apr 25, 2022

Pierre-Sassoulas commented Apr 25, 2022

jacobtylerwalls commented Apr 25, 2022

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas commented May 3, 2022

Pierre-Sassoulas commented May 3, 2022 • edited

Pierre-Sassoulas left a comment • edited

Choose a reason for hiding this comment

DanielNoord left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarandlorraine commented May 4, 2022

omarandlorraine commented May 4, 2022 • edited

omarandlorraine commented May 4, 2022

DanielNoord commented May 4, 2022

omarandlorraine commented May 4, 2022

Pierre-Sassoulas commented May 4, 2022 • edited

DanielNoord commented May 4, 2022

DanielNoord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord commented May 4, 2022

jacobtylerwalls commented May 4, 2022

omarandlorraine commented May 5, 2022

Add `comparison-of-constants` checker #6413

Add `comparison-of-constants` checker #6413

omarandlorraine commented Apr 20, 2022 •

edited

coveralls commented Apr 20, 2022 •

edited

DanielNoord commented Apr 21, 2022 •

edited

Pierre-Sassoulas commented May 3, 2022 •

edited

Pierre-Sassoulas left a comment •

edited

DanielNoord left a comment •

edited

omarandlorraine commented May 4, 2022 •

edited

Pierre-Sassoulas commented May 4, 2022 •

edited