diff --git a/resources/test/fixtures/pyflakes/F401_7.py b/resources/test/fixtures/pyflakes/F401_7.py new file mode 100644 index 0000000000..d2dddbbfed --- /dev/null +++ b/resources/test/fixtures/pyflakes/F401_7.py @@ -0,0 +1,16 @@ +"""Test: noqa directives.""" + +from module import ( + A, # noqa: F401 + B, +) + +from module import ( + A, # noqa: F401 + B, # noqa: F401 +) + +from module import ( + A, + B, +) diff --git a/src/check_ast.rs b/src/check_ast.rs index ed43ff2445..ead1790f23 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -25,6 +25,7 @@ use crate::ast::visitor::{walk_excepthandler, Visitor}; use crate::ast::{branch_detection, cast, helpers, operations, visitor}; use crate::checks::{Check, CheckCode, CheckKind, DeferralKeyword}; use crate::docstrings::definition::{Definition, DefinitionKind, Docstring, Documentable}; +use crate::noqa::Directive; use crate::python::builtins::{BUILTINS, MAGIC_GLOBALS}; use crate::python::future::ALL_FEATURE_NAMES; use crate::python::typing; @@ -38,8 +39,8 @@ use crate::{ docstrings, flake8_2020, flake8_annotations, flake8_bandit, flake8_blind_except, flake8_boolean_trap, flake8_bugbear, flake8_builtins, flake8_comprehensions, flake8_debugger, flake8_errmsg, flake8_import_conventions, flake8_print, flake8_return, flake8_simplify, - flake8_tidy_imports, flake8_unused_arguments, mccabe, pandas_vet, pep8_naming, pycodestyle, - pydocstyle, pyflakes, pygrep_hooks, pylint, pyupgrade, visibility, + flake8_tidy_imports, flake8_unused_arguments, mccabe, noqa, pandas_vet, pep8_naming, + pycodestyle, pydocstyle, pyflakes, pygrep_hooks, pylint, pyupgrade, visibility, }; const GLOBAL_SCOPE_INDEX: usize = 0; @@ -51,7 +52,9 @@ pub struct Checker<'a> { // Input data. path: &'a Path, autofix: bool, + ignore_noqa: bool, pub(crate) settings: &'a Settings, + pub(crate) noqa_line_for: &'a IntMap, pub(crate) locator: &'a SourceCodeLocator<'a>, // Computed checks. checks: Vec, @@ -98,13 +101,17 @@ pub struct Checker<'a> { impl<'a> Checker<'a> { pub fn new( settings: &'a Settings, + noqa_line_for: &'a IntMap, autofix: bool, + ignore_noqa: bool, path: &'a Path, locator: &'a SourceCodeLocator, ) -> Checker<'a> { Checker { settings, + noqa_line_for, autofix, + ignore_noqa, path, locator, checks: vec![], @@ -199,6 +206,30 @@ impl<'a> Checker<'a> { matches!(self.bindings[*index].kind, BindingKind::Builtin) }) } + + /// Return `true` if a `CheckCode` is disabled by a `noqa` directive. + pub fn is_ignored(&self, code: &CheckCode, lineno: usize) -> bool { + // TODO(charlie): `noqa` directives are mostly enforced in `check_lines.rs`. + // However, in rare cases, we need to check them here. For example, when + // removing unused imports, we create a single fix that's applied to all + // unused members on a single import. We need to pre-emptively omit any + // members from the fix that will eventually be excluded by a `noqa`. + // Unfortunately, we _do_ want to register a `Check` for each eventually-ignored + // import, so that our `noqa` counts are accurate. + if self.ignore_noqa { + return false; + } + let noqa_lineno = self.noqa_line_for.get(&lineno).unwrap_or(&lineno); + let line = self.locator.slice_source_code_range(&Range { + location: Location::new(*noqa_lineno, 0), + end_location: Location::new(noqa_lineno + 1, 0), + }); + match noqa::extract_noqa_directive(&line) { + Directive::None => false, + Directive::All(..) => true, + Directive::Codes(.., codes) => noqa::includes(code, &codes), + } + } } impl<'a, 'b> Visitor<'b> for Checker<'a> @@ -3396,6 +3427,8 @@ impl<'a> Checker<'a> { (&'a RefEquality<'b, Stmt>, Option<&'a RefEquality<'b, Stmt>>); let mut unused: FxHashMap> = FxHashMap::default(); + let mut ignored: FxHashMap> = + FxHashMap::default(); for (name, index) in scope .values @@ -3420,12 +3453,21 @@ impl<'a> Checker<'a> { let defined_by = binding.source.as_ref().unwrap(); let defined_in = self.child_to_parent.get(defined_by); - unused - .entry((defined_by, defined_in)) - .or_default() - .push((full_name, &binding.range)); + if self.is_ignored(&CheckCode::F401, binding.range.location.row()) { + ignored + .entry((defined_by, defined_in)) + .or_default() + .push((full_name, &binding.range)); + } else { + unused + .entry((defined_by, defined_in)) + .or_default() + .push((full_name, &binding.range)); + } } + let ignore_init = + self.settings.ignore_init_module_imports && self.path.ends_with("__init__.py"); for ((defined_by, defined_in), unused_imports) in unused .into_iter() .sorted_by_key(|((defined_by, _), _)| defined_by.0.location) @@ -3433,8 +3475,6 @@ impl<'a> Checker<'a> { let child = defined_by.0; let parent = defined_in.map(|defined_in| defined_in.0); - let ignore_init = self.settings.ignore_init_module_imports - && self.path.ends_with("__init__.py"); let fix = if !ignore_init && self.patch(&CheckCode::F401) { let deleted: Vec<&Stmt> = self.deletions.iter().map(|node| node.0).collect(); @@ -3471,6 +3511,17 @@ impl<'a> Checker<'a> { checks.push(check); } } + for (_, unused_imports) in ignored + .into_iter() + .sorted_by_key(|((defined_by, _), _)| defined_by.0.location) + { + for (full_name, range) in unused_imports { + checks.push(Check::new( + CheckKind::UnusedImport(full_name.clone(), ignore_init), + *range, + )); + } + } } } self.add_checks(checks.into_iter()); @@ -3705,11 +3756,13 @@ impl<'a> Checker<'a> { pub fn check_ast( python_ast: &Suite, locator: &SourceCodeLocator, + noqa_line_for: &IntMap, settings: &Settings, autofix: bool, + ignore_noqa: bool, path: &Path, ) -> Vec { - let mut checker = Checker::new(settings, autofix, path, locator); + let mut checker = Checker::new(settings, noqa_line_for, autofix, ignore_noqa, path, locator); checker.push_scope(Scope::new(ScopeKind::Module)); checker.bind_builtins(); diff --git a/src/linter.rs b/src/linter.rs index 6edfdcfd8e..639f0f339a 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -83,7 +83,15 @@ pub(crate) fn check_path( match rustpython_helpers::parse_program_tokens(tokens, "") { Ok(python_ast) => { if use_ast { - checks.extend(check_ast(&python_ast, locator, settings, autofix, path)); + checks.extend(check_ast( + &python_ast, + locator, + &directives.noqa_line_for, + settings, + autofix, + ignore_noqa, + path, + )); } if use_imports { checks.extend(check_imports( diff --git a/src/pycodestyle/plugins.rs b/src/pycodestyle/plugins.rs index b9e4b5d855..7aa98596b3 100644 --- a/src/pycodestyle/plugins.rs +++ b/src/pycodestyle/plugins.rs @@ -184,6 +184,8 @@ pub fn literal_comparisons( comparator = next; } + // TODO(charlie): Respect `noqa` directives. If one of the operators has a + // `noqa`, but another doesn't, both will be removed here. if !bad_ops.is_empty() { // Replace the entire comparison expression. let ops = ops diff --git a/src/pyflakes/fixes.rs b/src/pyflakes/fixes.rs index a3dd332e8c..edce435e79 100644 --- a/src/pyflakes/fixes.rs +++ b/src/pyflakes/fixes.rs @@ -1,7 +1,7 @@ use anyhow::{bail, Result}; use libcst_native::{ - Call, Codegen, CodegenState, Dict, DictElement, Expression, ImportNames, SmallStatement, - Statement, + Call, Codegen, CodegenState, Dict, DictElement, Expression, ImportNames, + ParenthesizableWhitespace, SmallStatement, Statement, }; use rustpython_ast::{Expr, Stmt}; @@ -60,8 +60,21 @@ pub fn remove_unused_imports( } } + // But avoid destroying any trailing comments. if let Some(alias) = aliases.last_mut() { - alias.comma = trailing_comma; + let has_comment = if let Some(comma) = &alias.comma { + match &comma.whitespace_after { + ParenthesizableWhitespace::SimpleWhitespace(_) => false, + ParenthesizableWhitespace::ParenthesizedWhitespace(whitespace) => { + whitespace.first_line.comment.is_some() + } + } + } else { + false + }; + if !has_comment { + alias.comma = trailing_comma; + } } if aliases.is_empty() { diff --git a/src/pyflakes/mod.rs b/src/pyflakes/mod.rs index 2a06775e8c..78aa713f79 100644 --- a/src/pyflakes/mod.rs +++ b/src/pyflakes/mod.rs @@ -28,6 +28,7 @@ mod tests { #[test_case(CheckCode::F401, Path::new("F401_4.py"); "F401_4")] #[test_case(CheckCode::F401, Path::new("F401_5.py"); "F401_5")] #[test_case(CheckCode::F401, Path::new("F401_6.py"); "F401_6")] + #[test_case(CheckCode::F401, Path::new("F401_7.py"); "F401_7")] #[test_case(CheckCode::F402, Path::new("F402.py"); "F402")] #[test_case(CheckCode::F403, Path::new("F403.py"); "F403")] #[test_case(CheckCode::F404, Path::new("F404.py"); "F404")] diff --git a/src/pyflakes/snapshots/ruff__pyflakes__tests__F401_F401_7.py.snap b/src/pyflakes/snapshots/ruff__pyflakes__tests__F401_F401_7.py.snap new file mode 100644 index 0000000000..e831e34c81 --- /dev/null +++ b/src/pyflakes/snapshots/ruff__pyflakes__tests__F401_F401_7.py.snap @@ -0,0 +1,59 @@ +--- +source: src/pyflakes/mod.rs +expression: checks +--- +- kind: + UnusedImport: + - module.B + - false + location: + row: 5 + column: 4 + end_location: + row: 5 + column: 5 + fix: + content: "from module import (\n A, # noqa: F401\n )" + location: + row: 3 + column: 0 + end_location: + row: 6 + column: 1 +- kind: + UnusedImport: + - module.A + - false + location: + row: 14 + column: 4 + end_location: + row: 14 + column: 5 + fix: + content: "" + location: + row: 13 + column: 0 + end_location: + row: 17 + column: 0 +- kind: + UnusedImport: + - module.B + - false + location: + row: 15 + column: 4 + end_location: + row: 15 + column: 5 + fix: + content: "" + location: + row: 13 + column: 0 + end_location: + row: 17 + column: 0 + diff --git a/src/pyupgrade/fixes.rs b/src/pyupgrade/fixes.rs index f945c84b0c..dab4fefacc 100644 --- a/src/pyupgrade/fixes.rs +++ b/src/pyupgrade/fixes.rs @@ -170,8 +170,21 @@ pub fn remove_unnecessary_future_import( aliases.remove(*index); } + // But avoid destroying any trailing comments. if let Some(alias) = aliases.last_mut() { - alias.comma = trailing_comma; + let has_comment = if let Some(comma) = &alias.comma { + match &comma.whitespace_after { + ParenthesizableWhitespace::SimpleWhitespace(_) => false, + ParenthesizableWhitespace::ParenthesizedWhitespace(whitespace) => { + whitespace.first_line.comment.is_some() + } + } + } else { + false + }; + if !has_comment { + alias.comma = trailing_comma; + } } if aliases.is_empty() {