From ddc5dc4b36695cfb82dc92a0d308abc0084d8d77 Mon Sep 17 00:00:00 2001 From: Arie Bovenberg Date: Thu, 2 Dec 2021 16:40:05 +0100 Subject: [PATCH 1/4] only disallow certain magic attributes (#2268) --- CHANGELOG.md | 2 + .../test_attributes/test_magic_attributes.py | 33 ++++- wemake_python_styleguide/violations/oop.py | 16 +- .../visitors/ast/attributes.py | 140 ++++++++++++++++-- 4 files changed, 168 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1171c47a3..5e68f5959 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,8 @@ 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 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 index 106a37007..d2088214b 100644 --- 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/violations/oop.py b/wemake_python_styleguide/violations/oop.py index de6c947df..0985bdbcc 100644 --- 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,20 @@ 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. .. 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 index 10a2fd5ae..e47f69b1c 100644 --- a/wemake_python_styleguide/visitors/ast/attributes.py +++ b/wemake_python_styleguide/visitors/ast/attributes.py @@ -23,14 +23,136 @@ class WrongAttributeVisitor(BaseNodeVisitor): 'mcs', )) - _allowed_magic_attributes: ClassVar[FrozenSet[str]] = frozenset(( - '__class__', - '__name__', - '__qualname__', - '__doc__', - '__subclasses__', - '__mro__', - '__version__', + _disallowed_magic_attributes: ClassVar[FrozenSet[str]] = frozenset(( + # docs.python.org/3/reference/datamodel.html#special-method-names + '__new__', + '__init__', + '__del__', + '__repr__', + '__str__', + '__unicode__', + '__bytes__', + '__format__', + '__cmp__', + '__lt__', + '__le__', + '__eq__', + '__ne__', + '__gt__', + '__ge__', + '__hash__', + '__bool__', + '__nonzero__', + '__getattr__', + '__getattribute__', + '__setattr__', + '__delattr__', + '__dir__', + '__sizeof__', + '__get__', + '__set__', + '__delete__', + '__init_subclass__', + '__set_name__', + '__instancecheck__', + '__subclasscheck__', + '__class_getitem__', + '__call__', + '__len__', + '__length_hint__', + '__getitem__', + '__setitem__', + '__missing__', + '__iter__', + '__next__', + '__reversed__', + '__contains__', + '__add__', + '__sub__', + '__mul__', + '__matmul__', + '__truediv__', + '__floordiv__', + '__mod__', + '__divmod__', + '__pow__', + '__lshift__', + '__rshift__', + '__and__', + '__xor__', + '__or__', + '__radd__', + '__rsub__', + '__rmul__', + '__rmatmul__', + '__rtruediv__', + '__rfloordiv__', + '__rmod__', + '__rdivmod__', + '__rpow__', + '__rlshift__', + '__rrshift__', + '__rand__', + '__rxor__', + '__ror__', + '__iadd__', + '__isub__', + '__imul__', + '__imatmul__', + '__itruediv__', + '__ifloordiv__', + '__imod__', + '__ipow__', + '__ilshift__', + '__irshift__', + '__iand__', + '__ixor__', + '__ior__', + '__neg__', + '__pos__', + '__abs__', + '__invert__', + '__complex__', + '__int__', + '__long__', + '__float__', + '__hex__', + '__oct__', + '__index__', + '__round__', + '__trunc__', + '__coerce__', + '__floor__', + '__ceil__', + '__enter__', + '__exit__', + '__await__', + '__aiter__', + '__anext__', + '__aenter__', + '__aexit__', + + # pickling + '__getnewargs_ex__', + '__getnewargs__', + '__getstate__', + '__setstate__', + '__reduce__', + '__reduce_ex__', + '__getinitargs__', + + # copy + '__copy__', + '__deepcopy__', + + # dataclasses + '__post_init__', + + # inspect + '__signature__', + + # os.path + '__fspath__', )) def visit_Attribute(self, node: ast.Attribute) -> None: @@ -59,7 +181,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 self._disallowed_magic_attributes: self._ensure_attribute_type( node, DirectMagicAttributeAccessViolation, ) From 17eceeef9e9e1764bbac23b11869a5639ddff67e Mon Sep 17 00:00:00 2001 From: Arie Bovenberg Date: Thu, 2 Dec 2021 18:10:38 +0100 Subject: [PATCH 2/4] use already defined magic methods in constants --- wemake_python_styleguide/constants.py | 35 +++++ wemake_python_styleguide/violations/oop.py | 5 + .../visitors/ast/attributes.py | 135 +----------------- 3 files changed, 42 insertions(+), 133 deletions(-) diff --git a/wemake_python_styleguide/constants.py b/wemake_python_styleguide/constants.py index 7859d49d7..8fb06a544 100644 --- 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,38 @@ '__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__', + + # 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 index 0985bdbcc..817c908ad 100644 --- a/wemake_python_styleguide/violations/oop.py +++ b/wemake_python_styleguide/violations/oop.py @@ -423,6 +423,11 @@ class DirectMagicAttributeAccessViolation(ASTViolation): ``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 index e47f69b1c..fa3bb8c51 100644 --- 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,138 +24,6 @@ class WrongAttributeVisitor(BaseNodeVisitor): 'mcs', )) - _disallowed_magic_attributes: ClassVar[FrozenSet[str]] = frozenset(( - # docs.python.org/3/reference/datamodel.html#special-method-names - '__new__', - '__init__', - '__del__', - '__repr__', - '__str__', - '__unicode__', - '__bytes__', - '__format__', - '__cmp__', - '__lt__', - '__le__', - '__eq__', - '__ne__', - '__gt__', - '__ge__', - '__hash__', - '__bool__', - '__nonzero__', - '__getattr__', - '__getattribute__', - '__setattr__', - '__delattr__', - '__dir__', - '__sizeof__', - '__get__', - '__set__', - '__delete__', - '__init_subclass__', - '__set_name__', - '__instancecheck__', - '__subclasscheck__', - '__class_getitem__', - '__call__', - '__len__', - '__length_hint__', - '__getitem__', - '__setitem__', - '__missing__', - '__iter__', - '__next__', - '__reversed__', - '__contains__', - '__add__', - '__sub__', - '__mul__', - '__matmul__', - '__truediv__', - '__floordiv__', - '__mod__', - '__divmod__', - '__pow__', - '__lshift__', - '__rshift__', - '__and__', - '__xor__', - '__or__', - '__radd__', - '__rsub__', - '__rmul__', - '__rmatmul__', - '__rtruediv__', - '__rfloordiv__', - '__rmod__', - '__rdivmod__', - '__rpow__', - '__rlshift__', - '__rrshift__', - '__rand__', - '__rxor__', - '__ror__', - '__iadd__', - '__isub__', - '__imul__', - '__imatmul__', - '__itruediv__', - '__ifloordiv__', - '__imod__', - '__ipow__', - '__ilshift__', - '__irshift__', - '__iand__', - '__ixor__', - '__ior__', - '__neg__', - '__pos__', - '__abs__', - '__invert__', - '__complex__', - '__int__', - '__long__', - '__float__', - '__hex__', - '__oct__', - '__index__', - '__round__', - '__trunc__', - '__coerce__', - '__floor__', - '__ceil__', - '__enter__', - '__exit__', - '__await__', - '__aiter__', - '__anext__', - '__aenter__', - '__aexit__', - - # pickling - '__getnewargs_ex__', - '__getnewargs__', - '__getstate__', - '__setstate__', - '__reduce__', - '__reduce_ex__', - '__getinitargs__', - - # copy - '__copy__', - '__deepcopy__', - - # dataclasses - '__post_init__', - - # inspect - '__signature__', - - # os.path - '__fspath__', - )) - def visit_Attribute(self, node: ast.Attribute) -> None: """Checks the `Attribute` node.""" self._check_protected_attribute(node) @@ -181,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 in self._disallowed_magic_attributes: + if node.attr in ALL_MAGIC_METHODS: self._ensure_attribute_type( node, DirectMagicAttributeAccessViolation, ) From 5388faf6e1fc23e0679e1876c6aefa3b56592fea Mon Sep 17 00:00:00 2001 From: Nikita Sobolev Date: Thu, 2 Dec 2021 23:11:06 +0300 Subject: [PATCH 3/4] Update wemake_python_styleguide/constants.py --- wemake_python_styleguide/constants.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/wemake_python_styleguide/constants.py b/wemake_python_styleguide/constants.py index 8fb06a544..60addbc2b 100644 --- a/wemake_python_styleguide/constants.py +++ b/wemake_python_styleguide/constants.py @@ -290,6 +290,11 @@ # dataclasses '__post_init__', + # attrs: + '__attrs_pre_init__', + '__attrs_init__', + '__attrs_post_init__', + # inspect '__signature__', From bda482a1eed6d4cc270c643f514a41dbb87a42e7 Mon Sep 17 00:00:00 2001 From: Arie Bovenberg Date: Thu, 2 Dec 2021 22:56:04 +0100 Subject: [PATCH 4/4] add changelog entry about fixing some forgotten magic methods --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e68f5959..f566ce5be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ Semantic versioning in our case means: - Fixes that `InconsistentComprehensionViolation` was ignoring misaligned `in` expressions #2075 +- Fixes some common magic methods not being recognized as such #2281 ### Misc