Flag, but don't fix, unused imports in ModuleNotFoundError blocks (#3658)

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

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::{
+use ruff_python_ast::helpers::{binding_range, extract_handled_exceptions, to_module_path};
-    binding_range, extract_handled_exceptions, to_module_path, Exceptions,
-};
 use ruff_python_ast::operations::{extract_all_names, AllNamesFlags};
 use ruff_python_ast::scope::{
-    Binding, BindingId, BindingKind, ClassDef, ExecutionContext, FunctionDef, Lambda, Scope,
+    Binding, BindingId, BindingKind, ClassDef, Exceptions, ExecutionContext, FunctionDef, Lambda,
-    ScopeId, ScopeKind, ScopeStack,
+    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<Range> = 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 {
+                                synthetic_usage: None,
-                                    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(),
                             },
                         );
                     } 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 {
+                                synthetic_usage: None,
-                                    Some((self.ctx.scope_id(), Range::from(alias)))
-                                } else {
-                                    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> =
+                type BindingContext<'a, 'b> = (
-                    (&'a RefEquality<'b, Stmt>, Option<&'a RefEquality<'b, Stmt>>);
+                    &'a RefEquality<'b, Stmt>,
+                    Option<&'a RefEquality<'b, Stmt>>,
+                    Exceptions,
+                );
 
                 let mut unused: FxHashMap<BindingContext, Vec<UnusedImport>> = FxHashMap::default();
                 let mut ignored: FxHashMap<BindingContext, Vec<UnusedImport>> =
@@ -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,

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<UnusedImportContext>,
     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 {
+        let UnusedImport { name, context, .. } = self;
-            name, ignore_init, ..
+        match context {
-        } = self;
+            Some(UnusedImportContext::ExceptHandler) => {
-        if *ignore_init {
+                format!(
-            format!(
+                    "`{name}` imported but unused; consider using `importlib.util.find_spec` to test for availability"
-                "`{name}` imported but unused; consider adding to `__all__` or using a redundant \
+                )
-                 alias"
+            }
-            )
+            Some(UnusedImportContext::Init) => {
-        } else {
+                format!(
-            format!("`{name}` imported but unused")
+                    "`{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<fn(&Self) -> String> {
-        let UnusedImport { ignore_init, .. } = self;
+        let UnusedImport { context, .. } = self;
-        if *ignore_init {
+        context
-            None
+            .is_none()
-        } else {
+            .then_some(|UnusedImport { name, multiple, .. }| {
-            Some(fmt_unused_import_autofix_msg)
+                if *multiple {
-        }
+                    "Remove unused import".to_string()
+                } else {
+                    format!("Remove unused import: `{name}`")
+                }
+            })
     }
 }
 #[violation]

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};

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: ~
 

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,
+    Binding, BindingId, BindingKind, Bindings, Exceptions, ExecutionContext, Scope, ScopeId,
-    ScopeStack, Scopes,
+    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
+    }
 }

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();

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> {