From 4ee37a9074605b8153609913fbbe72b3e87fe4f9 Mon Sep 17 00:00:00 2001 From: Raine-Yang-UofT <147955278+Raine-Yang-UofT@users.noreply.github.com> Date: Wed, 30 Oct 2024 09:27:46 -0400 Subject: [PATCH] Extend redundant-assignment check to parallel and multi-target assignments (#1098) --- CHANGELOG.md | 2 + .../checkers/redundant_assignment_checker.py | 38 ++++++-- .../test_redundant_assignment_checker.py | 88 +++++++++++++++++-- 3 files changed, 112 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c769b24ca..b05576b93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - Added `include_frames` filter to `snapshot` - Added `exclude_vars` filter to `snapshot` - Added new `python_ta.debug` module with an `SnapshotTracer` context manager for generating memory models +- Included the name of redundant variable in `E9959 redundant-assignment` message - Update to pylint v3.3 and and astroid v3.3. This added support for Python 3.13 and dropped support for Python 3.8. - Added a STRICT_NUMERIC_TYPES configuration to `python_ta.contracts` allowing to enable/disable stricter type checking of numeric types @@ -23,6 +24,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - Fixed issue where `snapshot` errors on unserializable values - Fixed issue within `Snapshot.py` where the `memory_viz_version` parameter was not respected +- Fixed issue where parallel assignment statements and assignment to multiple targets were not checked by `redundant_assignment_checker` - Fixed issue where annotated assignment statements were not checked by `redundant_assignment_checker` ### 🔧 Internal changes diff --git a/python_ta/checkers/redundant_assignment_checker.py b/python_ta/checkers/redundant_assignment_checker.py index bc881b3ac..7e3207b25 100644 --- a/python_ta/checkers/redundant_assignment_checker.py +++ b/python_ta/checkers/redundant_assignment_checker.py @@ -34,33 +34,39 @@ class RedundantAssignmentChecker(BaseChecker): name = "redundant_assignment" msgs = { "E9959": ( - "This assignment statement is redundant;" " You can remove it from the program.", + "Assigning to variable(s) %s here is redundant, because they are not used before getting reassigned later." + " You can remove the assignment(s) without changing the behaviour of this code.", "redundant-assignment", - "This assignment statement is redundant;" " You can remove it from the program.", + "Assigning to variable(s) is redundant, because they are not used before getting reassigned later." + " You can remove the assignment(s) without changing the behaviour of this code.", ) } def __init__(self, linter=None) -> None: super().__init__(linter=linter) - self._redundant_assignment: set[nodes.Assign] = set() + self._redundant_assignment: dict[ + nodes.Assign | nodes.AugAssign | nodes.AnnAssign, list[str] + ] = {} @only_required_for_messages("redundant-assignment") def visit_assign(self, node: nodes.Assign) -> None: """Visit the assign node""" if node in self._redundant_assignment: - self.add_message("redundant-assignment", node=node) + self.add_message( + "redundant-assignment", node=node, args=", ".join(self._redundant_assignment[node]) + ) @only_required_for_messages("redundant-assignment") def visit_augassign(self, node: nodes.AugAssign) -> None: """ "Visit the augmented assign node""" if node in self._redundant_assignment: - self.add_message("redundant-assignment", node=node) + self.add_message("redundant-assignment", node=node, args=node.target.name) @only_required_for_messages("redundant-assignment") def visit_annassign(self, node: nodes.AnnAssign) -> None: """Visit the annotated assign node""" if node in self._redundant_assignment: - self.add_message("redundant-assignment", node=node) + self.add_message("redundant-assignment", node=node, args=node.target.name) def visit_module(self, node: nodes.Module) -> None: """Visit the module""" @@ -120,10 +126,24 @@ def _transfer(self, block: CFGBlock, out_facts: set[str]) -> set[str]: nodes.FunctionDef, ): if isinstance(node, nodes.AssignName): + # checking for parent node that accounts for parallel assignment + parent = ( + node.parent.parent if isinstance(node.parent, nodes.Tuple) else node.parent + ) if node.name in gen.difference(kill): - self._redundant_assignment.add(node.parent) - elif node.parent in self._redundant_assignment: - self._redundant_assignment.remove(node.parent) + # add redundant assignment + if parent not in self._redundant_assignment: + self._redundant_assignment[parent] = [] + if node.name not in self._redundant_assignment[parent]: + self._redundant_assignment[parent].append(node.name) + elif ( + parent in self._redundant_assignment + and node.name in self._redundant_assignment[parent] + ): + # remove redundant assignment + self._redundant_assignment[parent].remove(node.name) + if len(self._redundant_assignment[parent]) == 0: + self._redundant_assignment.pop(parent) # When node.parent is an AugAssign, the name counts as a use of the variable, # and so is added to kill. diff --git a/tests/test_custom_checkers/test_redundant_assignment_checker.py b/tests/test_custom_checkers/test_redundant_assignment_checker.py index 848b04f29..fa6997616 100644 --- a/tests/test_custom_checkers/test_redundant_assignment_checker.py +++ b/tests/test_custom_checkers/test_redundant_assignment_checker.py @@ -38,7 +38,7 @@ def test_message_simple(self): self.checker.visit_module(mod) with self.assertAddsMessages( - pylint.testutils.MessageTest(msg_id="redundant-assignment", node=assign_1), + pylint.testutils.MessageTest(msg_id="redundant-assignment", node=assign_1, args="x"), ignore_position=True, ): self.checker.visit_assign(assign_1) @@ -60,7 +60,7 @@ def test_message_if_stmt(self): self.checker.visit_module(mod) with self.assertAddsMessages( - pylint.testutils.MessageTest(msg_id="redundant-assignment", node=assign_x), + pylint.testutils.MessageTest(msg_id="redundant-assignment", node=assign_x, args="x"), ignore_position=True, ): self.checker.visit_assign(assign_x) @@ -80,8 +80,8 @@ def test_message_loop_complex(self): self.checker.visit_module(mod) with self.assertAddsMessages( - pylint.testutils.MessageTest(msg_id="redundant-assignment", node=assign_y), - pylint.testutils.MessageTest(msg_id="redundant-assignment", node=assign_x1), + pylint.testutils.MessageTest(msg_id="redundant-assignment", node=assign_y, args="y"), + pylint.testutils.MessageTest(msg_id="redundant-assignment", node=assign_x1, args="x"), ignore_position=True, ): self.checker.visit_assign(assign_y) @@ -105,7 +105,7 @@ def func2(): self.checker.visit_module(mod) with self.assertAddsMessages( - pylint.testutils.MessageTest(msg_id="redundant-assignment", node=assign_x), + pylint.testutils.MessageTest(msg_id="redundant-assignment", node=assign_x, args="x"), ignore_position=True, ): self.checker.visit_assign(assign_x) @@ -240,7 +240,9 @@ def test_augassign_redundant(self): self.checker.visit_module(mod) with self.assertAddsMessages( - pylint.testutils.MessageTest(msg_id="redundant-assignment", node=augassign_node), + pylint.testutils.MessageTest( + msg_id="redundant-assignment", node=augassign_node, args="y_pos" + ), ignore_position=True, ): self.checker.visit_augassign(augassign_node) @@ -256,7 +258,79 @@ def test_annassign_redundant(self): self.checker.visit_module(mod) with self.assertAddsMessages( - pylint.testutils.MessageTest(msg_id="redundant-assignment", node=annassign_node), + pylint.testutils.MessageTest( + msg_id="redundant-assignment", node=annassign_node, args="y_pos" + ), ignore_position=True, ): self.checker.visit_annassign(annassign_node) + + def test_parallel_assign_redundant(self): + src = """ + x, y = 0, 0 + x, y = 10, 10 + """ + mod = astroid.parse(src) + mod.accept(CFGVisitor()) + assign_node, *_ = mod.nodes_of_class(nodes.Assign) + + self.checker.visit_module(mod) + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="redundant-assignment", node=assign_node, args="x, y" + ), + ignore_position=True, + ): + self.checker.visit_assign(assign_node) + + def test_parallel_assign_one_variable_redundant(self): + src = """ + x, y = 0, 0 + y = 10 + """ + mod = astroid.parse(src) + mod.accept(CFGVisitor()) + assign_node, *_ = mod.nodes_of_class(nodes.Assign) + + self.checker.visit_module(mod) + with self.assertAddsMessages( + pylint.testutils.MessageTest(msg_id="redundant-assignment", node=assign_node, args="y"), + ignore_position=True, + ): + self.checker.visit_assign(assign_node) + + def test_multiple_target_assign_redundant(self): + src = """ + x = y = z = 10 + x = 11 + y = 45 + z = 14 + """ + mod = astroid.parse(src) + mod.accept(CFGVisitor()) + assign_node, *_ = mod.nodes_of_class(nodes.Assign) + + self.checker.visit_module(mod) + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="redundant-assignment", node=assign_node, args="x, y, z" + ), + ignore_position=True, + ): + self.checker.visit_assign(assign_node) + + def test_multiple_target_assign_one_variable_redundant(self): + src = """ + x = y = z = 10 + y = 6 + """ + mod = astroid.parse(src) + mod.accept(CFGVisitor()) + assign_node, *_ = mod.nodes_of_class(nodes.Assign) + + self.checker.visit_module(mod) + with self.assertAddsMessages( + pylint.testutils.MessageTest(msg_id="redundant-assignment", node=assign_node, args="y"), + ignore_position=True, + ): + self.checker.visit_assign(assign_node)