Skip to content

Commit

Permalink
Use ExprFString for StringLike::FString variant (#10311)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
dhruvmanila committed Mar 14, 2024
1 parent f7802ad commit 5f40371
Show file tree
Hide file tree
Showing 16 changed files with 251 additions and 63 deletions.
Expand Up @@ -18,3 +18,7 @@ def func(address):
def my_func():
x = "0.0.0.0"
print(x)


# Implicit string concatenation
"0.0.0.0" f"0.0.0.0{expr}0.0.0.0"
Expand Up @@ -18,6 +18,13 @@
with open("/foo/bar", "w") as f:
f.write("def")

# Implicit string concatenation
with open("/tmp/" "abc", "w") as f:
f.write("def")

with open("/tmp/abc" f"/tmp/abc", "w") as f:
f.write("def")

# Using `tempfile` module should be ok
import tempfile
from tempfile import TemporaryDirectory
Expand Down
Expand Up @@ -64,3 +64,5 @@ def not_warnings_dot_deprecated(
"Not warnings.deprecated, so this one *should* lead to PYI053 in a stub!" # Error: PYI053
)
def not_a_deprecated_function() -> None: ...

fbaz: str = f"51 character {foo} stringgggggggggggggggggggggggggg" # Error: PYI053
Expand Up @@ -53,3 +53,6 @@ class Labware:


assert getattr(Labware(), "µL") == 1.5

# Implicit string concatenation
x = "𝐁ad" f"𝐁ad string"
13 changes: 2 additions & 11 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Expand Up @@ -45,7 +45,7 @@ use ruff_python_ast::helpers::{
use ruff_python_ast::identifier::Identifier;
use ruff_python_ast::name::QualifiedName;
use ruff_python_ast::str::Quote;
use ruff_python_ast::visitor::{walk_except_handler, walk_f_string_element, walk_pattern, Visitor};
use ruff_python_ast::visitor::{walk_except_handler, walk_pattern, Visitor};
use ruff_python_ast::{helpers, str, visitor, PySourceType};
use ruff_python_codegen::{Generator, Stylist};
use ruff_python_index::Indexer;
Expand Down Expand Up @@ -1407,6 +1407,7 @@ impl<'a> Visitor<'a> for Checker<'a> {
analyze::string_like(string_literal.into(), self);
}
Expr::BytesLiteral(bytes_literal) => analyze::string_like(bytes_literal.into(), self),
Expr::FString(f_string) => analyze::string_like(f_string.into(), self),
_ => {}
}

Expand Down Expand Up @@ -1573,16 +1574,6 @@ impl<'a> Visitor<'a> for Checker<'a> {
.push((bound, self.semantic.snapshot()));
}
}

fn visit_f_string_element(&mut self, f_string_element: &'a ast::FStringElement) {
// Step 2: Traversal
walk_f_string_element(self, f_string_element);

// Step 4: Analysis
if let Some(literal) = f_string_element.as_literal() {
analyze::string_like(literal.into(), self);
}
}
}

impl<'a> Checker<'a> {
Expand Down
Expand Up @@ -38,17 +38,37 @@ impl Violation for HardcodedBindAllInterfaces {

/// S104
pub(crate) fn hardcoded_bind_all_interfaces(checker: &mut Checker, string: StringLike) {
let is_bind_all_interface = match string {
StringLike::StringLiteral(ast::ExprStringLiteral { value, .. }) => value == "0.0.0.0",
StringLike::FStringLiteral(ast::FStringLiteralElement { value, .. }) => {
&**value == "0.0.0.0"
match string {
StringLike::String(ast::ExprStringLiteral { value, .. }) => {
if value == "0.0.0.0" {
checker
.diagnostics
.push(Diagnostic::new(HardcodedBindAllInterfaces, string.range()));
}
}
StringLike::BytesLiteral(_) => return,
StringLike::FString(ast::ExprFString { value, .. }) => {
for part in value {
match part {
ast::FStringPart::Literal(literal) => {
if &**literal == "0.0.0.0" {
checker
.diagnostics
.push(Diagnostic::new(HardcodedBindAllInterfaces, literal.range()));
}
}
ast::FStringPart::FString(f_string) => {
for literal in f_string.literals() {
if &**literal == "0.0.0.0" {
checker.diagnostics.push(Diagnostic::new(
HardcodedBindAllInterfaces,
literal.range(),
));
}
}
}
}
}
}
StringLike::Bytes(_) => (),
};

if is_bind_all_interface {
checker
.diagnostics
.push(Diagnostic::new(HardcodedBindAllInterfaces, string.range()));
}
}
@@ -1,5 +1,5 @@
use ruff_python_ast::{self as ast, Expr, StringLike};
use ruff_text_size::Ranged;
use ruff_text_size::{Ranged, TextRange};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
Expand Down Expand Up @@ -53,12 +53,29 @@ impl Violation for HardcodedTempFile {

/// S108
pub(crate) fn hardcoded_tmp_directory(checker: &mut Checker, string: StringLike) {
let value = match string {
StringLike::StringLiteral(ast::ExprStringLiteral { value, .. }) => value.to_str(),
StringLike::FStringLiteral(ast::FStringLiteralElement { value, .. }) => value,
StringLike::BytesLiteral(_) => return,
};
match string {
StringLike::String(ast::ExprStringLiteral { value, .. }) => {
check(checker, value.to_str(), string.range());
}
StringLike::FString(ast::ExprFString { value, .. }) => {
for part in value {
match part {
ast::FStringPart::Literal(literal) => {
check(checker, literal, literal.range());
}
ast::FStringPart::FString(f_string) => {
for literal in f_string.literals() {
check(checker, literal, literal.range());
}
}
}
}
}
StringLike::Bytes(_) => (),
}
}

fn check(checker: &mut Checker, value: &str, range: TextRange) {
if !checker
.settings
.flake8_bandit
Expand All @@ -85,6 +102,6 @@ pub(crate) fn hardcoded_tmp_directory(checker: &mut Checker, string: StringLike)
HardcodedTempFile {
string: value.to_string(),
},
string.range(),
range,
));
}
Expand Up @@ -42,4 +42,23 @@ S104.py:19:9: S104 Possible binding to all interfaces
20 | print(x)
|

S104.py:24:1: S104 Possible binding to all interfaces
|
23 | # Implicit string concatenation
24 | "0.0.0.0" f"0.0.0.0{expr}0.0.0.0"
| ^^^^^^^^^ S104
|

S104.py:24:13: S104 Possible binding to all interfaces
|
23 | # Implicit string concatenation
24 | "0.0.0.0" f"0.0.0.0{expr}0.0.0.0"
| ^^^^^^^ S104
|

S104.py:24:26: S104 Possible binding to all interfaces
|
23 | # Implicit string concatenation
24 | "0.0.0.0" f"0.0.0.0{expr}0.0.0.0"
| ^^^^^^^ S104
|
Expand Up @@ -37,4 +37,28 @@ S108.py:14:11: S108 Probable insecure usage of temporary file or directory: "/de
15 | f.write("def")
|

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

S108.py:25:11: S108 Probable insecure usage of temporary file or directory: "/tmp/abc"
|
23 | f.write("def")
24 |
25 | with open("/tmp/abc" f"/tmp/abc", "w") as f:
| ^^^^^^^^^^ S108
26 | f.write("def")
|

S108.py:25:24: S108 Probable insecure usage of temporary file or directory: "/tmp/abc"
|
23 | f.write("def")
24 |
25 | with open("/tmp/abc" f"/tmp/abc", "w") as f:
| ^^^^^^^^ S108
26 | f.write("def")
|
Expand Up @@ -45,4 +45,28 @@ S108.py:18:11: S108 Probable insecure usage of temporary file or directory: "/fo
19 | f.write("def")
|

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

S108.py:25:11: S108 Probable insecure usage of temporary file or directory: "/tmp/abc"
|
23 | f.write("def")
24 |
25 | with open("/tmp/abc" f"/tmp/abc", "w") as f:
| ^^^^^^^^^^ S108
26 | f.write("def")
|

S108.py:25:24: S108 Probable insecure usage of temporary file or directory: "/tmp/abc"
|
23 | f.write("def")
24 |
25 | with open("/tmp/abc" f"/tmp/abc", "w") as f:
| ^^^^^^^^ S108
26 | f.write("def")
|
Expand Up @@ -57,11 +57,9 @@ pub(crate) fn string_or_bytes_too_long(checker: &mut Checker, string: StringLike
}

let length = match string {
StringLike::StringLiteral(ast::ExprStringLiteral { value, .. }) => value.chars().count(),
StringLike::BytesLiteral(ast::ExprBytesLiteral { value, .. }) => value.len(),
StringLike::FStringLiteral(ast::FStringLiteralElement { value, .. }) => {
value.chars().count()
}
StringLike::String(ast::ExprStringLiteral { value, .. }) => value.chars().count(),
StringLike::Bytes(ast::ExprBytesLiteral { value, .. }) => value.len(),
StringLike::FString(node) => count_f_string_chars(node),
};
if length <= 50 {
return;
Expand All @@ -75,6 +73,26 @@ pub(crate) fn string_or_bytes_too_long(checker: &mut Checker, string: StringLike
checker.diagnostics.push(diagnostic);
}

/// Count the number of visible characters in an f-string. This accounts for
/// implicitly concatenated f-strings as well.
fn count_f_string_chars(f_string: &ast::ExprFString) -> usize {
f_string
.value
.iter()
.map(|part| match part {
ast::FStringPart::Literal(string) => string.chars().count(),
ast::FStringPart::FString(f_string) => f_string
.elements
.iter()
.map(|element| match element {
ast::FStringElement::Literal(string) => string.chars().count(),
ast::FStringElement::Expression(expr) => expr.range().len().to_usize(),
})
.sum(),
})
.sum()
}

fn is_warnings_dot_deprecated(expr: Option<&ast::Expr>, semantic: &SemanticModel) -> bool {
// Does `expr` represent a call to `warnings.deprecated` or `typing_extensions.deprecated`?
let Some(expr) = expr else {
Expand Down
Expand Up @@ -105,12 +105,12 @@ PYI053.pyi:34:14: PYI053 [*] String and bytes literals longer than 50 characters
36 36 | ffoo: str = f"50 character stringggggggggggggggggggggggggggggggg" # OK
37 37 |

PYI053.pyi:38:15: PYI053 [*] String and bytes literals longer than 50 characters are not permitted
PYI053.pyi:38:13: PYI053 [*] String and bytes literals longer than 50 characters are not permitted
|
36 | ffoo: str = f"50 character stringggggggggggggggggggggggggggggggg" # OK
37 |
38 | fbar: str = f"51 character stringgggggggggggggggggggggggggggggggg" # Error: PYI053
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI053
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI053
39 |
40 | class Demo:
|
Expand All @@ -121,7 +121,7 @@ PYI053.pyi:38:15: PYI053 [*] String and bytes literals longer than 50 characters
36 36 | ffoo: str = f"50 character stringggggggggggggggggggggggggggggggg" # OK
37 37 |
38 |-fbar: str = f"51 character stringgggggggggggggggggggggggggggggggg" # Error: PYI053
38 |+fbar: str = f"..." # Error: PYI053
38 |+fbar: str = ... # Error: PYI053
39 39 |
40 40 | class Demo:
41 41 | """Docstrings are excluded from this rule. Some padding.""" # OK
Expand All @@ -144,5 +144,20 @@ PYI053.pyi:64:5: PYI053 [*] String and bytes literals longer than 50 characters
64 |+ ... # Error: PYI053
65 65 | )
66 66 | def not_a_deprecated_function() -> None: ...
67 67 |

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

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

0 comments on commit 5f40371

Please sign in to comment.