Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 11 additions & 2 deletions crates/ruff/resources/test/fixtures/pyflakes/F401_10.py
Original file line number Diff line number Diff line change
@@ -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
112 changes: 63 additions & 49 deletions crates/ruff/src/checkers/ast/mod.rs

Large diffs are not rendered by default.

56 changes: 31 additions & 25 deletions crates/ruff/src/rules/pyflakes/rules/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
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<fn(&Self) -> 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]
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff/src/rules/pyflakes/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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: ~

16 changes: 13 additions & 3 deletions crates/ruff_python_ast/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::visibility::{module_visibility, Modifier, VisibleScope};
Expand Down Expand Up @@ -308,6 +308,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
Expand All @@ -318,4 +319,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
}
}
8 changes: 0 additions & 8 deletions crates/ruff_python_ast/src/helpers.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use std::path::Path;

use bitflags::bitflags;
use itertools::Itertools;
use log::error;
use once_cell::sync::Lazy;
Expand Down Expand Up @@ -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();
Expand Down
11 changes: 11 additions & 0 deletions crates/ruff_python_ast/src/scope.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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>,
Expand All @@ -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> {
Expand Down