From 63f6fee658bbcbc23ebf40326c5f7a7e516f43ce Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Thu, 20 Jul 2023 14:18:20 +0100 Subject: [PATCH 1/7] Add import-private-name rule --- 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 | 25 ++++ .../submodule/subsubmodule/__init__.py | 1 + .../src/checkers/ast/analyze/statement.rs | 10 ++ crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 4 + .../rules/pylint/rules/import_private_name.rs | 125 ++++++++++++++++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + ..._private_name__submodule____main__.py.snap | 52 ++++++++ ruff.schema.json | 3 + 16 files changed, 228 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..e3d481b4839b7 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/import_private_name/submodule/__main__.py @@ -0,0 +1,25 @@ +# Errors. +from _foo import bar +from foo._bar import baz +from _foo.bar import baz +from foo import _bar +from foo import _bar as bar + +# Non-errors. +import foo +import foo as _foo +from foo import bar +from foo import bar as _bar +from foo.bar import baz +from foo.bar import baz as _baz +from .foo import _bar # Relative import. +from .foo._bar import baz # Relative import. +from ._foo.bar import baz # Relative import. +from __future__ import annotations # __future__ is a special case. +from __main__ import main # __main__ is a special case. +from foo 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. 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/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index c803562bd5bf5..1fcd8377a6389 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -743,6 +743,16 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::DeprecatedImport) { pyupgrade::rules::deprecated_import(checker, stmt, names, module, level); } + if checker.enabled(Rule::ImportPrivateName) { + pylint::rules::import_private_name( + checker, + stmt, + names, + module, + level, + checker.module_path, + ); + } if checker.enabled(Rule::UnnecessaryBuiltinImport) { if let Some(module) = module { pyupgrade::rules::unnecessary_builtin_import(checker, stmt, module, names); diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 158d60d4a3f63..79029d31c0f6c 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -216,6 +216,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "C2403") => (RuleGroup::Preview, rules::pylint::rules::NonAsciiImportName), #[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 303395de50406..020e370fc5fb5 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -62,6 +62,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..886a307292c33 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs @@ -0,0 +1,125 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{Alias, Stmt}; + +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, even in a minor version bump. +/// +/// Instead, consider using the public API of the module. +/// +/// ## 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 +/// (directories that omit the `__init__.py` file). Instead, namespace packages +/// must be made known to ruff using the [`namespace-packages`] setting. +/// +/// ## Example +/// ```python +/// from foo import _bar +/// ``` +/// +/// ## Options +/// - [`namespace-packages`]: List of namespace packages that are known to ruff +/// +/// ## 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: &mut Checker, + stmt: &Stmt, + names: &[Alias], + module: Option<&str>, + level: Option, + module_path: Option<&[String]>, +) { + // Relative imports are not a public API, so we don't need to check them. + if level.map_or(false, |level| level > 0) { + return; + } + if let Some(module) = module { + if module.starts_with("__") { + return; + } + // Ignore private imports from the same module. + // TODO(tjkuson): Detect namespace packages automatically. + // https://github.com/astral-sh/ruff/issues/6114 + if let Some(module_path) = module_path { + let root_module = module_path.first().unwrap(); + if module.starts_with(root_module) { + return; + } + } + if module.starts_with('_') || module.contains("._") { + let private_name = module + .split('.') + .find(|name| name.starts_with('_')) + .unwrap_or(module); + let external_module = Some( + module + .split('.') + .take_while(|name| !name.starts_with('_')) + .collect::>() + .join("."), + ) + .filter(|module| !module.is_empty()); + checker.diagnostics.push(Diagnostic::new( + ImportPrivateName { + name: private_name.to_string(), + module: external_module, + }, + stmt.range(), + )); + } + for name in names { + if matches!(name.name.as_str(), "_") || name.name.starts_with("__") { + continue; + } + if name.name.starts_with('_') { + checker.diagnostics.push(Diagnostic::new( + ImportPrivateName { + name: name.name.to_string(), + module: Some(module.to_string()), + }, + name.range(), + )); + } + } + } +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 85699ab30e1ed..a205d0ddffcf3 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -19,6 +19,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::*; @@ -97,6 +98,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..87e5e66ab0832 --- /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,52 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +__main__.py:2:1: PLC2701 Private name import `_foo` + | +1 | # Errors. +2 | from _foo import bar + | ^^^^^^^^^^^^^^^^^^^^ PLC2701 +3 | from foo._bar import baz +4 | from _foo.bar import baz + | + +__main__.py:3:1: PLC2701 Private name import `_bar` from external module `foo` + | +1 | # Errors. +2 | from _foo import bar +3 | from foo._bar import baz + | ^^^^^^^^^^^^^^^^^^^^^^^^ PLC2701 +4 | from _foo.bar import baz +5 | from foo import _bar + | + +__main__.py:4:1: PLC2701 Private name import `_foo` + | +2 | from _foo import bar +3 | from foo._bar import baz +4 | from _foo.bar import baz + | ^^^^^^^^^^^^^^^^^^^^^^^^ PLC2701 +5 | from foo import _bar +6 | from foo import _bar as bar + | + +__main__.py:5:17: PLC2701 Private name import `_bar` from external module `foo` + | +3 | from foo._bar import baz +4 | from _foo.bar import baz +5 | from foo import _bar + | ^^^^ PLC2701 +6 | from foo import _bar as bar + | + +__main__.py:6:17: PLC2701 Private name import `_bar` from external module `foo` + | +4 | from _foo.bar import baz +5 | from foo import _bar +6 | from foo import _bar as bar + | ^^^^^^^^^^^ PLC2701 +7 | +8 | # Non-errors. + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 3d566f7964ff2..3c6758676a017 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3096,6 +3096,9 @@ "PLC240", "PLC2401", "PLC2403", + "PLC27", + "PLC270", + "PLC2701", "PLC3", "PLC30", "PLC300", From ca89ab2fd872ce0fadc1ffa23679aa359dcb5be3 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Thu, 21 Dec 2023 18:50:18 +0000 Subject: [PATCH 2/7] Defer --- .../import_private_name/submodule/__main__.py | 30 ++--- .../checkers/ast/analyze/deferred_scopes.rs | 5 + .../src/checkers/ast/analyze/statement.rs | 10 -- .../rules/pylint/rules/import_private_name.rs | 106 +++++++++--------- ..._private_name__submodule____main__.py.snap | 58 +++++----- 5 files changed, 105 insertions(+), 104 deletions(-) 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 index e3d481b4839b7..f54aa44c9328c 100644 --- 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 @@ -1,23 +1,23 @@ # Errors. -from _foo import bar -from foo._bar import baz -from _foo.bar import baz -from foo import _bar -from foo import _bar as bar +from _a import b +from c._d import e +from _f.g import h +from i import _j +from k import _l as m # Non-errors. -import foo -import foo as _foo -from foo import bar -from foo import bar as _bar -from foo.bar import baz -from foo.bar import baz as _baz -from .foo import _bar # Relative import. -from .foo._bar import baz # Relative import. -from ._foo.bar import baz # Relative import. +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 foo import __version__ # __version__ 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 ( 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 fad4926774219..cecb03eb6e6e2 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -14,6 +14,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, @@ -338,6 +339,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/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 1fcd8377a6389..c803562bd5bf5 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -743,16 +743,6 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::DeprecatedImport) { pyupgrade::rules::deprecated_import(checker, stmt, names, module, level); } - if checker.enabled(Rule::ImportPrivateName) { - pylint::rules::import_private_name( - checker, - stmt, - names, - module, - level, - checker.module_path, - ); - } if checker.enabled(Rule::UnnecessaryBuiltinImport) { if let Some(module) = module { pyupgrade::rules::unnecessary_builtin_import(checker, stmt, module, names); 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 index 886a307292c33..340202bfba5f0 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs @@ -1,7 +1,8 @@ +use itertools::join; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{Alias, Stmt}; +use ruff_python_semantic::{Imported, Scope}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -62,64 +63,69 @@ impl Violation for ImportPrivateName { /// PLC2701 pub(crate) fn import_private_name( - checker: &mut Checker, - stmt: &Stmt, - names: &[Alias], - module: Option<&str>, - level: Option, - module_path: Option<&[String]>, + checker: &Checker, + scope: &Scope, + diagnostics: &mut Vec, ) { - // Relative imports are not a public API, so we don't need to check them. - if level.map_or(false, |level| level > 0) { - return; - } - if let Some(module) = module { - if module.starts_with("__") { - return; + for binding_id in scope.binding_ids() { + let binding = checker.semantic().binding(binding_id); + + let Some(import) = binding.as_any_import() else { + continue; + }; + + let Some(import) = import.from_import() else { + continue; + }; + + let module = import.module_name(); + let member = import.member_name(); + + // Relative imports are not a public API. + // Ex) `from . import foo` + if module.starts_with(&["."]) { + continue; + } + + // We can also ignore dunder names. + // Ex) `from __future__ import annotations` + // Ex) `from foo import __version__` + let Some(root_module) = module.first() else { + continue; + }; + if root_module.starts_with("__") || member.starts_with("__") { + continue; } + // Ignore private imports from the same module. - // TODO(tjkuson): Detect namespace packages automatically. - // https://github.com/astral-sh/ruff/issues/6114 - if let Some(module_path) = module_path { - let root_module = module_path.first().unwrap(); - if module.starts_with(root_module) { - return; - } + let Some(package) = checker.package() else { + continue; + }; + if package.ends_with(root_module) { + continue; } - if module.starts_with('_') || module.contains("._") { - let private_name = module - .split('.') - .find(|name| name.starts_with('_')) - .unwrap_or(module); - let external_module = Some( - module - .split('.') - .take_while(|name| !name.starts_with('_')) - .collect::>() - .join("."), - ) + + // If any of the names in the call path start with an underscore, we + // have a private import. + let call_path = import.call_path(); + if call_path.iter().any(|name| name.starts_with('_')) { + // The private name is the first name that starts with an + // underscore, and the external module is everything before it, + // joined by dots. + let private_name = call_path.iter().find(|name| name.starts_with('_')).unwrap(); + let external_module = Some(join( + call_path.iter().take_while(|name| name != &private_name), + ".", + )) .filter(|module| !module.is_empty()); - checker.diagnostics.push(Diagnostic::new( + + diagnostics.push(Diagnostic::new( ImportPrivateName { - name: private_name.to_string(), + name: (*private_name).to_string(), module: external_module, }, - stmt.range(), + binding.range(), )); } - for name in names { - if matches!(name.name.as_str(), "_") || name.name.starts_with("__") { - continue; - } - if name.name.starts_with('_') { - checker.diagnostics.push(Diagnostic::new( - ImportPrivateName { - name: name.name.to_string(), - module: Some(module.to_string()), - }, - name.range(), - )); - } - } } } 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 index 87e5e66ab0832..cb6584200c692 100644 --- 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 @@ -1,50 +1,50 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -__main__.py:2:1: PLC2701 Private name import `_foo` +__main__.py:2:16: PLC2701 Private name import `_a` | 1 | # Errors. -2 | from _foo import bar - | ^^^^^^^^^^^^^^^^^^^^ PLC2701 -3 | from foo._bar import baz -4 | from _foo.bar import baz +2 | from _a import b + | ^ PLC2701 +3 | from c._d import e +4 | from _f.g import h | -__main__.py:3:1: PLC2701 Private name import `_bar` from external module `foo` +__main__.py:3:18: PLC2701 Private name import `_d` from external module `c` | 1 | # Errors. -2 | from _foo import bar -3 | from foo._bar import baz - | ^^^^^^^^^^^^^^^^^^^^^^^^ PLC2701 -4 | from _foo.bar import baz -5 | from foo import _bar +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:1: PLC2701 Private name import `_foo` +__main__.py:4:18: PLC2701 Private name import `_f` | -2 | from _foo import bar -3 | from foo._bar import baz -4 | from _foo.bar import baz - | ^^^^^^^^^^^^^^^^^^^^^^^^ PLC2701 -5 | from foo import _bar -6 | from foo import _bar as bar +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:17: PLC2701 Private name import `_bar` from external module `foo` +__main__.py:5:15: PLC2701 Private name import `_j` from external module `i` | -3 | from foo._bar import baz -4 | from _foo.bar import baz -5 | from foo import _bar - | ^^^^ PLC2701 -6 | from foo import _bar as bar +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 | -__main__.py:6:17: PLC2701 Private name import `_bar` from external module `foo` +__main__.py:6:21: PLC2701 Private name import `_l` from external module `k` | -4 | from _foo.bar import baz -5 | from foo import _bar -6 | from foo import _bar as bar - | ^^^^^^^^^^^ PLC2701 +4 | from _f.g import h +5 | from i import _j +6 | from k import _l as m + | ^ PLC2701 7 | 8 | # Non-errors. | From 288c466a5bde7aca85f950af35f1b4c94ec13ce2 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Thu, 21 Dec 2023 19:03:02 +0000 Subject: [PATCH 3/7] Minor tweaks --- .../rules/pylint/rules/import_private_name.rs | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) 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 index 340202bfba5f0..cda63be508406 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs @@ -79,7 +79,9 @@ pub(crate) fn import_private_name( }; let module = import.module_name(); - let member = import.member_name(); + let Some(root_module) = module.first() else { + continue; + }; // Relative imports are not a public API. // Ex) `from . import foo` @@ -90,28 +92,21 @@ pub(crate) fn import_private_name( // We can also ignore dunder names. // Ex) `from __future__ import annotations` // Ex) `from foo import __version__` - let Some(root_module) = module.first() else { - continue; - }; - if root_module.starts_with("__") || member.starts_with("__") { + if root_module.starts_with("__") || import.member_name().starts_with("__") { continue; } // Ignore private imports from the same module. - let Some(package) = checker.package() else { - continue; - }; - if package.ends_with(root_module) { + // Ex) `from foo import _bar` within `foo/baz.py` + if checker + .package() + .is_some_and(|path| path.ends_with(root_module)) + { continue; } - // If any of the names in the call path start with an underscore, we - // have a private import. let call_path = import.call_path(); if call_path.iter().any(|name| name.starts_with('_')) { - // The private name is the first name that starts with an - // underscore, and the external module is everything before it, - // joined by dots. let private_name = call_path.iter().find(|name| name.starts_with('_')).unwrap(); let external_module = Some(join( call_path.iter().take_while(|name| name != &private_name), From 3e28855b9130c3a9416e7c2e2d9bec9f3b7bd3f1 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Thu, 21 Dec 2023 19:22:30 +0000 Subject: [PATCH 4/7] Ignore imports used for typing --- .../import_private_name/submodule/__main__.py | 16 +++++++++++++ .../rules/pylint/rules/import_private_name.rs | 24 +++++++++++++++---- 2 files changed, 36 insertions(+), 4 deletions(-) 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 index f54aa44c9328c..f49abff080d52 100644 --- 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 @@ -23,3 +23,19 @@ 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 + +some_variable: _nn = None + +def func(arg: qq) -> ss: + pass + +class Class: + def __init__(self, arg: vv) -> "zz": + pass 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 index cda63be508406..1cd9e51c583f8 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs @@ -2,7 +2,7 @@ use itertools::join; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_semantic::{Imported, Scope}; +use ruff_python_semantic::{Imported, ResolvedReference, Scope}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -69,11 +69,9 @@ pub(crate) fn import_private_name( ) { for binding_id in scope.binding_ids() { let binding = checker.semantic().binding(binding_id); - let Some(import) = binding.as_any_import() else { continue; }; - let Some(import) = import.from_import() else { continue; }; @@ -107,13 +105,22 @@ pub(crate) fn import_private_name( let call_path = import.call_path(); if call_path.iter().any(|name| name.starts_with('_')) { + // Ignore private imports used for typing. + if binding.context.is_runtime() + && binding + .references() + .map(|reference_id| checker.semantic().reference(reference_id)) + .any(is_typing) + { + continue; + } + let private_name = call_path.iter().find(|name| name.starts_with('_')).unwrap(); let external_module = Some(join( call_path.iter().take_while(|name| name != &private_name), ".", )) .filter(|module| !module.is_empty()); - diagnostics.push(Diagnostic::new( ImportPrivateName { name: (*private_name).to_string(), @@ -124,3 +131,12 @@ pub(crate) fn import_private_name( } } } + +/// 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() +} From 44cc626b5901036c0114fad2431636daaabb0a78 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Thu, 21 Dec 2023 19:36:07 +0000 Subject: [PATCH 5/7] Document that type imports are ignored --- .../ruff_linter/src/rules/pylint/rules/import_private_name.rs | 4 ++++ 1 file changed, 4 insertions(+) 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 index 1cd9e51c583f8..58574cca511d3 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs @@ -11,6 +11,10 @@ use crate::checkers::ast::Checker; /// Checks for import statements that import a private name (a name starting /// with an underscore `_`) from another module. /// +/// Ignores private name imports that are used for type annotations. Ideally, +/// types would be public; however, this is not always possible when using +/// third-party libraries. +/// /// ## 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 From c98ef43a94dd1c1575467a9a83e5f629df4136fb Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Fri, 29 Dec 2023 10:09:22 +0000 Subject: [PATCH 6/7] Check non-from imports --- .../import_private_name/submodule/__main__.py | 5 ++ .../rules/pylint/rules/import_private_name.rs | 67 ++++++++++++++++--- ..._private_name__submodule____main__.py.snap | 42 +----------- 3 files changed, 63 insertions(+), 51 deletions(-) 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 index f49abff080d52..be095577fa996 100644 --- 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 @@ -4,6 +4,8 @@ from _f.g import h from i import _j from k import _l as m +import _aaa +import bbb._ccc # Non-errors. import n @@ -30,6 +32,7 @@ from _rr import ss from tt._uu import vv from _ww.xx import yy as zz +import _ddd as ddd some_variable: _nn = None @@ -37,5 +40,7 @@ def func(arg: qq) -> ss: pass class Class: + lst: list[ddd] + def __init__(self, arg: vv) -> "zz": pass 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 index 58574cca511d3..26b04804d0437 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs @@ -1,8 +1,9 @@ use itertools::join; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use std::borrow::Cow; -use ruff_python_semantic::{Imported, ResolvedReference, Scope}; +use ruff_python_semantic::{FromImport, Import, Imported, ResolvedReference, Scope}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -76,25 +77,27 @@ pub(crate) fn import_private_name( let Some(import) = binding.as_any_import() else { continue; }; - let Some(import) = import.from_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()), + _ => return, }; - let module = import.module_name(); - let Some(root_module) = module.first() else { + let Some(root_module) = import_info.module_name.first() else { continue; }; // Relative imports are not a public API. // Ex) `from . import foo` - if module.starts_with(&["."]) { + 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.member_name().starts_with("__") { + if root_module.starts_with("__") || import_info.member_name.starts_with("__") { continue; } @@ -107,8 +110,11 @@ pub(crate) fn import_private_name( continue; } - let call_path = import.call_path(); - if call_path.iter().any(|name| name.starts_with('_')) { + if import_info + .call_path + .iter() + .any(|name| name.starts_with('_')) + { // Ignore private imports used for typing. if binding.context.is_runtime() && binding @@ -119,9 +125,16 @@ pub(crate) fn import_private_name( continue; } - let private_name = call_path.iter().find(|name| name.starts_with('_')).unwrap(); + let private_name = import_info + .call_path + .iter() + .find(|name| name.starts_with('_')) + .unwrap(); let external_module = Some(join( - call_path.iter().take_while(|name| name != &private_name), + import_info + .call_path + .iter() + .take_while(|name| name != &private_name), ".", )) .filter(|module| !module.is_empty()); @@ -144,3 +157,35 @@ fn is_typing(reference: &ResolvedReference) -> bool { || 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/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 index cb6584200c692..6aff971da3018 100644 --- 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 @@ -1,52 +1,14 @@ --- 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 - | - __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 | -8 | # Non-errors. +7 | import _aaa +8 | import bbb._ccc | From bd483d3fc35fef88afddd7313ed64df142caa038 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 16 Jan 2024 00:09:52 -0500 Subject: [PATCH 7/7] Tweak typing rules --- .../rules/pylint/rules/import_private_name.rs | 82 +++++++++---------- 1 file changed, 39 insertions(+), 43 deletions(-) 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 index 26b04804d0437..b223ab4cfd7de 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs @@ -1,8 +1,9 @@ -use itertools::join; -use ruff_diagnostics::{Diagnostic, Violation}; -use ruff_macros::{derive_message_formats, violation}; 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; @@ -12,25 +13,25 @@ use crate::checkers::ast::Checker; /// Checks for import statements that import a private name (a name starting /// with an underscore `_`) from another module. /// -/// Ignores private name imports that are used for type annotations. Ideally, -/// types would be public; however, this is not always possible when using -/// third-party libraries. -/// /// ## 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, even in a minor version bump. +/// 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 -/// (directories that omit the `__init__.py` file). Instead, namespace packages -/// must be made known to ruff using the [`namespace-packages`] setting. +/// (i.e., directories that omit the `__init__.py` file). Namespace packages +/// must be configured via the [`namespace-packages`] setting. /// /// ## Example /// ```python @@ -38,7 +39,8 @@ use crate::checkers::ast::Checker; /// ``` /// /// ## Options -/// - [`namespace-packages`]: List of namespace packages that are known to ruff +/// - [`namespace-packages`]: List of packages that are defined as namespace +/// packages. /// /// ## References /// - [PEP 8: Naming Conventions](https://peps.python.org/pep-0008/#naming-conventions) @@ -110,42 +112,36 @@ pub(crate) fn import_private_name( continue; } - if import_info + // Ignore public imports; require at least one private name. + // Ex) `from foo import bar` + let Some((index, private_name)) = import_info .call_path .iter() - .any(|name| name.starts_with('_')) - { - // Ignore private imports used for typing. - if binding.context.is_runtime() - && binding - .references() - .map(|reference_id| checker.semantic().reference(reference_id)) - .any(is_typing) - { - continue; - } + .find_position(|name| name.starts_with('_')) + else { + continue; + }; - let private_name = import_info - .call_path - .iter() - .find(|name| name.starts_with('_')) - .unwrap(); - let external_module = Some(join( - import_info - .call_path - .iter() - .take_while(|name| name != &private_name), - ".", - )) - .filter(|module| !module.is_empty()); - diagnostics.push(Diagnostic::new( - ImportPrivateName { - name: (*private_name).to_string(), - module: external_module, - }, - binding.range(), - )); + // 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.module_name[..index].join(".")) + } else { + None + }; + diagnostics.push(Diagnostic::new( + ImportPrivateName { name, module }, + binding.range(), + )); } }