New check `simplifiable-condition` and add bool() check for `len-as-condition` #3644

ethan-leba · 2020-05-24T14:18:37Z

Steps

Add yourself to CONTRIBUTORS if you are a new contributor.
Add a ChangeLog entry describing what your PR does.
If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
Write a good description on what the PR does.

Description

Added check for bool(len(x)) to len-as-condition.
Added simplifiable-condition check to the refactoring checker, which will suggest removing constants that have no effect on a boolean condition, i.e. False or x -> x, and replacing conditions with a constant if there is a shortcircuiting value, i.e. True or x -> True.

Type of Changes

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

Related Issue

Closes #3407

coveralls · 2020-05-24T14:25:02Z

Coverage increased (+0.03%) to 90.726% when pulling 7dd2ae5 on ethan-leba:simplifiable-condition into 21d168a on PyCQA:master.

PCManticore · 2020-06-08T06:51:18Z

Hey @ethan-leba Thanks for the PR. Can you split this into three separate PRs? As it stands now, it includes three changes that are easier to review separately.

ethan-leba · 2020-06-08T12:05:22Z

Definitely agree this PR is a little overloaded, but the reason it ended up like that is because the main commit containing the checker depends on refactoring changes made in the first. I removed the last commit, and will PR it separately if/when the other changes get in, but I think the first two commits are too coupled to split into separate PRs.

PCManticore

Hey @ethan-leba Looking great, thanks for the work. Left several comments we should check out before we can pull it in.

pylint/checkers/refactoring.py

PCManticore · 2020-06-09T05:58:39Z

tests/functional/s/simplifiable_condition.py

+assert False and False # [simplifiable-condition]
+
+# Expressions not in one of the above situations will not emit a message
+CONSTANT or True


Let's add a test for something that is not uninferable:

Suggested change

CONSTANT or True

from unknown import Unknown

bool(Unknown or True) # should not emit

Shouldn't bool(Unknown or True) emit an error, as it can be simplified to True?

Not quite, as we do not know what Unknown actually is at runtime.

But in this case the value of Unknown shouldn't matter:

>>> Unknown = False >>> bool(Unknown or True) True >>> Unknown = True >>> bool(Unknown or True) True >>> Unknown = 123 >>> bool(Unknown or True) True

The reason why CONSTANT or True in the test-case above doesn't emit an error is b/c it's not in a condition, so the value of CONSTANT affects the result, for example:

>>> CONSTANT = 0 >>> CONSTANT or True True >>> CONSTANT = -134 >>> CONSTANT or True -134

Just wanted to point that out in case that was the source of confusion!

Can I move forward with splitting the checker, or should I wait for PCManticore's input on this? I don't think it should be too difficult.

Yeah I think you can go on and do it. Looking at @PCManticore profile he seems to be quite busy with "not gihub thing" lately and he's probably spammed with other github issues anyway. With two positives review + this comment I'm confident enough to merge myself.

Okay, sounds good! Just one question about suspicious-boolean-condition, should this checker target single constants in conditionals, i.e. if True: or bool(True)? Currently this checker would only emit a message if the expression can be reduced to a single constant, but not if the expression is a single constant.

Hmm, yes probably. If it's really hard to disentangle the code, we can probably create an issue to do it separately.

using-constant-test should cover the case of single constants mentioned above

tests/functional/s/simplifiable_condition.py

pylint/checkers/utils.py

ethan-leba · 2020-06-09T22:01:45Z

Thanks for the review @PCManticore! Revised the PR, just left one question above

Pierre-Sassoulas

Very nice feature!

pylint/checkers/refactoring.py

ethan-leba · 2020-08-08T20:49:01Z

Anything else that needs to be done on my part to move this PR forward @PCManticore ?

Pierre-Sassoulas · 2020-08-09T07:59:28Z

It seems like there is an ongoing discussion about the message of the recommendation that could be made better, but I'll let @PCManticore confirm if I understood correctly when he's available. In the meantime could you rebase on the latest master, please?

…actoring checker

ethan-leba · 2020-08-30T18:50:01Z

I've split off simplification to a constant to a separate check: condition-evals-to-constant. LMK if anything else needs to be done 👍

Pierre-Sassoulas

This look clean and well tested, thank you ! But I wonder what is the difference between simplifiable condition and condition eval to constant. Because the two functional test files look the same (with False and True inverted) and gives a very different result. Why is bool(CONSTANT or False) # [simplifiable-condition] and bool(CONSTANT or True) # [condition-evals-to-constant] ? What is the difference between the two ?

ethan-leba · 2020-08-31T12:42:50Z

simplifiable-condition is when constants can be removed from an and/or in a condition, condition-evals-to-constant is when an and/or can be simplified in a way that it no longer contains any variables. We discussed this in this thread: #3644 (comment)

CONSTANT or False -> CONSTANT
CONSTANT or True -> True

Pierre-Sassoulas · 2020-08-31T19:16:18Z

Sorry, it makes sense (again) now. Thank you for explaining it another time and thank you for this very nice addition to Pylint. Simplifying boolean conditions is always very satisfying and useful.

ethan-leba force-pushed the simplifiable-condition branch from f50bc26 to 413dbf8 Compare May 24, 2020 15:44

ethan-leba marked this pull request as ready for review May 24, 2020 15:59

ethan-leba force-pushed the simplifiable-condition branch 2 times, most recently from 961974b to 748657d Compare June 8, 2020 12:05

PCManticore approved these changes Jun 9, 2020

View reviewed changes

ethan-leba changed the title ~~New check simplifiable-condition, add bool() check for len-as-condition, and contextual messages for singleton-comparison~~ New check simplifiable-condition and add bool() check for len-as-condition Jun 9, 2020

ethan-leba force-pushed the simplifiable-condition branch from 748657d to 93f9491 Compare June 9, 2020 21:39

ethan-leba force-pushed the simplifiable-condition branch 2 times, most recently from f322004 to a42dd91 Compare June 12, 2020 00:25

Pierre-Sassoulas approved these changes Jun 13, 2020

View reviewed changes

pylint/checkers/refactoring.py Show resolved Hide resolved

ethan-leba force-pushed the simplifiable-condition branch from a42dd91 to b083152 Compare August 9, 2020 13:05

Add bool() check for len-as-condition

aca29ac

ethan-leba force-pushed the simplifiable-condition branch 3 times, most recently from 8eac6d4 to 4c28ea0 Compare August 30, 2020 18:31

ethan-leba requested a review from Pierre-Sassoulas August 30, 2020 18:46

Add simplifiable-condition and condition-evals-to-constant to the ref…

7dd2ae5

…actoring checker

ethan-leba force-pushed the simplifiable-condition branch from 4c28ea0 to 7dd2ae5 Compare August 30, 2020 18:49

Pierre-Sassoulas reviewed Aug 31, 2020

View reviewed changes

Pierre-Sassoulas merged commit 5b68441 into pylint-dev:master Aug 31, 2020

ethan-leba mentioned this pull request Sep 22, 2020

Improve singleton-comparison suggestion message #3820

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New check `simplifiable-condition` and add bool() check for `len-as-condition` #3644

New check `simplifiable-condition` and add bool() check for `len-as-condition` #3644

ethan-leba commented May 24, 2020 •

edited

Loading

coveralls commented May 24, 2020 •

edited

Loading

PCManticore commented Jun 8, 2020

ethan-leba commented Jun 8, 2020

PCManticore left a comment

PCManticore Jun 9, 2020

ethan-leba Jun 9, 2020

PCManticore Jun 10, 2020

ethan-leba Jun 10, 2020

ethan-leba Jun 10, 2020

ethan-leba Aug 9, 2020 •

edited

Loading

Pierre-Sassoulas Aug 9, 2020

ethan-leba Aug 9, 2020

Pierre-Sassoulas Aug 9, 2020

ethan-leba Aug 30, 2020

ethan-leba commented Jun 9, 2020

Pierre-Sassoulas left a comment

ethan-leba commented Aug 8, 2020

Pierre-Sassoulas commented Aug 9, 2020

ethan-leba commented Aug 30, 2020

Pierre-Sassoulas left a comment

ethan-leba commented Aug 31, 2020

Pierre-Sassoulas commented Aug 31, 2020

	CONSTANT or True
	from unknown import Unknown
	bool(Unknown or True) # should not emit

New check simplifiable-condition and add bool() check for len-as-condition #3644

New check simplifiable-condition and add bool() check for len-as-condition #3644

Conversation

ethan-leba commented May 24, 2020 • edited Loading

Steps

Description

Type of Changes

Related Issue

coveralls commented May 24, 2020 • edited Loading

PCManticore commented Jun 8, 2020

ethan-leba commented Jun 8, 2020

PCManticore 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

ethan-leba Aug 9, 2020 • edited Loading

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

ethan-leba commented Jun 9, 2020

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

ethan-leba commented Aug 8, 2020

Pierre-Sassoulas commented Aug 9, 2020

ethan-leba commented Aug 30, 2020

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

ethan-leba commented Aug 31, 2020

Pierre-Sassoulas commented Aug 31, 2020

New check `simplifiable-condition` and add bool() check for `len-as-condition` #3644

New check `simplifiable-condition` and add bool() check for `len-as-condition` #3644

ethan-leba commented May 24, 2020 •

edited

Loading

coveralls commented May 24, 2020 •

edited

Loading

ethan-leba Aug 9, 2020 •

edited

Loading