Add visit_format_spec to avoid false positives for F541 in f-string…

… format specifier (#1528)
astral-sh · Jan 1, 2023 · 509c6d5 · 509c6d5
1 parent 6695988
commit 509c6d5
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 18 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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: ~