From 509c6d5ec789d73346a0088d398892d267e2cdfc Mon Sep 17 00:00:00 2001 From: Harutaka Kawamura Date: Mon, 2 Jan 2023 03:03:32 +0900 Subject: [PATCH] Add `visit_format_spec` to avoid false positives for F541 in f-string format specifier (#1528) --- resources/test/fixtures/pyflakes/F541.py | 3 +- resources/test/fixtures/pyflakes/F821_0.py | 3 +- src/ast/visitor.rs | 5 ++- src/checkers/ast.rs | 21 ++++++------ .../ruff__pyflakes__tests__F541_F541.py.snap | 18 +++++++++++ ...ruff__pyflakes__tests__F821_F821_0.py.snap | 32 +++++++++++++++---- 6 files changed, 64 insertions(+), 18 deletions(-) diff --git a/resources/test/fixtures/pyflakes/F541.py b/resources/test/fixtures/pyflakes/F541.py index 977c7fe6361b4..ac6a20baa0128 100644 --- a/resources/test/fixtures/pyflakes/F541.py +++ b/resources/test/fixtures/pyflakes/F541.py @@ -20,7 +20,8 @@ # OK f"{v:0.2f}" +f"{f'{v:0.2f}'}" -# OK (false negatives) +# Errors f"{v:{f'0.2f'}}" f"{f''}" diff --git a/resources/test/fixtures/pyflakes/F821_0.py b/resources/test/fixtures/pyflakes/F821_0.py index bfc2b36d926f7..03b55606388c4 100644 --- a/resources/test/fixtures/pyflakes/F821_0.py +++ b/resources/test/fixtures/pyflakes/F821_0.py @@ -89,7 +89,8 @@ def update_tomato(): f'B' f'{B}' ) - +C = f'{A:{B}}' +C = f'{A:{f"{B}"}}' from typing import Annotated, Literal diff --git a/src/ast/visitor.rs b/src/ast/visitor.rs index f531bbab569e7..c6e91018e3fcb 100644 --- a/src/ast/visitor.rs +++ b/src/ast/visitor.rs @@ -38,6 +38,9 @@ pub trait Visitor<'a> { fn visit_excepthandler(&mut self, excepthandler: &'a Excepthandler) { walk_excepthandler(self, excepthandler); } + fn visit_format_spec(&mut self, format_spec: &'a Expr) { + walk_expr(self, format_spec); + } fn visit_arguments(&mut self, arguments: &'a Arguments) { walk_arguments(self, arguments); } @@ -387,7 +390,7 @@ pub fn walk_expr<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, expr: &'a Expr) { } => { visitor.visit_expr(value); if let Some(expr) = format_spec { - visitor.visit_expr(expr); + visitor.visit_format_spec(expr); } } ExprKind::JoinedStr { values } => { diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index fb5e1df50bec7..567268219519d 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -91,7 +91,6 @@ pub struct Checker<'a> { in_type_definition: bool, in_deferred_string_type_definition: bool, in_deferred_type_definition: bool, - in_f_string: bool, in_literal: bool, in_subscript: bool, seen_import_boundary: bool, @@ -148,7 +147,6 @@ impl<'a> Checker<'a> { in_type_definition: false, in_deferred_string_type_definition: false, in_deferred_type_definition: false, - in_f_string: false, in_literal: false, in_subscript: false, seen_import_boundary: false, @@ -1486,7 +1484,6 @@ where self.push_expr(expr); - let prev_in_f_string = self.in_f_string; let prev_in_literal = self.in_literal; let prev_in_type_definition = self.in_type_definition; @@ -2176,9 +2173,7 @@ where } } ExprKind::JoinedStr { values } => { - // Conversion flags are parsed as f-strings without placeholders, so skip - // nested f-strings, which would lead to false positives. - if !self.in_f_string && self.settings.enabled.contains(&CheckCode::F541) { + if self.settings.enabled.contains(&CheckCode::F541) { if !values .iter() .any(|value| matches!(value.node, ExprKind::FormattedValue { .. })) @@ -2682,9 +2677,7 @@ where self.in_subscript = prev_in_subscript; } ExprKind::JoinedStr { .. } => { - self.in_f_string = true; visitor::walk_expr(self, expr); - self.in_f_string = prev_in_f_string; } _ => visitor::walk_expr(self, expr), } @@ -2703,7 +2696,6 @@ where self.in_type_definition = prev_in_type_definition; self.in_literal = prev_in_literal; - self.in_f_string = prev_in_f_string; self.pop_expr(); } @@ -2807,6 +2799,17 @@ where } } + fn visit_format_spec(&mut self, format_spec: &'b Expr) { + match &format_spec.node { + ExprKind::JoinedStr { values } => { + for value in values { + self.visit_expr(value); + } + } + _ => unreachable!("Unexpected expression for format_spec"), + } + } + fn visit_arguments(&mut self, arguments: &'b Arguments) { if self.settings.enabled.contains(&CheckCode::B006) { flake8_bugbear::plugins::mutable_argument_default(self, arguments); diff --git a/src/pyflakes/snapshots/ruff__pyflakes__tests__F541_F541.py.snap b/src/pyflakes/snapshots/ruff__pyflakes__tests__F541_F541.py.snap index 6331262ce558d..a1c01ffdffd45 100644 --- a/src/pyflakes/snapshots/ruff__pyflakes__tests__F541_F541.py.snap +++ b/src/pyflakes/snapshots/ruff__pyflakes__tests__F541_F541.py.snap @@ -38,4 +38,22 @@ expression: checks column: 16 fix: ~ parent: ~ +- kind: FStringMissingPlaceholders + location: + row: 26 + column: 6 + end_location: + row: 26 + column: 13 + fix: ~ + parent: ~ +- kind: FStringMissingPlaceholders + location: + row: 27 + column: 3 + end_location: + row: 27 + column: 6 + fix: ~ + parent: ~ diff --git a/src/pyflakes/snapshots/ruff__pyflakes__tests__F821_F821_0.py.snap b/src/pyflakes/snapshots/ruff__pyflakes__tests__F821_F821_0.py.snap index 5695b62d4c2d9..f4ecce6bb0cf2 100644 --- a/src/pyflakes/snapshots/ruff__pyflakes__tests__F821_F821_0.py.snap +++ b/src/pyflakes/snapshots/ruff__pyflakes__tests__F821_F821_0.py.snap @@ -82,33 +82,53 @@ expression: checks column: 8 fix: ~ parent: ~ +- kind: + UndefinedName: B + location: + row: 92 + column: 10 + end_location: + row: 92 + column: 11 + fix: ~ + parent: ~ +- kind: + UndefinedName: B + location: + row: 93 + column: 13 + end_location: + row: 93 + column: 14 + fix: ~ + parent: ~ - kind: UndefinedName: PEP593Test123 location: - row: 114 + row: 115 column: 8 end_location: - row: 114 + row: 115 column: 23 fix: ~ parent: ~ - kind: UndefinedName: foo location: - row: 122 + row: 123 column: 13 end_location: - row: 122 + row: 123 column: 18 fix: ~ parent: ~ - kind: UndefinedName: bar location: - row: 122 + row: 123 column: 20 end_location: - row: 122 + row: 123 column: 25 fix: ~ parent: ~