Skip to content

Commit

Permalink
Add import-private-name rule
Browse files Browse the repository at this point in the history
  • Loading branch information
tjkuson committed Dec 21, 2023
1 parent c6d8076 commit 63f6fee
Show file tree
Hide file tree
Showing 16 changed files with 228 additions and 0 deletions.
Empty file added crates/ruff_linter/__init__.py
Empty file.
Empty file.
Empty file.
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
_top_level_secret = 0
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
_sibling_submodule_secret = 1
_another_sibling_submodule_secret = 2
some_value = 3
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
_submodule_secret = 1
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
_subsubmodule_secret = 42
10 changes: 10 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
4 changes: 4 additions & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand Down
125 changes: 125 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs
Original file line number Diff line number Diff line change
@@ -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<String>,
}

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<u32>,
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::<Vec<_>>()
.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(),
));
}
}
}
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
|


3 changes: 3 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 63f6fee

Please sign in to comment.