Skip to content

Commit

Permalink
Closes #703
Browse files Browse the repository at this point in the history
  • Loading branch information
sobolevn committed Aug 14, 2019
1 parent 3c1f481 commit 1683d2e
Show file tree
Hide file tree
Showing 7 changed files with 190 additions and 11 deletions.
1 change: 1 addition & 0 deletions tests/fixtures/noqa.py
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,7 @@ def some_other_function():
sum_container += sum_item

print(sum_container == []) # noqa: WPS520
print(sum_container is 0) # noqa: WPS521

try:
anti_z444 = 1
Expand Down
1 change: 1 addition & 0 deletions tests/test_checker/test_noqa.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@
'WPS518': 1,
'WPS519': 1,
'WPS520': 1,
'WPS521': 1,

'WPS600': 1,
'WPS601': 1,
Expand Down
14 changes: 11 additions & 3 deletions tests/test_visitors/test_ast/test_compares/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,12 @@

# Actual fixtures:

EQUAL_COMPARES = frozenset((
IS_COMPARES = frozenset((
if_with_is,
if_with_is_not,
))

EQUAL_COMPARES = frozenset((
if_with_eq,
if_with_not_eq,

Expand All @@ -55,12 +57,18 @@
))


@pytest.fixture(params=EQUAL_COMPARES | OTHER_COMPARES)
@pytest.fixture(params=IS_COMPARES | EQUAL_COMPARES | OTHER_COMPARES)
def simple_conditions(request):
"""Fixture that returns simple conditionals."""
return request.param


@pytest.fixture(params=IS_COMPARES)
def is_conditions(request):
"""Fixture that returns `is` and `is not` conditionals."""
return request.param


@pytest.fixture(params=EQUAL_COMPARES)
def eq_conditions(request):
"""Fixture that returns `eq` and `not eq` conditionals."""
Expand All @@ -69,7 +77,7 @@ def eq_conditions(request):

@pytest.fixture(params=OTHER_COMPARES)
def other_conditions(request):
"""Fixture that returns `eq` and `not eq` conditionals."""
"""Fixture that returns other compare conditionals."""
return request.param


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from wemake_python_styleguide.violations.refactoring import (
FalsyConstantCompareViolation,
WrongIsCompareViolation,
)
from wemake_python_styleguide.visitors.ast.compares import (
WrongConstantCompareVisitor,
Expand Down Expand Up @@ -36,6 +37,26 @@ def test_falsy_constant(
assert_errors(visitor, [FalsyConstantCompareViolation])


@pytest.mark.parametrize('comparators', wrong_comparators)
def test_falsy_constant_is(
assert_errors,
parse_ast_tree,
comparators,
is_conditions,
default_options,
):
"""Testing that compares with falsy contants are not allowed."""
tree = parse_ast_tree(is_conditions.format(*comparators))

visitor = WrongConstantCompareVisitor(default_options, tree=tree)
visitor.run()

assert_errors(visitor, [
FalsyConstantCompareViolation,
WrongIsCompareViolation,
])


@pytest.mark.parametrize('comparators', wrong_comparators)
def test_falsy_constant_not_eq(
assert_errors,
Expand Down Expand Up @@ -86,4 +107,4 @@ def test_correct_constant_compare(
visitor = WrongConstantCompareVisitor(default_options, tree=tree)
visitor.run()

assert_errors(visitor, [])
assert_errors(visitor, [], (WrongIsCompareViolation,))
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
# -*- coding: utf-8 -*-

import pytest

from wemake_python_styleguide.violations.refactoring import (
FalsyConstantCompareViolation,
WrongIsCompareViolation,
)
from wemake_python_styleguide.visitors.ast.compares import (
WrongConstantCompareVisitor,
)

wrong_comparators = (
('some', '[1, 2]'),
('some', '{}'), # noqa: P103
('some', '()'),
('some', '{1, 2, 3}'),

('some', '[x for x in a]'),
('some', '(x for x in a)'),
('some', '{x for x in a}'),
('some', '{"1": x for x in a}'),

('some', '0'),
('some', '1'),
('some', '1.2'),
('some', '-0.1'),
('some', '""'),
('some', '"abc"'),

('some', '(1, 2)'),
('[]', 'some'),
('{1, 2}', 'some'),
('()', 'some'),
('"test"', 'some'),
)


@pytest.mark.parametrize('comparators', wrong_comparators)
def test_wrong_constant_is(
assert_errors,
parse_ast_tree,
comparators,
is_conditions,
default_options,
):
"""Testing that compares with falsy contants are not allowed."""
tree = parse_ast_tree(is_conditions.format(*comparators))

visitor = WrongConstantCompareVisitor(default_options, tree=tree)
visitor.run()

assert_errors(visitor, [WrongIsCompareViolation], (
FalsyConstantCompareViolation,
))


@pytest.mark.parametrize('comparators', [
('some', 'None'),
('some', 'False'),
('some', 'True'),
('None', 'some'),
('some', 'other'),
('some', 'x + y'),
('some', 'other.attr'),
('some', 'call()'),
('some', 'other.method'),
('some', 'other[key]'),
('some', 'some'),
])
def test_correct_constant_is(
assert_errors,
parse_ast_tree,
comparators,
is_conditions,
default_options,
):
"""Testing that compares with falsy contants are not allowed."""
tree = parse_ast_tree(is_conditions.format(*comparators))

visitor = WrongConstantCompareVisitor(default_options, tree=tree)
visitor.run()

assert_errors(visitor, [])
39 changes: 39 additions & 0 deletions wemake_python_styleguide/violations/refactoring.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
ImplicitEnumerateViolation
ImplicitSumViolation
FalsyConstantCompareViolation
WrongIsCompareViolation
Refactoring opportunities
-------------------------
Expand All @@ -61,6 +62,7 @@
.. autoclass:: ImplicitEnumerateViolation
.. autoclass:: ImplicitSumViolation
.. autoclass:: FalsyConstantCompareViolation
.. autoclass:: WrongIsCompareViolation
"""

Expand Down Expand Up @@ -840,3 +842,40 @@ class FalsyConstantCompareViolation(ASTViolation):

code = 520
error_template = 'Found compare with falsy constant'


@final
class WrongIsCompareViolation(ASTViolation):
"""
Forbids to compare values with constants using ``is`` or ``is not``.
However, we allow to compare with ``None`` and booleans.
Reasoning:
``is`` compares might not do what you want them to do.
Firstly, they check for the same object, not equality.
Secondly, they behave unexpectedly even
with the simple values like ``257``.
Solution:
Use ``==`` to compare with constants.
Example::
# Correct:
if my_check == [1, 2, 3]:
...
# Wrong:
if my_check is [1, 2, 3]:
...
See also:
https://stackoverflow.com/a/33130014/4842742
.. versionadded:: 0.12.0
"""

code = 521
error_template = 'Found wrong `is` compare'
38 changes: 31 additions & 7 deletions wemake_python_styleguide/visitors/ast/compares.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
SimplifiableIfViolation,
UselessLenCompareViolation,
WrongInCompareTypeViolation,
WrongIsCompareViolation,
)
from wemake_python_styleguide.visitors.base import BaseNodeVisitor
from wemake_python_styleguide.visitors.decorators import alias
Expand Down Expand Up @@ -157,10 +158,20 @@ def _check_reversed_complex_compare(self, node: ast.Compare) -> None:
class WrongConstantCompareVisitor(BaseNodeVisitor):
"""Restricts incorrect compares with constants."""

_constant_types: ClassVar[AnyNodes] = (
_forbidden_for_is: ClassVar[AnyNodes] = (
ast.List,
ast.ListComp,
ast.Dict,
ast.DictComp,
ast.Tuple,
ast.GeneratorExp,
ast.Set,
ast.SetComp,

# We allow `ast.NameConstant`
ast.Num,
ast.Bytes,
ast.Str,
)

def visit_Compare(self, node: ast.Compare) -> None:
Expand All @@ -171,15 +182,16 @@ def visit_Compare(self, node: ast.Compare) -> None:
FalsyConstantCompareViolation
"""
self._check_falsy_constant(node)
self.generic_visit(node)
self._check_constant(node.ops[0], node.left)
self._check_is_constant_comprare(node.ops[0], node.left)

def _check_falsy_constant(self, node: ast.Compare) -> None:
self._detect_constant(node.ops[0], node.left)
for op, comparator in zip(node.ops, node.comparators):
self._detect_constant(op, comparator)
self._check_constant(op, comparator)
self._check_is_constant_comprare(op, comparator)

self.generic_visit(node)

def _detect_constant(self, op: ast.cmpop, comparator: ast.expr) -> None:
def _check_constant(self, op: ast.cmpop, comparator: ast.expr) -> None:
if not isinstance(op, (ast.Eq, ast.NotEq, ast.Is, ast.IsNot)):
return

Expand All @@ -193,6 +205,18 @@ def _detect_constant(self, op: ast.cmpop, comparator: ast.expr) -> None:
if not length:
self.add_violation(FalsyConstantCompareViolation(comparator))

def _check_is_constant_comprare(
self,
op: ast.cmpop,
comparator: ast.expr,
) -> None:
if not isinstance(op, (ast.Is, ast.IsNot)):
return

unwrapped = operators.unwrap_unary_node(comparator)
if isinstance(unwrapped, self._forbidden_for_is):
self.add_violation(WrongIsCompareViolation(comparator))


@final
class WrongComparisionOrderVisitor(BaseNodeVisitor):
Expand Down

0 comments on commit 1683d2e

Please sign in to comment.