diff options
| -rw-r--r-- | ChangeLog | 5 | ||||
| -rw-r--r-- | doc/whatsnew/2.13.rst | 4 | ||||
| -rw-r--r-- | pylint/checkers/__init__.py | 3 | ||||
| -rw-r--r-- | pylint/checkers/utils.py | 5 | ||||
| -rw-r--r-- | pylint/extensions/private_import.py | 248 | ||||
| -rw-r--r-- | tests/extensions/test_private_import.py | 69 | ||||
| -rw-r--r-- | tests/functional/ext/private_import/private_import.py | 121 | ||||
| -rw-r--r-- | tests/functional/ext/private_import/private_import.rc | 2 | ||||
| -rw-r--r-- | tests/functional/ext/private_import/private_import.txt | 20 |
9 files changed, 476 insertions, 1 deletions
@@ -54,6 +54,11 @@ Release date: TBA closes #5722 +* New extension ``import-private-name``: indicate imports of external private packages + and objects (prefixed with ``_``). It can be loaded using ``load-plugins=pylint.extensions.private_import``. + + Closes #5463 + * Fixed crash from ``arguments-differ`` and ``arguments-renamed`` when methods were defined outside the top level of a class. diff --git a/doc/whatsnew/2.13.rst b/doc/whatsnew/2.13.rst index 130a9ac69..d277d0d82 100644 --- a/doc/whatsnew/2.13.rst +++ b/doc/whatsnew/2.13.rst @@ -76,6 +76,10 @@ Removed checkers Extensions ========== +* New extension ``import-private-name``: indicate imports of external private packages + and objects (prefixed with ``_``). It can be loaded using ``load-plugins=pylint.extensions.private_import``. + + Closes #5463 * Pyreverse - add output in mermaid-js format and html which is an mermaid js diagram with html boilerplate diff --git a/pylint/checkers/__init__.py b/pylint/checkers/__init__.py index 160bad8af..61ce67ffe 100644 --- a/pylint/checkers/__init__.py +++ b/pylint/checkers/__init__.py @@ -45,7 +45,8 @@ Base id of standard checkers (used in msg and report ids): 24: non-ascii-names 25: unicode 26: unsupported_version -27-50: not yet used: reserved for future internal checkers. +27: private-import +28-50: not yet used: reserved for future internal checkers. This file is not updated. Use script/get_unused_message_id_category.py to get the next free checker id. diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index 4e775e3d2..f53ae8845 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -1664,6 +1664,11 @@ def is_typing_guard(node: nodes.If) -> bool: ) and node.test.as_string().endswith("TYPE_CHECKING") +def is_node_in_typing_guarded_import_block(node: nodes.NodeNG) -> bool: + """Return True if node is part for guarded `typing.TYPE_CHECKING` if block.""" + return isinstance(node.parent, nodes.If) and is_typing_guard(node.parent) + + def is_node_in_guarded_import_block(node: nodes.NodeNG) -> bool: """Return True if node is part for guarded if block. diff --git a/pylint/extensions/private_import.py b/pylint/extensions/private_import.py new file mode 100644 index 000000000..88033fa1e --- /dev/null +++ b/pylint/extensions/private_import.py @@ -0,0 +1,248 @@ +"""Check for imports on private external modules and names.""" +from pathlib import Path +from typing import TYPE_CHECKING, Dict, List, Union + +from astroid import nodes + +from pylint.checkers import BaseChecker, utils +from pylint.interfaces import HIGH, IAstroidChecker + +if TYPE_CHECKING: + from pylint.lint.pylinter import PyLinter + + +class PrivateImportChecker(BaseChecker): + + __implements__ = (IAstroidChecker,) + name = "import-private-name" + msgs = { + "C2701": ( + "Imported private %s (%s)", + "import-private-name", + "Used when a private module or object prefixed with _ is imported. " + "PEP8 guidance on Naming Conventions states that public attributes with " + "leading underscores should be considered private.", + ), + } + + def __init__(self, linter: "PyLinter") -> None: + BaseChecker.__init__(self, linter) + + # A mapping of private names used as a type annotation to whether it is an acceptable import + self.all_used_type_annotations: Dict[str, bool] = {} + self.populated_annotations = False + + @utils.check_messages("import-private-name") + def visit_import(self, node: nodes.Import) -> None: + if utils.is_node_in_typing_guarded_import_block(node): + return + names = [name[0] for name in node.names] + private_names = self._get_private_imports(names) + private_names = self._get_type_annotation_names(node, private_names) + if private_names: + imported_identifier = "modules" if len(private_names) > 1 else "module" + private_name_string = ", ".join(private_names) + self.add_message( + "import-private-name", + node=node, + args=(imported_identifier, private_name_string), + confidence=HIGH, + ) + + @utils.check_messages("import-private-name") + def visit_importfrom(self, node: nodes.ImportFrom) -> None: + if utils.is_node_in_typing_guarded_import_block(node): + return + # Only check imported names if the module is external + if self.same_root_dir(node, node.modname): + return + + names = [n[0] for n in node.names] + + # Check the imported objects first. If they are all valid type annotations, the package can be private + private_names = self._get_type_annotation_names(node, names) + if not private_names: + return + + # There are invalid imported objects, so check the name of the package + private_module_imports = self._get_private_imports([node.modname]) + private_module_imports = self._get_type_annotation_names( + node, private_module_imports + ) + if private_module_imports: + self.add_message( + "import-private-name", + node=node, + args=("module", private_module_imports[0]), + confidence=HIGH, + ) + return # Do not emit messages on the objects if the package is private + + private_names = self._get_private_imports(private_names) + + if private_names: + imported_identifier = "objects" if len(private_names) > 1 else "object" + private_name_string = ", ".join(private_names) + self.add_message( + "import-private-name", + node=node, + args=(imported_identifier, private_name_string), + confidence=HIGH, + ) + + def _get_private_imports(self, names: List[str]) -> List[str]: + """Returns the private names from input names by a simple string check.""" + return [name for name in names if self._name_is_private(name)] + + @staticmethod + def _name_is_private(name: str) -> bool: + """Returns true if the name exists, starts with `_`, and if len(name) > 4 + it is not a dunder, i.e. it does not begin and end with two underscores. + """ + return ( + bool(name) + and name[0] == "_" + and (len(name) <= 4 or name[1] != "_" or name[-2:] != "__") + ) + + def _get_type_annotation_names( + self, node: nodes.Import, names: List[str] + ) -> List[str]: + """Removes from names any names that are used as type annotations with no other illegal usages.""" + if names and not self.populated_annotations: + self._populate_type_annotations(node.root(), self.all_used_type_annotations) + self.populated_annotations = True + + return [ + n + for n in names + if n not in self.all_used_type_annotations + or ( + n in self.all_used_type_annotations + and not self.all_used_type_annotations[n] + ) + ] + + def _populate_type_annotations( + self, node: nodes.LocalsDictNodeNG, all_used_type_annotations: Dict[str, bool] + ) -> None: + """Adds into the dict `all_used_type_annotations` the names of all names ever used as a type annotation + in the scope and nested scopes of node and whether these names are only used for type checking. + """ + for name in node.locals: + # If we find a private type annotation, make sure we do not mask illegal usages + private_name = None + # All the assignments using this variable that we might have to check for illegal usages later + name_assignments = [] + for usage_node in node.locals[name]: + if isinstance(usage_node, nodes.AssignName) and isinstance( + usage_node.parent, (nodes.AnnAssign, nodes.Assign) + ): + assign_parent = usage_node.parent + if isinstance(assign_parent, nodes.AnnAssign): + name_assignments.append(assign_parent) + private_name = self._populate_type_annotations_annotation( + usage_node.parent.annotation, all_used_type_annotations + ) + elif isinstance(assign_parent, nodes.Assign): + name_assignments.append(assign_parent) + + if isinstance(usage_node, nodes.FunctionDef): + self._populate_type_annotations_function( + usage_node, all_used_type_annotations + ) + if isinstance(usage_node, nodes.LocalsDictNodeNG): + self._populate_type_annotations( + usage_node, all_used_type_annotations + ) + if private_name is not None: + # Found a new private annotation, make sure we are not accessing it elsewhere + all_used_type_annotations[ + private_name + ] = self._assignments_call_private_name(name_assignments, private_name) + + def _populate_type_annotations_function( + self, node: nodes.FunctionDef, all_used_type_annotations: Dict[str, bool] + ) -> None: + """Adds into the dict `all_used_type_annotations` the names of all names used as a type annotation + in the arguments and return type of the function node. + """ + if node.args and node.args.annotations: + for annotation in node.args.annotations: + self._populate_type_annotations_annotation( + annotation, all_used_type_annotations + ) + if node.returns: + self._populate_type_annotations_annotation( + node.returns, all_used_type_annotations + ) + + def _populate_type_annotations_annotation( + self, + node: Union[nodes.Attribute, nodes.Subscript, nodes.Name], + all_used_type_annotations: Dict[str, bool], + ) -> Union[str, None]: + """Handles the possiblity of an annotation either being a Name, i.e. just type, + or a Subscript e.g. `Optional[type]` or an Attribute, e.g. `pylint.lint.linter`. + """ + if isinstance(node, nodes.Name) and node.name not in all_used_type_annotations: + all_used_type_annotations[node.name] = True + return node.name + if isinstance(node, nodes.Subscript): # e.g. Optional[List[str]] + # slice is the next nested type + self._populate_type_annotations_annotation( + node.slice, all_used_type_annotations + ) + # value is the current type name: could be a Name or Attribute + return self._populate_type_annotations_annotation( + node.value, all_used_type_annotations + ) + if isinstance(node, nodes.Attribute): + # An attribute is a type like `pylint.lint.pylinter`. node.expr is the next level up, could be another attribute + return self._populate_type_annotations_annotation( + node.expr, all_used_type_annotations + ) + return None + + @staticmethod + def _assignments_call_private_name( + assignments: List[Union[nodes.AnnAssign, nodes.Assign]], private_name: str + ) -> bool: + """Returns True if no assignments involve accessing `private_name`.""" + if all(not assignment.value for assignment in assignments): + # Variable annotated but unassigned is unallowed because there may be a possible illegal access elsewhere + return False + for assignment in assignments: + current_attribute = None + if isinstance(assignment.value, nodes.Call): + current_attribute = assignment.value.func + elif isinstance(assignment.value, nodes.Attribute): + current_attribute = assignment.value + elif isinstance(assignment.value, nodes.Name): + current_attribute = assignment.value.name + if not current_attribute: + continue + while isinstance(current_attribute, (nodes.Attribute, nodes.Call)): + if isinstance(current_attribute, nodes.Call): + current_attribute = current_attribute.func + current_attribute = current_attribute.expr + if ( + isinstance(current_attribute, nodes.Name) + and current_attribute.name == private_name + ): + return False + return True + + @staticmethod + def same_root_dir(node: nodes.Import, import_mod_name: str) -> bool: + """Does the node's file's path contain the base name of `import_mod_name`?""" + if not import_mod_name: # from . import ... + return True + + base_import_package = import_mod_name.split(".")[0] + + return base_import_package in Path(node.root().file).parent.parts + + +def register(linter: "PyLinter") -> None: + linter.register_checker(PrivateImportChecker(linter)) diff --git a/tests/extensions/test_private_import.py b/tests/extensions/test_private_import.py new file mode 100644 index 000000000..10648a8f9 --- /dev/null +++ b/tests/extensions/test_private_import.py @@ -0,0 +1,69 @@ +"""Tests the local module directory comparison logic which requires mocking file directories""" + +from unittest.mock import patch + +import astroid + +from pylint.extensions import private_import +from pylint.interfaces import HIGH +from pylint.testutils import CheckerTestCase, MessageTest + + +class TestPrivateImport(CheckerTestCase): + """The mocked dirname is the directory of the file being linted, the node is code inside that file""" + + CHECKER_CLASS = private_import.PrivateImportChecker + + @patch("pathlib.Path.parent") + def test_internal_module(self, parent) -> None: + parent.parts = ("", "dir", "module") + import_from = astroid.extract_node("""from module import _file""") + + with self.assertNoMessages(): + self.checker.visit_importfrom(import_from) + + @patch("pathlib.Path.parent") + def test_external_module_nested(self, parent) -> None: + parent.parts = ("", "dir", "module", "module_files", "util") + + import_from = astroid.extract_node("""from module import _file""") + + with self.assertNoMessages(): + self.checker.visit_importfrom(import_from) + + @patch("pathlib.Path.parent") + def test_external_module_dot_import(self, parent) -> None: + parent.parts = ("", "dir", "outer", "inner", "module_files", "util") + + import_from = astroid.extract_node("""from outer.inner import _file""") + + with self.assertNoMessages(): + self.checker.visit_importfrom(import_from) + + @patch("pathlib.Path.parent") + def test_external_module_dot_import_outer_only(self, parent) -> None: + parent.parts = ("", "dir", "outer", "extensions") + + import_from = astroid.extract_node("""from outer.inner import _file""") + + with self.assertNoMessages(): + self.checker.visit_importfrom(import_from) + + @patch("pathlib.Path.parent") + def test_external_module(self, parent) -> None: + parent.parts = ("", "dir", "other") + + import_from = astroid.extract_node("""from module import _file""") + + msg = MessageTest( + msg_id="import-private-name", + node=import_from, + line=1, + col_offset=0, + end_line=1, + end_col_offset=24, + args=("object", "_file"), + confidence=HIGH, + ) + with self.assertAddsMessages(msg): + self.checker.visit_importfrom(import_from) diff --git a/tests/functional/ext/private_import/private_import.py b/tests/functional/ext/private_import/private_import.py new file mode 100644 index 000000000..52ef405c9 --- /dev/null +++ b/tests/functional/ext/private_import/private_import.py @@ -0,0 +1,121 @@ +"""Tests for import-private-name.""" +# pylint: disable=unused-import, missing-docstring, reimported, import-error, wrong-import-order +# pylint: disable=no-name-in-module, multiple-imports, ungrouped-imports, misplaced-future +# pylint: disable=wrong-import-position + +# Basic cases +from _world import hello # [import-private-name] +from _world import _hello # [import-private-name] +from city import _house # [import-private-name] +from city import a, _b, c, _d # [import-private-name] +from _city import a, _b, c, _d # [import-private-name] +from city import a, b, c, _d # [import-private-name] +import house +import _house # [import-private-name] +import _house, _chair, _stair # [import-private-name] +import house, _chair, _stair # [import-private-name] + +# Ignore dunders +import __asd__ +import __future__ +from __future__ import print_function +from __future__ import __print_function__ + +# Ignore local modules +# The check for local modules compares directory names in the path of the file being linted to +# the name of the module we are importing from. The use of `__init__.py` to indicate Python modules +# is deprecated so this is a heuristic solution. +# If we were importing from `pylint`, it would be counted as a valid internal private import +# and not emit a message as long as this file has a parent directory called `pylint`, even though +# we are not importing from that directory. (We would be importing from `pylint/pylint`.) +from private_import import _private # pylint: disable=import-self +from private_import.other_file import _private +from . import _private +from astroid import _private # [import-private-name] +from sys import _private # [import-private-name] + +# Ignore typecheck +from typing import TYPE_CHECKING, List, Optional + +if TYPE_CHECKING: + import _TreeType + from types import _TreeType + from _types import TreeType + from _types import _TreeType + +# No error since imports are used as type annotations +from classes import _PrivateClassA, safe_get_A +from classes import _PrivateClassB +from classes import _PrivateClassC + +a_var: _PrivateClassA = safe_get_A() + +def b_func(class_b: _PrivateClassB): + print(class_b) + +def c_func() -> _PrivateClassC: + return None + +# Used as typing in slices +from classes import _SubScriptA +from classes import _SubScriptB + +a: Optional[_SubScriptA] +b: Optional[_SubScriptB[List]] + +import _TypeContainerA +import _TypeContainerB +import _TypeContainerC + +import SafeContainerA +a2: _TypeContainerA.A = SafeContainerA.safe_get_a() + +def b_func2(class_b2: _TypeContainerB.B): + print(class_b2) + +def c2_func() -> _TypeContainerC.C: + return None + +# This is allowed since all the imports are used for typing +from _TypeContainerExtra import TypeExtraA, TypeExtraB +from MakerContainerExtra import GetA, GetB +extraA: TypeExtraA = GetA() +extraB: TypeExtraB = GetB() + +# This is not allowed because there is an import not used for typing +from _TypeContainerExtra2 import TypeExtra2, NotTypeExtra # [import-private-name] +extra2: TypeExtra2 + +# Try many cases to ensure that type annotation usages of a private import +# do not mask other illegal usages of the import +import _private_module # [import-private-name] +my_var: _private_module.Thing = _private_module.Thing() + +import _private_module2 # [import-private-name] +my_var2: _private_module2.Thing2 +my_var2 = _private_module2.Thing2() + +import _private_module3 # [import-private-name] +my_var3: _private_module3.Thing3 +my_var3 = _private_module3.Thing3 +my_var3_2: _private_module3.Thing3 + +import _private_module4 # [import-private-name] +my_var4: _private_module4.Thing4 +my_var4 = _private_module4.get_callers().get_thing4() + +from _private_module5 import PrivateClass # [import-private-name] +my_var5: PrivateClass +my_var5 = PrivateClass() + +from _private_module6 import PrivateClass2 # [import-private-name] +my_var6: PrivateClass2 = PrivateClass2() + +from public_module import _PrivateClass3 # [import-private-name] +my_var7: _PrivateClass3 = _PrivateClass3() + +# Even though we do not see the private call, the type check does not keep us from emitting +# because we do not use that variable +import _private_module_unreachable # [import-private-name] +my_var8: _private_module_unreachable.Thing8 +_private_module_unreachable.Thing8() diff --git a/tests/functional/ext/private_import/private_import.rc b/tests/functional/ext/private_import/private_import.rc new file mode 100644 index 000000000..c9bbc23f1 --- /dev/null +++ b/tests/functional/ext/private_import/private_import.rc @@ -0,0 +1,2 @@ +[MASTER] +load-plugins=pylint.extensions.private_import, diff --git a/tests/functional/ext/private_import/private_import.txt b/tests/functional/ext/private_import/private_import.txt new file mode 100644 index 000000000..f618d5858 --- /dev/null +++ b/tests/functional/ext/private_import/private_import.txt @@ -0,0 +1,20 @@ +import-private-name:7:0:7:24::Imported private module (_world):HIGH +import-private-name:8:0:8:25::Imported private module (_world):HIGH +import-private-name:9:0:9:23::Imported private object (_house):HIGH +import-private-name:10:0:10:29::Imported private objects (_b, _d):HIGH +import-private-name:11:0:11:30::Imported private module (_city):HIGH +import-private-name:12:0:12:28::Imported private object (_d):HIGH +import-private-name:14:0:14:13::Imported private module (_house):HIGH +import-private-name:15:0:15:29::Imported private modules (_house, _chair, _stair):HIGH +import-private-name:16:0:16:28::Imported private modules (_chair, _stair):HIGH +import-private-name:34:0:34:28::Imported private object (_private):HIGH +import-private-name:35:0:35:24::Imported private object (_private):HIGH +import-private-name:86:0:86:57::Imported private module (_TypeContainerExtra2):HIGH +import-private-name:91:0:91:22::Imported private module (_private_module):HIGH +import-private-name:94:0:94:23::Imported private module (_private_module2):HIGH +import-private-name:98:0:98:23::Imported private module (_private_module3):HIGH +import-private-name:103:0:103:23::Imported private module (_private_module4):HIGH +import-private-name:107:0:107:41::Imported private module (_private_module5):HIGH +import-private-name:111:0:111:42::Imported private module (_private_module6):HIGH +import-private-name:114:0:114:40::Imported private object (_PrivateClass3):HIGH +import-private-name:119:0:119:34::Imported private module (_private_module_unreachable):HIGH |