From f426c0fdafe3c75c6e10fdbf7b370236a451736d Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Tue, 16 Jan 2024 19:03:11 +0000 Subject: [PATCH] [`pylint`] (Re-)Implement `import-private-name` (`C2701`) (#9553) ## Summary #5920 with a fix for the erroneous slice in `module_name`. Fixes #9547. ## Test Plan Added `import bbb.ccc._ddd as eee` to the test fixture to ensure it no longer panics. `cargo test` --- crates/ruff_linter/__init__.py | 0 crates/ruff_linter/resources/__init__.py | 0 crates/ruff_linter/resources/test/__init__.py | 0 .../resources/test/fixtures/__init__.py | 0 .../pylint/import_private_name/__init__.py | 1 + .../sibling_module/__init__.py | 3 + .../import_private_name/submodule/__init__.py | 1 + .../import_private_name/submodule/__main__.py | 46 +++++ .../submodule/subsubmodule/__init__.py | 1 + .../checkers/ast/analyze/deferred_scopes.rs | 5 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 4 + .../rules/pylint/rules/import_private_name.rs | 187 ++++++++++++++++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + ..._private_name__submodule____main__.py.snap | 72 +++++++ ruff.schema.json | 3 + 16 files changed, 326 insertions(+) create mode 100644 crates/ruff_linter/__init__.py create mode 100644 crates/ruff_linter/resources/__init__.py create mode 100644 crates/ruff_linter/resources/test/__init__.py create mode 100644 crates/ruff_linter/resources/test/fixtures/__init__.py create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/__init__.py create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/sibling_module/__init__.py create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/submodule/__init__.py create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/submodule/__main__.py create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/submodule/subsubmodule/__init__.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC2701_import_private_name__submodule____main__.py.snap diff --git a/crates/ruff_linter/__init__.py b/crates/ruff_linter/__init__.py new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/crates/ruff_linter/resources/__init__.py b/crates/ruff_linter/resources/__init__.py new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/crates/ruff_linter/resources/test/__init__.py b/crates/ruff_linter/resources/test/__init__.py new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/crates/ruff_linter/resources/test/fixtures/__init__.py b/crates/ruff_linter/resources/test/fixtures/__init__.py new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/__init__.py b/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/__init__.py new file mode 100644 index 0000000000000..8655d6131b1c6 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/__init__.py @@ -0,0 +1 @@ +_top_level_secret = 0 diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/sibling_module/__init__.py b/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/sibling_module/__init__.py new file mode 100644 index 0000000000000..da71f515f606a --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/sibling_module/__init__.py @@ -0,0 +1,3 @@ +_sibling_submodule_secret = 1 +_another_sibling_submodule_secret = 2 +some_value = 3 diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/submodule/__init__.py b/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/submodule/__init__.py new file mode 100644 index 0000000000000..52e02e918f991 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/submodule/__init__.py @@ -0,0 +1 @@ +_submodule_secret = 1 diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/submodule/__main__.py b/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/submodule/__main__.py new file mode 100644 index 0000000000000..dcfad466026ae --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/submodule/__main__.py @@ -0,0 +1,46 @@ +# Errors. +from _a import b +from c._d import e +from _f.g import h +from i import _j +from k import _l as m +import _aaa +import bbb.ccc._ddd as eee # Panicked in https://github.com/astral-sh/ruff/pull/5920 + +# Non-errors. +import n +import o as _p +from q import r +from s import t as _v +from w.x import y +from z.aa import bb as _cc +from .dd import _ee # Relative import. +from .ff._gg import hh # Relative import. +from ._ii.jj import kk # Relative import. +from __future__ import annotations # __future__ is a special case. +from __main__ import main # __main__ is a special case. +from ll import __version__ # __version__ is a special case. +from import_private_name import _top_level_secret # Can import from self. +from import_private_name.submodule import _submodule_secret # Can import from self. +from import_private_name.submodule.subsubmodule import ( + _subsubmodule_secret, +) # Can import from self. + +# Non-errors (used for type annotations). +from mm import _nn +from oo import _pp as qq +from _rr import ss +from tt._uu import vv +from _ww.xx import yy as zz +import _ddd as ddd + +some_variable: _nn = None + +def func(arg: qq) -> ss: + pass + +class Class: + lst: list[ddd] + + def __init__(self, arg: vv) -> "zz": + pass diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/submodule/subsubmodule/__init__.py b/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/submodule/subsubmodule/__init__.py new file mode 100644 index 0000000000000..9d2a3c1eb1315 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/submodule/subsubmodule/__init__.py @@ -0,0 +1 @@ +_subsubmodule_secret = 42 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs index acb40c189c960..665257b139b8c 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -16,6 +16,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { if !checker.any_enabled(&[ Rule::AsyncioDanglingTask, Rule::GlobalVariableNotAssigned, + Rule::ImportPrivateName, Rule::ImportShadowedByLoopVar, Rule::NoSelfUse, Rule::RedefinedArgumentFromLocal, @@ -372,6 +373,10 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { if checker.enabled(Rule::UnusedImport) { pyflakes::rules::unused_import(checker, scope, &mut diagnostics); } + + if checker.enabled(Rule::ImportPrivateName) { + pylint::rules::import_private_name(checker, scope, &mut diagnostics); + } } if scope.kind.is_function() { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 013b5cf3a8fb7..d073f0223f089 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -217,6 +217,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "C2801") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDunderCall), #[allow(deprecated)] (Pylint, "C1901") => (RuleGroup::Nursery, rules::pylint::rules::CompareToEmptyString), + (Pylint, "C2701") => (RuleGroup::Preview, rules::pylint::rules::ImportPrivateName), (Pylint, "C3002") => (RuleGroup::Stable, rules::pylint::rules::UnnecessaryDirectLambdaCall), (Pylint, "E0100") => (RuleGroup::Stable, rules::pylint::rules::YieldInInit), (Pylint, "E0101") => (RuleGroup::Stable, rules::pylint::rules::ReturnInInit), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 4401880645436..9182956716f34 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -63,6 +63,10 @@ mod tests { Path::new("global_variable_not_assigned.py") )] #[test_case(Rule::ImportOutsideTopLevel, Path::new("import_outside_top_level.py"))] + #[test_case( + Rule::ImportPrivateName, + Path::new("import_private_name/submodule/__main__.py") + )] #[test_case(Rule::ImportSelf, Path::new("import_self/module.py"))] #[test_case(Rule::InvalidAllFormat, Path::new("invalid_all_format.py"))] #[test_case(Rule::InvalidAllObject, Path::new("invalid_all_object.py"))] diff --git a/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs b/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs new file mode 100644 index 0000000000000..5163a754bea9f --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs @@ -0,0 +1,187 @@ +use std::borrow::Cow; + +use itertools::Itertools; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_semantic::{FromImport, Import, Imported, ResolvedReference, Scope}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for import statements that import a private name (a name starting +/// with an underscore `_`) from another module. +/// +/// ## Why is this bad? +/// [PEP 8] states that names starting with an underscore are private. Thus, +/// they are not intended to be used outside of the module in which they are +/// defined. +/// +/// Further, as private imports are not considered part of the public API, they +/// are prone to unexpected changes, especially outside of semantic versioning. +/// +/// Instead, consider using the public API of the module. +/// +/// This rule ignores private name imports that are exclusively used in type +/// annotations. Ideally, types would be public; however, this is not always +/// possible when using third-party libraries. +/// +/// ## Known problems +/// Does not ignore private name imports from within the module that defines +/// the private name if the module is defined with [PEP 420] namespace packages +/// (i.e., directories that omit the `__init__.py` file). Namespace packages +/// must be configured via the [`namespace-packages`] setting. +/// +/// ## Example +/// ```python +/// from foo import _bar +/// ``` +/// +/// ## Options +/// - [`namespace-packages`]: List of packages that are defined as namespace +/// packages. +/// +/// ## References +/// - [PEP 8: Naming Conventions](https://peps.python.org/pep-0008/#naming-conventions) +/// - [Semantic Versioning](https://semver.org/) +/// +/// [PEP 8]: https://www.python.org/dev/peps/pep-0008/ +/// [PEP 420]: https://www.python.org/dev/peps/pep-0420/ +/// [`namespace-packages`]: https://beta.ruff.rs/docs/settings/#namespace-packages +#[violation] +pub struct ImportPrivateName { + name: String, + module: Option, +} + +impl Violation for ImportPrivateName { + #[derive_message_formats] + fn message(&self) -> String { + let ImportPrivateName { name, module } = self; + match module { + Some(module) => { + format!("Private name import `{name}` from external module `{module}`") + } + None => format!("Private name import `{name}`"), + } + } +} + +/// PLC2701 +pub(crate) fn import_private_name( + checker: &Checker, + scope: &Scope, + diagnostics: &mut Vec, +) { + for binding_id in scope.binding_ids() { + let binding = checker.semantic().binding(binding_id); + let Some(import) = binding.as_any_import() else { + continue; + }; + + let import_info = match import { + import if import.is_import() => ImportInfo::from(import.import().unwrap()), + import if import.is_from_import() => ImportInfo::from(import.from_import().unwrap()), + _ => continue, + }; + + let Some(root_module) = import_info.module_name.first() else { + continue; + }; + + // Relative imports are not a public API. + // Ex) `from . import foo` + if import_info.module_name.starts_with(&["."]) { + continue; + } + + // We can also ignore dunder names. + // Ex) `from __future__ import annotations` + // Ex) `from foo import __version__` + if root_module.starts_with("__") || import_info.member_name.starts_with("__") { + continue; + } + + // Ignore private imports from the same module. + // Ex) `from foo import _bar` within `foo/baz.py` + if checker + .package() + .is_some_and(|path| path.ends_with(root_module)) + { + continue; + } + + // Ignore public imports; require at least one private name. + // Ex) `from foo import bar` + let Some((index, private_name)) = import_info + .call_path + .iter() + .find_position(|name| name.starts_with('_')) + else { + continue; + }; + + // Ignore private imports used exclusively for typing. + if !binding.references.is_empty() + && binding + .references() + .map(|reference_id| checker.semantic().reference(reference_id)) + .all(is_typing) + { + continue; + } + + let name = (*private_name).to_string(); + let module = if index > 0 { + Some(import_info.call_path[..index].join(".")) + } else { + None + }; + diagnostics.push(Diagnostic::new( + ImportPrivateName { name, module }, + binding.range(), + )); + } +} + +/// Returns `true` if the [`ResolvedReference`] is in a typing context. +fn is_typing(reference: &ResolvedReference) -> bool { + reference.in_type_checking_block() + || reference.in_typing_only_annotation() + || reference.in_complex_string_type_definition() + || reference.in_simple_string_type_definition() + || reference.in_runtime_evaluated_annotation() +} + +struct ImportInfo<'a> { + module_name: &'a [&'a str], + member_name: Cow<'a, str>, + call_path: &'a [&'a str], +} + +impl<'a> From<&'a FromImport<'_>> for ImportInfo<'a> { + fn from(import: &'a FromImport) -> Self { + let module_name = import.module_name(); + let member_name = import.member_name(); + let call_path = import.call_path(); + Self { + module_name, + member_name, + call_path, + } + } +} + +impl<'a> From<&'a Import<'_>> for ImportInfo<'a> { + fn from(import: &'a Import) -> Self { + let module_name = import.module_name(); + let member_name = import.member_name(); + let call_path = import.call_path(); + Self { + module_name, + member_name, + call_path, + } + } +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index e22096424260f..ba5f2916dca13 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -20,6 +20,7 @@ pub(crate) use global_at_module_level::*; pub(crate) use global_statement::*; pub(crate) use global_variable_not_assigned::*; pub(crate) use import_outside_top_level::*; +pub(crate) use import_private_name::*; pub(crate) use import_self::*; pub(crate) use invalid_all_format::*; pub(crate) use invalid_all_object::*; @@ -101,6 +102,7 @@ mod global_at_module_level; mod global_statement; mod global_variable_not_assigned; mod import_outside_top_level; +mod import_private_name; mod import_self; mod invalid_all_format; mod invalid_all_object; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC2701_import_private_name__submodule____main__.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC2701_import_private_name__submodule____main__.py.snap new file mode 100644 index 0000000000000..1132b30cc9848 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC2701_import_private_name__submodule____main__.py.snap @@ -0,0 +1,72 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +__main__.py:2:16: PLC2701 Private name import `_a` + | +1 | # Errors. +2 | from _a import b + | ^ PLC2701 +3 | from c._d import e +4 | from _f.g import h + | + +__main__.py:3:18: PLC2701 Private name import `_d` from external module `c` + | +1 | # Errors. +2 | from _a import b +3 | from c._d import e + | ^ PLC2701 +4 | from _f.g import h +5 | from i import _j + | + +__main__.py:4:18: PLC2701 Private name import `_f` + | +2 | from _a import b +3 | from c._d import e +4 | from _f.g import h + | ^ PLC2701 +5 | from i import _j +6 | from k import _l as m + | + +__main__.py:5:15: PLC2701 Private name import `_j` from external module `i` + | +3 | from c._d import e +4 | from _f.g import h +5 | from i import _j + | ^^ PLC2701 +6 | from k import _l as m +7 | import _aaa + | + +__main__.py:6:21: PLC2701 Private name import `_l` from external module `k` + | +4 | from _f.g import h +5 | from i import _j +6 | from k import _l as m + | ^ PLC2701 +7 | import _aaa +8 | import bbb.ccc._ddd as eee # Panicked in https://github.com/astral-sh/ruff/pull/5920 + | + +__main__.py:7:8: PLC2701 Private name import `_aaa` + | +5 | from i import _j +6 | from k import _l as m +7 | import _aaa + | ^^^^ PLC2701 +8 | import bbb.ccc._ddd as eee # Panicked in https://github.com/astral-sh/ruff/pull/5920 + | + +__main__.py:8:24: PLC2701 Private name import `_ddd` from external module `bbb.ccc` + | + 6 | from k import _l as m + 7 | import _aaa + 8 | import bbb.ccc._ddd as eee # Panicked in https://github.com/astral-sh/ruff/pull/5920 + | ^^^ PLC2701 + 9 | +10 | # Non-errors. + | + + diff --git a/ruff.schema.json b/ruff.schema.json index e58cd3cccf087..ef61102357472 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3098,6 +3098,9 @@ "PLC240", "PLC2401", "PLC2403", + "PLC27", + "PLC270", + "PLC2701", "PLC28", "PLC280", "PLC2801",