Skip to content

Commit

Permalink
Implement pylint import-outside-toplevel rule (C0415) (#5180)
Browse files Browse the repository at this point in the history
## Summary

Implements pylint C0415 (import-outside-toplevel) — imports should be at
the top level of a file.

The great debate I had on this implementation is whether "top-level" is
one word or two (`toplevel` or `top_level`). I opted for 2 because that
seemed to be how it is used in the codebase but the rule string itself
uses one-word "toplevel." 🤷 I'd be happy to change it as desired.

I suppose this could be auto-fixed by moving the import to the
top-level, but it seems likely that the author's intent was to actually
import this dynamically, so I view the main point of this rule is to
force some sort of explanation, and auto-fixing might be annoying.

For reference, this is what "pylint" reports:
```
> pylint crates/ruff/resources/test/fixtures/pylint/import_outside_top_level.py
************* Module import_outside_top_level
...
crates/ruff/resources/test/fixtures/pylint/import_outside_top_level.py:4:4: C0415: Import outside toplevel (string) (import-outside-toplevel)
```

ruff would now report:

```
import_outside_top_level.py:4:5: PLC0415 `import` should be used only at the top level of a file
  |
3 | def import_outside_top_level():
4 |     import string # [import-outside-toplevel]
  |     ^^^^^^^^^^^^^ PLC0415
  |
```

Contributes to #970.

## Test Plan

Snapshot test.
  • Loading branch information
ashanbrown committed Oct 29, 2023
1 parent d7b966d commit 9b89bf7
Show file tree
Hide file tree
Showing 8 changed files with 171 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
from typing import TYPE_CHECKING

# Verify that statements nested in conditionals (such as top-level type-checking blocks)
# are still considered top-level
if TYPE_CHECKING:
import string


def import_in_function():
import symtable # [import-outside-toplevel]
import os, sys # [import-outside-toplevel]
import time as thyme # [import-outside-toplevel]
import random as rand, socket as sock # [import-outside-toplevel]
from collections import defaultdict # [import-outside-toplevel]
from math import sin as sign, cos as cosplay # [import-outside-toplevel]


class ClassWithImports:
import tokenize # [import-outside-toplevel]

def __init__(self):
import trace # [import-outside-toplevel]
6 changes: 6 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::ModuleImportNotAtTopOfFile) {
pycodestyle::rules::module_import_not_at_top_of_file(checker, stmt);
}
if checker.enabled(Rule::ImportOutsideTopLevel) {
pylint::rules::import_outside_top_level(checker, stmt);
}
if checker.enabled(Rule::GlobalStatement) {
for name in names {
if let Some(asname) = name.asname.as_ref() {
Expand Down Expand Up @@ -706,6 +709,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::ModuleImportNotAtTopOfFile) {
pycodestyle::rules::module_import_not_at_top_of_file(checker, stmt);
}
if checker.enabled(Rule::ImportOutsideTopLevel) {
pylint::rules::import_outside_top_level(checker, stmt);
}
if checker.enabled(Rule::GlobalStatement) {
for name in names {
if let Some(asname) = name.asname.as_ref() {
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 @@ -211,6 +211,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "C0205") => (RuleGroup::Stable, rules::pylint::rules::SingleStringSlots),
(Pylint, "C0208") => (RuleGroup::Stable, rules::pylint::rules::IterationOverSet),
(Pylint, "C0414") => (RuleGroup::Stable, rules::pylint::rules::UselessImportAlias),
(Pylint, "C0415") => (RuleGroup::Preview, rules::pylint::rules::ImportOutsideTopLevel),
(Pylint, "C2401") => (RuleGroup::Preview, rules::pylint::rules::NonAsciiName),
(Pylint, "C2403") => (RuleGroup::Preview, rules::pylint::rules::NonAsciiImportName),
#[allow(deprecated)]
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ mod tests {
Rule::GlobalVariableNotAssigned,
Path::new("global_variable_not_assigned.py")
)]
#[test_case(Rule::ImportOutsideTopLevel, Path::new("import_outside_top_level.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
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::Stmt;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for `import` statements outside of a module's top-level scope, such
/// as within a function or class definition.
///
/// ## Why is this bad?
/// [PEP 8] recommends placing imports not only at the top-level of a module,
/// but at the very top of the file, "just after any module comments and
/// docstrings, and before module globals and constants."
///
/// `import` statements have effects that are global in scope; defining them at
/// the top level has a number of benefits. For example, it makes it easier to
/// identify the dependencies of a module, and ensures that any invalid imports
/// are caught regardless of whether a specific function is called or class is
/// instantiated.
///
/// An import statement would typically be placed within a function only to
/// avoid a circular dependency, to defer a costly module load, or to avoid
/// loading a dependency altogether in a certain runtime environment.
///
/// ## Example
/// ```python
/// def print_python_version():
/// import platform
///
/// print(python.python_version())
/// ```
///
/// Use instead:
/// ```python
/// import platform
///
///
/// def print_python_version():
/// print(python.python_version())
/// ```
///
/// [PEP 8]: https://peps.python.org/pep-0008/#imports
#[violation]
pub struct ImportOutsideTopLevel;

impl Violation for ImportOutsideTopLevel {
#[derive_message_formats]
fn message(&self) -> String {
format!("`import` should be at the top-level of a file")
}
}

/// C0415
pub(crate) fn import_outside_top_level(checker: &mut Checker, stmt: &Stmt) {
if !checker.semantic().current_scope().kind.is_module() {
checker
.diagnostics
.push(Diagnostic::new(ImportOutsideTopLevel, stmt.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 @@ -18,6 +18,7 @@ pub(crate) use eq_without_hash::*;
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_self::*;
pub(crate) use invalid_all_format::*;
pub(crate) use invalid_all_object::*;
Expand Down Expand Up @@ -88,6 +89,7 @@ mod eq_without_hash;
mod global_at_module_level;
mod global_statement;
mod global_variable_not_assigned;
mod import_outside_top_level;
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,76 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
import_outside_top_level.py:10:5: PLC0415 `import` should be at the top-level of a file
|
9 | def import_in_function():
10 | import symtable # [import-outside-toplevel]
| ^^^^^^^^^^^^^^^ PLC0415
11 | import os, sys # [import-outside-toplevel]
12 | import time as thyme # [import-outside-toplevel]
|

import_outside_top_level.py:11:5: PLC0415 `import` should be at the top-level of a file
|
9 | def import_in_function():
10 | import symtable # [import-outside-toplevel]
11 | import os, sys # [import-outside-toplevel]
| ^^^^^^^^^^^^^^ PLC0415
12 | import time as thyme # [import-outside-toplevel]
13 | import random as rand, socket as sock # [import-outside-toplevel]
|

import_outside_top_level.py:12:5: PLC0415 `import` should be at the top-level of a file
|
10 | import symtable # [import-outside-toplevel]
11 | import os, sys # [import-outside-toplevel]
12 | import time as thyme # [import-outside-toplevel]
| ^^^^^^^^^^^^^^^^^^^^ PLC0415
13 | import random as rand, socket as sock # [import-outside-toplevel]
14 | from collections import defaultdict # [import-outside-toplevel]
|

import_outside_top_level.py:13:5: PLC0415 `import` should be at the top-level of a file
|
11 | import os, sys # [import-outside-toplevel]
12 | import time as thyme # [import-outside-toplevel]
13 | import random as rand, socket as sock # [import-outside-toplevel]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLC0415
14 | from collections import defaultdict # [import-outside-toplevel]
15 | from math import sin as sign, cos as cosplay # [import-outside-toplevel]
|

import_outside_top_level.py:14:5: PLC0415 `import` should be at the top-level of a file
|
12 | import time as thyme # [import-outside-toplevel]
13 | import random as rand, socket as sock # [import-outside-toplevel]
14 | from collections import defaultdict # [import-outside-toplevel]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLC0415
15 | from math import sin as sign, cos as cosplay # [import-outside-toplevel]
|

import_outside_top_level.py:15:5: PLC0415 `import` should be at the top-level of a file
|
13 | import random as rand, socket as sock # [import-outside-toplevel]
14 | from collections import defaultdict # [import-outside-toplevel]
15 | from math import sin as sign, cos as cosplay # [import-outside-toplevel]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLC0415
|

import_outside_top_level.py:19:5: PLC0415 `import` should be at the top-level of a file
|
18 | class ClassWithImports:
19 | import tokenize # [import-outside-toplevel]
| ^^^^^^^^^^^^^^^ PLC0415
20 |
21 | def __init__(self):
|

import_outside_top_level.py:22:9: PLC0415 `import` should be at the top-level of a file
|
21 | def __init__(self):
22 | import trace # [import-outside-toplevel]
| ^^^^^^^^^^^^ PLC0415
|


1 change: 1 addition & 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 9b89bf7

Please sign in to comment.