Skip to content

Commit f426c0f

Browse files
authored
[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`
1 parent 3aae16f commit f426c0f

File tree

16 files changed

+326
-0
lines changed

16 files changed

+326
-0
lines changed

crates/ruff_linter/__init__.py

Whitespace-only changes.

crates/ruff_linter/resources/__init__.py

Whitespace-only changes.

crates/ruff_linter/resources/test/__init__.py

Whitespace-only changes.

crates/ruff_linter/resources/test/fixtures/__init__.py

Whitespace-only changes.
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
_top_level_secret = 0
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
_sibling_submodule_secret = 1
2+
_another_sibling_submodule_secret = 2
3+
some_value = 3
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
_submodule_secret = 1
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
# Errors.
2+
from _a import b
3+
from c._d import e
4+
from _f.g import h
5+
from i import _j
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
9+
10+
# Non-errors.
11+
import n
12+
import o as _p
13+
from q import r
14+
from s import t as _v
15+
from w.x import y
16+
from z.aa import bb as _cc
17+
from .dd import _ee # Relative import.
18+
from .ff._gg import hh # Relative import.
19+
from ._ii.jj import kk # Relative import.
20+
from __future__ import annotations # __future__ is a special case.
21+
from __main__ import main # __main__ is a special case.
22+
from ll import __version__ # __version__ is a special case.
23+
from import_private_name import _top_level_secret # Can import from self.
24+
from import_private_name.submodule import _submodule_secret # Can import from self.
25+
from import_private_name.submodule.subsubmodule import (
26+
_subsubmodule_secret,
27+
) # Can import from self.
28+
29+
# Non-errors (used for type annotations).
30+
from mm import _nn
31+
from oo import _pp as qq
32+
from _rr import ss
33+
from tt._uu import vv
34+
from _ww.xx import yy as zz
35+
import _ddd as ddd
36+
37+
some_variable: _nn = None
38+
39+
def func(arg: qq) -> ss:
40+
pass
41+
42+
class Class:
43+
lst: list[ddd]
44+
45+
def __init__(self, arg: vv) -> "zz":
46+
pass
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
_subsubmodule_secret = 42

crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
1616
if !checker.any_enabled(&[
1717
Rule::AsyncioDanglingTask,
1818
Rule::GlobalVariableNotAssigned,
19+
Rule::ImportPrivateName,
1920
Rule::ImportShadowedByLoopVar,
2021
Rule::NoSelfUse,
2122
Rule::RedefinedArgumentFromLocal,
@@ -372,6 +373,10 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
372373
if checker.enabled(Rule::UnusedImport) {
373374
pyflakes::rules::unused_import(checker, scope, &mut diagnostics);
374375
}
376+
377+
if checker.enabled(Rule::ImportPrivateName) {
378+
pylint::rules::import_private_name(checker, scope, &mut diagnostics);
379+
}
375380
}
376381

377382
if scope.kind.is_function() {

crates/ruff_linter/src/codes.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
217217
(Pylint, "C2801") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDunderCall),
218218
#[allow(deprecated)]
219219
(Pylint, "C1901") => (RuleGroup::Nursery, rules::pylint::rules::CompareToEmptyString),
220+
(Pylint, "C2701") => (RuleGroup::Preview, rules::pylint::rules::ImportPrivateName),
220221
(Pylint, "C3002") => (RuleGroup::Stable, rules::pylint::rules::UnnecessaryDirectLambdaCall),
221222
(Pylint, "E0100") => (RuleGroup::Stable, rules::pylint::rules::YieldInInit),
222223
(Pylint, "E0101") => (RuleGroup::Stable, rules::pylint::rules::ReturnInInit),

crates/ruff_linter/src/rules/pylint/mod.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,10 @@ mod tests {
6363
Path::new("global_variable_not_assigned.py")
6464
)]
6565
#[test_case(Rule::ImportOutsideTopLevel, Path::new("import_outside_top_level.py"))]
66+
#[test_case(
67+
Rule::ImportPrivateName,
68+
Path::new("import_private_name/submodule/__main__.py")
69+
)]
6670
#[test_case(Rule::ImportSelf, Path::new("import_self/module.py"))]
6771
#[test_case(Rule::InvalidAllFormat, Path::new("invalid_all_format.py"))]
6872
#[test_case(Rule::InvalidAllObject, Path::new("invalid_all_object.py"))]
Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
use std::borrow::Cow;
2+
3+
use itertools::Itertools;
4+
5+
use ruff_diagnostics::{Diagnostic, Violation};
6+
use ruff_macros::{derive_message_formats, violation};
7+
use ruff_python_semantic::{FromImport, Import, Imported, ResolvedReference, Scope};
8+
use ruff_text_size::Ranged;
9+
10+
use crate::checkers::ast::Checker;
11+
12+
/// ## What it does
13+
/// Checks for import statements that import a private name (a name starting
14+
/// with an underscore `_`) from another module.
15+
///
16+
/// ## Why is this bad?
17+
/// [PEP 8] states that names starting with an underscore are private. Thus,
18+
/// they are not intended to be used outside of the module in which they are
19+
/// defined.
20+
///
21+
/// Further, as private imports are not considered part of the public API, they
22+
/// are prone to unexpected changes, especially outside of semantic versioning.
23+
///
24+
/// Instead, consider using the public API of the module.
25+
///
26+
/// This rule ignores private name imports that are exclusively used in type
27+
/// annotations. Ideally, types would be public; however, this is not always
28+
/// possible when using third-party libraries.
29+
///
30+
/// ## Known problems
31+
/// Does not ignore private name imports from within the module that defines
32+
/// the private name if the module is defined with [PEP 420] namespace packages
33+
/// (i.e., directories that omit the `__init__.py` file). Namespace packages
34+
/// must be configured via the [`namespace-packages`] setting.
35+
///
36+
/// ## Example
37+
/// ```python
38+
/// from foo import _bar
39+
/// ```
40+
///
41+
/// ## Options
42+
/// - [`namespace-packages`]: List of packages that are defined as namespace
43+
/// packages.
44+
///
45+
/// ## References
46+
/// - [PEP 8: Naming Conventions](https://peps.python.org/pep-0008/#naming-conventions)
47+
/// - [Semantic Versioning](https://semver.org/)
48+
///
49+
/// [PEP 8]: https://www.python.org/dev/peps/pep-0008/
50+
/// [PEP 420]: https://www.python.org/dev/peps/pep-0420/
51+
/// [`namespace-packages`]: https://beta.ruff.rs/docs/settings/#namespace-packages
52+
#[violation]
53+
pub struct ImportPrivateName {
54+
name: String,
55+
module: Option<String>,
56+
}
57+
58+
impl Violation for ImportPrivateName {
59+
#[derive_message_formats]
60+
fn message(&self) -> String {
61+
let ImportPrivateName { name, module } = self;
62+
match module {
63+
Some(module) => {
64+
format!("Private name import `{name}` from external module `{module}`")
65+
}
66+
None => format!("Private name import `{name}`"),
67+
}
68+
}
69+
}
70+
71+
/// PLC2701
72+
pub(crate) fn import_private_name(
73+
checker: &Checker,
74+
scope: &Scope,
75+
diagnostics: &mut Vec<Diagnostic>,
76+
) {
77+
for binding_id in scope.binding_ids() {
78+
let binding = checker.semantic().binding(binding_id);
79+
let Some(import) = binding.as_any_import() else {
80+
continue;
81+
};
82+
83+
let import_info = match import {
84+
import if import.is_import() => ImportInfo::from(import.import().unwrap()),
85+
import if import.is_from_import() => ImportInfo::from(import.from_import().unwrap()),
86+
_ => continue,
87+
};
88+
89+
let Some(root_module) = import_info.module_name.first() else {
90+
continue;
91+
};
92+
93+
// Relative imports are not a public API.
94+
// Ex) `from . import foo`
95+
if import_info.module_name.starts_with(&["."]) {
96+
continue;
97+
}
98+
99+
// We can also ignore dunder names.
100+
// Ex) `from __future__ import annotations`
101+
// Ex) `from foo import __version__`
102+
if root_module.starts_with("__") || import_info.member_name.starts_with("__") {
103+
continue;
104+
}
105+
106+
// Ignore private imports from the same module.
107+
// Ex) `from foo import _bar` within `foo/baz.py`
108+
if checker
109+
.package()
110+
.is_some_and(|path| path.ends_with(root_module))
111+
{
112+
continue;
113+
}
114+
115+
// Ignore public imports; require at least one private name.
116+
// Ex) `from foo import bar`
117+
let Some((index, private_name)) = import_info
118+
.call_path
119+
.iter()
120+
.find_position(|name| name.starts_with('_'))
121+
else {
122+
continue;
123+
};
124+
125+
// Ignore private imports used exclusively for typing.
126+
if !binding.references.is_empty()
127+
&& binding
128+
.references()
129+
.map(|reference_id| checker.semantic().reference(reference_id))
130+
.all(is_typing)
131+
{
132+
continue;
133+
}
134+
135+
let name = (*private_name).to_string();
136+
let module = if index > 0 {
137+
Some(import_info.call_path[..index].join("."))
138+
} else {
139+
None
140+
};
141+
diagnostics.push(Diagnostic::new(
142+
ImportPrivateName { name, module },
143+
binding.range(),
144+
));
145+
}
146+
}
147+
148+
/// Returns `true` if the [`ResolvedReference`] is in a typing context.
149+
fn is_typing(reference: &ResolvedReference) -> bool {
150+
reference.in_type_checking_block()
151+
|| reference.in_typing_only_annotation()
152+
|| reference.in_complex_string_type_definition()
153+
|| reference.in_simple_string_type_definition()
154+
|| reference.in_runtime_evaluated_annotation()
155+
}
156+
157+
struct ImportInfo<'a> {
158+
module_name: &'a [&'a str],
159+
member_name: Cow<'a, str>,
160+
call_path: &'a [&'a str],
161+
}
162+
163+
impl<'a> From<&'a FromImport<'_>> for ImportInfo<'a> {
164+
fn from(import: &'a FromImport) -> Self {
165+
let module_name = import.module_name();
166+
let member_name = import.member_name();
167+
let call_path = import.call_path();
168+
Self {
169+
module_name,
170+
member_name,
171+
call_path,
172+
}
173+
}
174+
}
175+
176+
impl<'a> From<&'a Import<'_>> for ImportInfo<'a> {
177+
fn from(import: &'a Import) -> Self {
178+
let module_name = import.module_name();
179+
let member_name = import.member_name();
180+
let call_path = import.call_path();
181+
Self {
182+
module_name,
183+
member_name,
184+
call_path,
185+
}
186+
}
187+
}

crates/ruff_linter/src/rules/pylint/rules/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ pub(crate) use global_at_module_level::*;
2020
pub(crate) use global_statement::*;
2121
pub(crate) use global_variable_not_assigned::*;
2222
pub(crate) use import_outside_top_level::*;
23+
pub(crate) use import_private_name::*;
2324
pub(crate) use import_self::*;
2425
pub(crate) use invalid_all_format::*;
2526
pub(crate) use invalid_all_object::*;
@@ -101,6 +102,7 @@ mod global_at_module_level;
101102
mod global_statement;
102103
mod global_variable_not_assigned;
103104
mod import_outside_top_level;
105+
mod import_private_name;
104106
mod import_self;
105107
mod invalid_all_format;
106108
mod invalid_all_object;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
---
2+
source: crates/ruff_linter/src/rules/pylint/mod.rs
3+
---
4+
__main__.py:2:16: PLC2701 Private name import `_a`
5+
|
6+
1 | # Errors.
7+
2 | from _a import b
8+
| ^ PLC2701
9+
3 | from c._d import e
10+
4 | from _f.g import h
11+
|
12+
13+
__main__.py:3:18: PLC2701 Private name import `_d` from external module `c`
14+
|
15+
1 | # Errors.
16+
2 | from _a import b
17+
3 | from c._d import e
18+
| ^ PLC2701
19+
4 | from _f.g import h
20+
5 | from i import _j
21+
|
22+
23+
__main__.py:4:18: PLC2701 Private name import `_f`
24+
|
25+
2 | from _a import b
26+
3 | from c._d import e
27+
4 | from _f.g import h
28+
| ^ PLC2701
29+
5 | from i import _j
30+
6 | from k import _l as m
31+
|
32+
33+
__main__.py:5:15: PLC2701 Private name import `_j` from external module `i`
34+
|
35+
3 | from c._d import e
36+
4 | from _f.g import h
37+
5 | from i import _j
38+
| ^^ PLC2701
39+
6 | from k import _l as m
40+
7 | import _aaa
41+
|
42+
43+
__main__.py:6:21: PLC2701 Private name import `_l` from external module `k`
44+
|
45+
4 | from _f.g import h
46+
5 | from i import _j
47+
6 | from k import _l as m
48+
| ^ PLC2701
49+
7 | import _aaa
50+
8 | import bbb.ccc._ddd as eee # Panicked in https://github.com/astral-sh/ruff/pull/5920
51+
|
52+
53+
__main__.py:7:8: PLC2701 Private name import `_aaa`
54+
|
55+
5 | from i import _j
56+
6 | from k import _l as m
57+
7 | import _aaa
58+
| ^^^^ PLC2701
59+
8 | import bbb.ccc._ddd as eee # Panicked in https://github.com/astral-sh/ruff/pull/5920
60+
|
61+
62+
__main__.py:8:24: PLC2701 Private name import `_ddd` from external module `bbb.ccc`
63+
|
64+
6 | from k import _l as m
65+
7 | import _aaa
66+
8 | import bbb.ccc._ddd as eee # Panicked in https://github.com/astral-sh/ruff/pull/5920
67+
| ^^^ PLC2701
68+
9 |
69+
10 | # Non-errors.
70+
|
71+
72+

ruff.schema.json

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)