From d827e6e36a8810ffd149490c5e8a3bb2311b2f8f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 21 Sep 2022 12:13:40 -0400 Subject: [PATCH] Implement F405 (#243) --- README.md | 11 ++- resources/test/fixtures/F405.py | 11 +++ src/ast/types.rs | 2 + src/check_ast.rs | 96 ++++++++++++++++---- src/checks.rs | 89 ++++++++++-------- src/linter.rs | 17 ++++ src/snapshots/ruff__linter__tests__f405.snap | 21 +++++ 7 files changed, 185 insertions(+), 62 deletions(-) create mode 100644 resources/test/fixtures/F405.py create mode 100644 src/snapshots/ruff__linter__tests__f405.snap diff --git a/README.md b/README.md index ad9ae8aa80..1776fa342e 100644 --- a/README.md +++ b/README.md @@ -151,14 +151,14 @@ ruff's goal is to achieve feature-parity with Flake8 when used (1) without any p stylistic checks; limiting to Python 3 obviates the need for certain compatibility checks.) Under those conditions, Flake8 implements about 60 rules, give or take. At time of writing, ruff -implements 43 rules. (Note that these 43 rules likely cover a disproportionate share of errors: +implements 44 rules. (Note that these 44 rules likely cover a disproportionate share of errors: unused imports, undefined variables, etc.) The unimplemented rules are tracked in #170, and include: - 14 rules related to string `.format` calls. -- 4 logical rules. -- 1 rule related to parsing. +- 1 logical rule. +- 1 rule related to docstring parsing. Beyond rule-set parity, ruff suffers from the following limitations vis-à-vis Flake8: @@ -182,12 +182,13 @@ Beyond rule-set parity, ruff suffers from the following limitations vis-à-vis F | E741 | AmbiguousVariableName | ambiguous variable name '...' | | E742 | AmbiguousClassName | ambiguous class name '...' | | E743 | AmbiguousFunctionName | ambiguous function name '...' | -| E902 | IOError | No such file or directory: `...` | +| E902 | IOError | ... | | E999 | SyntaxError | SyntaxError: ... | | F401 | UnusedImport | `...` imported but unused | | F402 | ImportShadowedByLoopVar | import '...' from line 1 shadowed by loop variable | -| F403 | ImportStarUsage | `from ... import *` used; unable to detect undefined names | +| F403 | ImportStarUsed | `from ... import *` used; unable to detect undefined names | | F404 | LateFutureImport | from __future__ imports must occur at the beginning of the file | +| F405 | ImportStarUsage | '...' may be undefined, or defined from star imports: ... | | F406 | ImportStarNotPermitted | `from ... import *` only allowed at module level | | F407 | FutureFeatureNotDefined | future feature '...' is not defined | | F541 | FStringMissingPlaceholders | f-string without any placeholders | diff --git a/resources/test/fixtures/F405.py b/resources/test/fixtures/F405.py new file mode 100644 index 0000000000..38e9822b9a --- /dev/null +++ b/resources/test/fixtures/F405.py @@ -0,0 +1,11 @@ +from mymodule import * + + +def print_name(): + print(name) + + +def print_name(name): + print(name) + +__all__ = ['a'] diff --git a/src/ast/types.rs b/src/ast/types.rs index e02d30e2c0..3bd3c1e76c 100644 --- a/src/ast/types.rs +++ b/src/ast/types.rs @@ -25,6 +25,7 @@ pub enum ScopeKind { pub struct Scope { pub id: usize, pub kind: ScopeKind, + pub import_starred: bool, pub values: BTreeMap, } @@ -33,6 +34,7 @@ impl Scope { Scope { id: id(), kind, + import_starred: false, values: BTreeMap::new(), } } diff --git a/src/check_ast.rs b/src/check_ast.rs index e5f192036c..6fe074d9b6 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -391,7 +391,11 @@ where } } } - StmtKind::ImportFrom { names, module, .. } => { + StmtKind::ImportFrom { + names, + module, + level, + } => { if self .settings .select @@ -447,8 +451,14 @@ where .push(Check::new(CheckKind::LateFutureImport, stmt.location)); } } else if alias.node.name == "*" { + let module_name = format!( + "{}{}", + ".".repeat(level.unwrap_or_default()), + module.clone().unwrap_or_else(|| "module".to_string()), + ); + self.add_binding( - name, + module_name.to_string(), Binding { kind: BindingKind::StarImportation, used: None, @@ -456,27 +466,29 @@ where }, ); - if self.settings.select.contains(&CheckCode::F403) { - self.checks.push(Check::new( - CheckKind::ImportStarUsage( - module.clone().unwrap_or_else(|| "module".to_string()), - ), - stmt.location, - )); - } - if self.settings.select.contains(&CheckCode::F406) { let scope = &self.scopes [*(self.scope_stack.last().expect("No current scope found."))]; if !matches!(scope.kind, ScopeKind::Module) { self.checks.push(Check::new( - CheckKind::ImportStarNotPermitted( - module.clone().unwrap_or_else(|| "module".to_string()), - ), + CheckKind::ImportStarNotPermitted(module_name.to_string()), stmt.location, )); } } + + if self.settings.select.contains(&CheckCode::F403) { + self.checks.push(Check::new( + CheckKind::ImportStarUsed(module_name.to_string()), + stmt.location, + )); + } + + let scope = &mut self.scopes[*(self + .scope_stack + .last_mut() + .expect("No current scope found."))]; + scope.import_starred = true; } else { let binding = Binding { kind: BindingKind::Importation(match module { @@ -1138,6 +1150,7 @@ impl<'a> Checker<'a> { let mut first_iter = true; let mut in_generator = false; + let mut import_starred = false; for scope_index in self.scope_stack.iter().rev() { let scope = &mut self.scopes[*scope_index]; if matches!(scope.kind, ScopeKind::Class) { @@ -1154,6 +1167,28 @@ impl<'a> Checker<'a> { first_iter = false; in_generator = matches!(scope.kind, ScopeKind::Generator); + import_starred = import_starred || scope.import_starred; + } + + if import_starred { + if self.settings.select.contains(&CheckCode::F405) { + let mut from_list = vec![]; + for scope_index in self.scope_stack.iter().rev() { + let scope = &self.scopes[*scope_index]; + for (name, binding) in scope.values.iter() { + if matches!(binding.kind, BindingKind::StarImportation) { + from_list.push(name.as_str()); + } + } + } + from_list.sort(); + + self.checks.push(Check::new( + CheckKind::ImportStarUsage(id.clone(), from_list.join(", ")), + expr.location, + )); + } + return; } if self.settings.select.contains(&CheckCode::F821) { @@ -1364,8 +1399,9 @@ impl<'a> Checker<'a> { } fn check_dead_scopes(&mut self) { - if !self.settings.select.contains(&CheckCode::F822) - && !self.settings.select.contains(&CheckCode::F401) + if !self.settings.select.contains(&CheckCode::F401) + && !self.settings.select.contains(&CheckCode::F405) + && !self.settings.select.contains(&CheckCode::F822) { return; } @@ -1380,15 +1416,39 @@ impl<'a> Checker<'a> { }); if self.settings.select.contains(&CheckCode::F822) + && !scope.import_starred && !self.path.ends_with("__init__.py") { - if let Some(binding) = all_binding { + if let Some(all_binding) = all_binding { if let Some(names) = all_names { for name in names { if !scope.values.contains_key(name) { self.checks.push(Check::new( CheckKind::UndefinedExport(name.to_string()), - binding.location, + all_binding.location, + )); + } + } + } + } + } + + if self.settings.select.contains(&CheckCode::F405) && scope.import_starred { + if let Some(all_binding) = all_binding { + if let Some(names) = all_names { + let mut from_list = vec![]; + for (name, binding) in scope.values.iter() { + if matches!(binding.kind, BindingKind::StarImportation) { + from_list.push(name.as_str()); + } + } + from_list.sort(); + + for name in names { + if !scope.values.contains_key(name) { + self.checks.push(Check::new( + CheckKind::ImportStarUsage(name.clone(), from_list.join(", ")), + all_binding.location, )); } } diff --git a/src/checks.rs b/src/checks.rs index 881f118cd6..ea985d03d7 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -6,7 +6,7 @@ use regex::Regex; use rustpython_parser::ast::Location; use serde::{Deserialize, Serialize}; -pub const ALL_CHECK_CODES: [CheckCode; 43] = [ +pub const ALL_CHECK_CODES: [CheckCode; 44] = [ CheckCode::E402, CheckCode::E501, CheckCode::E711, @@ -25,6 +25,7 @@ pub const ALL_CHECK_CODES: [CheckCode; 43] = [ CheckCode::F402, CheckCode::F403, CheckCode::F404, + CheckCode::F405, CheckCode::F406, CheckCode::F407, CheckCode::F541, @@ -72,6 +73,7 @@ pub enum CheckCode { F402, F403, F404, + F405, F406, F407, F541, @@ -122,6 +124,7 @@ impl FromStr for CheckCode { "F402" => Ok(CheckCode::F402), "F403" => Ok(CheckCode::F403), "F404" => Ok(CheckCode::F404), + "F405" => Ok(CheckCode::F405), "F406" => Ok(CheckCode::F406), "F407" => Ok(CheckCode::F407), "F541" => Ok(CheckCode::F541), @@ -173,6 +176,7 @@ impl CheckCode { CheckCode::F402 => "F402", CheckCode::F403 => "F403", CheckCode::F404 => "F404", + CheckCode::F405 => "F405", CheckCode::F406 => "F406", CheckCode::F407 => "F407", CheckCode::F541 => "F541", @@ -213,49 +217,50 @@ impl CheckCode { /// A placeholder representation of the CheckKind for the check. pub fn kind(&self) -> CheckKind { match self { - CheckCode::E742 => CheckKind::AmbiguousClassName("...".to_string()), - CheckCode::E743 => CheckKind::AmbiguousFunctionName("...".to_string()), - CheckCode::E741 => CheckKind::AmbiguousVariableName("...".to_string()), - CheckCode::F631 => CheckKind::AssertTuple, - CheckCode::F701 => CheckKind::BreakOutsideLoop, - CheckCode::F702 => CheckKind::ContinueOutsideLoop, - CheckCode::F707 => CheckKind::DefaultExceptNotLast, - CheckCode::E731 => CheckKind::DoNotAssignLambda, - CheckCode::E722 => CheckKind::DoNotUseBareExcept, - CheckCode::F831 => CheckKind::DuplicateArgumentName, - CheckCode::F541 => CheckKind::FStringMissingPlaceholders, - CheckCode::F722 => CheckKind::ForwardAnnotationSyntaxError("...".to_string()), - CheckCode::F407 => CheckKind::FutureFeatureNotDefined("...".to_string()), - CheckCode::E902 => CheckKind::IOError("...".to_string()), - CheckCode::F634 => CheckKind::IfTuple, - CheckCode::F402 => CheckKind::ImportShadowedByLoopVar("...".to_string(), 1), - CheckCode::F406 => CheckKind::ImportStarNotPermitted("...".to_string()), - CheckCode::F403 => CheckKind::ImportStarUsage("...".to_string()), - CheckCode::F633 => CheckKind::InvalidPrintSyntax, - CheckCode::F632 => CheckKind::IsLiteral, - CheckCode::F404 => CheckKind::LateFutureImport, - CheckCode::E501 => CheckKind::LineTooLong(89, 88), CheckCode::E402 => CheckKind::ModuleImportNotAtTopOfFile, - CheckCode::F601 => CheckKind::MultiValueRepeatedKeyLiteral, - CheckCode::F602 => CheckKind::MultiValueRepeatedKeyVariable("...".to_string()), - CheckCode::R002 => CheckKind::NoAssertEquals, + CheckCode::E501 => CheckKind::LineTooLong(89, 88), CheckCode::E711 => CheckKind::NoneComparison(RejectedCmpop::Eq), + CheckCode::E712 => CheckKind::TrueFalseComparison(true, RejectedCmpop::Eq), CheckCode::E713 => CheckKind::NotInTest, CheckCode::E714 => CheckKind::NotIsTest, - CheckCode::F901 => CheckKind::RaiseNotImplemented, - CheckCode::F706 => CheckKind::ReturnOutsideFunction, - CheckCode::E999 => CheckKind::SyntaxError("...".to_string()), - CheckCode::F621 => CheckKind::TooManyExpressionsInStarredAssignment, - CheckCode::E712 => CheckKind::TrueFalseComparison(true, RejectedCmpop::Eq), - CheckCode::F622 => CheckKind::TwoStarredExpressions, CheckCode::E721 => CheckKind::TypeComparison, + CheckCode::E722 => CheckKind::DoNotUseBareExcept, + CheckCode::E731 => CheckKind::DoNotAssignLambda, + CheckCode::E741 => CheckKind::AmbiguousVariableName("...".to_string()), + CheckCode::E742 => CheckKind::AmbiguousClassName("...".to_string()), + CheckCode::E743 => CheckKind::AmbiguousFunctionName("...".to_string()), + CheckCode::E902 => CheckKind::IOError("...".to_string()), + CheckCode::E999 => CheckKind::SyntaxError("...".to_string()), + CheckCode::F401 => CheckKind::UnusedImport("...".to_string()), + CheckCode::F402 => CheckKind::ImportShadowedByLoopVar("...".to_string(), 1), + CheckCode::F403 => CheckKind::ImportStarUsed("...".to_string()), + CheckCode::F404 => CheckKind::LateFutureImport, + CheckCode::F405 => CheckKind::ImportStarUsage("...".to_string(), "...".to_string()), + CheckCode::F406 => CheckKind::ImportStarNotPermitted("...".to_string()), + CheckCode::F407 => CheckKind::FutureFeatureNotDefined("...".to_string()), + CheckCode::F541 => CheckKind::FStringMissingPlaceholders, + CheckCode::F601 => CheckKind::MultiValueRepeatedKeyLiteral, + CheckCode::F602 => CheckKind::MultiValueRepeatedKeyVariable("...".to_string()), + CheckCode::F621 => CheckKind::TooManyExpressionsInStarredAssignment, + CheckCode::F622 => CheckKind::TwoStarredExpressions, + CheckCode::F631 => CheckKind::AssertTuple, + CheckCode::F632 => CheckKind::IsLiteral, + CheckCode::F633 => CheckKind::InvalidPrintSyntax, + CheckCode::F634 => CheckKind::IfTuple, + CheckCode::F701 => CheckKind::BreakOutsideLoop, + CheckCode::F702 => CheckKind::ContinueOutsideLoop, + CheckCode::F704 => CheckKind::YieldOutsideFunction, + CheckCode::F706 => CheckKind::ReturnOutsideFunction, + CheckCode::F707 => CheckKind::DefaultExceptNotLast, + CheckCode::F722 => CheckKind::ForwardAnnotationSyntaxError("...".to_string()), + CheckCode::F821 => CheckKind::UndefinedName("...".to_string()), CheckCode::F822 => CheckKind::UndefinedExport("...".to_string()), CheckCode::F823 => CheckKind::UndefinedLocal("...".to_string()), - CheckCode::F821 => CheckKind::UndefinedName("...".to_string()), - CheckCode::F401 => CheckKind::UnusedImport("...".to_string()), + CheckCode::F831 => CheckKind::DuplicateArgumentName, CheckCode::F841 => CheckKind::UnusedVariable("...".to_string()), + CheckCode::F901 => CheckKind::RaiseNotImplemented, CheckCode::R001 => CheckKind::UselessObjectInheritance("...".to_string()), - CheckCode::F704 => CheckKind::YieldOutsideFunction, + CheckCode::R002 => CheckKind::NoAssertEquals, } } } @@ -292,7 +297,8 @@ pub enum CheckKind { IfTuple, ImportShadowedByLoopVar(String, usize), ImportStarNotPermitted(String), - ImportStarUsage(String), + ImportStarUsage(String, String), + ImportStarUsed(String), InvalidPrintSyntax, IsLiteral, LateFutureImport, @@ -341,7 +347,8 @@ impl CheckKind { CheckKind::IfTuple => "IfTuple", CheckKind::ImportShadowedByLoopVar(_, _) => "ImportShadowedByLoopVar", CheckKind::ImportStarNotPermitted(_) => "ImportStarNotPermitted", - CheckKind::ImportStarUsage(_) => "ImportStarUsage", + CheckKind::ImportStarUsage(_, _) => "ImportStarUsage", + CheckKind::ImportStarUsed(_) => "ImportStarUsed", CheckKind::InvalidPrintSyntax => "InvalidPrintSyntax", CheckKind::IsLiteral => "IsLiteral", CheckKind::LateFutureImport => "LateFutureImport", @@ -392,7 +399,8 @@ impl CheckKind { CheckKind::IfTuple => &CheckCode::F634, CheckKind::ImportShadowedByLoopVar(_, _) => &CheckCode::F402, CheckKind::ImportStarNotPermitted(_) => &CheckCode::F406, - CheckKind::ImportStarUsage(_) => &CheckCode::F403, + CheckKind::ImportStarUsed(_) => &CheckCode::F403, + CheckKind::ImportStarUsage(_, _) => &CheckCode::F405, CheckKind::InvalidPrintSyntax => &CheckCode::F633, CheckKind::IsLiteral => &CheckCode::F632, CheckKind::LateFutureImport => &CheckCode::F404, @@ -466,9 +474,12 @@ impl CheckKind { CheckKind::ImportStarNotPermitted(name) => { format!("`from {name} import *` only allowed at module level") } - CheckKind::ImportStarUsage(name) => { + CheckKind::ImportStarUsed(name) => { format!("`from {name} import *` used; unable to detect undefined names") } + CheckKind::ImportStarUsage(name, sources) => { + format!("'{name}' may be undefined, or defined from star imports: {sources}") + } CheckKind::IsLiteral => "use ==/!= to compare constant literals".to_string(), CheckKind::LateFutureImport => { "from __future__ imports must occur at the beginning of the file".to_string() diff --git a/src/linter.rs b/src/linter.rs index 3a8a948d0d..2cce088ded 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -383,6 +383,23 @@ mod tests { Ok(()) } + #[test] + fn f405() -> Result<()> { + let mut checks = check_path( + Path::new("./resources/test/fixtures/F405.py"), + &settings::Settings { + line_length: 88, + exclude: vec![], + extend_exclude: vec![], + select: BTreeSet::from([CheckCode::F405]), + }, + &fixer::Mode::Generate, + )?; + checks.sort_by_key(|check| check.location); + insta::assert_yaml_snapshot!(checks); + Ok(()) + } + #[test] fn f406() -> Result<()> { let mut checks = check_path( diff --git a/src/snapshots/ruff__linter__tests__f405.snap b/src/snapshots/ruff__linter__tests__f405.snap new file mode 100644 index 0000000000..645ba3f30d --- /dev/null +++ b/src/snapshots/ruff__linter__tests__f405.snap @@ -0,0 +1,21 @@ +--- +source: src/linter.rs +expression: checks +--- +- kind: + ImportStarUsed: + - name + - mymodule + location: + row: 5 + column: 11 + fix: ~ +- kind: + ImportStarUsed: + - a + - mymodule + location: + row: 11 + column: 1 + fix: ~ +