-
Notifications
You must be signed in to change notification settings - Fork 944
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
Update string nodes for implicit concatenation #7927
Changes from 13 commits
86d799d
223380f
21ed36c
0c6e2e7
59ef011
6be8eca
1c6dc52
da89c76
6cdf149
95f71da
6d6e1c7
bcabcca
8193ff9
1e72af2
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 |
---|---|---|
|
@@ -5,13 +5,12 @@ use ruff_python_ast::{self as ast, Expr, Operator}; | |
use ruff_diagnostics::{Diagnostic, Violation}; | ||
use ruff_macros::{derive_message_formats, violation}; | ||
use ruff_python_ast::helpers::any_over_expr; | ||
use ruff_python_semantic::SemanticModel; | ||
use ruff_python_ast::str::raw_contents; | ||
use ruff_source_file::Locator; | ||
use ruff_text_size::Ranged; | ||
|
||
use crate::checkers::ast::Checker; | ||
|
||
use super::super::helpers::string_literal; | ||
|
||
static SQL_REGEX: Lazy<Regex> = Lazy::new(|| { | ||
Regex::new(r"(?i)\b(select\s.+\sfrom\s|delete\s+from\s|(insert|replace)\s.+\svalues\s|update\s.+\sset\s)") | ||
.unwrap() | ||
|
@@ -46,53 +45,77 @@ impl Violation for HardcodedSQLExpression { | |
} | ||
} | ||
|
||
fn has_string_literal(expr: &Expr) -> bool { | ||
string_literal(expr).is_some() | ||
} | ||
|
||
fn matches_sql_statement(string: &str) -> bool { | ||
SQL_REGEX.is_match(string) | ||
/// Concatenates the contents of an f-string, without the prefix and quotes, | ||
/// and escapes any special characters. | ||
/// | ||
/// ## Example | ||
/// | ||
/// ```python | ||
/// "foo" f"bar {x}" "baz" | ||
/// ``` | ||
/// | ||
/// becomes `foobar {x}baz`. | ||
fn concatenated_f_string(expr: &ast::ExprFString, locator: &Locator) -> String { | ||
expr.value | ||
.parts() | ||
.filter_map(|part| { | ||
raw_contents(locator.slice(part)).map(|s| s.escape_default().to_string()) | ||
}) | ||
.collect() | ||
} | ||
|
||
fn matches_string_format_expression(expr: &Expr, semantic: &SemanticModel) -> bool { | ||
match expr { | ||
/// S608 | ||
pub(crate) fn hardcoded_sql_expression(checker: &mut Checker, expr: &Expr) { | ||
let content = match expr { | ||
// "select * from table where val = " + "str" + ... | ||
// "select * from table where val = %s" % ... | ||
Expr::BinOp(ast::ExprBinOp { | ||
op: Operator::Add | Operator::Mod, | ||
.. | ||
op: Operator::Add, .. | ||
}) => { | ||
// Only evaluate the full BinOp, not the nested components. | ||
if semantic | ||
if !checker | ||
.semantic() | ||
.current_expression_parent() | ||
.map_or(true, |parent| !parent.is_bin_op_expr()) | ||
{ | ||
if any_over_expr(expr, &has_string_literal) { | ||
return true; | ||
} | ||
return; | ||
} | ||
if !any_over_expr(expr, &Expr::is_string_literal_expr) { | ||
return; | ||
} | ||
false | ||
checker.generator().expr(expr) | ||
} | ||
// "select * from table where val = %s" % ... | ||
Expr::BinOp(ast::ExprBinOp { | ||
left, | ||
op: Operator::Mod, | ||
.. | ||
}) => { | ||
Comment on lines
+87
to
+92
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. This is now a separate branch because there could be implicit concatenations on the left side of |
||
let Some(string) = left.as_string_literal_expr() else { | ||
return; | ||
}; | ||
string.value.escape_default().to_string() | ||
} | ||
Expr::Call(ast::ExprCall { func, .. }) => { | ||
let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() else { | ||
return false; | ||
return; | ||
}; | ||
// "select * from table where val = {}".format(...) | ||
attr == "format" && string_literal(value).is_some() | ||
if attr != "format" { | ||
return; | ||
} | ||
let Some(string) = value.as_string_literal_expr() else { | ||
return; | ||
}; | ||
string.value.escape_default().to_string() | ||
} | ||
// f"select * from table where val = {val}" | ||
Expr::FString(_) => true, | ||
_ => false, | ||
} | ||
} | ||
Expr::FString(f_string) => concatenated_f_string(f_string, checker.locator()), | ||
_ => return, | ||
}; | ||
|
||
/// S608 | ||
pub(crate) fn hardcoded_sql_expression(checker: &mut Checker, expr: &Expr) { | ||
if matches_string_format_expression(expr, checker.semantic()) { | ||
if matches_sql_statement(&checker.generator().expr(expr)) { | ||
checker | ||
.diagnostics | ||
.push(Diagnostic::new(HardcodedSQLExpression, expr.range())); | ||
} | ||
if SQL_REGEX.is_match(&content) { | ||
checker | ||
.diagnostics | ||
.push(Diagnostic::new(HardcodedSQLExpression, expr.range())); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,7 +130,7 @@ pub(crate) fn unrecognized_platform(checker: &mut Checker, test: &Expr) { | |
if !matches!(value.as_str(), "linux" | "win32" | "cygwin" | "darwin") { | ||
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. This is an example where... we can probably do this without the concatenation? Imagine if we had a custom matcher that worked on implicit concatenations (i.e., a vector of strings). \cc @BurntSushi in case you have obvious ideas here. 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. I think the closest "out of the box" experience you can get is stream searching with But I would say to write the simple/obvious code for now, and if this ends up popping up on a profile then we can revisit it and do something bespoke here or use 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. I think I would prefer to avoid the complexity for such simple cases but I do understand that this could be useful in similar cases where the strings are long. |
||
checker.diagnostics.push(Diagnostic::new( | ||
UnrecognizedPlatformName { | ||
platform: value.clone(), | ||
platform: value.to_string(), | ||
}, | ||
right.range(), | ||
)); | ||
|
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.
Escapes are required for multi-line strings to escape the newline characters.