From 1b3e54231c5a600840ce50502d50b6aa27d96228 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 22 Mar 2023 13:03:30 -0400 Subject: [PATCH] Flag, but don't fix, unused imports in `ModuleNotFoundError` blocks (#3658) --- .../test/fixtures/pyflakes/F401_10.py | 13 +- crates/ruff/src/checkers/ast/mod.rs | 112 ++++++++++-------- .../ruff/src/rules/pyflakes/rules/imports.rs | 56 +++++---- crates/ruff/src/rules/pyflakes/rules/mod.rs | 2 +- ...les__pyflakes__tests__F401_F401_10.py.snap | 34 +++++- crates/ruff_python_ast/src/context.rs | 16 ++- crates/ruff_python_ast/src/helpers.rs | 8 -- crates/ruff_python_ast/src/scope.rs | 11 ++ 8 files changed, 163 insertions(+), 89 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/pyflakes/F401_10.py b/crates/ruff/resources/test/fixtures/pyflakes/F401_10.py index 14e116f673..e1cf01eec1 100644 --- a/crates/ruff/resources/test/fixtures/pyflakes/F401_10.py +++ b/crates/ruff/resources/test/fixtures/pyflakes/F401_10.py @@ -1,10 +1,19 @@ -"""Test: imports within `ModuleNotFoundError` handlers.""" +"""Test: imports within `ModuleNotFoundError` and `ImportError` handlers.""" -def check_orjson(): +def module_not_found_error(): try: import orjson return True except ModuleNotFoundError: return False + + +def import_error(): + try: + import orjson + + return True + except ImportError: + return False diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index cb06c83c3f..8b62f6dde6 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -14,13 +14,11 @@ use rustpython_parser::ast::{ use ruff_diagnostics::Diagnostic; use ruff_python_ast::context::Context; -use ruff_python_ast::helpers::{ - binding_range, extract_handled_exceptions, to_module_path, Exceptions, -}; +use ruff_python_ast::helpers::{binding_range, extract_handled_exceptions, to_module_path}; use ruff_python_ast::operations::{extract_all_names, AllNamesFlags}; use ruff_python_ast::scope::{ - Binding, BindingId, BindingKind, ClassDef, ExecutionContext, FunctionDef, Lambda, Scope, - ScopeId, ScopeKind, ScopeStack, + Binding, BindingId, BindingKind, ClassDef, Exceptions, ExecutionContext, FunctionDef, Lambda, + Scope, ScopeId, ScopeKind, ScopeStack, }; use ruff_python_ast::source_code::{Indexer, Locator, Stylist}; use ruff_python_ast::types::{Node, Range, RefEquality}; @@ -198,6 +196,7 @@ where if !scope_index.is_global() { // Add the binding to the current scope. let context = self.ctx.execution_context(); + let exceptions = self.ctx.exceptions(); let scope = &mut self.ctx.scopes[scope_index]; let usage = Some((scope.id, Range::from(stmt))); for (name, range) in names.iter().zip(ranges.iter()) { @@ -209,6 +208,7 @@ where range: *range, source: Some(RefEquality(stmt)), context, + exceptions, }); scope.add(name, id); } @@ -226,6 +226,7 @@ where let ranges: Vec = helpers::find_names(stmt, self.locator).collect(); if !scope_index.is_global() { let context = self.ctx.execution_context(); + let exceptions = self.ctx.exceptions(); let scope = &mut self.ctx.scopes[scope_index]; let usage = Some((scope.id, Range::from(stmt))); for (name, range) in names.iter().zip(ranges.iter()) { @@ -238,6 +239,7 @@ where range: *range, source: Some(RefEquality(stmt)), context, + exceptions, }); scope.add(name, id); } @@ -639,7 +641,6 @@ where self.visit_expr(expr); } - let context = self.ctx.execution_context(); self.add_binding( name, Binding { @@ -649,7 +650,8 @@ where typing_usage: None, range: Range::from(stmt), source: Some(self.ctx.current_stmt().clone()), - context, + context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); } @@ -834,14 +836,6 @@ where pyupgrade::rules::deprecated_mock_import(self, stmt); } - // If a module is imported within a `ModuleNotFoundError` body, treat that as a - // synthetic usage. - let is_handled = self - .ctx - .handled_exceptions - .iter() - .any(|exceptions| exceptions.contains(Exceptions::MODULE_NOT_FOUND_ERROR)); - for alias in names { if alias.node.name == "__future__" { let name = alias.node.asname.as_ref().unwrap_or(&alias.node.name); @@ -856,6 +850,7 @@ where range: Range::from(alias), source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); @@ -877,15 +872,12 @@ where Binding { kind: BindingKind::SubmoduleImportation(name, full_name), runtime_usage: None, - synthetic_usage: if is_handled { - Some((self.ctx.scope_id(), Range::from(alias))) - } else { - None - }, + synthetic_usage: None, typing_usage: None, range: Range::from(alias), source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); } else { @@ -907,7 +899,7 @@ where Binding { kind: BindingKind::Importation(name, full_name), runtime_usage: None, - synthetic_usage: if is_handled || is_explicit_reexport { + synthetic_usage: if is_explicit_reexport { Some((self.ctx.scope_id(), Range::from(alias))) } else { None @@ -916,6 +908,7 @@ where range: Range::from(alias), source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); @@ -1160,14 +1153,6 @@ where } } - // If a module is imported within a `ModuleNotFoundError` body, treat that as a - // synthetic usage. - let is_handled = self - .ctx - .handled_exceptions - .iter() - .any(|exceptions| exceptions.contains(Exceptions::MODULE_NOT_FOUND_ERROR)); - for alias in names { if let Some("__future__") = module.as_deref() { let name = alias.node.asname.as_ref().unwrap_or(&alias.node.name); @@ -1182,6 +1167,7 @@ where range: Range::from(alias), source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); @@ -1207,15 +1193,12 @@ where Binding { kind: BindingKind::StarImportation(*level, module.clone()), runtime_usage: None, - synthetic_usage: if is_handled { - Some((self.ctx.scope_id(), Range::from(alias))) - } else { - None - }, + synthetic_usage: None, typing_usage: None, range: Range::from(stmt), source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); @@ -1283,16 +1266,16 @@ where Binding { kind: BindingKind::FromImportation(name, full_name), runtime_usage: None, - synthetic_usage: if is_handled || is_explicit_reexport { + synthetic_usage: if is_explicit_reexport { Some((self.ctx.scope_id(), Range::from(alias))) } else { None }, typing_usage: None, range: Range::from(alias), - source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); } @@ -1914,6 +1897,7 @@ where range: Range::from(stmt), source: Some(RefEquality(stmt)), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }); self.ctx.global_scope_mut().add(name, id); } @@ -1976,6 +1960,7 @@ where range: Range::from(*stmt), source: Some(RefEquality(stmt)), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }); self.ctx.global_scope_mut().add(name, id); } @@ -2125,6 +2110,7 @@ where range: Range::from(stmt), source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); } @@ -3906,6 +3892,7 @@ where range: Range::from(arg), source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); @@ -3949,6 +3936,7 @@ where range: Range::from(pattern), source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); } @@ -4127,6 +4115,7 @@ impl<'a> Checker<'a> { typing_usage: None, source: None, context: ExecutionContext::Runtime, + exceptions: Exceptions::empty(), }); scope.add(builtin, id); } @@ -4351,6 +4340,7 @@ impl<'a> Checker<'a> { range: Range::from(expr), source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); return; @@ -4371,6 +4361,7 @@ impl<'a> Checker<'a> { range: Range::from(expr), source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); return; @@ -4387,6 +4378,7 @@ impl<'a> Checker<'a> { range: Range::from(expr), source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); return; @@ -4452,6 +4444,7 @@ impl<'a> Checker<'a> { range: Range::from(expr), source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); return; @@ -4468,6 +4461,7 @@ impl<'a> Checker<'a> { range: Range::from(expr), source: Some(self.ctx.current_stmt().clone()), context: self.ctx.execution_context(), + exceptions: self.ctx.exceptions(), }, ); } @@ -4925,8 +4919,11 @@ impl<'a> Checker<'a> { // Collect all unused imports by location. (Multiple unused imports at the same // location indicates an `import from`.) type UnusedImport<'a> = (&'a str, &'a Range); - type BindingContext<'a, 'b> = - (&'a RefEquality<'b, Stmt>, Option<&'a RefEquality<'b, Stmt>>); + type BindingContext<'a, 'b> = ( + &'a RefEquality<'b, Stmt>, + Option<&'a RefEquality<'b, Stmt>>, + Exceptions, + ); let mut unused: FxHashMap> = FxHashMap::default(); let mut ignored: FxHashMap> = @@ -4948,6 +4945,7 @@ impl<'a> Checker<'a> { let defined_by = binding.source.as_ref().unwrap(); let defined_in = self.ctx.child_to_parent.get(defined_by); + let exceptions = binding.exceptions; let child: &Stmt = defined_by.into(); let diagnostic_lineno = binding.range.location.row(); @@ -4965,27 +4963,30 @@ impl<'a> Checker<'a> { }) { ignored - .entry((defined_by, defined_in)) + .entry((defined_by, defined_in, exceptions)) .or_default() .push((full_name, &binding.range)); } else { unused - .entry((defined_by, defined_in)) + .entry((defined_by, defined_in, exceptions)) .or_default() .push((full_name, &binding.range)); } } - let ignore_init = + let in_init = self.settings.ignore_init_module_imports && self.path.ends_with("__init__.py"); - for ((defined_by, defined_in), unused_imports) in unused + for ((defined_by, defined_in, exceptions), unused_imports) in unused .into_iter() - .sorted_by_key(|((defined_by, _), _)| defined_by.location) + .sorted_by_key(|((defined_by, ..), ..)| defined_by.location) { let child: &Stmt = defined_by.into(); let parent: Option<&Stmt> = defined_in.map(Into::into); + let multiple = unused_imports.len() > 1; + let in_except_handler = exceptions + .intersects(Exceptions::MODULE_NOT_FOUND_ERROR | Exceptions::IMPORT_ERROR); - let fix = if !ignore_init && self.patch(Rule::UnusedImport) { + let fix = if !in_init && !in_except_handler && self.patch(Rule::UnusedImport) { let deleted: Vec<&Stmt> = self.deletions.iter().map(Into::into).collect(); match autofix::helpers::remove_unused_imports( unused_imports.iter().map(|(full_name, _)| *full_name), @@ -5011,12 +5012,17 @@ impl<'a> Checker<'a> { None }; - let multiple = unused_imports.len() > 1; for (full_name, range) in unused_imports { let mut diagnostic = Diagnostic::new( pyflakes::rules::UnusedImport { name: full_name.to_string(), - ignore_init, + context: if in_except_handler { + Some(pyflakes::rules::UnusedImportContext::ExceptHandler) + } else if in_init { + Some(pyflakes::rules::UnusedImportContext::Init) + } else { + None + }, multiple, }, *range, @@ -5032,17 +5038,25 @@ impl<'a> Checker<'a> { diagnostics.push(diagnostic); } } - for ((defined_by, ..), unused_imports) in ignored + for ((defined_by, .., exceptions), unused_imports) in ignored .into_iter() - .sorted_by_key(|((defined_by, _), _)| defined_by.location) + .sorted_by_key(|((defined_by, ..), ..)| defined_by.location) { let child: &Stmt = defined_by.into(); let multiple = unused_imports.len() > 1; + let in_except_handler = exceptions + .intersects(Exceptions::MODULE_NOT_FOUND_ERROR | Exceptions::IMPORT_ERROR); for (full_name, range) in unused_imports { let mut diagnostic = Diagnostic::new( pyflakes::rules::UnusedImport { name: full_name.to_string(), - ignore_init, + context: if in_except_handler { + Some(pyflakes::rules::UnusedImportContext::ExceptHandler) + } else if in_init { + Some(pyflakes::rules::UnusedImportContext::Init) + } else { + None + }, multiple, }, *range, diff --git a/crates/ruff/src/rules/pyflakes/rules/imports.rs b/crates/ruff/src/rules/pyflakes/rules/imports.rs index 9fc3efab9b..77c56b5832 100644 --- a/crates/ruff/src/rules/pyflakes/rules/imports.rs +++ b/crates/ruff/src/rules/pyflakes/rules/imports.rs @@ -9,46 +9,52 @@ use ruff_python_stdlib::future::ALL_FEATURE_NAMES; use crate::checkers::ast::Checker; +#[derive(Debug, PartialEq, Eq)] +pub enum UnusedImportContext { + ExceptHandler, + Init, +} + #[violation] pub struct UnusedImport { pub name: String, - pub ignore_init: bool, + pub context: Option, pub multiple: bool, } -fn fmt_unused_import_autofix_msg(unused_import: &UnusedImport) -> String { - let UnusedImport { name, multiple, .. } = unused_import; - if *multiple { - "Remove unused import".to_string() - } else { - format!("Remove unused import: `{name}`") - } -} impl Violation for UnusedImport { const AUTOFIX: AutofixKind = AutofixKind::Sometimes; #[derive_message_formats] fn message(&self) -> String { - let UnusedImport { - name, ignore_init, .. - } = self; - if *ignore_init { - format!( - "`{name}` imported but unused; consider adding to `__all__` or using a redundant \ - alias" - ) - } else { - format!("`{name}` imported but unused") + let UnusedImport { name, context, .. } = self; + match context { + Some(UnusedImportContext::ExceptHandler) => { + format!( + "`{name}` imported but unused; consider using `importlib.util.find_spec` to test for availability" + ) + } + Some(UnusedImportContext::Init) => { + format!( + "`{name}` imported but unused; consider adding to `__all__` or using a redundant \ + alias" + ) + } + None => format!("`{name}` imported but unused"), } } fn autofix_title_formatter(&self) -> Option String> { - let UnusedImport { ignore_init, .. } = self; - if *ignore_init { - None - } else { - Some(fmt_unused_import_autofix_msg) - } + let UnusedImport { context, .. } = self; + context + .is_none() + .then_some(|UnusedImport { name, multiple, .. }| { + if *multiple { + "Remove unused import".to_string() + } else { + format!("Remove unused import: `{name}`") + } + }) } } #[violation] diff --git a/crates/ruff/src/rules/pyflakes/rules/mod.rs b/crates/ruff/src/rules/pyflakes/rules/mod.rs index 34c07fa11f..4f466f57f1 100644 --- a/crates/ruff/src/rules/pyflakes/rules/mod.rs +++ b/crates/ruff/src/rules/pyflakes/rules/mod.rs @@ -10,7 +10,7 @@ pub use if_tuple::{if_tuple, IfTuple}; pub use imports::{ future_feature_not_defined, FutureFeatureNotDefined, ImportShadowedByLoopVar, LateFutureImport, UndefinedLocalWithImportStar, UndefinedLocalWithImportStarUsage, - UndefinedLocalWithNestedImportStarUsage, UnusedImport, + UndefinedLocalWithNestedImportStarUsage, UnusedImport, UnusedImportContext, }; pub use invalid_literal_comparisons::{invalid_literal_comparison, IsLiteral}; pub use invalid_print_syntax::{invalid_print_syntax, InvalidPrintSyntax}; diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F401_F401_10.py.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F401_F401_10.py.snap index bd5537b631..4cbd0ed012 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F401_F401_10.py.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F401_F401_10.py.snap @@ -2,5 +2,37 @@ source: crates/ruff/src/rules/pyflakes/mod.rs expression: diagnostics --- -[] +- kind: + name: UnusedImport + body: "`orjson` imported but unused; consider using `importlib.util.find_spec` to test for availability" + suggestion: ~ + fixable: false + location: + row: 6 + column: 15 + end_location: + row: 6 + column: 21 + fix: ~ + parent: ~ +- kind: + name: UnusedImport + body: "`orjson` imported but unused" + suggestion: "Remove unused import: `orjson`" + fixable: true + location: + row: 15 + column: 15 + end_location: + row: 15 + column: 21 + fix: + content: "" + location: + row: 15 + column: 0 + end_location: + row: 16 + column: 0 + parent: ~ diff --git a/crates/ruff_python_ast/src/context.rs b/crates/ruff_python_ast/src/context.rs index bfaacd7c88..7d7e4a95eb 100644 --- a/crates/ruff_python_ast/src/context.rs +++ b/crates/ruff_python_ast/src/context.rs @@ -8,10 +8,10 @@ use smallvec::smallvec; use ruff_python_stdlib::path::is_python_stub_file; use ruff_python_stdlib::typing::TYPING_EXTENSIONS; -use crate::helpers::{collect_call_path, from_relative_import, Exceptions}; +use crate::helpers::{collect_call_path, from_relative_import}; use crate::scope::{ - Binding, BindingId, BindingKind, Bindings, ExecutionContext, Scope, ScopeId, ScopeKind, - ScopeStack, Scopes, + Binding, BindingId, BindingKind, Bindings, Exceptions, ExecutionContext, Scope, ScopeId, + ScopeKind, ScopeStack, Scopes, }; use crate::types::{CallPath, RefEquality}; use crate::typing::AnnotationKind; @@ -309,6 +309,7 @@ impl<'a> Context<'a> { self.in_exception_handler } + /// Return the [`ExecutionContext`] of the current scope. pub const fn execution_context(&self) -> ExecutionContext { if self.in_type_checking_block || self.in_annotation @@ -319,4 +320,13 @@ impl<'a> Context<'a> { ExecutionContext::Runtime } } + + /// Return the union of all handled exceptions as an [`Exceptions`] bitflag. + pub fn exceptions(&self) -> Exceptions { + let mut exceptions = Exceptions::empty(); + for exception in &self.handled_exceptions { + exceptions.insert(*exception); + } + exceptions + } } diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 0c399fe6a7..f626d81108 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -1,6 +1,5 @@ use std::path::Path; -use bitflags::bitflags; use itertools::Itertools; use log::error; use once_cell::sync::Lazy; @@ -577,13 +576,6 @@ pub fn has_non_none_keyword(keywords: &[Keyword], keyword: &str) -> bool { }) } -bitflags! { - pub struct Exceptions: u32 { - const NAME_ERROR = 0b0000_0001; - const MODULE_NOT_FOUND_ERROR = 0b0000_0010; - } -} - /// Extract the names of all handled exceptions. pub fn extract_handled_exceptions(handlers: &[Excepthandler]) -> Vec<&Expr> { let mut handled_exceptions = Vec::new(); diff --git a/crates/ruff_python_ast/src/scope.rs b/crates/ruff_python_ast/src/scope.rs index e4f1a991f0..90c04222e7 100644 --- a/crates/ruff_python_ast/src/scope.rs +++ b/crates/ruff_python_ast/src/scope.rs @@ -1,4 +1,5 @@ use crate::types::{Range, RefEquality}; +use bitflags::bitflags; use rustc_hash::FxHashMap; use rustpython_parser::ast::{Arguments, Expr, Keyword, Stmt}; use std::num::TryFromIntError; @@ -222,6 +223,14 @@ impl Default for ScopeStack { } } +bitflags! { + pub struct Exceptions: u32 { + const NAME_ERROR = 0b0000_0001; + const MODULE_NOT_FOUND_ERROR = 0b0000_0010; + const IMPORT_ERROR = 0b0000_0100; + } +} + #[derive(Debug, Clone)] pub struct Binding<'a> { pub kind: BindingKind<'a>, @@ -241,6 +250,8 @@ pub struct Binding<'a> { /// (e.g.) `__future__` imports, explicit re-exports, and other bindings /// that should be considered used even if they're never referenced. pub synthetic_usage: Option<(ScopeId, Range)>, + /// The exceptions that were handled when the binding was defined. + pub exceptions: Exceptions, } impl<'a> Binding<'a> {