Skip to content

Commit

Permalink
[pylint] (Re-)Implement import-private-name (C2701) (#9553)
Browse files Browse the repository at this point in the history
## 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`
  • Loading branch information
tjkuson authored Jan 16, 2024
1 parent 3aae16f commit f426c0f
Show file tree
Hide file tree
Showing 16 changed files with 326 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,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
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
_subsubmodule_secret = 42
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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() {
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 @@ -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),
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 @@ -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"))]
Expand Down
187 changes: 187 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,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<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: &Checker,
scope: &Scope,
diagnostics: &mut Vec<Diagnostic>,
) {
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,
}
}
}
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 @@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
|


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 f426c0f

Please sign in to comment.