Skip to content

Commit 5f40371

Browse files
dhruvmaniladhruvmanila
authored
Use ExprFString for StringLike::FString variant (#10311)
## Summary This PR updates the `StringLike::FString` variant to use `ExprFString` instead of `FStringLiteralElement`. For context, the reason it used `FStringLiteralElement` is that the node is actually the string part of an f-string ("foo" in `f"foo{x}"`). But, this is inconsistent with other variants where the captured value is the _entire_ string. This is also problematic w.r.t. implicitly concatenated strings. Any rules which work with `StringLike::FString` doesn't account for the string part in an implicitly concatenated f-strings. For example, we don't flag confusable character in the first part of `"𝐁ad" f"𝐁ad string"`, but only the second part (https://play.ruff.rs/16071c4c-a1dd-4920-b56f-e2ce2f69c843). ### Update `PYI053` _This is included in this PR because otherwise it requires a temporary workaround to be compatible with the old logic._ This PR also updates the `PYI053` (`string-or-bytes-too-long`) rule for f-string to consider _all_ the visible characters in a f-string, including the ones which are implicitly concatenated. This is consistent with implicitly concatenated strings and bytes. For example, ```python def foo( # We count all the characters here arg1: str = '51 character ' 'stringgggggggggggggggggggggggggggggggg', # But not here because of the `{x}` replacement field which _breaks_ them up into two chunks arg2: str = f'51 character {x} stringgggggggggggggggggggggggggggggggggggggggggggg', ) -> None: ... ``` This PR fixes it to consider all _visible_ characters inside an f-string which includes expressions as well. fixes: #10310 fixes: #10307 ## Test Plan Add new test cases and update the snapshots. ## Review To facilitate the review process, the change have been split into two commits: one which has the code change while the other has the test cases and updated snapshots.
1 parent f7802ad commit 5f40371

16 files changed

+251
-63
lines changed

‎crates/ruff_linter/resources/test/fixtures/flake8_bandit/S104.py

+4
Original file line numberDiff line numberDiff line change
@@ -18,3 +18,7 @@ def func(address):
1818
def my_func():
1919
x = "0.0.0.0"
2020
print(x)
21+
22+
23+
# Implicit string concatenation
24+
"0.0.0.0" f"0.0.0.0{expr}0.0.0.0"

‎crates/ruff_linter/resources/test/fixtures/flake8_bandit/S108.py

+7
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,13 @@
1818
with open("/foo/bar", "w") as f:
1919
f.write("def")
2020

21+
# Implicit string concatenation
22+
with open("/tmp/" "abc", "w") as f:
23+
f.write("def")
24+
25+
with open("/tmp/abc" f"/tmp/abc", "w") as f:
26+
f.write("def")
27+
2128
# Using `tempfile` module should be ok
2229
import tempfile
2330
from tempfile import TemporaryDirectory

‎crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI053.pyi

+2
Original file line numberDiff line numberDiff line change
@@ -64,3 +64,5 @@ def not_warnings_dot_deprecated(
6464
"Not warnings.deprecated, so this one *should* lead to PYI053 in a stub!" # Error: PYI053
6565
)
6666
def not_a_deprecated_function() -> None: ...
67+
68+
fbaz: str = f"51 character {foo} stringgggggggggggggggggggggggggg" # Error: PYI053

‎crates/ruff_linter/resources/test/fixtures/ruff/confusables.py

+3
Original file line numberDiff line numberDiff line change
@@ -53,3 +53,6 @@ class Labware:
5353

5454

5555
assert getattr(Labware(), "µL") == 1.5
56+
57+
# Implicit string concatenation
58+
x = "𝐁ad" f"𝐁ad string"

‎crates/ruff_linter/src/checkers/ast/mod.rs

+2-11
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ use ruff_python_ast::helpers::{
4545
use ruff_python_ast::identifier::Identifier;
4646
use ruff_python_ast::name::QualifiedName;
4747
use ruff_python_ast::str::Quote;
48-
use ruff_python_ast::visitor::{walk_except_handler, walk_f_string_element, walk_pattern, Visitor};
48+
use ruff_python_ast::visitor::{walk_except_handler, walk_pattern, Visitor};
4949
use ruff_python_ast::{helpers, str, visitor, PySourceType};
5050
use ruff_python_codegen::{Generator, Stylist};
5151
use ruff_python_index::Indexer;
@@ -1407,6 +1407,7 @@ impl<'a> Visitor<'a> for Checker<'a> {
14071407
analyze::string_like(string_literal.into(), self);
14081408
}
14091409
Expr::BytesLiteral(bytes_literal) => analyze::string_like(bytes_literal.into(), self),
1410+
Expr::FString(f_string) => analyze::string_like(f_string.into(), self),
14101411
_ => {}
14111412
}
14121413

@@ -1573,16 +1574,6 @@ impl<'a> Visitor<'a> for Checker<'a> {
15731574
.push((bound, self.semantic.snapshot()));
15741575
}
15751576
}
1576-
1577-
fn visit_f_string_element(&mut self, f_string_element: &'a ast::FStringElement) {
1578-
// Step 2: Traversal
1579-
walk_f_string_element(self, f_string_element);
1580-
1581-
// Step 4: Analysis
1582-
if let Some(literal) = f_string_element.as_literal() {
1583-
analyze::string_like(literal.into(), self);
1584-
}
1585-
}
15861577
}
15871578

15881579
impl<'a> Checker<'a> {

‎crates/ruff_linter/src/rules/flake8_bandit/rules/hardcoded_bind_all_interfaces.rs

+31-11
Original file line numberDiff line numberDiff line change
@@ -38,17 +38,37 @@ impl Violation for HardcodedBindAllInterfaces {
3838

3939
/// S104
4040
pub(crate) fn hardcoded_bind_all_interfaces(checker: &mut Checker, string: StringLike) {
41-
let is_bind_all_interface = match string {
42-
StringLike::StringLiteral(ast::ExprStringLiteral { value, .. }) => value == "0.0.0.0",
43-
StringLike::FStringLiteral(ast::FStringLiteralElement { value, .. }) => {
44-
&**value == "0.0.0.0"
41+
match string {
42+
StringLike::String(ast::ExprStringLiteral { value, .. }) => {
43+
if value == "0.0.0.0" {
44+
checker
45+
.diagnostics
46+
.push(Diagnostic::new(HardcodedBindAllInterfaces, string.range()));
47+
}
4548
}
46-
StringLike::BytesLiteral(_) => return,
49+
StringLike::FString(ast::ExprFString { value, .. }) => {
50+
for part in value {
51+
match part {
52+
ast::FStringPart::Literal(literal) => {
53+
if &**literal == "0.0.0.0" {
54+
checker
55+
.diagnostics
56+
.push(Diagnostic::new(HardcodedBindAllInterfaces, literal.range()));
57+
}
58+
}
59+
ast::FStringPart::FString(f_string) => {
60+
for literal in f_string.literals() {
61+
if &**literal == "0.0.0.0" {
62+
checker.diagnostics.push(Diagnostic::new(
63+
HardcodedBindAllInterfaces,
64+
literal.range(),
65+
));
66+
}
67+
}
68+
}
69+
}
70+
}
71+
}
72+
StringLike::Bytes(_) => (),
4773
};
48-
49-
if is_bind_all_interface {
50-
checker
51-
.diagnostics
52-
.push(Diagnostic::new(HardcodedBindAllInterfaces, string.range()));
53-
}
5474
}

‎crates/ruff_linter/src/rules/flake8_bandit/rules/hardcoded_tmp_directory.rs

+24-7
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use ruff_python_ast::{self as ast, Expr, StringLike};
2-
use ruff_text_size::Ranged;
2+
use ruff_text_size::{Ranged, TextRange};
33

44
use ruff_diagnostics::{Diagnostic, Violation};
55
use ruff_macros::{derive_message_formats, violation};
@@ -53,12 +53,29 @@ impl Violation for HardcodedTempFile {
5353

5454
/// S108
5555
pub(crate) fn hardcoded_tmp_directory(checker: &mut Checker, string: StringLike) {
56-
let value = match string {
57-
StringLike::StringLiteral(ast::ExprStringLiteral { value, .. }) => value.to_str(),
58-
StringLike::FStringLiteral(ast::FStringLiteralElement { value, .. }) => value,
59-
StringLike::BytesLiteral(_) => return,
60-
};
56+
match string {
57+
StringLike::String(ast::ExprStringLiteral { value, .. }) => {
58+
check(checker, value.to_str(), string.range());
59+
}
60+
StringLike::FString(ast::ExprFString { value, .. }) => {
61+
for part in value {
62+
match part {
63+
ast::FStringPart::Literal(literal) => {
64+
check(checker, literal, literal.range());
65+
}
66+
ast::FStringPart::FString(f_string) => {
67+
for literal in f_string.literals() {
68+
check(checker, literal, literal.range());
69+
}
70+
}
71+
}
72+
}
73+
}
74+
StringLike::Bytes(_) => (),
75+
}
76+
}
6177

78+
fn check(checker: &mut Checker, value: &str, range: TextRange) {
6279
if !checker
6380
.settings
6481
.flake8_bandit
@@ -85,6 +102,6 @@ pub(crate) fn hardcoded_tmp_directory(checker: &mut Checker, string: StringLike)
85102
HardcodedTempFile {
86103
string: value.to_string(),
87104
},
88-
string.range(),
105+
range,
89106
));
90107
}

‎crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S104_S104.py.snap

+19
Original file line numberDiff line numberDiff line change
@@ -42,4 +42,23 @@ S104.py:19:9: S104 Possible binding to all interfaces
4242
20 | print(x)
4343
|
4444

45+
S104.py:24:1: S104 Possible binding to all interfaces
46+
|
47+
23 | # Implicit string concatenation
48+
24 | "0.0.0.0" f"0.0.0.0{expr}0.0.0.0"
49+
| ^^^^^^^^^ S104
50+
|
51+
52+
S104.py:24:13: S104 Possible binding to all interfaces
53+
|
54+
23 | # Implicit string concatenation
55+
24 | "0.0.0.0" f"0.0.0.0{expr}0.0.0.0"
56+
| ^^^^^^^ S104
57+
|
4558

59+
S104.py:24:26: S104 Possible binding to all interfaces
60+
|
61+
23 | # Implicit string concatenation
62+
24 | "0.0.0.0" f"0.0.0.0{expr}0.0.0.0"
63+
| ^^^^^^^ S104
64+
|

‎crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S108_S108.py.snap

+24
Original file line numberDiff line numberDiff line change
@@ -37,4 +37,28 @@ S108.py:14:11: S108 Probable insecure usage of temporary file or directory: "/de
3737
15 | f.write("def")
3838
|
3939

40+
S108.py:22:11: S108 Probable insecure usage of temporary file or directory: "/tmp/abc"
41+
|
42+
21 | # Implicit string concatenation
43+
22 | with open("/tmp/" "abc", "w") as f:
44+
| ^^^^^^^^^^^^^ S108
45+
23 | f.write("def")
46+
|
4047

48+
S108.py:25:11: S108 Probable insecure usage of temporary file or directory: "/tmp/abc"
49+
|
50+
23 | f.write("def")
51+
24 |
52+
25 | with open("/tmp/abc" f"/tmp/abc", "w") as f:
53+
| ^^^^^^^^^^ S108
54+
26 | f.write("def")
55+
|
56+
57+
S108.py:25:24: S108 Probable insecure usage of temporary file or directory: "/tmp/abc"
58+
|
59+
23 | f.write("def")
60+
24 |
61+
25 | with open("/tmp/abc" f"/tmp/abc", "w") as f:
62+
| ^^^^^^^^ S108
63+
26 | f.write("def")
64+
|

‎crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S108_extend.snap

+24
Original file line numberDiff line numberDiff line change
@@ -45,4 +45,28 @@ S108.py:18:11: S108 Probable insecure usage of temporary file or directory: "/fo
4545
19 | f.write("def")
4646
|
4747

48+
S108.py:22:11: S108 Probable insecure usage of temporary file or directory: "/tmp/abc"
49+
|
50+
21 | # Implicit string concatenation
51+
22 | with open("/tmp/" "abc", "w") as f:
52+
| ^^^^^^^^^^^^^ S108
53+
23 | f.write("def")
54+
|
55+
56+
S108.py:25:11: S108 Probable insecure usage of temporary file or directory: "/tmp/abc"
57+
|
58+
23 | f.write("def")
59+
24 |
60+
25 | with open("/tmp/abc" f"/tmp/abc", "w") as f:
61+
| ^^^^^^^^^^ S108
62+
26 | f.write("def")
63+
|
4864

65+
S108.py:25:24: S108 Probable insecure usage of temporary file or directory: "/tmp/abc"
66+
|
67+
23 | f.write("def")
68+
24 |
69+
25 | with open("/tmp/abc" f"/tmp/abc", "w") as f:
70+
| ^^^^^^^^ S108
71+
26 | f.write("def")
72+
|

‎crates/ruff_linter/src/rules/flake8_pyi/rules/string_or_bytes_too_long.rs

+23-5
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,9 @@ pub(crate) fn string_or_bytes_too_long(checker: &mut Checker, string: StringLike
5757
}
5858

5959
let length = match string {
60-
StringLike::StringLiteral(ast::ExprStringLiteral { value, .. }) => value.chars().count(),
61-
StringLike::BytesLiteral(ast::ExprBytesLiteral { value, .. }) => value.len(),
62-
StringLike::FStringLiteral(ast::FStringLiteralElement { value, .. }) => {
63-
value.chars().count()
64-
}
60+
StringLike::String(ast::ExprStringLiteral { value, .. }) => value.chars().count(),
61+
StringLike::Bytes(ast::ExprBytesLiteral { value, .. }) => value.len(),
62+
StringLike::FString(node) => count_f_string_chars(node),
6563
};
6664
if length <= 50 {
6765
return;
@@ -75,6 +73,26 @@ pub(crate) fn string_or_bytes_too_long(checker: &mut Checker, string: StringLike
7573
checker.diagnostics.push(diagnostic);
7674
}
7775

76+
/// Count the number of visible characters in an f-string. This accounts for
77+
/// implicitly concatenated f-strings as well.
78+
fn count_f_string_chars(f_string: &ast::ExprFString) -> usize {
79+
f_string
80+
.value
81+
.iter()
82+
.map(|part| match part {
83+
ast::FStringPart::Literal(string) => string.chars().count(),
84+
ast::FStringPart::FString(f_string) => f_string
85+
.elements
86+
.iter()
87+
.map(|element| match element {
88+
ast::FStringElement::Literal(string) => string.chars().count(),
89+
ast::FStringElement::Expression(expr) => expr.range().len().to_usize(),
90+
})
91+
.sum(),
92+
})
93+
.sum()
94+
}
95+
7896
fn is_warnings_dot_deprecated(expr: Option<&ast::Expr>, semantic: &SemanticModel) -> bool {
7997
// Does `expr` represent a call to `warnings.deprecated` or `typing_extensions.deprecated`?
8098
let Some(expr) = expr else {

‎crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI053_PYI053.pyi.snap

+18-3
Original file line numberDiff line numberDiff line change
@@ -105,12 +105,12 @@ PYI053.pyi:34:14: PYI053 [*] String and bytes literals longer than 50 characters
105105
36 36 | ffoo: str = f"50 character stringggggggggggggggggggggggggggggggg" # OK
106106
37 37 |
107107

108-
PYI053.pyi:38:15: PYI053 [*] String and bytes literals longer than 50 characters are not permitted
108+
PYI053.pyi:38:13: PYI053 [*] String and bytes literals longer than 50 characters are not permitted
109109
|
110110
36 | ffoo: str = f"50 character stringggggggggggggggggggggggggggggggg" # OK
111111
37 |
112112
38 | fbar: str = f"51 character stringgggggggggggggggggggggggggggggggg" # Error: PYI053
113-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI053
113+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI053
114114
39 |
115115
40 | class Demo:
116116
|
@@ -121,7 +121,7 @@ PYI053.pyi:38:15: PYI053 [*] String and bytes literals longer than 50 characters
121121
36 36 | ffoo: str = f"50 character stringggggggggggggggggggggggggggggggg" # OK
122122
37 37 |
123123
38 |-fbar: str = f"51 character stringgggggggggggggggggggggggggggggggg" # Error: PYI053
124-
38 |+fbar: str = f"..." # Error: PYI053
124+
38 |+fbar: str = ... # Error: PYI053
125125
39 39 |
126126
40 40 | class Demo:
127127
41 41 | """Docstrings are excluded from this rule. Some padding.""" # OK
@@ -144,5 +144,20 @@ PYI053.pyi:64:5: PYI053 [*] String and bytes literals longer than 50 characters
144144
64 |+ ... # Error: PYI053
145145
65 65 | )
146146
66 66 | def not_a_deprecated_function() -> None: ...
147+
67 67 |
147148

149+
PYI053.pyi:68:13: PYI053 [*] String and bytes literals longer than 50 characters are not permitted
150+
|
151+
66 | def not_a_deprecated_function() -> None: ...
152+
67 |
153+
68 | fbaz: str = f"51 character {foo} stringgggggggggggggggggggggggggg" # Error: PYI053
154+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI053
155+
|
156+
= help: Replace with `...`
148157

158+
ℹ Safe fix
159+
65 65 | )
160+
66 66 | def not_a_deprecated_function() -> None: ...
161+
67 67 |
162+
68 |-fbaz: str = f"51 character {foo} stringgggggggggggggggggggggggggg" # Error: PYI053
163+
68 |+fbaz: str = ... # Error: PYI053

0 commit comments

Comments
 (0)