Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pylint] (Re-)Implement import-private-name (C2701) #9553

Merged
merged 2 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.

Loading