diff --git a/.gitignore b/.gitignore index 52b6cd06b..3740acd5f 100644 --- a/.gitignore +++ b/.gitignore @@ -212,3 +212,6 @@ fabric.properties # Used for testing: ex.py setup.py + +# CodeRabbit logs: +logs/ diff --git a/tests/test_visitors/test_ast/test_conditions/test_simplified_match_with_sequence_or_mapping.py b/tests/test_visitors/test_ast/test_conditions/test_simplified_match_with_sequence_or_mapping.py new file mode 100644 index 000000000..2ad183f91 --- /dev/null +++ b/tests/test_visitors/test_ast/test_conditions/test_simplified_match_with_sequence_or_mapping.py @@ -0,0 +1,169 @@ +import pytest + +from wemake_python_styleguide.violations.consistency import ( + SimplifiableMatchWithSequenceOrMappingViolation, +) +from wemake_python_styleguide.visitors.ast.conditions import ( + SimplifiableMatchWithSequenceOrMappingVisitor, +) + +# Wrong: Simple sequence patterns +simple_list_match = """ +match data: + case [1, 2]: + handle_pair() + case _: + ignore() +""" + +simple_nested_list_match = """ +match data: + case [[1, 2], [3, 4]]: + handle_nested() + case _: + ignore() +""" + +simple_tuple_match = """ +match data: + case (1, "a"): + handle_tuple() + case _: + ignore() +""" + +simple_dict_match = """ +match data: + case {"key": "value"}: + handle_dict() + case _: + ignore() +""" + +simple_nested_dict_match = """ +match data: + case {"outer": {"inner": 42}}: + handle_nested_dict() + case _: + ignore() +""" + +mixed_list_dict_match = """ +match data: + case [{"key": "value"}, 42]: + handle_mixed() + case _: + ignore() +""" + +# Correct: Complex patterns that should not be simplified +complex_with_binding = """ +match data: + case [x, y]: + handle_with_binding(x, y) + case _: + ignore() +""" + +with_star_pattern = """ +match data: + case [first, *rest]: + handle_with_rest(first, rest) + case _: + ignore() +""" + +with_rest_name = """ +match data: + case {"key": value, **rest}: + handle_with_rest(value, rest) + case _: + ignore() +""" + +with_guard = """ +match data: + case [1, 2] if condition > 0: + handle_guarded() + case _: + ignore() +""" + +complex_match = """ +match data: + case SomeClass(x): + handle_class() + case _: + ignore() +""" + +no_wildcard = """ +match data: + case [1, 2]: + handle_first() + case [3, 4]: + handle_second() +""" + +more_than_two_cases = """ +match data: + case [1, 2]: + handle_first() + case [3, 4]: + handle_second() + case _: + handle_default() +""" + + +@pytest.mark.parametrize( + 'code', + [ + simple_list_match, + simple_nested_list_match, + simple_tuple_match, + simple_dict_match, + simple_nested_dict_match, + mixed_list_dict_match, + ], +) +def test_simplifiable_sequence_or_mapping_match( + code, + assert_errors, + parse_ast_tree, + default_options, +): + """Test that simple sequence and mapping matches raise a violation.""" + tree = parse_ast_tree(code) + visitor = SimplifiableMatchWithSequenceOrMappingVisitor( + default_options, tree=tree + ) + visitor.run() + assert_errors(visitor, [SimplifiableMatchWithSequenceOrMappingViolation]) + + +@pytest.mark.parametrize( + 'template', + [ + complex_with_binding, + with_star_pattern, + with_rest_name, + with_guard, + complex_match, + no_wildcard, + more_than_two_cases, + ], +) +def test_not_simplifiable_sequence_or_mapping_match( + template, + assert_errors, + parse_ast_tree, + default_options, +): + """Test that complex or non-simplifiable matches do not raise violations.""" + tree = parse_ast_tree(template) + visitor = SimplifiableMatchWithSequenceOrMappingVisitor( + default_options, tree=tree + ) + visitor.run() + assert_errors(visitor, []) diff --git a/wemake_python_styleguide/logic/tree/pattern_matching.py b/wemake_python_styleguide/logic/tree/pattern_matching.py index bfb8f4e01..74e97c918 100644 --- a/wemake_python_styleguide/logic/tree/pattern_matching.py +++ b/wemake_python_styleguide/logic/tree/pattern_matching.py @@ -102,3 +102,96 @@ def is_irrefutable_binding(pattern: ast.pattern) -> bool: and pattern.pattern is None and pattern.name is not None ) + + +def is_simple_sequence_or_mapping_pattern(pattern: ast.pattern) -> bool: + """ + Checks if a pattern is a simple sequence or mapping without + variable bindings. + + Supports: + - Simple lists: ``[1, 2]``, ``["a", "b"]``, ``[const.A, const.B]`` + - Simple tuples: ``(1, 2)``, ``("a", "b")`` + - Simple dicts: ``{"key": "value"}``, ``{1: 2}`` + - No variable bindings (like ``[x, y]``) or starred patterns + (like ``[first, *rest]``) + - No guards allowed + """ + # Check for simple list or tuple patterns + if isinstance(pattern, ast.MatchSequence): + # Check that there are no starred patterns (*rest) + if not _has_star_pattern(pattern): + # Check that all elements are simple patterns (not binding vars) + return all( + _is_simple_pattern_element(element) + for element in pattern.patterns + ) + + # Check for simple dict patterns + elif isinstance(pattern, ast.MatchMapping): + # Check that all keys are simple (not variables) and all + # values are simple patterns + if any( + key is None or not _is_simple_key_pattern(key) + for key in pattern.keys + ): + return False + if pattern.patterns and any( + not _is_simple_pattern_element(value) for value in pattern.patterns + ): + return False + # If rest name is present (has variable binding), it's not simple + return pattern.rest is None + + return False + + +def _has_star_pattern(pattern: ast.MatchSequence) -> bool: + """Check if the sequence pattern contains starred patterns.""" + # ast.MatchStar has been added in Python 3.10 for starred patterns + for sub_pattern in pattern.patterns: + if isinstance(sub_pattern, ast.MatchStar): + return True + return False + + +def _is_simple_pattern_element(pattern: ast.pattern) -> bool: + """Check if a pattern element is simple (not binding variables).""" + # Handle simple value patterns (literals, constants, attributes) + if isinstance(pattern, ast.MatchValue): + return isinstance( + pattern.value, (ast.Constant, ast.Name, ast.Attribute) + ) + + # Handle simple singleton patterns (True, False, None) + if isinstance(pattern, ast.MatchSingleton): + return True + + # Handle simple nested patterns + if isinstance(pattern, (ast.MatchSequence, ast.MatchMapping)): + return is_simple_sequence_or_mapping_pattern(pattern) + + # Handle Union patterns (| operator) + if isinstance(pattern, ast.MatchOr): + return all(_is_simple_pattern_element(sub) for sub in pattern.patterns) + + # Handle MatchAs patterns - not simple if it's binding a variable + if isinstance(pattern, ast.MatchAs): + # If pattern.name is not None, it's a binding (not simple) + if pattern.name is not None: + return False + # If pattern.name is None but pattern.pattern is not None, + # check if the inner pattern is simple + if pattern.pattern is not None: + return _is_simple_pattern_element(pattern.pattern) + # This case should not happen in valid Python ASTs + return False + + # Other pattern types are not simple (like MatchClass with constructors) + return False + + +def _is_simple_key_pattern(key: ast.expr) -> bool: + """Check if a mapping key is simple (not a variable).""" + # Keys should be constants or attributes, not variable names + return isinstance(key, (ast.Constant, ast.Name, ast.Attribute)) diff --git a/wemake_python_styleguide/presets/types/tree.py b/wemake_python_styleguide/presets/types/tree.py index 8cb87565d..ffdbb17d9 100644 --- a/wemake_python_styleguide/presets/types/tree.py +++ b/wemake_python_styleguide/presets/types/tree.py @@ -71,6 +71,7 @@ conditions.MatchVisitor, conditions.ChainedIsVisitor, conditions.SimplifiableMatchVisitor, + conditions.SimplifiableMatchWithSequenceOrMappingVisitor, conditions.LeakingForLoopVisitor, iterables.IterableUnpackingVisitor, blocks.AfterBlockVariablesVisitor, diff --git a/wemake_python_styleguide/violations/consistency.py b/wemake_python_styleguide/violations/consistency.py index dc0ac8a6f..734ac8999 100644 --- a/wemake_python_styleguide/violations/consistency.py +++ b/wemake_python_styleguide/violations/consistency.py @@ -2460,3 +2460,54 @@ class SimplifiableMatchViolation(ASTViolation): 'Found simplifiable `match` statement that can be just `if`' ) code = 365 + + +@final +class SimplifiableMatchWithSequenceOrMappingViolation(ASTViolation): + """ + Some ``match`` statements with simple sequences or mappings can be. + + Reasoning: + Using ``match`` for exact structural comparisons of simple literals + (like lists or dicts) is unnecessarily verbose. While match excels + at deconstruction, using it to check for an exact list or dict value + is better expressed with a direct equality comparison (``==``), + which is more readable and performant. + + Solution: + Replace ``match`` statements that check for simple sequences or + mappings (with no deconstruction) with ``if`` statements using ``==``. + + When is this violation is raised? + - When there are exactly two ``case`` statements + - When the first case uses a simple sequence or mapping pattern + - When the second case is a wildcard: ``case _:`` + - When the pattern contains only literals/contants (no + variable bindings) + - When there are no guards or starred patterns + + + Example:: + + # Correct: + if data == [1, 2]: + handle_pair() + else: + ignore() + + # Wrong: + match data: + case [1, 2]: + handle_pair() + case _: + ignore() + + .. versionadded:: 1.5.0 + + """ + + error_template = ( + 'Found simplifiable `match` statement with sequence or mapping ' + 'that can be just `if`' + ) + code = 366 diff --git a/wemake_python_styleguide/visitors/ast/conditions.py b/wemake_python_styleguide/visitors/ast/conditions.py index 0487facd0..63e624cc9 100644 --- a/wemake_python_styleguide/visitors/ast/conditions.py +++ b/wemake_python_styleguide/visitors/ast/conditions.py @@ -220,6 +220,38 @@ def _check_simplifiable_match(self, node: ast.Match) -> None: self.add_violation(consistency.SimplifiableMatchViolation(node)) +@final +class SimplifiableMatchWithSequenceOrMappingVisitor(BaseNodeVisitor): + """Checks for match statements with simple sequences and mappings.""" + + def visit_Match(self, node: ast.Match) -> None: + """Checks match statements with simple sequences and mappings.""" + self._check_simplifiable_match_seq_or_map(node) + self.generic_visit(node) + + def _check_simplifiable_match_seq_or_map(self, node: ast.Match) -> None: + cases = node.cases + if len(cases) == 2: + first, second = cases + + if not pattern_matching.is_wildcard_pattern(second): + return + + # Check if the first pattern is a simple sequence or mapping pattern + # without variable bindings AND no guard is present + if ( + pattern_matching.is_simple_sequence_or_mapping_pattern( + first.pattern + ) + and first.guard is None # No guard clause + ): + self.add_violation( + consistency.SimplifiableMatchWithSequenceOrMappingViolation( + node + ) + ) + + @final class LeakingForLoopVisitor(BaseNodeVisitor): """Finds 'for' loops directly inside class or module bodies."""