From b4aa3f150516f401ebcc8d9a728b8fdb38c22987 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Mon, 6 Nov 2023 15:34:43 +0300 Subject: [PATCH] Forbid importing the same object under different aliases --- CHANGELOG.md | 2 + tests/fixtures/noqa/noqa.py | 1 + tests/test_checker/test_noqa.py | 3 +- .../test_import_object_collision.py | 116 ++++++++++++++++++ .../logic/tree/imports.py | 14 +++ .../violations/best_practices.py | 28 +++++ .../visitors/ast/imports.py | 14 ++- 7 files changed, 176 insertions(+), 2 deletions(-) create mode 100644 tests/test_visitors/test_ast/test_imports/test_import_object_collision.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d80686c5..525583072 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,8 @@ Semantic versioning in our case means: - `wemake` output formatter now respects `NO_COLOR=1` option to disable text highlighting. See https://no-color.org +- Add `ImportObjectCollisionViolation` to detect + the same objects imported under different aliases ### Bugfixes diff --git a/tests/fixtures/noqa/noqa.py b/tests/fixtures/noqa/noqa.py index b3e8a1109..3abfff7b6 100644 --- a/tests/fixtures/noqa/noqa.py +++ b/tests/fixtures/noqa/noqa.py @@ -16,6 +16,7 @@ from foo import bar from foo.bar import baz # noqa: WPS458 +from package import module, module as alias # noqa: WPS474 from .version import get_version # noqa: WPS300 diff --git a/tests/test_checker/test_noqa.py b/tests/test_checker/test_noqa.py index 889294f19..33df17307 100644 --- a/tests/test_checker/test_noqa.py +++ b/tests/test_checker/test_noqa.py @@ -265,6 +265,8 @@ 'WPS470': 1, 'WPS471': 1, 'WPS472': 1, + 'WPS473': 0, + 'WPS474': 1, 'WPS500': 1, 'WPS501': 1, @@ -317,7 +319,6 @@ 'WPS614': 1, 'WPS615': 2, 'WPS616': 1, - 'WPS473': 0, }) # Violations which may be tweaked by `i_control_code` option: diff --git a/tests/test_visitors/test_ast/test_imports/test_import_object_collision.py b/tests/test_visitors/test_ast/test_imports/test_import_object_collision.py new file mode 100644 index 000000000..42a217ea0 --- /dev/null +++ b/tests/test_visitors/test_ast/test_imports/test_import_object_collision.py @@ -0,0 +1,116 @@ +import pytest + +from wemake_python_styleguide.violations.best_practices import ( + ImportObjectCollisionViolation, +) +from wemake_python_styleguide.violations.consistency import ( + DottedRawImportViolation, + LocalFolderImportViolation, +) +from wemake_python_styleguide.visitors.ast.imports import WrongImportVisitor + +# Correct: + +correct_single_import_from = 'from utils import public' +correct_no_colliding_imports = """ +from other import public +from other import correct +from third import name1, name2 +""" + +correct_aliases = """ +from other import public as alias1 +from my import public as alias2 +from third import public +from four.alias1 import public as alias3, other as alias4 +""" + +correct_same_module_imports = """ +from my import name1 as alias1 +from my import name2 as alias2 +""" + +correct_dot_imports = """ +from . import sub +from .. import sub as alias1 +from ... import sub as alias2 +""" + +correct_relative_imports = """ +from .sub import name +from ..sub import name as alias1 +from ...sub import name as alias2 +""" + +# Wrong: + +colliding_object_import1 = """ +from module import name, name as alias +""" + +colliding_object_import2 = """ +from module import name +from module import name as alias +""" + +colliding_nested_object_import = """ +from module.name import sub +from module.name import sub as alias +""" + +colliding_dot_import1 = """ +from . import sub +from . import sub as alias +""" + +colliding_dot_import2 = """ +from ...package import sub +from ...package import sub as alias +""" + + +@pytest.mark.parametrize('code', [ + correct_single_import_from, + correct_no_colliding_imports, + correct_aliases, + correct_same_module_imports, + correct_dot_imports, + correct_relative_imports, +]) +def test_correct_imports( + assert_errors, + parse_ast_tree, + code, + default_options, +): + """Testing that no colliding imports are allowed.""" + tree = parse_ast_tree(code) + + visitor = WrongImportVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, [], ignored_types=(LocalFolderImportViolation,)) + + +@pytest.mark.parametrize('code', [ + colliding_object_import1, + colliding_object_import2, + colliding_nested_object_import, +]) +def test_imports_collision( + assert_errors, + parse_ast_tree, + code, + default_options, +): + """Testing that colliding imports are restricted.""" + tree = parse_ast_tree(code) + + visitor = WrongImportVisitor(default_options, tree=tree) + visitor.run() + + assert_errors( + visitor, + [ImportObjectCollisionViolation], + ignored_types=(LocalFolderImportViolation,) + ) diff --git a/wemake_python_styleguide/logic/tree/imports.py b/wemake_python_styleguide/logic/tree/imports.py index c81c27823..2f796be3b 100644 --- a/wemake_python_styleguide/logic/tree/imports.py +++ b/wemake_python_styleguide/logic/tree/imports.py @@ -16,6 +16,20 @@ class ImportedObjectInfo(NamedTuple): node: AnyImport +def get_module_name(node: ast.ImportFrom) -> str: + """ + Returns module name for any ``ImportFrom``. + + Handles all corner cases, including: + - `from . import a` -> `.` + - `from ..sub import b` -> `..sub` + """ + return '{0}{1}'.format( + '.' * node.level, + node.module or '' + ) + + def get_import_parts(node: AnyImport) -> List[str]: """Returns list of import modules.""" module_path = getattr(node, 'module', '') or '' diff --git a/wemake_python_styleguide/violations/best_practices.py b/wemake_python_styleguide/violations/best_practices.py index b5081f614..4bb7440d8 100644 --- a/wemake_python_styleguide/violations/best_practices.py +++ b/wemake_python_styleguide/violations/best_practices.py @@ -2819,3 +2819,31 @@ def func(name): error_template = 'Found too many empty lines in `def`: {0}' code = 473 + + +@final +class ImportObjectCollisionViolation(ASTViolation): + """ + Do not allow importing the same object under different aliases. + + Reasoning: + This can lead to reader confusion, + because two names usually mean two different things. + + Solution: + Remove useless aliases. + + Example:: + + # Correct: + from module import name + + # Wrong: + from module import name, name as alias + + .. versionadded:: 0.19.0 + + """ + + error_template = 'Found import object collision: {0}' + code = 474 diff --git a/wemake_python_styleguide/visitors/ast/imports.py b/wemake_python_styleguide/visitors/ast/imports.py index 05c871c22..b1fb0e8c6 100644 --- a/wemake_python_styleguide/visitors/ast/imports.py +++ b/wemake_python_styleguide/visitors/ast/imports.py @@ -1,6 +1,7 @@ import ast +from collections import defaultdict from itertools import chain, product -from typing import Iterable, List +from typing import DefaultDict, Iterable, List, Set from typing_extensions import Final, final @@ -13,6 +14,7 @@ from wemake_python_styleguide.violations.best_practices import ( FutureImportViolation, ImportCollisionViolation, + ImportObjectCollisionViolation, NestedImportViolation, ProtectedModuleMemberViolation, ProtectedModuleViolation, @@ -145,6 +147,9 @@ class _ImportCollisionValidator(object): def __init__(self, error_callback: ErrorCallback) -> None: self._error_callback = error_callback self._imported_names: List[imports.ImportedObjectInfo] = [] + # This helps us to detect cases like: + # `from x import y, y as z` + self._imported_objects: DefaultDict[str, Set[str]] = defaultdict(set) def validate(self) -> None: """Validates that there are no intersecting imported modules.""" @@ -173,6 +178,13 @@ def add_import(self, node: ast.Import) -> None: def add_import_from(self, node: ast.ImportFrom) -> None: """Extract info needed for validation from ``ast.ImportFrom``.""" for alias in node.names: + identifier = imports.get_module_name(node) + if alias.name in self._imported_objects[identifier]: + self._error_callback( + ImportObjectCollisionViolation(node, alias.name), + ) + self._imported_objects[identifier].add(alias.name) + if not alias.asname: self._imported_names.append(imports.ImportedObjectInfo( _MODULE_MEMBERS_SEPARATOR.join(