Skip to content

Commit d8debb7

Browse files
authored
Simplify logic for RUF027 (#12907)
## Summary This PR is a pure refactor to simplify some of the logic for `RUF027`. This will make it easier to file some followup PRs to help reduce the false positives from this rule. I'm separating the refactor out into a separate PR so it's easier to review, and so I can double-check from the ecosystem report that this doesn't have any user-facing impact. ## Test Plan `cargo test -p ruff_linter --lib`
1 parent bd4a947 commit d8debb7

File tree

1 file changed

+55
-61
lines changed

1 file changed

+55
-61
lines changed

crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs

+55-61
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
22
use ruff_macros::{derive_message_formats, violation};
3-
use ruff_python_ast::{self as ast};
3+
use ruff_python_ast as ast;
44
use ruff_python_literal::format::FormatSpec;
55
use ruff_python_parser::parse_expression;
6-
use ruff_python_semantic::analyze::logging;
6+
use ruff_python_semantic::analyze::logging::is_logger_candidate;
77
use ruff_python_semantic::SemanticModel;
88
use ruff_source_file::Locator;
99
use ruff_text_size::{Ranged, TextRange};
@@ -33,6 +33,8 @@ use crate::checkers::ast::Checker;
3333
/// 4. The string has no `{...}` expression sections, or uses invalid f-string syntax.
3434
/// 5. The string references variables that are not in scope, or it doesn't capture variables at all.
3535
/// 6. Any format specifiers in the potential f-string are invalid.
36+
/// 7. The string is part of a function call that is known to expect a template string rather than an
37+
/// evaluated f-string: for example, a `logging` call or a [`gettext`] call
3638
///
3739
/// ## Example
3840
///
@@ -48,6 +50,9 @@ use crate::checkers::ast::Checker;
4850
/// day_of_week = "Tuesday"
4951
/// print(f"Hello {name}! It is {day_of_week} today!")
5052
/// ```
53+
///
54+
/// [`logging`]: https://docs.python.org/3/howto/logging-cookbook.html#using-particular-formatting-styles-throughout-your-application
55+
/// [`gettext`]: https://docs.python.org/3/library/gettext.html
5156
#[violation]
5257
pub struct MissingFStringSyntax;
5358

@@ -75,11 +80,22 @@ pub(crate) fn missing_fstring_syntax(checker: &mut Checker, literal: &ast::Strin
7580
}
7681
}
7782

78-
// We also want to avoid expressions that are intended to be translated.
79-
if semantic.current_expressions().any(|expr| {
80-
is_gettext(expr, semantic)
81-
|| is_logger_call(expr, semantic, &checker.settings.logger_objects)
82-
}) {
83+
let logger_objects = &checker.settings.logger_objects;
84+
85+
// We also want to avoid:
86+
// - Expressions inside `gettext()` calls
87+
// - Expressions passed to logging calls (since the `logging` module evaluates them lazily:
88+
// https://docs.python.org/3/howto/logging-cookbook.html#using-particular-formatting-styles-throughout-your-application)
89+
// - Expressions where a method is immediately called on the string literal
90+
if semantic
91+
.current_expressions()
92+
.filter_map(ast::Expr::as_call_expr)
93+
.any(|call_expr| {
94+
is_method_call_on_literal(call_expr, literal)
95+
|| is_gettext(call_expr, semantic)
96+
|| is_logger_candidate(&call_expr.func, semantic, logger_objects)
97+
})
98+
{
8399
return;
84100
}
85101

@@ -90,13 +106,6 @@ pub(crate) fn missing_fstring_syntax(checker: &mut Checker, literal: &ast::Strin
90106
}
91107
}
92108

93-
fn is_logger_call(expr: &ast::Expr, semantic: &SemanticModel, logger_objects: &[String]) -> bool {
94-
let ast::Expr::Call(ast::ExprCall { func, .. }) = expr else {
95-
return false;
96-
};
97-
logging::is_logger_candidate(func, semantic, logger_objects)
98-
}
99-
100109
/// Returns `true` if an expression appears to be a `gettext` call.
101110
///
102111
/// We want to avoid statement expressions and assignments related to aliases
@@ -107,12 +116,9 @@ fn is_logger_call(expr: &ast::Expr, semantic: &SemanticModel, logger_objects: &[
107116
/// and replace the original string with its translated counterpart. If the
108117
/// string contains variable placeholders or formatting, it can complicate the
109118
/// translation process, lead to errors or incorrect translations.
110-
fn is_gettext(expr: &ast::Expr, semantic: &SemanticModel) -> bool {
111-
let ast::Expr::Call(ast::ExprCall { func, .. }) = expr else {
112-
return false;
113-
};
114-
115-
let short_circuit = match func.as_ref() {
119+
fn is_gettext(call_expr: &ast::ExprCall, semantic: &SemanticModel) -> bool {
120+
let func = &*call_expr.func;
121+
let short_circuit = match func {
116122
ast::Expr::Name(ast::ExprName { id, .. }) => {
117123
matches!(id.as_str(), "gettext" | "ngettext" | "_")
118124
}
@@ -136,6 +142,21 @@ fn is_gettext(expr: &ast::Expr, semantic: &SemanticModel) -> bool {
136142
})
137143
}
138144

145+
/// Return `true` if `call_expr` is a method call on an [`ast::ExprStringLiteral`]
146+
/// in which `literal` is one of the [`ast::StringLiteral`] parts.
147+
///
148+
/// For example: `expr` is a node representing the expression `"{foo}".format(foo="bar")`,
149+
/// and `literal` is the node representing the string literal `"{foo}"`.
150+
fn is_method_call_on_literal(call_expr: &ast::ExprCall, literal: &ast::StringLiteral) -> bool {
151+
let ast::Expr::Attribute(ast::ExprAttribute { value, .. }) = &*call_expr.func else {
152+
return false;
153+
};
154+
let ast::Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = &**value else {
155+
return false;
156+
};
157+
value.as_slice().contains(literal)
158+
}
159+
139160
/// Returns `true` if `literal` is likely an f-string with a missing `f` prefix.
140161
/// See [`MissingFStringSyntax`] for the validation criteria.
141162
fn should_be_fstring(
@@ -158,55 +179,28 @@ fn should_be_fstring(
158179
};
159180

160181
let mut arg_names = FxHashSet::default();
161-
let mut last_expr: Option<&ast::Expr> = None;
162-
for expr in semantic.current_expressions() {
163-
match expr {
164-
ast::Expr::Call(ast::ExprCall {
165-
arguments: ast::Arguments { keywords, args, .. },
166-
func,
167-
..
168-
}) => {
169-
if let ast::Expr::Attribute(ast::ExprAttribute { value, .. }) = func.as_ref() {
170-
match value.as_ref() {
171-
// if the first part of the attribute is the string literal,
172-
// we want to ignore this literal from the lint.
173-
// for example: `"{x}".some_method(...)`
174-
ast::Expr::StringLiteral(expr_literal)
175-
if expr_literal.value.as_slice().contains(literal) =>
176-
{
177-
return false;
178-
}
179-
// if the first part of the attribute was the expression we
180-
// just went over in the last iteration, then we also want to pass
181-
// this over in the lint.
182-
// for example: `some_func("{x}").some_method(...)`
183-
value if last_expr == Some(value) => {
184-
return false;
185-
}
186-
_ => {}
187-
}
188-
}
189-
for keyword in &**keywords {
190-
if let Some(ident) = keyword.arg.as_ref() {
191-
arg_names.insert(ident.as_str());
192-
}
193-
}
194-
for arg in &**args {
195-
if let ast::Expr::Name(ast::ExprName { id, .. }) = arg {
196-
arg_names.insert(id.as_str());
197-
}
198-
}
182+
for expr in semantic
183+
.current_expressions()
184+
.filter_map(ast::Expr::as_call_expr)
185+
{
186+
let ast::Arguments { keywords, args, .. } = &expr.arguments;
187+
for keyword in &**keywords {
188+
if let Some(ident) = keyword.arg.as_ref() {
189+
arg_names.insert(&ident.id);
190+
}
191+
}
192+
for arg in &**args {
193+
if let ast::Expr::Name(ast::ExprName { id, .. }) = arg {
194+
arg_names.insert(id);
199195
}
200-
_ => continue,
201196
}
202-
last_expr.replace(expr);
203197
}
204198

205199
for f_string in value.f_strings() {
206200
let mut has_name = false;
207201
for element in f_string.elements.expressions() {
208202
if let ast::Expr::Name(ast::ExprName { id, .. }) = element.expression.as_ref() {
209-
if arg_names.contains(id.as_str()) {
203+
if arg_names.contains(id) {
210204
return false;
211205
}
212206
if semantic

0 commit comments

Comments
 (0)