1824 add getting first element using unpacking violation (#1927)

* Closes #1824

* Changes according to review

* Refactoring: introduce logic.naming.access.looks_like_unused()

* Fixes CI

Co-authored-by: sobolevn <[email protected]>

CHANGELOG.md

@@ -68,6 +68,12 @@ Semantic versioning in our case means:
 - Adds documentation (and tests) for how to run project on Jupyter Notebooks
 - Updates `mypy` to `0.902` and fixes type issues
 
+## 0.16.0
+
+### Features
+
+- Forbids getting first element of list by unpacking
+
 
 ## 0.15.2
 

tests/fixtures/noqa/noqa.py

@@ -789,7 +789,6 @@ def get_item():  # noqa: WPS463
    for number in numbers # noqa: WPS361
 ]
 
-
 def bare_raise_function():
     raise # noqa: WPS467
 
@@ -813,3 +812,4 @@ class TestClass(object, **{}):  # noqa: WPS470
 
 
 secondary_slice = items[1:][:3]  # noqa: WPS471
+first, *_rest = some_collection  # noqa: WPS472

tests/test_checker/test_noqa.py

@@ -251,6 +251,7 @@
     'WPS469': 1,
     'WPS470': 1,
     'WPS471': 1,
+    'WPS472': 1,
 
     'WPS500': 1,
     'WPS501': 1,

tests/test_violations/test_codes.py

@@ -1,10 +1,22 @@
+from collections import Counter
+
 
 def test_all_unique_violation_codes(all_violations):
     """Ensures that all violations have unique violation codes."""
     codes = [int(violation.code) for violation in all_violations]
     assert len(set(codes)) == len(all_violations)
 
 
+def test_all_unique_violation_messages(all_violations):
+    """Ensures that all violations have unique violation messages."""
+    messages = Counter([
+        violation.error_template
+        for violation in all_violations
+    ])
+    for message, count in messages.items():
+        assert count == 1, message
+
+
 def test_all_violations_correct_numbers(all_module_violations):
     """Ensures that all violations has correct violation code numbers."""
     assert len(all_module_violations) == 7

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

@@ -1,6 +1,7 @@
 import pytest
 
 from wemake_python_styleguide.violations.best_practices import (
+    GettingElementByUnpackingViolation,
     SingleElementDestructuringViolation,
     WrongUnpackingViolation,
 )
@@ -19,6 +20,7 @@
 
 spread_assignment1 = '{0}, *second = [1, 2, 3]'
 spread_assignment2 = 'first, *{0} = [1, 2, 3]'
+spread_assignment3 = '*{0}, second = [1, 2, 3]'
 
 for_assignment = """
 def wrapper():
@@ -274,3 +276,27 @@ def test_single_element_destructing(
         [SingleElementDestructuringViolation],
         ignored_types=UnpackingIterableToListViolation,
     )
+
+
+@pytest.mark.parametrize('code', [
+    spread_assignment2,
+    spread_assignment3,
+])
+@pytest.mark.parametrize('definition', [
+    '_',
+    '_data',
+])
+def test_element_getting_by_unpacking(
+    assert_errors,
+    parse_ast_tree,
+    code,
+    definition,
+    default_options,
+):
+    """Testing that getting element by unpacking is restricted."""
+    tree = parse_ast_tree(code.format(definition))
+
+    visitor = WrongAssignmentVisitor(default_options, tree=tree)
+    visitor.run()
+
+    assert_errors(visitor, [GettingElementByUnpackingViolation])

wemake_python_styleguide/logic/naming/access.py

@@ -26,6 +26,31 @@ def is_unused(name: str) -> bool:
     return _UNUSED_VARIABLE_REGEX.match(name) is not None
 
 
+def looks_like_unused(name: str) -> bool:
+    """
+    Checks whether the given ``name`` is probably unused.
+
+    Tolerant version of ``is_unused``, which takes everything prefixed with
+    "_" as unused. See: https://github.com/wemake-services\
+    /wemake-python-styleguide/pull/1927#discussion_r591688670
+
+
+    >>> looks_like_unused('_')
+    True
+
+    >>> looks_like_unused('___')
+    True
+
+    >>> looks_like_unused('_protected')
+    True
+
+    >>> looks_like_unused('__private')
+    False
+
+    """
+    return is_unused(name) or is_protected(name)
+
+
 def is_magic(name: str) -> bool:
     """
     Checks whether the given ``name`` is magic.

wemake_python_styleguide/logic/tree/variables.py

@@ -1,7 +1,8 @@
 import ast
-from typing import Union
+from typing import List, Union
 
 from wemake_python_styleguide.logic import nodes
+from wemake_python_styleguide.logic.naming import access
 
 _VarDefinition = Union[ast.AST, ast.expr]
 _LocalVariable = Union[ast.Name, ast.ExceptHandler]
@@ -54,3 +55,24 @@ def _is_valid_single(node: _VarDefinition) -> bool:
         isinstance(node, ast.Name) or
         isinstance(node, ast.Starred) and isinstance(node.value, ast.Name)
     )
+
+
+def is_getting_element_by_unpacking(targets: List[ast.expr]) -> bool:
+    """Checks if unpacking targets used to get first or last element."""
+    if len(targets) != 2:
+        return False
+    first_item = (
+        isinstance(targets[0], ast.Name) and
+        isinstance(targets[1], ast.Starred) and
+        _is_unused_variable_name(targets[1].value)
+    )
+    last_item = (
+        isinstance(targets[1], ast.Name) and
+        isinstance(targets[0], ast.Starred) and
+        _is_unused_variable_name(targets[0].value)
+    )
+    return first_item or last_item
+
+
+def _is_unused_variable_name(node: ast.expr) -> bool:
+    return isinstance(node, ast.Name) and access.looks_like_unused(node.id)

wemake_python_styleguide/violations/best_practices.py

@@ -88,6 +88,7 @@
    RaiseFromItselfViolation
    KwargsUnpackingInClassDefinitionViolation
    ConsecutiveSlicesViolation
+   GettingElementByUnpackingViolation
 
 Best practices
 --------------
@@ -164,6 +165,7 @@
 .. autoclass:: RaiseFromItselfViolation
 .. autoclass:: KwargsUnpackingInClassDefinitionViolation
 .. autoclass:: ConsecutiveSlicesViolation
+.. autoclass:: GettingElementByUnpackingViolation
 
 """
 
@@ -309,7 +311,7 @@ class OveruseOfNoCoverCommentViolation(SimpleViolation):
 
     """
 
-    error_template = 'Found `noqa` comments overuse: {0}'
+    error_template = 'Found `no cover` comments overuse: {0}'
     code = 403
 
 
@@ -2640,7 +2642,7 @@ class RedundantEnumerateViolation(ASTViolation):
         for _, item in enumerate(items):
           ...
 
-    .. versionadded:: 0.18.0
+    .. versionadded:: 0.16.0
 
     """
 
@@ -2736,3 +2738,35 @@ class ConsecutiveSlicesViolation(ASTViolation):
 
     error_template = 'Found consecutive slices'
     code = 471
+
+
+@final
+class GettingElementByUnpackingViolation(ASTViolation):
+    """
+    Forbid getting first element using unpacking.
+
+    Reasoning:
+        Performance. Prefixing unused variables with underscore is nothing
+        more than convention, Python still creates these variables.
+        So, unpacking above makes a new unused list which is slow.
+
+    Solution:
+        Use `collection[0]` or `next(iter(collection))`
+
+    Example::
+
+        # Correct:
+        first = some_collection[0]
+        first = next(iter(collection))
+
+        # Wrong:
+        first, *_rest = some_collection
+
+    .. versionadded:: 0.16.0
+
+    """
+
+    error_template = (
+        'Found unpacking used to get a single element from a collection'
+    )
+    code = 472

wemake_python_styleguide/visitors/ast/builtins.py

@@ -378,6 +378,12 @@ def _check_unpacking_targets(
             self.add_violation(
                 best_practices.SingleElementDestructuringViolation(node),
             )
+        elif variables.is_getting_element_by_unpacking(targets):
+            self.add_violation(
+                best_practices.GettingElementByUnpackingViolation(
+                    node,
+                ),
+            )
 
         for target in targets:
             if not variables.is_valid_unpacking_target(target):