Adds new rules about unpacking, closes #405, closes #397

Author: sobolevn

CHANGELOG.md

@@ -19,6 +19,7 @@ We used to have incremental versioning before `0.1.0`.
 - Forbids to use multi-line function type annotations
 - Forbids to use uppercase string modifiers
 - Forbids to use assign chains: now we only can use one assign per line
+- Forbids to use assign with unpacking for any nodes except `Name`
 - Forbids to have duplicate `except` blocks
 
 ### Bugfixes
@@ -30,6 +31,7 @@ We used to have incremental versioning before `0.1.0`.
 - Fixes bug when it was possible to provide non-unique aliases
 - Fixes incorrect line number for incorrect parameter names
 - Fixes bug when names like `__some__value__` were not treated as underscored
+- Fixes bug when assignment to anything rather than name was raising an error
 
 ### Misc
 

tests/fixtures/noqa.py

@@ -252,8 +252,10 @@ async def function_with_unreachable():
 
 first = second = 2  # noqa: Z445
 
+index, nodes[0] = range(2)  # noqa: Z446
 
-try:  # noqa: Z446
+
+try:  # noqa: Z447
     anti_z444 = 1
 except ValueError:
     anti_z444 = 1

tests/test_checker/test_noqa.py

@@ -115,6 +115,7 @@ def test_noqa_fixture_disabled(absolute_path, all_violations):
         'Z444': 2,
         'Z445': 1,
         'Z446': 1,
+        'Z447': 1,
     }
 
     process = subprocess.Popen(

tests/test_visitors/test_ast/test_builtins/test_multiple_assign.py renamed to tests/test_visitors/test_ast/test_builtins/test_assing/test_multiple_assign.py

@@ -0,0 +1,127 @@
+# -*- coding: utf-8 -*-
+
+import pytest
+
+from wemake_python_styleguide.violations.best_practices import (
+    IncorrectUnpackingViolation,
+)
+from wemake_python_styleguide.visitors.ast.builtins import (
+    WrongAssignmentVisitor,
+)
+
+single_assignment = '{0} = 1'
+
+tuple_assignment1 = 'first, {0} = (1, 2)'
+tuple_assignment1 = '{0}, second = (1, 2)'
+
+spread_assignment1 = '{0}, *second = [1, 2, 3]'
+spread_assignment2 = 'first, *{0} = [1, 2, 3]'
+
+for_assignment = """
+for {0} in []:
+    ...
+"""
+
+for_unpacking1 = """
+for {0}, second in enumerate([]):
+    ...
+"""
+
+for_unpacking2 = """
+for first, {0} in enumerate([]):
+    ...
+"""
+
+with_assignment = """
+with some() as {0}:
+    ...
+"""
+
+with_unpacking1 = """
+with some() as ({0}, second):
+    ...
+"""
+
+with_unpacking2 = """
+with some() as (first, {0}):
+    ...
+"""
+
+
+@pytest.mark.parametrize('code', [
+    single_assignment,
+    tuple_assignment1,
+    tuple_assignment1,
+    spread_assignment1,
+    spread_assignment2,
+    for_assignment,
+    for_unpacking1,
+    for_unpacking2,
+    with_assignment,
+    with_unpacking1,
+    with_unpacking2,
+])
+def test_correct_unpacking(
+    assert_errors,
+    parse_ast_tree,
+    code,
+    default_options,
+):
+    """Testing that correct assignments work."""
+    tree = parse_ast_tree(code.format('some_name'))
+
+    visitor = WrongAssignmentVisitor(default_options, tree=tree)
+    visitor.run()
+
+    assert_errors(visitor, [])
+
+
+@pytest.mark.parametrize('assignment', [
+    'some[0]',
+    'some["key"]',
+    'some.attr',
+])
+def test_correct_assignment(
+    assert_errors,
+    parse_ast_tree,
+    assignment,
+    default_options,
+):
+    """Testing that correct assignments work."""
+    tree = parse_ast_tree(single_assignment.format(assignment))
+
+    visitor = WrongAssignmentVisitor(default_options, tree=tree)
+    visitor.run()
+
+    assert_errors(visitor, [])
+
+
+@pytest.mark.parametrize('code', [
+    tuple_assignment1,
+    tuple_assignment1,
+    spread_assignment1,
+    spread_assignment2,
+    for_unpacking1,
+    for_unpacking2,
+    with_unpacking1,
+    with_unpacking2,
+])
+@pytest.mark.parametrize('assignment', [
+    'some[0]',
+    'some["key"]',
+    'some.attr',
+])
+def test_multiple_assignments(
+    assert_errors,
+    parse_ast_tree,
+    code,
+    assignment,
+    default_options,
+):
+    """Testing that multiple assignments are restricted."""
+    tree = parse_ast_tree(code.format(assignment))
+
+    visitor = WrongAssignmentVisitor(default_options, tree=tree)
+    visitor.run()
+
+    assert_errors(visitor, [IncorrectUnpackingViolation])

tests/test_visitors/test_ast/test_builtins/test_assing/test_unpacking_rules.py

@@ -81,6 +81,11 @@ def __init__(self):
 {0} = 'test'
 """
 
+# See: https://github.com/wemake-services/wemake-python-styleguide/issues/405
+unpacking_variables = """
+first.attr, {0} = range(2)
+"""
+
 for_variable = """
 def container():
     for {0} in []:
@@ -131,6 +136,7 @@ def container():
 
     # Variables:
     variable_def,
+    unpacking_variables,
     for_variable,
     with_variable,
     exception,

tests/test_visitors/test_ast/test_naming/conftest.py

@@ -46,6 +46,7 @@
    UnreachableCodeViolation
    StatementHasNoEffectViolation
    MultipleAssignmentsViolation
+   IncorrectUnpackingViolation
    DuplicateExceptionViolation
 
 Comments
@@ -89,6 +90,7 @@
 .. autoclass:: UnreachableCodeViolation
 .. autoclass:: StatementHasNoEffectViolation
 .. autoclass:: MultipleAssignmentsViolation
+.. autoclass:: IncorrectUnpackingViolation
 .. autoclass:: DuplicateExceptionViolation
 
 """
@@ -915,6 +917,7 @@ class LambdaInsideLoopViolation(ASTViolation):
     code = 442
 
 
+@final
 class UnreachableCodeViolation(ASTViolation):
     """
     Forbids to have unreachable code.
@@ -958,6 +961,7 @@ def some_function():
     code = 443
 
 
+@final
 class StatementHasNoEffectViolation(ASTViolation):
     """
     Forbids to have statements that do nothing.
@@ -991,6 +995,7 @@ def some_function():
     code = 444
 
 
+@final
 class MultipleAssignmentsViolation(ASTViolation):
     """
     Forbids to have statements that do nothing.
@@ -1020,6 +1025,35 @@ class MultipleAssignmentsViolation(ASTViolation):
     code = 445
 
 
+@final
+class IncorrectUnpackingViolation(ASTViolation):
+    """
+    Forbids to have statements that do nothing.
+
+    Reasoning:
+        Having unpacking with side-effects is very dirty.
+        You might get in serious and very hard-to-debug troubles because of
+        this technique. So, do not use it.
+
+    Solution:
+        Use unpacking with only variables, not any other entities.
+
+    Example::
+
+        # Correct:
+        first, second = some()
+
+        # Wrong:
+        first, some_dict['alias'] = some()
+
+    .. versionadded:: 0.6.0
+
+    """
+
+    error_template = 'Found incorrect unpacking target'
+    code = 446
+
+
 @final
 class DuplicateExceptionViolation(ASTViolation):
     """
@@ -1054,4 +1088,4 @@ class DuplicateExceptionViolation(ASTViolation):
     """
 
     error_template = 'Found duplicate exception: {0}'
-    code = 446
+    code = 447

wemake_python_styleguide/violations/best_practices.py

@@ -1,11 +1,12 @@
 # -*- coding: utf-8 -*-
 
 import ast
-from typing import ClassVar, Optional
+from typing import ClassVar, Iterable, Optional
 
 from wemake_python_styleguide import constants
 from wemake_python_styleguide.types import AnyNodes, final
 from wemake_python_styleguide.violations.best_practices import (
+    IncorrectUnpackingViolation,
     MagicNumberViolation,
     MultipleAssignmentsViolation,
 )
@@ -102,13 +103,54 @@ def _check_assign_targets(self, node: ast.Assign) -> None:
         if len(node.targets) > 1:
             self.add_violation(MultipleAssignmentsViolation(node))
 
+    def _check_unpacking_targets(
+        self,
+        node: ast.AST,
+        targets: Iterable[ast.AST],
+    ) -> None:
+        for target in targets:
+            if isinstance(target, ast.Starred):
+                target = target.value
+            if not isinstance(target, ast.Name):
+                self.add_violation(IncorrectUnpackingViolation(node))
+
+    def visit_With(self, node: ast.With) -> None:
+        """
+        Checks assignments inside context managers to be correct.
+
+        Raises:
+            IncorrectUnpackingViolation
+
+        """
+        for withitem in node.items:
+            if isinstance(withitem.optional_vars, ast.Tuple):
+                self._check_unpacking_targets(
+                    node, withitem.optional_vars.elts,
+                )
+        self.generic_visit(node)
+
+    def visit_For(self, node: ast.For) -> None:
+        """
+        Checks assignments inside ``for`` loops to be correct.
+
+        Raises:
+            IncorrectUnpackingViolation
+
+        """
+        if isinstance(node.target, ast.Tuple):
+            self._check_unpacking_targets(node, node.target.elts)
+        self.generic_visit(node)
+
     def visit_Assign(self, node: ast.Assign) -> None:
         """
-        Checks assingments to be correct.
+        Checks assignments to be correct.
 
         Raises:
             MultipleAssignmentsViolation
+            IncorrectUnpackingViolation
 
         """
         self._check_assign_targets(node)
+        if isinstance(node.targets[0], ast.Tuple):
+            self._check_unpacking_targets(node, node.targets[0].elts)
         self.generic_visit(node)

wemake_python_styleguide/visitors/ast/builtins.py

@@ -267,6 +267,7 @@ def _create_target_names(
                 for index, _ in enumerate(target_names):
                     target_names[index] = tuple(
                         name.id for name in target_names[index]
+                        if isinstance(name, ast.Name)
                     )
         return target_names