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

Add visit_format_spec to avoid false positives for F541 in f-string format specifier #1528

Merged
merged 3 commits into from
Jan 1, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions resources/test/fixtures/pyflakes/F541.py
Expand Up @@ -20,6 +20,7 @@

# OK
f"{v:0.2f}"
f"{f'{v:0.2f}'}"

# OK (false negatives)
f"{v:{f'0.2f'}}"
Expand Down
3 changes: 2 additions & 1 deletion resources/test/fixtures/pyflakes/F821_0.py
Expand Up @@ -89,7 +89,8 @@ def update_tomato():
f'B'
f'{B}'
)

C = f'{A:{B}}'
C = f'{A:{f"{B}"}}'
Comment on lines +92 to +93
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test cases to ensure we do visit child nodes in format_spec.


from typing import Annotated, Literal

Expand Down
5 changes: 4 additions & 1 deletion src/ast/visitor.rs
Expand Up @@ -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);
}
Comment on lines +41 to +43
Copy link
Contributor Author

@harupy harupy Jan 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a special method to separate normal JoinedStr and format-spec JoinedStr.

fn visit_arguments(&mut self, arguments: &'a Arguments) {
walk_arguments(self, arguments);
}
Expand Down Expand Up @@ -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 } => {
Expand Down
21 changes: 12 additions & 9 deletions src/checkers/ast.rs
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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 { .. }))
Expand Down Expand Up @@ -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),
}
Expand All @@ -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();
}
Expand Down Expand Up @@ -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);
Expand Down
18 changes: 18 additions & 0 deletions src/pyflakes/snapshots/ruff__pyflakes__tests__F541_F541.py.snap
Expand Up @@ -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: ~

32 changes: 26 additions & 6 deletions src/pyflakes/snapshots/ruff__pyflakes__tests__F821_F821_0.py.snap
Expand Up @@ -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: ~
Expand Down