Skip to content

Add improve-conditionals check in the CodeStyle extension #10600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add improve-conditionals check in the CodeStyle extension #10600

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
30e24c0
Add improve-conditionals check in the CodeStyle extension
cdce8p Sep 29, 2025
923bd4c
Update pylint conditionals
cdce8p Sep 29, 2025
3e0fda5
Fix docs
cdce8p Sep 29, 2025
b08beb4
Don't emit message for 'x < 0 or x > 100'
cdce8p Oct 1, 2025
7e4bb03
Code review
cdce8p Oct 1, 2025
87e5786
Enable improve-conditionals check for pylint itself
cdce8p Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/data/messages/i/improve-conditionals/bad.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
def func(expr, node_cls):
# +1:[improve-conditionals]
if not isinstance(expr, node_cls) or expr.attrname != "__init__":
...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def func(expr, node_cls):
# +1:[improve-conditionals]
if not isinstance(expr, node_cls) or expr.attrname != "__init__":
...
def is_platypus(animal):
# The platypus is both the only mammal with a beak and without nipples.
# +1:[improve-conditionals]
return animal.is_mammal() and (not animal.has_nipples() or animal.has_beak)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your example wouldn't actually raise the message. Yes, it could be inverted but so far I've chosen not to emit it if we'd need to add not, only if it could be removed / moved before the BoolOp. I.e. it would only be emitted for

# bad
x and (not y or not z)

# good
x and not (y and z)

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas Sep 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok the checker is a lot less opinionated than I thought then : good.

I had to do some extensive phenology for this one, turn out uniquely identifying an animal by lack of a characteristic is not so easy.

Suggested change
def func(expr, node_cls):
# +1:[improve-conditionals]
if not isinstance(expr, node_cls) or expr.attrname != "__init__":
...
def is_penguin(animal):
# Penguins are the only flightless, kneeless sea birds
# +1:[improve-conditionals]
return animal.is_seabird() and (not animal.can_fly() or not animal.has_visible_knee())

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok the checker is a lot less opinionated than I thought then : good.

I went through all of the warning for Home Assistant. They looked quite good. There was just one common pattern I don't think the checker should emit a warning for.

# is x between 0 and 100
if x > 0 or x < 100:
    ...

Addressed that one in b08beb4

3 changes: 3 additions & 0 deletions doc/data/messages/i/improve-conditionals/good.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
def func(expr, node_cls):
if not (isinstance(expr, node_cls) and expr.attrname == "__init__"):
...
2 changes: 2 additions & 0 deletions doc/data/messages/i/improve-conditionals/pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[MAIN]
load-plugins=pylint.extensions.code_style
2 changes: 2 additions & 0 deletions doc/user_guide/checkers/extensions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ Code Style checker Messages
'typing.NamedTuple' uses the well-known 'class' keyword with type-hints for
readability (it's also faster as it avoids an internal exec call). Disabled
by default!
:improve-conditionals (R6106): *Rewrite conditional expression to '%s'*
Rewrite negated if expressions to improve readability.


.. _pylint.extensions.comparison_placement:
Expand Down
1 change: 1 addition & 0 deletions doc/user_guide/messages/messages_overview.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,7 @@ All messages in the refactor category:
refactor/duplicate-code
refactor/else-if-used
refactor/empty-comment
refactor/improve-conditionals
refactor/inconsistent-return-statements
refactor/literal-comparison
refactor/magic-value-comparison
Expand Down
3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/10600.new_check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Add :ref:`improve-conditionals` check to the Code Style extension.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Add :ref:`improve-conditionals` check to the Code Style extension.
Add :ref:`improve-conditionals` check to the Code Style extension to suggest `not(x and y)` instead of `not x or not y` in order to facilitate the detection of match case refactors. The code style extension must be enabled and `improve-conditionals` itself needs to be explicitely enabled.


Refs #10600
4 changes: 2 additions & 2 deletions pylint/checkers/base/name_checker/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ def _redefines_import(node: nodes.AssignName) -> bool:
current = node
while current and not isinstance(current.parent, nodes.ExceptHandler):
current = current.parent
if not current or not utils.error_of_type(current.parent, ImportError):
if not (current and utils.error_of_type(current.parent, ImportError)):
return False
try_block = current.parent.parent
for import_node in try_block.nodes_of_class((nodes.ImportFrom, nodes.Import)):
Expand Down Expand Up @@ -160,7 +160,7 @@ def _is_multi_naming_match(
match is not None
and match.lastgroup is not None
and match.lastgroup not in EXEMPT_NAME_CATEGORIES
and (node_type != "method" or confidence != interfaces.INFERENCE_FAILURE)
and not (node_type == "method" and confidence == interfaces.INFERENCE_FAILURE)
)


Expand Down
15 changes: 8 additions & 7 deletions pylint/checkers/classes/class_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,10 @@ def _is_trivial_super_delegation(function: nodes.FunctionDef) -> bool:
# Should be a super call with the MRO pointer being the
# current class and the type being the current instance.
current_scope = function.parent.scope()
if (
super_call.mro_pointer != current_scope
or not isinstance(super_call.type, astroid.Instance)
or super_call.type.name != current_scope.name
if not (
super_call.mro_pointer == current_scope
and isinstance(super_call.type, astroid.Instance)
and super_call.type.name == current_scope.name
):
return False

Expand Down Expand Up @@ -1139,8 +1139,9 @@ def _check_unused_private_variables(self, node: nodes.ClassDef) -> None:

def _check_unused_private_attributes(self, node: nodes.ClassDef) -> None:
for assign_attr in node.nodes_of_class(nodes.AssignAttr):
if not is_attr_private(assign_attr.attrname) or not isinstance(
assign_attr.expr, nodes.Name
if not (
is_attr_private(assign_attr.attrname)
and isinstance(assign_attr.expr, nodes.Name)
):
continue

Expand Down Expand Up @@ -1928,7 +1929,7 @@ def _check_protected_attribute_access(
callee = node.expr.as_string()
parents_callee = callee.split(".")
for callee in reversed(parents_callee):
if not outer_klass or callee != outer_klass.name:
if not (outer_klass and callee == outer_klass.name):
inside_klass = False
break

Expand Down
2 changes: 1 addition & 1 deletion pylint/checkers/dataclass_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def _check_invalid_field_call(self, node: nodes.Call) -> None:
self._check_invalid_field_call_within_call(node, scope_node)
return

if not scope_node or not scope_node.is_dataclass:
if not (scope_node and scope_node.is_dataclass):
self.add_message(
"invalid-field-call",
node=node,
Expand Down
4 changes: 2 additions & 2 deletions pylint/checkers/design_analysis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,8 +660,8 @@ def visit_if(self, node: nodes.If) -> None:
self._check_boolean_expressions(node)
branches = 1
# don't double count If nodes coming from some 'elif'
if node.orelse and (
len(node.orelse) > 1 or not isinstance(node.orelse[0], nodes.If)
if node.orelse and not (
len(node.orelse) == 1 and isinstance(node.orelse[0], nodes.If)
):
branches += 1
self._inc_branch(node, branches)
Expand Down
2 changes: 1 addition & 1 deletion pylint/checkers/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ def _check_misplaced_bare_raise(self, node: nodes.Raise) -> None:
current = current.parent

expected = (nodes.ExceptHandler,)
if not current or not isinstance(current.parent, expected):
if not (current and isinstance(current.parent, expected)):
self.add_message("misplaced-bare-raise", node=node, confidence=HIGH)

def _check_bad_exception_cause(self, node: nodes.Raise) -> None:
Expand Down
11 changes: 7 additions & 4 deletions pylint/checkers/imports.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1172,10 +1172,13 @@ def _report_dependencies_graph(
) -> None:
"""Write dependencies as a dot (graphviz) file."""
dep_info = self.linter.stats.dependencies
if not dep_info or not (
self.linter.config.import_graph
or self.linter.config.ext_import_graph
or self.linter.config.int_import_graph
if not (
dep_info
and (
self.linter.config.import_graph
or self.linter.config.ext_import_graph
or self.linter.config.int_import_graph
)
):
raise EmptyReportError()
filename = self.linter.config.import_graph
Expand Down
21 changes: 11 additions & 10 deletions pylint/checkers/refactoring/recommendation_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,10 +281,10 @@ def _check_consider_using_dict_items(self, node: nodes.For) -> None:
continue

value = subscript.slice
if (
not isinstance(value, nodes.Name)
or value.name != node.target.name
or iterating_object_name != subscript.value.as_string()
if not (
isinstance(value, nodes.Name)
and value.name == node.target.name
and iterating_object_name == subscript.value.as_string()
):
continue
last_definition_lineno = value.lookup(value.name)[1][-1].lineno
Expand Down Expand Up @@ -334,10 +334,10 @@ def _check_consider_using_dict_items_comprehension(
continue

value = subscript.slice
if (
not isinstance(value, nodes.Name)
or value.name != node.target.name
or iterating_object_name != subscript.value.as_string()
if not (
isinstance(value, nodes.Name)
and value.name == node.target.name
and iterating_object_name == subscript.value.as_string()
):
continue

Expand Down Expand Up @@ -428,8 +428,9 @@ def _detect_replacable_format_call(self, node: nodes.Const) -> None:
return

# If % applied to another type than str, it's modulo and can't be replaced by formatting
if not hasattr(node.parent.left, "value") or not isinstance(
node.parent.left.value, str
if not (
hasattr(node.parent.left, "value")
and isinstance(node.parent.left.value, str)
):
return

Expand Down
59 changes: 31 additions & 28 deletions pylint/checkers/refactoring/refactoring_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,7 @@ def _check_simplifiable_if(self, node: nodes.If) -> None:
for target in else_branch.targets
if isinstance(target, nodes.AssignName)
]
if not first_branch_targets or not else_branch_targets:
if not (first_branch_targets and else_branch_targets):
return
if sorted(first_branch_targets) != sorted(else_branch_targets):
return
Expand All @@ -645,7 +645,7 @@ def _check_simplifiable_if(self, node: nodes.If) -> None:
case _:
return

if not first_branch_is_bool or not else_branch_is_bool:
if not (first_branch_is_bool and else_branch_is_bool):
return
if not first_branch.value.value:
# This is a case that can't be easily simplified and
Expand Down Expand Up @@ -1053,7 +1053,7 @@ def visit_raise(self, node: nodes.Raise) -> None:
def _check_stop_iteration_inside_generator(self, node: nodes.Raise) -> None:
"""Check if an exception of type StopIteration is raised inside a generator."""
frame = node.frame()
if not isinstance(frame, nodes.FunctionDef) or not frame.is_generator():
if not (isinstance(frame, nodes.FunctionDef) and frame.is_generator()):
return
if utils.node_ignores_exception(node, StopIteration):
return
Expand Down Expand Up @@ -1319,11 +1319,11 @@ def _duplicated_isinstance_types(node: nodes.BoolOp) -> dict[str, set[str]]:
all_types: collections.defaultdict[str, set[str]] = collections.defaultdict(set)

for call in node.values:
if not isinstance(call, nodes.Call) or len(call.args) != 2:
if not (isinstance(call, nodes.Call) and len(call.args) == 2):
continue

inferred = utils.safe_infer(call.func)
if not inferred or not utils.is_builtin_object(inferred):
if not (inferred and utils.is_builtin_object(inferred)):
continue

if inferred.name != "isinstance":
Expand Down Expand Up @@ -1365,7 +1365,7 @@ def _check_consider_merging_isinstance(self, node: nodes.BoolOp) -> None:
def _check_consider_using_in(self, node: nodes.BoolOp) -> None:
allowed_ops = {"or": "==", "and": "!="}

if node.op not in allowed_ops or len(node.values) < 2:
if not (node.op in allowed_ops and len(node.values) >= 2):
return

for value in node.values:
Expand Down Expand Up @@ -1416,7 +1416,7 @@ def _check_chained_comparison(self, node: nodes.BoolOp) -> None:

Care is taken to avoid simplifying a < b < c and b < d.
"""
if node.op != "and" or len(node.values) < 2:
if not (node.op == "and" and len(node.values) >= 2):
return

def _find_lower_upper_bounds(
Expand Down Expand Up @@ -1566,7 +1566,7 @@ def _is_simple_assignment(node: nodes.NodeNG | None) -> bool:
return False

def _check_swap_variables(self, node: nodes.Return | nodes.Assign) -> None:
if not node.next_sibling() or not node.next_sibling().next_sibling():
if not (node.next_sibling() and node.next_sibling().next_sibling()):
return
assignments = [node, node.next_sibling(), node.next_sibling().next_sibling()]
if not all(self._is_simple_assignment(node) for node in assignments):
Expand Down Expand Up @@ -1643,10 +1643,10 @@ def _append_context_managers_to_stack(self, node: nodes.Assign) -> None:
if not isinstance(value, nodes.Call):
continue
inferred = utils.safe_infer(value.func)
if (
not inferred
or inferred.qname() not in CALLS_RETURNING_CONTEXT_MANAGERS
or not isinstance(assignee, (nodes.AssignName, nodes.AssignAttr))
if not (
inferred
and inferred.qname() in CALLS_RETURNING_CONTEXT_MANAGERS
and isinstance(assignee, (nodes.AssignName, nodes.AssignAttr))
):
continue
stack = self._consider_using_with_stack.get_stack_for_frame(node.frame())
Expand Down Expand Up @@ -1685,8 +1685,11 @@ def _check_consider_using_with(self, node: nodes.Call) -> None:
# checked when leaving the scope.
return
inferred = utils.safe_infer(node.func)
if not inferred or not isinstance(
inferred, (nodes.FunctionDef, nodes.ClassDef, bases.BoundMethod)
if not (
inferred
and isinstance(
inferred, (nodes.FunctionDef, nodes.ClassDef, bases.BoundMethod)
)
):
return
could_be_used_in_with = (
Expand Down Expand Up @@ -2178,12 +2181,12 @@ def _check_unnecessary_dict_index_lookup(

# Case where .items is assigned to k,v (i.e., for k, v in d.items())
if isinstance(value, nodes.Name):
if (
not isinstance(node.target, nodes.Tuple)
if not (
isinstance(node.target, nodes.Tuple)
# Ignore 1-tuples: for k, in d.items()
or len(node.target.elts) < 2
or value.name != node.target.elts[0].name
or iterating_object_name != subscript.value.as_string()
and len(node.target.elts) >= 2
and value.name == node.target.elts[0].name
and iterating_object_name == subscript.value.as_string()
):
continue

Expand Down Expand Up @@ -2213,11 +2216,11 @@ def _check_unnecessary_dict_index_lookup(

# Case where .items is assigned to single var (i.e., for item in d.items())
elif isinstance(value, nodes.Subscript):
if (
not isinstance(node.target, nodes.AssignName)
or not isinstance(value.value, nodes.Name)
or node.target.name != value.value.name
or iterating_object_name != subscript.value.as_string()
if not (
isinstance(node.target, nodes.AssignName)
and isinstance(value.value, nodes.Name)
and node.target.name == value.value.name
and iterating_object_name == subscript.value.as_string()
):
continue

Expand All @@ -2234,7 +2237,7 @@ def _check_unnecessary_dict_index_lookup(

# check if subscripted by 0 (key)
inferred = utils.safe_infer(value.slice)
if not isinstance(inferred, nodes.Const) or inferred.value != 0:
if not (isinstance(inferred, nodes.Const) and inferred.value == 0):
continue

if has_nested_loops:
Expand Down Expand Up @@ -2350,9 +2353,9 @@ def _check_unnecessary_list_index_lookup(

index = subscript.slice
if isinstance(index, nodes.Name):
if (
index.name != name1
or iterating_object_name != subscript.value.as_string()
if not (
index.name == name1
and iterating_object_name == subscript.value.as_string()
):
continue

Expand Down
14 changes: 7 additions & 7 deletions pylint/checkers/spelling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,11 @@ def next(self) -> tuple[str, int]:
pre_text, post_text = self._text.split("/", 1)
self._text = post_text
self._offset = 0
if (
not pre_text
or not post_text
or not pre_text[-1].isalpha()
or not post_text[0].isalpha()
if not (
pre_text
and post_text
and pre_text[-1].isalpha()
and post_text[0].isalpha()
):
self._text = ""
self._offset = 0
Expand All @@ -177,9 +177,9 @@ def _next(self) -> tuple[str, Literal[0]]:
if "/" not in self._text:
return self._text, 0
pre_text, post_text = self._text.split("/", 1)
if not pre_text or not post_text:
if not (pre_text and post_text):
break
if not pre_text[-1].isalpha() or not post_text[0].isalpha():
if not (pre_text[-1].isalpha() and post_text[0].isalpha()):
raise StopIteration()
self._text = pre_text + " " + post_text
raise StopIteration()
Expand Down
4 changes: 2 additions & 2 deletions pylint/checkers/stdlib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,7 @@ def _check_bad_thread_instantiation(self, node: nodes.Call) -> None:
if "target" in func_kwargs:
return

if len(node.args) < 2 and (not node.kwargs or "target" not in func_kwargs):
if len(node.args) < 2 and not (node.kwargs and "target" in func_kwargs):
self.add_message(
"bad-thread-instantiation", node=node, confidence=interfaces.HIGH
)
Expand Down Expand Up @@ -885,7 +885,7 @@ def _check_open_call(

if not mode_arg or (
isinstance(mode_arg, nodes.Const)
and (not mode_arg.value or "b" not in str(mode_arg.value))
and not (mode_arg.value and "b" in str(mode_arg.value))
):
confidence = HIGH
try:
Expand Down
Loading
Loading