-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New AST nodes for f-string elements #8835
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
Changes from all commits
cc395c9
698fa3c
95da9fb
bcdd7dd
50f4670
9cc8287
623d868
8bd17d2
2e29353
04d90d5
a36eebd
b6be853
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ def func(address): | |
| # Error | ||
| "0.0.0.0" | ||
| '0.0.0.0' | ||
| f"0.0.0.0" | ||
|
|
||
|
|
||
| # Error | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -815,8 +815,7 @@ where | |
|
|
||
| fn visit_expr(&mut self, expr: &'b Expr) { | ||
| // Step 0: Pre-processing | ||
| if !self.semantic.in_f_string() | ||
| && !self.semantic.in_typing_literal() | ||
| if !self.semantic.in_typing_literal() | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Smart. |
||
| && !self.semantic.in_deferred_type_definition() | ||
| && self.semantic.in_type_definition() | ||
| && self.semantic.future_annotations() | ||
|
|
@@ -1238,10 +1237,7 @@ where | |
| } | ||
| } | ||
| Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => { | ||
| if self.semantic.in_type_definition() | ||
| && !self.semantic.in_typing_literal() | ||
| && !self.semantic.in_f_string() | ||
| { | ||
| if self.semantic.in_type_definition() && !self.semantic.in_typing_literal() { | ||
| self.deferred.string_type_definitions.push(( | ||
| expr.range(), | ||
| value.to_str(), | ||
|
|
@@ -1326,17 +1322,6 @@ where | |
| self.semantic.flags = flags_snapshot; | ||
| } | ||
|
|
||
| fn visit_format_spec(&mut self, format_spec: &'b Expr) { | ||
| match format_spec { | ||
| Expr::FString(ast::ExprFString { value, .. }) => { | ||
| for expr in value.elements() { | ||
| self.visit_expr(expr); | ||
| } | ||
| } | ||
| _ => unreachable!("Unexpected expression for format_spec"), | ||
| } | ||
| } | ||
|
|
||
| fn visit_parameters(&mut self, parameters: &'b Parameters) { | ||
| // Step 1: Binding. | ||
| // Bind, but intentionally avoid walking default expressions, as we handle them | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what happens here if we have, like
func(f"/tmp" f"/bad"), and the rule is matching against"/tmp/bad"? I guess that would now be a false negative? Or how do we handle concatenations here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we already wouldn't flag that on
main, though I'm curious if it should be considered a bug. E.g., forS104, should we flagf"0.0" f".0.0"?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point and yes it will be a false negative now. It's not just
f"/tmp" f"/bad"but also"/tmp" f"/bad"(string and f-string)We can introduce a method which iterates over the concatenated literal parts of an f-string. For example,
The above code would return two strings:
"foobar "and" bazend". Here, I don't see a way to avoid string allocation here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm moving ahead with this limitation for now but we can revisit if it proves to be a problem. My hunch is this shouldn't be but you never know 🤷♂️