2268 only disallow certain magic attributes (#2281)

* only disallow certain magic attributes (#2268) * use already defined magic methods in constants * Update wemake_python_styleguide/constants.py * add changelog entry about fixing some forgotten magic methods Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
wemake-services · Dec 2, 2021 · e37b8ae · e37b8ae
1 parent 5f35ea1
commit e37b8ae
Show file tree

Hide file tree

Showing 5 changed files with 85 additions and 25 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -28,11 +28,14 @@ Semantic versioning in our case means:
 - Adds `RaiseFromItselfViolation` #2133
 - Adds `ConsecutiveSlicesViolation` #2064
 - Adds `KwargsUnpackingInClassDefinitionViolation` #1714
+- `DirectMagicAttributeAccessViolation` now only flags instances for which
+  a known alternative exists #2268
 
 ### Bugfixes
 
 - Fixes that `InconsistentComprehensionViolation` was ignoring
   misaligned `in` expressions #2075
+- Fixes some common magic methods not being recognized as such  #2281
 
 ### Misc
 

diff --git a/tests/test_visitors/test_ast/test_attributes/test_magic_attributes.py b/tests/test_visitors/test_ast/test_attributes/test_magic_attributes.py
@@ -109,7 +109,15 @@ def method(cls):
 
 
 @pytest.mark.parametrize('attribute', [
-    '__magic__',
+    '__truediv__',
+    '__radd__',
+    '__iter__',
+    '__int__',
+    '__float__',
+    '__repr__',
+    '__coerce__',
+    '__str__',
+    '__next__',
 ])
 @pytest.mark.parametrize('code', [
     magic_attribute_assigned,
@@ -120,7 +128,7 @@ def method(cls):
     magic_container_method,
     magic_callable_attribute,
 ])
-def test_magic_attribute_is_restricted(
+def test_disallowed_magic_attribute_is_restricted(
     assert_errors,
     assert_error_text,
     parse_ast_tree,
@@ -129,7 +137,7 @@ def test_magic_attribute_is_restricted(
     default_options,
     mode,
 ):
-    """Ensures that it is impossible to use magic attributes."""
+    """Ensures that it is impossible to use certain magic attributes."""
     tree = parse_ast_tree(mode(code.format(attribute)))
 
     visitor = WrongAttributeVisitor(default_options, tree=tree)
@@ -141,6 +149,12 @@ def test_magic_attribute_is_restricted(
 
 @pytest.mark.parametrize('attribute', [
     '__magic__',
+    '__str__',
+    '__float__',
+    '__members__',
+    '__path__',
+    '__foo__',
+    '__unknown__',
 ])
 @pytest.mark.parametrize('code', [
     magic_name_definition,
@@ -165,7 +179,7 @@ def test_magic_attribute_correct_contexts(
     default_options,
     mode,
 ):
-    """Ensures that it is possible to use magic attributes."""
+    """Ensures it is possible to use magic attributes in certain contexts."""
     tree = parse_ast_tree(mode(code.format(attribute)))
 
     visitor = WrongAttributeVisitor(default_options, tree=tree)
@@ -183,6 +197,11 @@ def test_magic_attribute_correct_contexts(
     '__subclasses__',
     '__mro__',
     '__version__',
+    '__path__',
+    '__bases__',
+    '__members__',
+    '__unknown__',
+    '__foo__',
 ])
 @pytest.mark.parametrize('code', [
     magic_attribute_assigned,
@@ -206,7 +225,7 @@ def test_magic_attribute_correct_contexts(
     magic_super_cls_attribute,
     magic_super_cls_method,
 ])
-def test_whitelist_regular_attributes_allowed(
+def test_other_magic_attributs_allowed(
     assert_errors,
     parse_ast_tree,
     code,
@@ -217,8 +236,8 @@ def test_whitelist_regular_attributes_allowed(
     """
     Tests if regular attributes are allowed.
 
-    Ensures that it is possible to use regular and
-    whitelisted magic attributes.
+    Ensures that it is possible to use regular attributes, as well as
+    magic attributes for which no (known) alternative exists.
     """
     tree = parse_ast_tree(mode(code.format(attribute)))
 

diff --git a/wemake_python_styleguide/constants.py b/wemake_python_styleguide/constants.py
@@ -198,6 +198,7 @@
     '__delitem__',
     '__missing__',
     '__iter__',
+    '__next__',
     '__reversed__',
     '__contains__',
 
@@ -254,6 +255,8 @@
     '__trunc__',
     '__floor__',
     '__ceil__',
+    '__oct__',
+    '__hex__',
 
     '__enter__',
     '__exit__',
@@ -263,6 +266,43 @@
     '__anext__',
     '__aenter__',
     '__aexit__',
+
+    # pickling
+    '__getnewargs_ex__',
+    '__getnewargs__',
+    '__getstate__',
+    '__setstate__',
+    '__reduce__',
+    '__reduce_ex__',
+    '__getinitargs__',
+
+    # Python 2
+    '__long__',
+    '__coerce__',
+    '__nonzero__',
+    '__unicode__',
+    '__cmp__',
+
+    # copy
+    '__copy__',
+    '__deepcopy__',
+
+    # dataclasses
+    '__post_init__',
+
+    # attrs:
+    '__attrs_pre_init__',
+    '__attrs_init__',
+    '__attrs_post_init__',
+
+    # inspect
+    '__signature__',
+
+    # os.path
+    '__fspath__',
+
+    # sys
+    '__sizeof__',
 ))
 
 #: List of magic methods that are forbidden to use.

diff --git a/wemake_python_styleguide/violations/oop.py b/wemake_python_styleguide/violations/oop.py
@@ -399,12 +399,11 @@ class WrongSuperCallViolation(ASTViolation):
 @final
 class DirectMagicAttributeAccessViolation(ASTViolation):
     """
-    Forbid direct magic attributes and methods.
+    Forbid directly calling certain magic attributes and methods.
 
     Reasoning:
-        When using direct magic attributes or method
-        it means that you are doing something wrong.
-        Magic methods are not suited to be directly called or accessed.
+        Certain magic methods are only meant to be called by particular
+        functions or operators, not directly accessed.
 
     Solution:
         Use special syntax constructs that will call underlying magic methods.
@@ -413,17 +412,25 @@ class DirectMagicAttributeAccessViolation(ASTViolation):
 
         # Correct:
         super().__init__()
+        mymodule.__name__
 
         # Wrong:
-        2..__truediv__(2)
-        d.__delitem__('a')
+        foo.__str__()  # use `str(foo)`
+        2..__truediv__(2)  # use `2 / 2`
+        d.__delitem__('a')  # use del d['a']
 
-    Note, that it is possible to use direct magic attributes with
+    Note, that it is possible to directly use these magic attributes with
     ``self``, ``cls``, and ``super()`` as base names.
     We allow this because a lot of internal logic relies on these methods.
 
+    See
+    :py:data:`~wemake_python_styleguide.constants.ALL_MAGIC_METHODS`
+    for the full list of magic attributes disallowed from being
+    accessed directly.
+
     .. versionadded:: 0.8.0
     .. versionchanged:: 0.11.0
+    .. versionchanged:: 0.16.0
 
     """
 

diff --git a/wemake_python_styleguide/visitors/ast/attributes.py b/wemake_python_styleguide/visitors/ast/attributes.py
@@ -3,6 +3,7 @@
 
 from typing_extensions import final
 
+from wemake_python_styleguide.constants import ALL_MAGIC_METHODS
 from wemake_python_styleguide.logic.naming import access
 from wemake_python_styleguide.violations.best_practices import (
     ProtectedAttributeViolation,
@@ -23,16 +24,6 @@ class WrongAttributeVisitor(BaseNodeVisitor):
         'mcs',
     ))
 
-    _allowed_magic_attributes: ClassVar[FrozenSet[str]] = frozenset((
-        '__class__',
-        '__name__',
-        '__qualname__',
-        '__doc__',
-        '__subclasses__',
-        '__mro__',
-        '__version__',
-    ))
-
     def visit_Attribute(self, node: ast.Attribute) -> None:
         """Checks the `Attribute` node."""
         self._check_protected_attribute(node)
@@ -59,7 +50,7 @@ def _check_protected_attribute(self, node: ast.Attribute) -> None:
 
     def _check_magic_attribute(self, node: ast.Attribute) -> None:
         if access.is_magic(node.attr):
-            if node.attr not in self._allowed_magic_attributes:
+            if node.attr in ALL_MAGIC_METHODS:
                 self._ensure_attribute_type(
                     node, DirectMagicAttributeAccessViolation,
                 )