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

Use ExprFString for StringLike::FString variant #10311

Merged
merged 4 commits into from Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
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"
Comment on lines +23 to +24
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not exactly correct (#10308) but for now it's fine.

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(),
));
}
}
}
}
}
}
Comment on lines +49 to +71
Copy link
Member Author

Choose a reason for hiding this comment

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

This logic is being repeated for all three rules changed in this PR. The logic being:

  • Loop over each f-string part
    • If it's a string literal, then directly run the check on it
    • If it's an f-string, loop over each literal and expression element of it

I'm not exactly sure on how to avoid duplicating this logic or if it's possible without introducing additional complexity.

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