From f58ad682615c8cc2e6119cf18b414f12b71d9954 Mon Sep 17 00:00:00 2001 From: Martin Thoma Date: Sun, 29 Nov 2020 09:57:33 +0100 Subject: [PATCH] ENH: Add SIM114 to detect duplicate blocks See issue #10 --- .pre-commit-config.yaml | 8 +-- flake8_simplify.py | 119 +++++++++++++++++++++++++++++++++++++--- requirements-dev.in | 1 - requirements-dev.txt | 56 +++++++++---------- tests/test_simplify.py | 56 ++++++++++++++++++- 5 files changed, 194 insertions(+), 46 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index ef16322..3c753ad 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,7 +1,7 @@ # pre-commit run --all-files repos: - repo: https://github.com/pre-commit/pre-commit-hooks - rev: v3.2.0 + rev: v3.3.0 hooks: - id: check-ast - id: check-byte-order-marker @@ -18,7 +18,7 @@ repos: # hooks: # - id: flake8 - repo: https://github.com/pre-commit/mirrors-mypy - rev: v0.782 + rev: v0.790 hooks: - id: mypy - repo: https://github.com/asottile/seed-isort-config @@ -26,7 +26,7 @@ repos: hooks: - id: seed-isort-config - repo: https://github.com/pre-commit/mirrors-isort - rev: v5.5.2 + rev: v5.6.4 hooks: - id: isort - repo: https://github.com/psf/black @@ -34,7 +34,7 @@ repos: hooks: - id: black - repo: https://github.com/asottile/pyupgrade - rev: v2.7.2 + rev: v2.7.4 hooks: - id: pyupgrade args: [--py36-plus] diff --git a/flake8_simplify.py b/flake8_simplify.py index 7c18486..5611c2d 100644 --- a/flake8_simplify.py +++ b/flake8_simplify.py @@ -1,5 +1,6 @@ # Core Library import ast +import itertools import sys from collections import defaultdict from typing import Any, DefaultDict, Dict, Generator, List, Tuple, Type, Union @@ -34,6 +35,7 @@ SIM111 = "SIM111 Use 'return all({check} for {target} in {iterable})'" SIM112 = "SIM112 Use '{expected}' instead of '{original}'" SIM113 = "SIM113 Use enumerate instead of '{variable}'" +SIM114 = "SIM114 Use logical or (({cond1}) or ({cond2})) and a single body" SIM201 = "SIM201 Use '{left} != {right}' instead of 'not {left} == {right}'" SIM202 = "SIM202 Use '{left} == {right}' instead of 'not {left} != {right}'" SIM203 = "SIM203 Use '{a} not in {b}' instead of 'not {a} in {b}'" @@ -585,20 +587,30 @@ def _get_sim112(node: ast.Expr) -> List[Tuple[int, int, str]]: and isinstance(node.value.value.value, ast.Name) and node.value.value.value.id == "os" and node.value.value.attr == "environ" - and isinstance(node.value.slice, ast.Index) - and isinstance(node.value.slice.value, STR_TYPES) + and ( + ( + isinstance(node.value.slice, ast.Index) + and isinstance(node.value.slice.value, STR_TYPES) + ) + or isinstance(node.value.slice, ast.Constant) + ) ) if is_index_call: subscript = node.value assert isinstance(subscript, ast.Subscript) slice_ = subscript.slice - assert isinstance(slice_, ast.Index) - string_part = slice_.value - assert isinstance(string_part, STR_TYPES) - if isinstance(string_part, ast.Str): - env_name = string_part.s # Python 3.6 / 3.7 fallback - else: - env_name = string_part.value + if isinstance(slice_, ast.Index): + # Python < 3.9 + string_part = slice_.value + assert isinstance(string_part, STR_TYPES) + if isinstance(string_part, ast.Str): + env_name = string_part.s # Python 3.6 / 3.7 fallback + else: + env_name = string_part.value + elif isinstance(slice_, ast.Constant): + # Python 3.9 + env_name = slice_.value + # Check if this has a change has_change = env_name != env_name.upper() @@ -693,6 +705,94 @@ def _get_sim113(node: ast.For) -> List[Tuple[int, int, str]]: return errors +def _get_sim114(node: ast.If) -> List[Tuple[int, int, str]]: + """ + Find same bodys. + + Examples + -------- + If( + test=Name(id='a', ctx=Load()), + body=[ + Expr( + value=Name(id='b', ctx=Load()), + ), + ], + orelse=[ + If( + test=Name(id='c', ctx=Load()), + body=[ + Expr( + value=Name(id='b', ctx=Load()), + ), + ], + orelse=[], + ), + ], + ), + """ + errors: List[Tuple[int, int, str]] = [] + if_body_pairs = get_if_body_pairs(node) + error_pairs = [] + for ifbody1, ifbody2 in itertools.combinations(if_body_pairs, 2): + if is_body_same(ifbody1[1], ifbody2[1]): + error_pairs.append((ifbody1, ifbody2)) + for ifbody1, ifbody2 in error_pairs: + errors.append( + ( + ifbody1[0].lineno, + ifbody1[0].col_offset, + SIM114.format( + cond1=to_source(ifbody1[0]), cond2=to_source(ifbody2[0]) + ), + ) + ) + return errors + + +def get_if_body_pairs(node: ast.If) -> List[Tuple[ast.expr, List[ast.stmt]]]: + pairs = [(node.test, node.body)] + orelse = node.orelse + while ( + isinstance(orelse, list) + and len(orelse) == 1 + and isinstance(orelse[0], ast.If) + ): + pairs.append((orelse[0].test, orelse[0].body)) + orelse = orelse[0].orelse + return pairs + + +def is_body_same(body1: List[ast.stmt], body2: List[ast.stmt]) -> bool: + """Check if two lists of expressions are equivalent.""" + if len(body1) != len(body2): + return False + for a, b in zip(body1, body2): + try: + stmt_equal = is_stmt_equal(a, b) + except RecursionError: # maximum recursion depth + stmt_equal = False + if not stmt_equal: + return False + return True + + +def is_stmt_equal(a: ast.stmt, b: ast.stmt) -> bool: + if type(a) is not type(b): + return False + if isinstance(a, ast.AST): + for k, v in vars(a).items(): + if k in ("lineno", "col_offset", "ctx", "end_lineno"): + continue + if not is_stmt_equal(v, getattr(b, k)): + return False + return True + elif isinstance(a, list): + return all(itertools.starmap(is_stmt_equal, zip(a, b))) + else: + return a == b + + def is_constant_increase(expr: ast.AugAssign) -> bool: return isinstance(expr.op, ast.Add) and isinstance( expr.value, (ast.Constant, ast.Num) @@ -1107,6 +1207,7 @@ def visit_If(self, node: ast.If) -> None: self.errors += _get_sim103(node) self.errors += _get_sim106(node) self.errors += _get_sim108(node) + self.errors += _get_sim114(node) self.generic_visit(node) def visit_For(self, node: ast.For) -> None: diff --git a/requirements-dev.in b/requirements-dev.in index 29a2894..a53239b 100644 --- a/requirements-dev.in +++ b/requirements-dev.in @@ -1,5 +1,4 @@ junit2html -mutmut pip-tools pre-commit pytest diff --git a/requirements-dev.txt b/requirements-dev.txt index d01061b..3cb492d 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -5,56 +5,50 @@ # pip-compile requirements-dev.in # appdirs==1.4.4 # via virtualenv -attrs==20.2.0 # via pytest +attrs==20.3.0 # via pytest bleach==3.2.1 # via readme-renderer -certifi==2020.6.20 # via requests -cffi==1.14.2 # via cryptography +certifi==2020.11.8 # via requests +cffi==1.14.4 # via cryptography cfgv==3.2.0 # via pre-commit chardet==3.0.4 # via requests -click==7.1.2 # via mutmut, pip-tools -colorama==0.4.3 # via twine +click==7.1.2 # via pip-tools +colorama==0.4.4 # via twine coverage==5.3 # via pytest-cov -cryptography==3.1 # via secretstorage +cryptography==3.2.1 # via secretstorage distlib==0.3.1 # via virtualenv docutils==0.16 # via readme-renderer filelock==3.0.12 # via virtualenv -glob2==0.7 # via mutmut -identify==1.5.3 # via pre-commit +identify==1.5.10 # via pre-commit idna==2.10 # via requests -iniconfig==1.0.1 # via pytest -jeepney==0.4.3 # via keyring, secretstorage -junit-xml==1.8 # via mutmut -junit2html==25 # via -r requirements-dev.in -keyring==21.4.0 # via twine -more-itertools==8.5.0 # via pytest -mutmut==2.1.0 # via -r requirements-dev.in +iniconfig==1.1.1 # via pytest +jeepney==0.6.0 # via keyring, secretstorage +junit2html==26 # via -r requirements-dev.in +keyring==21.5.0 # via twine nodeenv==1.5.0 # via pre-commit -packaging==20.4 # via bleach, pytest -parso==0.8.0 # via mutmut -pip-tools==5.3.1 # via -r requirements-dev.in -pkginfo==1.5.0.1 # via twine +packaging==20.7 # via bleach, pytest +pip-tools==5.4.0 # via -r requirements-dev.in +pkginfo==1.6.1 # via twine pluggy==0.13.1 # via pytest -pony==0.7.13 # via mutmut -pre-commit==2.7.1 # via -r requirements-dev.in +pre-commit==2.9.2 # via -r requirements-dev.in py==1.9.0 # via pytest pycparser==2.20 # via cffi -pygments==2.7.1 # via readme-renderer +pygments==2.7.2 # via readme-renderer pyparsing==2.4.7 # via packaging +pytest==6.1.2 # via -r requirements-dev.in, pytest-cov, pytest-timeout pytest-cov==2.10.1 # via -r requirements-dev.in pytest-timeout==1.4.2 # via -r requirements-dev.in -pytest==6.0.2 # via -r requirements-dev.in, pytest-cov, pytest-timeout pyyaml==5.3.1 # via pre-commit -readme-renderer==26.0 # via twine +readme-renderer==28.0 # via twine +requests==2.25.0 # via requests-toolbelt, twine requests-toolbelt==0.9.1 # via twine -requests==2.24.0 # via requests-toolbelt, twine rfc3986==1.4.0 # via twine -secretstorage==3.1.2 # via keyring -six==1.15.0 # via bleach, cryptography, junit-xml, packaging, pip-tools, readme-renderer, virtualenv -toml==0.10.1 # via pre-commit, pytest -tqdm==4.49.0 # via twine +secretstorage==3.3.0 # via keyring +six==1.15.0 # via bleach, cryptography, pip-tools, readme-renderer, virtualenv +toml==0.10.2 # via pre-commit, pytest +tqdm==4.54.0 # via twine twine==3.2.0 # via -r requirements-dev.in -urllib3==1.25.10 # via requests -virtualenv==20.0.31 # via pre-commit +urllib3==1.26.2 # via requests +virtualenv==20.2.1 # via pre-commit webencodings==0.5.1 # via bleach wheel==0.35.1 # via -r requirements-dev.in diff --git a/tests/test_simplify.py b/tests/test_simplify.py index d652c55..b7b8e96 100644 --- a/tests/test_simplify.py +++ b/tests/test_simplify.py @@ -3,7 +3,7 @@ from typing import Set # First party -from flake8_simplify import Plugin +from flake8_simplify import Plugin, get_if_body_pairs def _results(code: str) -> Set[str]: @@ -296,6 +296,60 @@ def test_sim_113_false_positive(): assert ret == set() +def test_sim_114(): + ret = _results( + """if a: + b +elif c: + b""" + ) + assert ret == {"1:3 SIM114 Use logical or ((a) or (c)) and a single body"} + + +def test_get_if_body_pairs(): + ret = ast.parse( + """if a == b: + foo(a) + foo(b)""" + ).body[0] + assert isinstance(ret, ast.If) + result = get_if_body_pairs(ret) + assert len(result) == 1 + comp = result[0][0] + assert isinstance(comp, ast.Compare) + assert isinstance(result[0][1], list) + assert isinstance(comp.left, ast.Name) + assert len(comp.ops) == 1 + assert isinstance(comp.ops[0], ast.Eq) + assert len(comp.comparators) == 1 + assert isinstance(comp.comparators[0], ast.Name) + assert comp.left.id == "a" + assert comp.comparators[0].id == "b" + + +def test_get_if_body_pairs_2(): + ret = ast.parse( + """if a == b: + foo(a) + foo(b) +elif a == b: + foo(c)""" + ).body[0] + assert isinstance(ret, ast.If) + result = get_if_body_pairs(ret) + assert len(result) == 2 + comp = result[0][0] + assert isinstance(comp, ast.Compare) + assert isinstance(result[0][1], list) + assert isinstance(comp.left, ast.Name) + assert len(comp.ops) == 1 + assert isinstance(comp.ops[0], ast.Eq) + assert len(comp.comparators) == 1 + assert isinstance(comp.comparators[0], ast.Name) + assert comp.left.id == "a" + assert comp.comparators[0].id == "b" + + def test_sim_201(): ret = _results("not a == b") assert ret == {"1:0 SIM201 Use 'a != b' instead of 'not a == b'"}