From 33c5da324df2b5d43ba0fb090eb08ca05ffdf0bb Mon Sep 17 00:00:00 2001 From: "Andrew S. Brown" Date: Sun, 18 Jun 2023 21:16:10 -0700 Subject: [PATCH] Add import-outside-toplevel rule --- .../pylint/import_outside_top_level.py | 22 ++++++ .../src/checkers/ast/analyze/statement.rs | 6 ++ crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../pylint/rules/import_outside_top_level.rs | 65 ++++++++++++++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + ...__PLC0415_import_outside_top_level.py.snap | 76 +++++++++++++++++++ ruff.schema.json | 1 + 8 files changed, 174 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/import_outside_top_level.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/import_outside_top_level.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0415_import_outside_top_level.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/import_outside_top_level.py b/crates/ruff_linter/resources/test/fixtures/pylint/import_outside_top_level.py new file mode 100644 index 00000000000000..0ad9dc782a630b --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/import_outside_top_level.py @@ -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] diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 29e5a721734406..92ef893a2cf63c 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -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() { @@ -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() { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 249cddfa4276ab..7db6023706d549 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -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)] diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 2a80dcd63df7dd..8fa46ffa5372cf 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -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"))] diff --git a/crates/ruff_linter/src/rules/pylint/rules/import_outside_top_level.rs b/crates/ruff_linter/src/rules/pylint/rules/import_outside_top_level.rs new file mode 100644 index 00000000000000..2d654c72151c4f --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/import_outside_top_level.rs @@ -0,0 +1,65 @@ +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()) +/// ``` +/// +/// ## References +/// - [PEP 8 - Style Guide for Python Code](https://peps.python.org/pep-0008/#imports) +/// +/// [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 used only 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())); + } +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 5ac1858a41f7ed..7405deedadf5e5 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -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::*; @@ -87,6 +88,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; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0415_import_outside_top_level.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0415_import_outside_top_level.py.snap new file mode 100644 index 00000000000000..50cf44624653a1 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0415_import_outside_top_level.py.snap @@ -0,0 +1,76 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +import_outside_top_level.py:10:5: PLC0415 `import` should be used only 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 used only 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 used only 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 used only 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 used only 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 used only 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 used only 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 used only at the top-level of a file + | +21 | def __init__(self): +22 | import trace # [import-outside-toplevel] + | ^^^^^^^^^^^^ PLC0415 + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 82500eadbda0af..958175d80fdd37 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2976,6 +2976,7 @@ "PLC04", "PLC041", "PLC0414", + "PLC0415", "PLC1", "PLC19", "PLC190",