diff --git a/README.md b/README.md index 3d64d36632..b9e2d92220 100644 --- a/README.md +++ b/README.md @@ -419,14 +419,14 @@ For more, see [Pyflakes](https://pypi.org/project/pyflakes/2.5.0/) on PyPI. | F501 | PercentFormatInvalidFormat | '...' % ... has invalid format string: ... | | | F502 | PercentFormatExpectedMapping | '...' % ... expected mapping but got sequence | | | F503 | PercentFormatExpectedSequence | '...' % ... expected sequence but got mapping | | -| F504 | PercentFormatExtraNamedArguments | '...' % ... has unused named argument(s): ... | | +| F504 | PercentFormatExtraNamedArguments | '...' % ... has unused named argument(s): ... | 🛠 | | F505 | PercentFormatMissingArgument | '...' % ... is missing argument(s) for placeholder(s): ... | | | F506 | PercentFormatMixedPositionalAndNamed | '...' % ... has mixed positional and named placeholders | | | F507 | PercentFormatPositionalCountMismatch | '...' % ... has 4 placeholder(s) but 2 substitution(s) | | | F508 | PercentFormatStarRequiresSequence | '...' % ... `*` specifier requires sequence | | | F509 | PercentFormatUnsupportedFormatCharacter | '...' % ... has unsupported format character 'c' | | | F521 | StringDotFormatInvalidFormat | '...'.format(...) has invalid format string: ... | | -| F522 | StringDotFormatExtraNamedArguments | '...'.format(...) has unused named argument(s): ... | | +| F522 | StringDotFormatExtraNamedArguments | '...'.format(...) has unused named argument(s): ... | 🛠 | | F523 | StringDotFormatExtraPositionalArguments | '...'.format(...) has unused arguments at position(s): ... | | | F524 | StringDotFormatMissingArguments | '...'.format(...) is missing argument(s) for placeholder(s): ... | | | F525 | StringDotFormatMixingAutomatic | '...'.format(...) mixes automatic and manual numbering | | @@ -816,6 +816,7 @@ For more, see [Pylint](https://pypi.org/project/pylint/2.15.7/) on PyPI. | PLC0414 | UselessImportAlias | Import alias does not rename original package | 🛠 | | PLC2201 | MisplacedComparisonConstant | Comparison should be ... | 🛠 | | PLC3002 | UnnecessaryDirectLambdaCall | Lambda expression called directly. Execute the expression inline instead. | | +| PLE0117 | NonlocalWithoutBinding | Nonlocal name `...` found without binding | | | PLE0118 | UsedPriorGlobalDeclaration | Name `...` is used prior to global declaration on line 1 | | | PLE1142 | AwaitOutsideAsync | `await` should be used within an async function | | | PLR0206 | PropertyWithParameters | Cannot have defined parameters for properties | | @@ -823,6 +824,7 @@ For more, see [Pylint](https://pypi.org/project/pylint/2.15.7/) on PyPI. | PLR1701 | ConsiderMergingIsinstance | Merge these isinstance calls: `isinstance(..., (...))` | | | PLR1722 | UseSysExit | Use `sys.exit()` instead of `exit` | 🛠 | | PLW0120 | UselessElseOnLoop | Else clause on loop without a break statement, remove the else and de-indent all the code inside it | | +| PLW0602 | GlobalVariableNotAssigned | Using global for `...` but no assignment is done | | ### Ruff-specific rules (RUF) diff --git a/resources/test/fixtures/pyflakes/F504.py b/resources/test/fixtures/pyflakes/F504.py index f3b0dc08a3..6d33309eaf 100644 --- a/resources/test/fixtures/pyflakes/F504.py +++ b/resources/test/fixtures/pyflakes/F504.py @@ -4,3 +4,6 @@ a = "wrong" hidden = {"a": "!"} "%(a)s %(c)s" % {"x": 1, **hidden} # Ok (cannot see through splat) + +"%(a)s" % {"a": 1, r"b": "!"} # F504 ("b" not used) +"%(a)s" % {'a': 1, u"b": "!"} # F504 ("b" not used) diff --git a/src/check_ast.rs b/src/check_ast.rs index 930016c618..bc6c1c7151 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -1481,21 +1481,16 @@ where } Ok(summary) => { if self.settings.enabled.contains(&CheckCode::F522) { - if let Some(check) = pyflakes::checks::string_dot_format_extra_named_arguments( + pyflakes::checks::string_dot_format_extra_named_arguments(self, &summary, keywords, location, - ) - { - self.add_check(check); - } + ); } if self.settings.enabled.contains(&CheckCode::F523) { - if let Some(check) = pyflakes::checks::string_dot_format_extra_positional_arguments( + pyflakes::checks::string_dot_format_extra_positional_arguments( + self, &summary, args, location, - ) - { - self.add_check(check); - } + ); } if self.settings.enabled.contains(&CheckCode::F524) { @@ -1984,13 +1979,9 @@ where } } if self.settings.enabled.contains(&CheckCode::F504) { - if let Some(check) = - pyflakes::checks::percent_format_extra_named_arguments( - &summary, right, location, - ) - { - self.add_check(check); - } + pyflakes::checks::percent_format_extra_named_arguments( + self, &summary, right, location, + ); } if self.settings.enabled.contains(&CheckCode::F505) { if let Some(check) = diff --git a/src/checks.rs b/src/checks.rs index cb4bd016f8..b879f71bb1 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -2675,6 +2675,7 @@ impl CheckKind { | CheckKind::NotIsTest | CheckKind::OneBlankLineAfterClass(..) | CheckKind::OneBlankLineBeforeClass(..) + | CheckKind::PercentFormatExtraNamedArguments(..) | CheckKind::PEP3120UnnecessaryCodingComment | CheckKind::PPrintFound | CheckKind::PrintFound @@ -2687,6 +2688,7 @@ impl CheckKind { | CheckKind::SectionUnderlineMatchesSectionLength(..) | CheckKind::SectionUnderlineNotOverIndented(..) | CheckKind::SetAttrWithConstant + | CheckKind::StringDotFormatExtraNamedArguments(..) | CheckKind::SuperCallWithParameters | CheckKind::TrueFalseComparison(..) | CheckKind::TypeOfPrimitive(..) diff --git a/src/pyflakes/checks.rs b/src/pyflakes/checks.rs index 4d0e40b01d..a586c5bd64 100644 --- a/src/pyflakes/checks.rs +++ b/src/pyflakes/checks.rs @@ -8,8 +8,12 @@ use rustpython_parser::ast::{ }; use crate::ast::types::{Binding, BindingKind, Range, Scope, ScopeKind}; +use crate::check_ast::Checker; use crate::checks::{Check, CheckKind}; use crate::pyflakes::cformat::CFormatSummary; +use crate::pyflakes::fixes::{ + remove_unused_format_arguments_from_dict, remove_unused_keyword_arguments_from_format_call, +}; use crate::pyflakes::format::FormatSummary; fn has_star_star_kwargs(keywords: &[Keyword]) -> bool { @@ -21,7 +25,7 @@ fn has_star_star_kwargs(keywords: &[Keyword]) -> bool { fn has_star_args(args: &[Expr]) -> bool { args.iter() - .any(|a| matches!(&a.node, ExprKind::Starred { .. })) + .any(|arg| matches!(&arg.node, ExprKind::Starred { .. })) } /// F502 @@ -70,21 +74,22 @@ pub(crate) fn percent_format_expected_sequence( /// F504 pub(crate) fn percent_format_extra_named_arguments( + checker: &mut Checker, summary: &CFormatSummary, right: &Expr, location: Range, -) -> Option { +) { if summary.num_positional > 0 { - return None; + return; } let ExprKind::Dict { keys, values } = &right.node else { - return None; + return; }; if values.len() > keys.len() { - return None; // contains **x splat + return; // contains **x splat } - let missing: Vec<&String> = keys + let missing: Vec<&str> = keys .iter() .filter_map(|k| match &k.node { // We can only check that string literals exist @@ -95,7 +100,7 @@ pub(crate) fn percent_format_extra_named_arguments( if summary.keywords.contains(value) { None } else { - Some(value) + Some(value.as_str()) } } _ => None, @@ -103,13 +108,22 @@ pub(crate) fn percent_format_extra_named_arguments( .collect(); if missing.is_empty() { - return None; + return; } - Some(Check::new( - CheckKind::PercentFormatExtraNamedArguments(missing.iter().map(|&s| s.clone()).collect()), + let mut check = Check::new( + CheckKind::PercentFormatExtraNamedArguments( + missing.iter().map(|&arg| arg.to_string()).collect(), + ), location, - )) + ); + if checker.patch(check.kind.code()) { + if let Ok(fix) = remove_unused_format_arguments_from_dict(checker.locator, &missing, right) + { + check.amend(fix); + } + } + checker.add_check(check); } /// F505 @@ -232,12 +246,13 @@ pub(crate) fn percent_format_star_requires_sequence( /// F522 pub(crate) fn string_dot_format_extra_named_arguments( + checker: &mut Checker, summary: &FormatSummary, keywords: &[Keyword], location: Range, -) -> Option { +) { if has_star_star_kwargs(keywords) { - return None; + return; } let keywords = keywords.iter().filter_map(|k| { @@ -245,45 +260,64 @@ pub(crate) fn string_dot_format_extra_named_arguments( arg.as_ref() }); - let missing: Vec<&String> = keywords - .filter(|&k| !summary.keywords.contains(k)) + let missing: Vec<&str> = keywords + .filter_map(|arg| { + if summary.keywords.contains(arg) { + None + } else { + Some(arg.as_str()) + } + }) .collect(); if missing.is_empty() { - None - } else { - Some(Check::new( - CheckKind::StringDotFormatExtraNamedArguments( - missing.iter().map(|&s| s.clone()).collect(), - ), - location, - )) + return; } + + let mut check = Check::new( + CheckKind::StringDotFormatExtraNamedArguments( + missing.iter().map(|&arg| arg.to_string()).collect(), + ), + location, + ); + if checker.patch(check.kind.code()) { + if let Ok(fix) = + remove_unused_keyword_arguments_from_format_call(checker.locator, &missing, location) + { + check.amend(fix); + } + } + checker.add_check(check); } /// F523 pub(crate) fn string_dot_format_extra_positional_arguments( + checker: &mut Checker, summary: &FormatSummary, args: &[Expr], location: Range, -) -> Option { +) { if has_star_args(args) { - return None; + return; } - let missing: Vec = (0..args.len()) + let missing: Vec = (0..args.len()) .filter(|i| !(summary.autos.contains(i) || summary.indexes.contains(i))) - .map(|i| i.to_string()) .collect(); if missing.is_empty() { - None - } else { - Some(Check::new( - CheckKind::StringDotFormatExtraPositionalArguments(missing), - location, - )) + return; } + + checker.add_check(Check::new( + CheckKind::StringDotFormatExtraPositionalArguments( + missing + .iter() + .map(std::string::ToString::to_string) + .collect::>(), + ), + location, + )); } /// F524 diff --git a/src/pyflakes/fixes.rs b/src/pyflakes/fixes.rs index 74acb6a730..7bcddeb8e2 100644 --- a/src/pyflakes/fixes.rs +++ b/src/pyflakes/fixes.rs @@ -1,14 +1,18 @@ use anyhow::{bail, Result}; -use libcst_native::{Codegen, CodegenState, ImportNames, SmallStatement, Statement}; -use rustpython_ast::Stmt; +use libcst_native::{ + Call, Codegen, CodegenState, Dict, DictElement, Expression, ImportNames, SmallStatement, + Statement, +}; +use rustpython_ast::{Expr, Stmt}; use crate::ast::types::Range; use crate::autofix::{helpers, Fix}; use crate::cst::helpers::compose_module_path; -use crate::cst::matchers::match_module; +use crate::cst::matchers::{match_expr, match_module}; +use crate::python::string::strip_quotes_and_prefixes; use crate::source_code_locator::SourceCodeLocator; -/// Generate a Fix to remove any unused imports from an `import` statement. +/// Generate a `Fix` to remove any unused imports from an `import` statement. pub fn remove_unused_imports( locator: &SourceCodeLocator, unused_imports: &Vec<(&String, &Range)>, @@ -73,3 +77,93 @@ pub fn remove_unused_imports( )) } } + +/// Generate a `Fix` to remove unused keys from format dict. +pub fn remove_unused_format_arguments_from_dict( + locator: &SourceCodeLocator, + unused_arguments: &[&str], + stmt: &Expr, +) -> Result { + let module_text = locator.slice_source_code_range(&Range::from_located(stmt)); + let mut tree = match_module(&module_text)?; + let mut body = match_expr(&mut tree)?; + + let new_dict = { + let Expression::Dict(dict) = &body.value else { + bail!("Expected Expression::Dict") + }; + + Dict { + lbrace: dict.lbrace.clone(), + lpar: dict.lpar.clone(), + rbrace: dict.rbrace.clone(), + rpar: dict.rpar.clone(), + elements: dict + .elements + .iter() + .filter_map(|e| match e { + DictElement::Simple { + key: Expression::SimpleString(name), + .. + } if unused_arguments.contains(&strip_quotes_and_prefixes(name.value)) => None, + e => Some(e.clone()), + }) + .collect(), + } + }; + + body.value = Expression::Dict(Box::new(new_dict)); + + let mut state = CodegenState::default(); + tree.codegen(&mut state); + + Ok(Fix::replacement( + state.to_string(), + stmt.location, + stmt.end_location.unwrap(), + )) +} + +/// Generate a `Fix` to remove unused keyword arguments from format call. +pub fn remove_unused_keyword_arguments_from_format_call( + locator: &SourceCodeLocator, + unused_arguments: &[&str], + location: Range, +) -> Result { + let module_text = locator.slice_source_code_range(&location); + let mut tree = match_module(&module_text)?; + let mut body = match_expr(&mut tree)?; + + let new_call = { + let Expression::Call(call) = &body.value else { + bail!("Expected Expression::Call") + }; + + Call { + func: call.func.clone(), + lpar: call.lpar.clone(), + rpar: call.rpar.clone(), + whitespace_before_args: call.whitespace_before_args.clone(), + whitespace_after_func: call.whitespace_after_func.clone(), + args: call + .args + .iter() + .filter_map(|e| match &e.keyword { + Some(kw) if unused_arguments.contains(&kw.value) => None, + _ => Some(e.clone()), + }) + .collect(), + } + }; + + body.value = Expression::Call(Box::new(new_call)); + + let mut state = CodegenState::default(); + tree.codegen(&mut state); + + Ok(Fix::replacement( + state.to_string(), + location.location, + location.end_location, + )) +} diff --git a/src/pyflakes/snapshots/ruff__pyflakes__tests__F504_F504.py.snap b/src/pyflakes/snapshots/ruff__pyflakes__tests__F504_F504.py.snap index 63f3aec3e1..0b81ae4f17 100644 --- a/src/pyflakes/snapshots/ruff__pyflakes__tests__F504_F504.py.snap +++ b/src/pyflakes/snapshots/ruff__pyflakes__tests__F504_F504.py.snap @@ -11,5 +11,46 @@ expression: checks end_location: row: 3 column: 34 - fix: ~ + fix: + content: "{a: \"?\", }" + location: + row: 3 + column: 16 + end_location: + row: 3 + column: 34 +- kind: + PercentFormatExtraNamedArguments: + - b + location: + row: 8 + column: 8 + end_location: + row: 8 + column: 29 + fix: + content: "{\"a\": 1, }" + location: + row: 8 + column: 10 + end_location: + row: 8 + column: 29 +- kind: + PercentFormatExtraNamedArguments: + - b + location: + row: 9 + column: 8 + end_location: + row: 9 + column: 29 + fix: + content: "{'a': 1, }" + location: + row: 9 + column: 10 + end_location: + row: 9 + column: 29 diff --git a/src/pyflakes/snapshots/ruff__pyflakes__tests__F504_F50x.py.snap b/src/pyflakes/snapshots/ruff__pyflakes__tests__F504_F50x.py.snap index f73adfafde..c001f73f46 100644 --- a/src/pyflakes/snapshots/ruff__pyflakes__tests__F504_F50x.py.snap +++ b/src/pyflakes/snapshots/ruff__pyflakes__tests__F504_F50x.py.snap @@ -11,5 +11,12 @@ expression: checks end_location: row: 8 column: 32 - fix: ~ + fix: + content: "{'bar': 1, }" + location: + row: 8 + column: 12 + end_location: + row: 8 + column: 32 diff --git a/src/pyflakes/snapshots/ruff__pyflakes__tests__F522_F522.py.snap b/src/pyflakes/snapshots/ruff__pyflakes__tests__F522_F522.py.snap index efc9f38b1f..a8ac1d400b 100644 --- a/src/pyflakes/snapshots/ruff__pyflakes__tests__F522_F522.py.snap +++ b/src/pyflakes/snapshots/ruff__pyflakes__tests__F522_F522.py.snap @@ -11,7 +11,14 @@ expression: checks end_location: row: 1 column: 21 - fix: ~ + fix: + content: "\"{}\".format(1, )" + location: + row: 1 + column: 0 + end_location: + row: 1 + column: 21 - kind: StringDotFormatExtraNamedArguments: - spam @@ -21,7 +28,14 @@ expression: checks end_location: row: 2 column: 34 - fix: ~ + fix: + content: "\"{bar}{}\".format(1, bar=2, )" + location: + row: 2 + column: 0 + end_location: + row: 2 + column: 34 - kind: StringDotFormatExtraNamedArguments: - eggs @@ -32,5 +46,12 @@ expression: checks end_location: row: 4 column: 51 - fix: ~ + fix: + content: "\"{bar:{spam}}\".format(bar=2, spam=3, )" + location: + row: 4 + column: 0 + end_location: + row: 4 + column: 51 diff --git a/src/python/string.rs b/src/python/string.rs index e508102e26..180433e31d 100644 --- a/src/python/string.rs +++ b/src/python/string.rs @@ -1,3 +1,9 @@ +use once_cell::sync::Lazy; +use regex::Regex; + +pub static STRING_QUOTE_PREFIX_REGEX: Lazy = + Lazy::new(|| Regex::new(r#"^(?i)[urb]*['"](?P.*)['"]$"#).unwrap()); + pub fn is_lower(s: &str) -> bool { let mut cased = false; for c in s.chars() { @@ -22,9 +28,22 @@ pub fn is_upper(s: &str) -> bool { cased } +/// Remove prefixes (u, r, b) and quotes around a string. This expects the given +/// string to be a valid Python string representation, it doesn't do any +/// validation. +pub fn strip_quotes_and_prefixes(s: &str) -> &str { + match STRING_QUOTE_PREFIX_REGEX.captures(s) { + Some(caps) => match caps.name("raw") { + Some(m) => m.as_str(), + None => s, + }, + None => s, + } +} + #[cfg(test)] mod tests { - use crate::python::string::{is_lower, is_upper}; + use crate::python::string::{is_lower, is_upper, strip_quotes_and_prefixes}; #[test] fn test_is_lower() { @@ -47,4 +66,12 @@ mod tests { assert!(!is_upper("")); assert!(!is_upper("_")); } + + #[test] + fn test_strip_quotes_and_prefixes() { + assert_eq!(strip_quotes_and_prefixes(r#"'a'"#), "a"); + assert_eq!(strip_quotes_and_prefixes(r#"bur'a'"#), "a"); + assert_eq!(strip_quotes_and_prefixes(r#"UrB'a'"#), "a"); + assert_eq!(strip_quotes_and_prefixes(r#""a""#), "a"); + } }