Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forbid importing the same object under different aliases #2769

Merged
merged 4 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions tests/fixtures/noqa/noqa.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 2 additions & 1 deletion tests/test_checker/test_noqa.py
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,8 @@
'WPS470': 1,
'WPS471': 1,
'WPS472': 1,
'WPS473': 0,
'WPS474': 1,

'WPS500': 1,
'WPS501': 1,
Expand Down Expand Up @@ -317,7 +319,6 @@
'WPS614': 1,
'WPS615': 2,
'WPS616': 1,
'WPS473': 0,
})

# Violations which may be tweaked by `i_control_code` option:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
import pytest

from wemake_python_styleguide.violations.best_practices import (
ImportObjectCollisionViolation,
)
from wemake_python_styleguide.violations.consistency import (
sobolevn marked this conversation as resolved.
Show resolved Hide resolved
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,)
sobolevn marked this conversation as resolved.
Show resolved Hide resolved
)
14 changes: 14 additions & 0 deletions wemake_python_styleguide/logic/tree/imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 ''
sobolevn marked this conversation as resolved.
Show resolved Hide resolved
)


def get_import_parts(node: AnyImport) -> List[str]:
"""Returns list of import modules."""
module_path = getattr(node, 'module', '') or ''
Expand Down
28 changes: 28 additions & 0 deletions wemake_python_styleguide/violations/best_practices.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
14 changes: 13 additions & 1 deletion wemake_python_styleguide/visitors/ast/imports.py
Original file line number Diff line number Diff line change
@@ -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

Expand All @@ -13,6 +14,7 @@
from wemake_python_styleguide.violations.best_practices import (
FutureImportViolation,
ImportCollisionViolation,
ImportObjectCollisionViolation,
NestedImportViolation,
ProtectedModuleMemberViolation,
ProtectedModuleViolation,
Expand Down Expand Up @@ -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."""
Expand Down Expand Up @@ -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(
Expand Down
Loading