Skip to content

Commit

Permalink
RUF027 no longer has false negatives with string literals inside of m…
Browse files Browse the repository at this point in the history
…ethod calls (astral-sh#9865)

Fixes astral-sh#9857.

## Summary

Statements like `logging.info("Today it is: {day}")` will no longer be
ignored by RUF027. As before, statements like `"Today it is:
{day}".format(day="Tuesday")` will continue to be ignored.

## Test Plan

The snapshot tests were expanded to include new cases. Additionally, the
snapshot tests have been split in two to separate positive cases from
negative cases.
  • Loading branch information
snowsignal authored and nkxxll committed Mar 4, 2024
1 parent 2131804 commit df3da93
Show file tree
Hide file tree
Showing 8 changed files with 445 additions and 395 deletions.
86 changes: 0 additions & 86 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF027.py

This file was deleted.

70 changes: 70 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF027_0.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
val = 2

"always ignore this: {val}"

print("but don't ignore this: {val}") # RUF027


def simple_cases():
a = 4
b = "{a}" # RUF027
c = "{a} {b} f'{val}' " # RUF027


def escaped_string():
a = 4
b = "escaped string: {{ brackets surround me }}" # RUF027


def raw_string():
a = 4
b = r"raw string with formatting: {a}" # RUF027
c = r"raw string with \backslashes\ and \"escaped quotes\": {a}" # RUF027


def print_name(name: str):
a = 4
print("Hello, {name}!") # RUF027
print("The test value we're using today is {a}") # RUF027


def nested_funcs():
a = 4
print(do_nothing(do_nothing("{a}"))) # RUF027


def tripled_quoted():
a = 4
c = a
single_line = """ {a} """ # RUF027
# RUF027
multi_line = a = """b { # comment
c} d
"""


def single_quoted_multi_line():
a = 4
# RUF027
b = " {\
a} \
"


def implicit_concat():
a = 4
b = "{a}" "+" "{b}" r" \\ " # RUF027 for the first part only
print(f"{a}" "{a}" f"{b}") # RUF027


def escaped_chars():
a = 4
b = "\"not escaped:\" '{a}' \"escaped:\": '{{c}}'" # RUF027


def method_calls():
value = {}
value.method = print_name
first = "Wendy"
last = "Appleseed"
value.method("{first} {last}") # RUF027
36 changes: 36 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
def do_nothing(a):
return a


def alternative_formatter(src, **kwargs):
src.format(**kwargs)


def format2(src, *args):
pass


# These should not cause an RUF027 message
def negative_cases():
a = 4
positive = False
"""{a}"""
"don't format: {a}"
c = """ {b} """
d = "bad variable: {invalid}"
e = "incorrect syntax: {}"
f = "uses a builtin: {max}"
json = "{ positive: false }"
json2 = "{ 'positive': false }"
json3 = "{ 'positive': 'false' }"
alternative_formatter("{a}", a=5)
formatted = "{a}".fmt(a=7)
print(do_nothing("{a}".format(a=3)))
print(do_nothing(alternative_formatter("{a}", a=5)))
print(format(do_nothing("{a}"), a=5))
print("{a}".to_upper())
print(do_nothing("{a}").format(a="Test"))
print(do_nothing("{a}").format2(a))
print(("{a}" "{c}").format(a=1, c=2))
print("{a}".attribute.chaining.call(a=2))
print("{a} {c}".format(a))
3 changes: 2 additions & 1 deletion crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ mod tests {
#[test_case(Rule::MutableFromkeysValue, Path::new("RUF024.py"))]
#[test_case(Rule::UnnecessaryDictComprehensionForIterable, Path::new("RUF025.py"))]
#[test_case(Rule::DefaultFactoryKwarg, Path::new("RUF026.py"))]
#[test_case(Rule::MissingFStringSyntax, Path::new("RUF027.py"))]
#[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_0.py"))]
#[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_1.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
48 changes: 35 additions & 13 deletions crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use rustc_hash::FxHashSet;
/// this lint will disqualify any literal that satisfies any of the following conditions:
///
/// 1. The string literal is a standalone expression. For example, a docstring.
/// 2. The literal is part of a function call with keyword arguments that match at least one variable (for example: `format("Message: {value}", value = "Hello World")`)
/// 2. The literal is part of a function call with argument names that match at least one variable (for example: `format("Message: {value}", value = "Hello World")`)
/// 3. The literal (or a parent expression of the literal) has a direct method call on it (for example: `"{value}".format(...)`)
/// 4. The string has no `{...}` expression sections, or uses invalid f-string syntax.
/// 5. The string references variables that are not in scope, or it doesn't capture variables at all.
Expand Down Expand Up @@ -94,29 +94,51 @@ fn should_be_fstring(
return false;
};

let mut kwargs = vec![];
let mut arg_names = FxHashSet::default();
let mut last_expr: Option<&ast::Expr> = None;
for expr in semantic.current_expressions() {
match expr {
ast::Expr::Call(ast::ExprCall {
arguments: ast::Arguments { keywords, .. },
arguments: ast::Arguments { keywords, args, .. },
func,
..
}) => {
if let ast::Expr::Attribute(ast::ExprAttribute { .. }) = func.as_ref() {
return false;
if let ast::Expr::Attribute(ast::ExprAttribute { value, .. }) = func.as_ref() {
match value.as_ref() {
// if the first part of the attribute is the string literal,
// we want to ignore this literal from the lint.
// for example: `"{x}".some_method(...)`
ast::Expr::StringLiteral(expr_literal)
if expr_literal.value.as_slice().contains(literal) =>
{
return false;
}
// if the first part of the attribute was the expression we
// just went over in the last iteration, then we also want to pass
// this over in the lint.
// for example: `some_func("{x}").some_method(...)`
value if last_expr == Some(value) => {
return false;
}
_ => {}
}
}
for keyword in keywords {
if let Some(ident) = keyword.arg.as_ref() {
arg_names.insert(ident.as_str());
}
}
for arg in args {
if let ast::Expr::Name(ast::ExprName { id, .. }) = arg {
arg_names.insert(id.as_str());
}
}
kwargs.extend(keywords.iter());
}
_ => continue,
}
last_expr.replace(expr);
}

let kw_idents: FxHashSet<&str> = kwargs
.iter()
.filter_map(|k| k.arg.as_ref())
.map(ast::Identifier::as_str)
.collect();

for f_string in value.f_strings() {
let mut has_name = false;
for element in f_string
Expand All @@ -125,7 +147,7 @@ fn should_be_fstring(
.filter_map(|element| element.as_expression())
{
if let ast::Expr::Name(ast::ExprName { id, .. }) = element.expression.as_ref() {
if kw_idents.contains(id.as_str()) {
if arg_names.contains(id.as_str()) {
return false;
}
if semantic
Expand Down
Loading

0 comments on commit df3da93

Please sign in to comment.