From d56ed599d85ab5fecca9f01086a746c04172f43d Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Wed, 21 Sep 2022 12:51:01 -0300 Subject: [PATCH 01/11] add case to detect `manual` augomented assign --- doc/whatsnew/fragments/4562.bugfix | 3 ++ pylint/checkers/typecheck.py | 2 +- pylint/checkers/utils.py | 33 +++++++++++++++++++ tests/functional/n/no/no_member_augassign.py | 25 ++++++++++++++ tests/functional/n/no/no_member_augassign.txt | 4 +++ 5 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 doc/whatsnew/fragments/4562.bugfix create mode 100644 tests/functional/n/no/no_member_augassign.py create mode 100644 tests/functional/n/no/no_member_augassign.txt diff --git a/doc/whatsnew/fragments/4562.bugfix b/doc/whatsnew/fragments/4562.bugfix new file mode 100644 index 0000000000..4e153d4876 --- /dev/null +++ b/doc/whatsnew/fragments/4562.bugfix @@ -0,0 +1,3 @@ +Fix ``no-member`` false negative when augmented assign is done manually, without ``+=``. + +Closes #4562 diff --git a/pylint/checkers/typecheck.py b/pylint/checkers/typecheck.py index aabda22295..ba97e552b8 100644 --- a/pylint/checkers/typecheck.py +++ b/pylint/checkers/typecheck.py @@ -1160,7 +1160,7 @@ def visit_attribute( try: if isinstance( attr_node.statement(future=True), nodes.AugAssign - ): + ) or utils.is_augassign(attr_node, attr_parent): continue except astroid.exceptions.StatementMissing: break diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index fe462b258a..4a104f7b61 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -1963,3 +1963,36 @@ def is_hashable(node: nodes.NodeNG) -> bool: return False except astroid.InferenceError: return True + + +def is_augassign(node: nodes.NodeNG, parent: nodes.NodeNG) -> bool: + """ + Determine if the node is being assigned to a modified version of + itself, aka an augmented assignment, but without a AugAssign being used. + + For example, + obj.value = 1 + obj.value + is the same as + obj.value += 1 + and this function is intended to detect the first case. + """ + if not isinstance(node, nodes.AssignAttr): + return False + + children = [x for x in parent.get_children()] + binops = [x for x in children if isinstance(x, nodes.BinOp)] + if not binops: + return False + + binop = binops[0] + + attr = None + if isinstance(binop.right, nodes.Attribute): + attr = binop.right + elif isinstance(binop.left, nodes.Attribute): + attr = binop.left + + if not attr: + return False + + return binop.op == "+" and attr.attrname == node.attrname diff --git a/tests/functional/n/no/no_member_augassign.py b/tests/functional/n/no/no_member_augassign.py new file mode 100644 index 0000000000..1ffd9a1682 --- /dev/null +++ b/tests/functional/n/no/no_member_augassign.py @@ -0,0 +1,25 @@ +"""Tests for no-member in relation to AugAssign operations.""" +# pylint: disable=missing-module-docstring, too-few-public-methods, missing-class-docstring, invalid-name + +# Test for: https://github.com/PyCQA/pylint/issues/4562 +class A: + value: int + +obj_a = A() +obj_a.value += 1 # [no-member] + + +class B: + value: int + +obj_b = B() +obj_b.value = 1 + obj_b.value # [no-member] + + +class C: + value: int + + +obj_c = C() +obj_c.value += 1 # [no-member] +obj_c.value = 1 + obj_c.value # [no-member] diff --git a/tests/functional/n/no/no_member_augassign.txt b/tests/functional/n/no/no_member_augassign.txt new file mode 100644 index 0000000000..68abf0b935 --- /dev/null +++ b/tests/functional/n/no/no_member_augassign.txt @@ -0,0 +1,4 @@ +no-member:9:0:9:11::Instance of 'A' has no 'value' member:INFERENCE +no-member:16:18:16:29::Instance of 'B' has no 'value' member:INFERENCE +no-member:24:0:24:11::Instance of 'C' has no 'value' member:INFERENCE +no-member:25:18:25:29::Instance of 'C' has no 'value' member:INFERENCE From 8b3de4d5b20bf65db4461a2772598bae4a9e3822 Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Wed, 21 Sep 2022 16:04:30 -0300 Subject: [PATCH 02/11] code cleanup --- pylint/checkers/utils.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index 4a104f7b61..2f41acaaf7 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -1979,20 +1979,18 @@ def is_augassign(node: nodes.NodeNG, parent: nodes.NodeNG) -> bool: if not isinstance(node, nodes.AssignAttr): return False - children = [x for x in parent.get_children()] - binops = [x for x in children if isinstance(x, nodes.BinOp)] - if not binops: + binops = [x for x in parent.get_children() if isinstance(x, nodes.BinOp)] + + if not binops or binops[0].op != "+": return False binop = binops[0] - attr = None if isinstance(binop.right, nodes.Attribute): attr = binop.right elif isinstance(binop.left, nodes.Attribute): attr = binop.left - - if not attr: + else: return False - return binop.op == "+" and attr.attrname == node.attrname + return attr.attrname == node.attrname From de151b61b79c19caf12800dd220c601212f10bbd Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Thu, 22 Sep 2022 08:11:25 -0300 Subject: [PATCH 03/11] add ignore no-any-return --- pylint/checkers/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index 2f41acaaf7..79cee2ab95 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -1993,4 +1993,4 @@ def is_augassign(node: nodes.NodeNG, parent: nodes.NodeNG) -> bool: else: return False - return attr.attrname == node.attrname + return attr.attrname == node.attrname # type: ignore[no-any-return] From 8ea3acd75aac77c128a69e986ed1b0ede5661687 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20van=20Noord?= <13665637+DanielNoord@users.noreply.github.com> Date: Thu, 22 Sep 2022 14:23:01 +0200 Subject: [PATCH 04/11] Add ``consider-using-augmented-assign`` checker --- .../c/consider-using-augmented-assign/bad.py | 2 + .../c/consider-using-augmented-assign/good.py | 2 + doc/whatsnew/fragments/3391.new_check | 4 + doc/whatsnew/fragments/4562.bugfix | 3 - .../refactoring/recommendation_checker.py | 20 +++++ pylint/checkers/typecheck.py | 2 +- pylint/checkers/utils.py | 79 +++++++++++++------ .../consider_using_augmented_assign.py | 47 +++++++++++ .../consider_using_augmented_assign.txt | 7 ++ tests/functional/n/no/no_member_augassign.py | 25 ------ tests/functional/n/no/no_member_augassign.txt | 4 - tests/test_similar.py | 2 +- 12 files changed, 140 insertions(+), 57 deletions(-) create mode 100644 doc/data/messages/c/consider-using-augmented-assign/bad.py create mode 100644 doc/data/messages/c/consider-using-augmented-assign/good.py create mode 100644 doc/whatsnew/fragments/3391.new_check delete mode 100644 doc/whatsnew/fragments/4562.bugfix create mode 100644 tests/functional/c/consider/consider_using_augmented_assign.py create mode 100644 tests/functional/c/consider/consider_using_augmented_assign.txt delete mode 100644 tests/functional/n/no/no_member_augassign.py delete mode 100644 tests/functional/n/no/no_member_augassign.txt diff --git a/doc/data/messages/c/consider-using-augmented-assign/bad.py b/doc/data/messages/c/consider-using-augmented-assign/bad.py new file mode 100644 index 0000000000..90b8931a66 --- /dev/null +++ b/doc/data/messages/c/consider-using-augmented-assign/bad.py @@ -0,0 +1,2 @@ +x = 1 +x = x + 1 # [consider-using-augmented-assign] diff --git a/doc/data/messages/c/consider-using-augmented-assign/good.py b/doc/data/messages/c/consider-using-augmented-assign/good.py new file mode 100644 index 0000000000..3e34f6b266 --- /dev/null +++ b/doc/data/messages/c/consider-using-augmented-assign/good.py @@ -0,0 +1,2 @@ +x = 1 +x += 1 diff --git a/doc/whatsnew/fragments/3391.new_check b/doc/whatsnew/fragments/3391.new_check new file mode 100644 index 0000000000..7111a3a5ce --- /dev/null +++ b/doc/whatsnew/fragments/3391.new_check @@ -0,0 +1,4 @@ +Added ``consider-using-augmented-assign`` which flags ``x = x + 1`` to simplify to +``x += 1``. + +Closes #3391 diff --git a/doc/whatsnew/fragments/4562.bugfix b/doc/whatsnew/fragments/4562.bugfix deleted file mode 100644 index 4e153d4876..0000000000 --- a/doc/whatsnew/fragments/4562.bugfix +++ /dev/null @@ -1,3 +0,0 @@ -Fix ``no-member`` false negative when augmented assign is done manually, without ``+=``. - -Closes #4562 diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index f610763beb..14f81e964b 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -9,6 +9,7 @@ from pylint import checkers from pylint.checkers import utils +from pylint.interfaces import HIGH class RecommendationChecker(checkers.BaseChecker): @@ -60,6 +61,12 @@ class RecommendationChecker(checkers.BaseChecker): "which could potentially be a f-string. The use of f-strings is preferred. " "Requires Python 3.6 and ``py-version >= 3.6``.", ), + "C0210": ( + "Use '%s' to do an augmented assign directly", + "consider-using-augmented-assign", + "Emitted when an assignment is referring to the object that it is assigning " + "to. This can be changed to be an augmented assign.", + ), } def open(self) -> None: @@ -417,3 +424,16 @@ def _detect_replacable_format_call(self, node: nodes.Const) -> None: line=node.lineno, col_offset=node.col_offset, ) + + @utils.only_required_for_messages("consider-using-augmented-assign") + def visit_assign(self, node: nodes.Assign) -> None: + is_aug, op = utils.is_augmented_assign(node) + if is_aug: + self.add_message( + "consider-using-augmented-assign", + args=f"{op}=", + node=node, + line=node.lineno, + col_offset=node.col_offset, + confidence=HIGH, + ) diff --git a/pylint/checkers/typecheck.py b/pylint/checkers/typecheck.py index ba97e552b8..aabda22295 100644 --- a/pylint/checkers/typecheck.py +++ b/pylint/checkers/typecheck.py @@ -1160,7 +1160,7 @@ def visit_attribute( try: if isinstance( attr_node.statement(future=True), nodes.AugAssign - ) or utils.is_augassign(attr_node, attr_parent): + ): continue except astroid.exceptions.StatementMissing: break diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index 79cee2ab95..2853c8db2a 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -1965,32 +1965,65 @@ def is_hashable(node: nodes.NodeNG) -> bool: return True -def is_augassign(node: nodes.NodeNG, parent: nodes.NodeNG) -> bool: - """ - Determine if the node is being assigned to a modified version of - itself, aka an augmented assignment, but without a AugAssign being used. - - For example, - obj.value = 1 + obj.value - is the same as - obj.value += 1 - and this function is intended to detect the first case. - """ - if not isinstance(node, nodes.AssignAttr): +def _is_target_name_in_binop_side( + target: nodes.AssignName | nodes.AssignAttr, side: nodes.NodeNG | None +) -> bool: + """Determine whether the target name-like node is referenced in the side node.""" + if isinstance(side, nodes.Name): + if isinstance(target, nodes.AssignName): + return target.name == side.name # type: ignore[no-any-return] + return False + if isinstance(side, nodes.Attribute): + if isinstance(target, nodes.AssignAttr): + target_parent = target.parent + target_string = target.attrname or "" + while isinstance(target_parent, (nodes.Attribute, nodes.Name)): + if isinstance(target_parent, nodes.Attribute): + target_string = f"{target_parent.attrname}.{target_string}" + else: + target_string = f"{target_parent.name}.{target_string}" + target_parent = target_parent.parent + + side_parent = side.parent + side_string = side.attrname or "" + while isinstance(side_parent, (nodes.Attribute, nodes.Name)): + if isinstance(side_parent, nodes.Attribute): + side_string = f"{side_parent.attrname}.{side_string}" + else: + side_string = f"{side_parent.name}.{side_string}" + side_parent = side_parent.parent + return target_string == side_string return False + return False - binops = [x for x in parent.get_children() if isinstance(x, nodes.BinOp)] - if not binops or binops[0].op != "+": - return False +def is_augmented_assign(node: nodes.Assign) -> tuple[bool, str]: + """Determine if the node is assigning itself (with modifications) to itself. - binop = binops[0] + For example, x = 1 + x + """ + if not isinstance(node.value, nodes.BinOp): + return False, "" - if isinstance(binop.right, nodes.Attribute): - attr = binop.right - elif isinstance(binop.left, nodes.Attribute): - attr = binop.left - else: - return False + binop = node.value + target = node.targets[0] + + if not isinstance(target, (nodes.AssignName, nodes.AssignAttr)): + return False, "" + + # We don't want to catch x = "1" + x + if isinstance(binop.left, nodes.Const) and isinstance( + binop.left.value, (str, bytes) + ): + return False, "" + + # This could probably be improved but for now we disregard all assignments from calls + if isinstance(binop.left, nodes.Call) or isinstance(binop.right, nodes.Call): + return False, "" + + if _is_target_name_in_binop_side(target, binop.left): + return True, binop.op + if _is_target_name_in_binop_side(target, binop.right): + return True, binop.op - return attr.attrname == node.attrname # type: ignore[no-any-return] + return False, "" diff --git a/tests/functional/c/consider/consider_using_augmented_assign.py b/tests/functional/c/consider/consider_using_augmented_assign.py new file mode 100644 index 0000000000..1f614ae521 --- /dev/null +++ b/tests/functional/c/consider/consider_using_augmented_assign.py @@ -0,0 +1,47 @@ +"""Tests for consider-using-augmented-assign.""" + +# pylint: disable=invalid-name,too-few-public-methods + +x = 1 +x = x + 1 # [consider-using-augmented-assign] +x = 1 + x # [consider-using-augmented-assign] +x, y = 1 + x, 2 + x +# We don't warn on intricate expressions as we lack knowledge +# of simplifying such expressions which is necessary to see +# if they can become augmented +x = 1 + x - 2 +x = 1 + x + 2 + + +class MyClass: + """Simple base class.""" + + def __init__(self) -> None: + self.x = 1 + self.x = self.x + 1 # [consider-using-augmented-assign] + self.x = 1 + self.x # [consider-using-augmented-assign] + + x = 1 # [redefined-outer-name] + self.x = x + + +instance = MyClass() + +x = instance.x + 1 + +my_str = "" +my_str = my_str + "foo" # [consider-using-augmented-assign] +my_str = "foo" + my_str + +my_bytes = b"" +my_bytes = my_bytes + b"foo" # [consider-using-augmented-assign] +my_bytes = b"foo" + my_bytes + + +def return_str() -> str: + """Return a string.""" + return "" + + +# Currently we disregard all calls +my_str = return_str() + my_str diff --git a/tests/functional/c/consider/consider_using_augmented_assign.txt b/tests/functional/c/consider/consider_using_augmented_assign.txt new file mode 100644 index 0000000000..5926193832 --- /dev/null +++ b/tests/functional/c/consider/consider_using_augmented_assign.txt @@ -0,0 +1,7 @@ +consider-using-augmented-assign:6:0:6:9::Use '+=' to do an augmented assign directly:HIGH +consider-using-augmented-assign:7:0:7:9::Use '+=' to do an augmented assign directly:HIGH +consider-using-augmented-assign:21:8:21:27:MyClass.__init__:Use '+=' to do an augmented assign directly:HIGH +consider-using-augmented-assign:22:8:22:27:MyClass.__init__:Use '+=' to do an augmented assign directly:HIGH +redefined-outer-name:24:8:24:9:MyClass.__init__:Redefining name 'x' from outer scope (line 5):UNDEFINED +consider-using-augmented-assign:33:0:33:23::Use '+=' to do an augmented assign directly:HIGH +consider-using-augmented-assign:37:0:37:28::Use '+=' to do an augmented assign directly:HIGH diff --git a/tests/functional/n/no/no_member_augassign.py b/tests/functional/n/no/no_member_augassign.py deleted file mode 100644 index 1ffd9a1682..0000000000 --- a/tests/functional/n/no/no_member_augassign.py +++ /dev/null @@ -1,25 +0,0 @@ -"""Tests for no-member in relation to AugAssign operations.""" -# pylint: disable=missing-module-docstring, too-few-public-methods, missing-class-docstring, invalid-name - -# Test for: https://github.com/PyCQA/pylint/issues/4562 -class A: - value: int - -obj_a = A() -obj_a.value += 1 # [no-member] - - -class B: - value: int - -obj_b = B() -obj_b.value = 1 + obj_b.value # [no-member] - - -class C: - value: int - - -obj_c = C() -obj_c.value += 1 # [no-member] -obj_c.value = 1 + obj_c.value # [no-member] diff --git a/tests/functional/n/no/no_member_augassign.txt b/tests/functional/n/no/no_member_augassign.txt deleted file mode 100644 index 68abf0b935..0000000000 --- a/tests/functional/n/no/no_member_augassign.txt +++ /dev/null @@ -1,4 +0,0 @@ -no-member:9:0:9:11::Instance of 'A' has no 'value' member:INFERENCE -no-member:16:18:16:29::Instance of 'B' has no 'value' member:INFERENCE -no-member:24:0:24:11::Instance of 'C' has no 'value' member:INFERENCE -no-member:25:18:25:29::Instance of 'C' has no 'value' member:INFERENCE diff --git a/tests/test_similar.py b/tests/test_similar.py index f054d7473e..5558b70e73 100644 --- a/tests/test_similar.py +++ b/tests/test_similar.py @@ -36,7 +36,7 @@ def _runtest(self, args: list[str], code: int) -> None: @staticmethod def _run_pylint(args: list[str], out: TextIO) -> int: """Runs pylint with a patched output.""" - args = args + [ + args += [ "--persistent=no", "--enable=astroid-error", # Enable functionality that will build another ast From a00e904f6a8dad6fbfa7b61241488f8980c25f35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20van=20Noord?= <13665637+DanielNoord@users.noreply.github.com> Date: Thu, 22 Sep 2022 14:35:06 +0200 Subject: [PATCH 05/11] Update tests --- tests/functional/e/e1101_9588_base_attr_aug_assign.py | 2 +- tests/functional/i/inconsistent/inconsistent_returns.py | 6 +++--- tests/functional/u/use/use_maxsplit_arg.py | 2 +- .../functional/u/used/used_before_assignment_conditional.py | 3 +++ .../u/used/used_before_assignment_conditional.txt | 4 ++-- tests/functional/u/used/used_before_assignment_nonlocal.py | 6 +++--- 6 files changed, 13 insertions(+), 10 deletions(-) diff --git a/tests/functional/e/e1101_9588_base_attr_aug_assign.py b/tests/functional/e/e1101_9588_base_attr_aug_assign.py index 9fe95fd4ba..131dfc2c9b 100644 --- a/tests/functional/e/e1101_9588_base_attr_aug_assign.py +++ b/tests/functional/e/e1101_9588_base_attr_aug_assign.py @@ -31,7 +31,7 @@ class NegativeClass(BaseClass): def __init__(self): "Ordinary assignment is OK." BaseClass.__init__(self) - self.e1101 = self.e1101 + 1 + self.e1101 += self.e1101 def countup(self): "No problem." diff --git a/tests/functional/i/inconsistent/inconsistent_returns.py b/tests/functional/i/inconsistent/inconsistent_returns.py index 08dde253e9..c1183b288e 100644 --- a/tests/functional/i/inconsistent/inconsistent_returns.py +++ b/tests/functional/i/inconsistent/inconsistent_returns.py @@ -91,13 +91,13 @@ def explicit_returns6(x, y, z): def explicit_returns7(arg): if arg < 0: - arg = 2 * arg + arg *= 2 return 'below 0' elif arg == 0: print("Null arg") return '0' else: - arg = 3 * arg + arg *= 3 return 'above 0' def bug_1772(): @@ -184,7 +184,7 @@ def explicit_implicit_returns3(arg): # [inconsistent-return-statements] def returns_missing_in_catched_exceptions(arg): # [inconsistent-return-statements] try: - arg = arg**2 + arg **= arg raise ValueError('test') except ValueError: print('ValueError') diff --git a/tests/functional/u/use/use_maxsplit_arg.py b/tests/functional/u/use/use_maxsplit_arg.py index d0d43c2b98..28973c0f40 100644 --- a/tests/functional/u/use/use_maxsplit_arg.py +++ b/tests/functional/u/use/use_maxsplit_arg.py @@ -1,5 +1,5 @@ """Emit a message for accessing first/last element of string.split""" -# pylint: disable=line-too-long,missing-docstring,unsubscriptable-object,too-few-public-methods,invalid-name,redefined-builtin +# pylint: disable=line-too-long,missing-docstring,unsubscriptable-object,too-few-public-methods,invalid-name,redefined-builtin,consider-using-augmented-assign # Test subscripting .split() get_first = '1,2,3'.split(',')[0] # [use-maxsplit-arg] diff --git a/tests/functional/u/used/used_before_assignment_conditional.py b/tests/functional/u/used/used_before_assignment_conditional.py index b5d16925eb..2691bd099e 100644 --- a/tests/functional/u/used/used_before_assignment_conditional.py +++ b/tests/functional/u/used/used_before_assignment_conditional.py @@ -1,4 +1,7 @@ """used-before-assignment cases involving IF conditions""" + +# pylint: disable=consider-using-augmented-assign + if 1 + 1 == 2: x = x + 1 # [used-before-assignment] diff --git a/tests/functional/u/used/used_before_assignment_conditional.txt b/tests/functional/u/used/used_before_assignment_conditional.txt index 56626f0fee..9e009df288 100644 --- a/tests/functional/u/used/used_before_assignment_conditional.txt +++ b/tests/functional/u/used/used_before_assignment_conditional.txt @@ -1,2 +1,2 @@ -used-before-assignment:3:8:3:9::Using variable 'x' before assignment:HIGH -used-before-assignment:5:3:5:4::Using variable 'y' before assignment:HIGH +used-before-assignment:6:8:6:9::Using variable 'x' before assignment:HIGH +used-before-assignment:8:3:8:4::Using variable 'y' before assignment:HIGH diff --git a/tests/functional/u/used/used_before_assignment_nonlocal.py b/tests/functional/u/used/used_before_assignment_nonlocal.py index 18e16177d0..15ef946fb6 100644 --- a/tests/functional/u/used/used_before_assignment_nonlocal.py +++ b/tests/functional/u/used/used_before_assignment_nonlocal.py @@ -7,14 +7,14 @@ def test_ok(): cnt = 1 def wrap(): nonlocal cnt - cnt = cnt + 1 + cnt += 1 wrap() def test_fail(): """ doesn't use nonlocal """ cnt = 1 def wrap(): - cnt = cnt + 1 # [used-before-assignment] + cnt += 1 # [used-before-assignment] wrap() def test_fail2(): @@ -23,7 +23,7 @@ def test_fail2(): count = 1 def wrap(): nonlocal count - cnt = cnt + 1 # [used-before-assignment] + cnt += 1 # [used-before-assignment] wrap() def test_fail3(arg: test_fail4): # [used-before-assignment] From be5463a682e34800c00847c8cf483a13b8d07f22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20van=20Noord?= <13665637+DanielNoord@users.noreply.github.com> Date: Thu, 22 Sep 2022 14:42:26 +0200 Subject: [PATCH 06/11] Fix more tests --- tests/functional/u/undefined/undefined_variable_py38.py | 2 +- .../functional/u/used/used_before_assignment_nonlocal.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/functional/u/undefined/undefined_variable_py38.py b/tests/functional/u/undefined/undefined_variable_py38.py index 61a70d4724..79a23da13d 100644 --- a/tests/functional/u/undefined/undefined_variable_py38.py +++ b/tests/functional/u/undefined/undefined_variable_py38.py @@ -1,5 +1,5 @@ """Tests for undefined variable with assignment expressions""" -# pylint: disable=using-constant-test, expression-not-assigned +# pylint: disable=using-constant-test, expression-not-assigned, consider-using-augmented-assign # Tests for annotation of variables and potentially undefinition diff --git a/tests/functional/u/used/used_before_assignment_nonlocal.py b/tests/functional/u/used/used_before_assignment_nonlocal.py index 15ef946fb6..f48281aae9 100644 --- a/tests/functional/u/used/used_before_assignment_nonlocal.py +++ b/tests/functional/u/used/used_before_assignment_nonlocal.py @@ -1,5 +1,5 @@ """Check for nonlocal and used-before-assignment""" -# pylint: disable=missing-docstring, unused-variable, too-few-public-methods +# pylint: disable=missing-docstring, unused-variable, too-few-public-methods, consider-using-augmented-assign def test_ok(): @@ -7,14 +7,14 @@ def test_ok(): cnt = 1 def wrap(): nonlocal cnt - cnt += 1 + cnt = cnt + 1 wrap() def test_fail(): """ doesn't use nonlocal """ cnt = 1 def wrap(): - cnt += 1 # [used-before-assignment] + cnt = cnt + 1 # [used-before-assignment] wrap() def test_fail2(): @@ -23,7 +23,7 @@ def test_fail2(): count = 1 def wrap(): nonlocal count - cnt += 1 # [used-before-assignment] + cnt = cnt + 1 # [used-before-assignment] wrap() def test_fail3(arg: test_fail4): # [used-before-assignment] From 3e32b311707f2db9476d633870085a2e6182efb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20van=20Noord?= <13665637+DanielNoord@users.noreply.github.com> Date: Thu, 22 Sep 2022 16:19:32 +0200 Subject: [PATCH 07/11] Add some robustness to the tests --- .../refactoring/recommendation_checker.py | 4 ++-- pylint/checkers/utils.py | 4 +++- .../consider/consider_using_augmented_assign.py | 12 +++++++++++- .../consider_using_augmented_assign.txt | 17 ++++++++++------- 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 14f81e964b..8a3b2d2044 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -9,7 +9,7 @@ from pylint import checkers from pylint.checkers import utils -from pylint.interfaces import HIGH +from pylint.interfaces import INFERENCE class RecommendationChecker(checkers.BaseChecker): @@ -435,5 +435,5 @@ def visit_assign(self, node: nodes.Assign) -> None: node=node, line=node.lineno, col_offset=node.col_offset, - confidence=HIGH, + confidence=INFERENCE, ) diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index 2853c8db2a..16ba820df3 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -2011,7 +2011,7 @@ def is_augmented_assign(node: nodes.Assign) -> tuple[bool, str]: if not isinstance(target, (nodes.AssignName, nodes.AssignAttr)): return False, "" - # We don't want to catch x = "1" + x + # We don't want to catch x = "1" + x or x = "%s" % x if isinstance(binop.left, nodes.Const) and isinstance( binop.left.value, (str, bytes) ): @@ -2024,6 +2024,8 @@ def is_augmented_assign(node: nodes.Assign) -> tuple[bool, str]: if _is_target_name_in_binop_side(target, binop.left): return True, binop.op if _is_target_name_in_binop_side(target, binop.right): + if binop.op == "%" and safe_infer(binop.left) is astroid.Uninferable: + return False, "" return True, binop.op return False, "" diff --git a/tests/functional/c/consider/consider_using_augmented_assign.py b/tests/functional/c/consider/consider_using_augmented_assign.py index 1f614ae521..66c430b4e6 100644 --- a/tests/functional/c/consider/consider_using_augmented_assign.py +++ b/tests/functional/c/consider/consider_using_augmented_assign.py @@ -1,6 +1,8 @@ """Tests for consider-using-augmented-assign.""" -# pylint: disable=invalid-name,too-few-public-methods +# pylint: disable=invalid-name,too-few-public-methods,import-error,consider-using-f-string + +from unknown import Unknown x = 1 x = x + 1 # [consider-using-augmented-assign] @@ -45,3 +47,11 @@ def return_str() -> str: # Currently we disregard all calls my_str = return_str() + my_str + +my_str = my_str % return_str() +my_str = my_str % 1 # [consider-using-augmented-assign] +my_str = my_str % (1, 2) # [consider-using-augmented-assign] +my_str = "%s" % my_str +my_str = return_str() % my_str +my_str = Unknown % my_str +my_str = my_str % Unknown # [consider-using-augmented-assign] diff --git a/tests/functional/c/consider/consider_using_augmented_assign.txt b/tests/functional/c/consider/consider_using_augmented_assign.txt index 5926193832..ab8055d747 100644 --- a/tests/functional/c/consider/consider_using_augmented_assign.txt +++ b/tests/functional/c/consider/consider_using_augmented_assign.txt @@ -1,7 +1,10 @@ -consider-using-augmented-assign:6:0:6:9::Use '+=' to do an augmented assign directly:HIGH -consider-using-augmented-assign:7:0:7:9::Use '+=' to do an augmented assign directly:HIGH -consider-using-augmented-assign:21:8:21:27:MyClass.__init__:Use '+=' to do an augmented assign directly:HIGH -consider-using-augmented-assign:22:8:22:27:MyClass.__init__:Use '+=' to do an augmented assign directly:HIGH -redefined-outer-name:24:8:24:9:MyClass.__init__:Redefining name 'x' from outer scope (line 5):UNDEFINED -consider-using-augmented-assign:33:0:33:23::Use '+=' to do an augmented assign directly:HIGH -consider-using-augmented-assign:37:0:37:28::Use '+=' to do an augmented assign directly:HIGH +consider-using-augmented-assign:8:0:8:9::Use '+=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:9:0:9:9::Use '+=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:23:8:23:27:MyClass.__init__:Use '+=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:24:8:24:27:MyClass.__init__:Use '+=' to do an augmented assign directly:INFERENCE +redefined-outer-name:26:8:26:9:MyClass.__init__:Redefining name 'x' from outer scope (line 7):UNDEFINED +consider-using-augmented-assign:35:0:35:23::Use '+=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:39:0:39:28::Use '+=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:52:0:52:19::Use '%=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:53:0:53:24::Use '%=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:57:0:57:25::Use '%=' to do an augmented assign directly:INFERENCE From 104935d53c06c7179d23b3a8d07b5f25ef18bcb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20van=20Noord?= <13665637+DanielNoord@users.noreply.github.com> Date: Thu, 22 Sep 2022 17:23:12 +0200 Subject: [PATCH 08/11] Fix --- pylint/checkers/utils.py | 10 ++++++---- .../consider/consider_using_augmented_assign.py | 5 +++++ .../consider/consider_using_augmented_assign.txt | 16 ++++++++-------- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index 16ba820df3..d42b118fd7 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -2024,8 +2024,10 @@ def is_augmented_assign(node: nodes.Assign) -> tuple[bool, str]: if _is_target_name_in_binop_side(target, binop.left): return True, binop.op if _is_target_name_in_binop_side(target, binop.right): - if binop.op == "%" and safe_infer(binop.left) is astroid.Uninferable: - return False, "" - return True, binop.op - + inferred_left = safe_infer(binop.left) + if isinstance(inferred_left, nodes.Const) and isinstance( + inferred_left.value, int + ): + return True, binop.op + return False, "" return False, "" diff --git a/tests/functional/c/consider/consider_using_augmented_assign.py b/tests/functional/c/consider/consider_using_augmented_assign.py index 66c430b4e6..1e991a26ec 100644 --- a/tests/functional/c/consider/consider_using_augmented_assign.py +++ b/tests/functional/c/consider/consider_using_augmented_assign.py @@ -14,6 +14,11 @@ x = 1 + x - 2 x = 1 + x + 2 +# For anything other than a float or an int we only want to warn on +# assignments where the 'itself' is on the left side of the assignment +my_list = [2, 3, 4] +my_list = [1] + my_list + class MyClass: """Simple base class.""" diff --git a/tests/functional/c/consider/consider_using_augmented_assign.txt b/tests/functional/c/consider/consider_using_augmented_assign.txt index ab8055d747..bc17c8117e 100644 --- a/tests/functional/c/consider/consider_using_augmented_assign.txt +++ b/tests/functional/c/consider/consider_using_augmented_assign.txt @@ -1,10 +1,10 @@ consider-using-augmented-assign:8:0:8:9::Use '+=' to do an augmented assign directly:INFERENCE consider-using-augmented-assign:9:0:9:9::Use '+=' to do an augmented assign directly:INFERENCE -consider-using-augmented-assign:23:8:23:27:MyClass.__init__:Use '+=' to do an augmented assign directly:INFERENCE -consider-using-augmented-assign:24:8:24:27:MyClass.__init__:Use '+=' to do an augmented assign directly:INFERENCE -redefined-outer-name:26:8:26:9:MyClass.__init__:Redefining name 'x' from outer scope (line 7):UNDEFINED -consider-using-augmented-assign:35:0:35:23::Use '+=' to do an augmented assign directly:INFERENCE -consider-using-augmented-assign:39:0:39:28::Use '+=' to do an augmented assign directly:INFERENCE -consider-using-augmented-assign:52:0:52:19::Use '%=' to do an augmented assign directly:INFERENCE -consider-using-augmented-assign:53:0:53:24::Use '%=' to do an augmented assign directly:INFERENCE -consider-using-augmented-assign:57:0:57:25::Use '%=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:28:8:28:27:MyClass.__init__:Use '+=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:29:8:29:27:MyClass.__init__:Use '+=' to do an augmented assign directly:INFERENCE +redefined-outer-name:31:8:31:9:MyClass.__init__:Redefining name 'x' from outer scope (line 7):UNDEFINED +consider-using-augmented-assign:40:0:40:23::Use '+=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:44:0:44:28::Use '+=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:57:0:57:19::Use '%=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:58:0:58:24::Use '%=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:62:0:62:25::Use '%=' to do an augmented assign directly:INFERENCE From 025e1d3f636c7a5160d3bce89ba6609968fa15e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20van=20Noord?= <13665637+DanielNoord@users.noreply.github.com> Date: Thu, 22 Sep 2022 20:15:31 +0200 Subject: [PATCH 09/11] Apply suggestions from code review Co-authored-by: Mark Byrne <31762852+mbyrnepr2@users.noreply.github.com> --- pylint/checkers/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index d42b118fd7..37702222f1 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -2000,7 +2000,7 @@ def _is_target_name_in_binop_side( def is_augmented_assign(node: nodes.Assign) -> tuple[bool, str]: """Determine if the node is assigning itself (with modifications) to itself. - For example, x = 1 + x + For example: x = 1 + x """ if not isinstance(node.value, nodes.BinOp): return False, "" From 364c438bf6ebcb03737612eaf71a0249b6ea7010 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20van=20Noord?= <13665637+DanielNoord@users.noreply.github.com> Date: Thu, 22 Sep 2022 20:19:58 +0200 Subject: [PATCH 10/11] Extract additional utility --- pylint/checkers/utils.py | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index 37702222f1..2a4426083c 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -1965,6 +1965,22 @@ def is_hashable(node: nodes.NodeNG) -> bool: return True +def get_full_name_of_attribute(node: nodes.Attribute | nodes.AssignAttr) -> str: + """Return the full name of an attribute and the classes it belongs to. + + For example: "Class1.Class2.attr" + """ + parent = node.parent + ret = node.attrname or "" + while isinstance(parent, (nodes.Attribute, nodes.Name)): + if isinstance(parent, nodes.Attribute): + ret = f"{parent.attrname}.{ret}" + else: + ret = f"{parent.name}.{ret}" + parent = parent.parent + return ret + + def _is_target_name_in_binop_side( target: nodes.AssignName | nodes.AssignAttr, side: nodes.NodeNG | None ) -> bool: @@ -1973,27 +1989,8 @@ def _is_target_name_in_binop_side( if isinstance(target, nodes.AssignName): return target.name == side.name # type: ignore[no-any-return] return False - if isinstance(side, nodes.Attribute): - if isinstance(target, nodes.AssignAttr): - target_parent = target.parent - target_string = target.attrname or "" - while isinstance(target_parent, (nodes.Attribute, nodes.Name)): - if isinstance(target_parent, nodes.Attribute): - target_string = f"{target_parent.attrname}.{target_string}" - else: - target_string = f"{target_parent.name}.{target_string}" - target_parent = target_parent.parent - - side_parent = side.parent - side_string = side.attrname or "" - while isinstance(side_parent, (nodes.Attribute, nodes.Name)): - if isinstance(side_parent, nodes.Attribute): - side_string = f"{side_parent.attrname}.{side_string}" - else: - side_string = f"{side_parent.name}.{side_string}" - side_parent = side_parent.parent - return target_string == side_string - return False + if isinstance(side, nodes.Attribute) and isinstance(target, nodes.AssignAttr): + return get_full_name_of_attribute(target) == get_full_name_of_attribute(side) return False From f5861b17f5da3d6de6dc1a6bd9b09f752b742745 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20van=20Noord?= <13665637+DanielNoord@users.noreply.github.com> Date: Thu, 22 Sep 2022 20:25:20 +0200 Subject: [PATCH 11/11] Add more tests --- .../c/consider/consider_using_augmented_assign.py | 15 +++++++++++++++ .../consider/consider_using_augmented_assign.txt | 10 ++++++++++ 2 files changed, 25 insertions(+) diff --git a/tests/functional/c/consider/consider_using_augmented_assign.py b/tests/functional/c/consider/consider_using_augmented_assign.py index 1e991a26ec..e4af956864 100644 --- a/tests/functional/c/consider/consider_using_augmented_assign.py +++ b/tests/functional/c/consider/consider_using_augmented_assign.py @@ -60,3 +60,18 @@ def return_str() -> str: my_str = return_str() % my_str my_str = Unknown % my_str my_str = my_str % Unknown # [consider-using-augmented-assign] + +x = x - 3 # [consider-using-augmented-assign] +x = x * 3 # [consider-using-augmented-assign] +x = x / 3 # [consider-using-augmented-assign] +x = x // 3 # [consider-using-augmented-assign] +x = x << 3 # [consider-using-augmented-assign] +x = x >> 3 # [consider-using-augmented-assign] +x = x % 3 # [consider-using-augmented-assign] +x = x**3 # [consider-using-augmented-assign] +x = x ^ 3 # [consider-using-augmented-assign] +x = x & 3 # [consider-using-augmented-assign] +x = x > 3 +x = x < 3 +x = x >= 3 +x = x <= 3 diff --git a/tests/functional/c/consider/consider_using_augmented_assign.txt b/tests/functional/c/consider/consider_using_augmented_assign.txt index bc17c8117e..1684953e9e 100644 --- a/tests/functional/c/consider/consider_using_augmented_assign.txt +++ b/tests/functional/c/consider/consider_using_augmented_assign.txt @@ -8,3 +8,13 @@ consider-using-augmented-assign:44:0:44:28::Use '+=' to do an augmented assign d consider-using-augmented-assign:57:0:57:19::Use '%=' to do an augmented assign directly:INFERENCE consider-using-augmented-assign:58:0:58:24::Use '%=' to do an augmented assign directly:INFERENCE consider-using-augmented-assign:62:0:62:25::Use '%=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:64:0:64:9::Use '-=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:65:0:65:9::Use '*=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:66:0:66:9::Use '/=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:67:0:67:10::Use '//=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:68:0:68:10::Use '<<=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:69:0:69:10::Use '>>=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:70:0:70:9::Use '%=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:71:0:71:8::Use '**=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:72:0:72:9::Use '^=' to do an augmented assign directly:INFERENCE +consider-using-augmented-assign:73:0:73:9::Use '&=' to do an augmented assign directly:INFERENCE