Skip to content

Commit

Permalink
Account for possibly-empty f-string values in truthiness logic
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Jan 12, 2024
1 parent 350dcb8 commit 23793d5
Show file tree
Hide file tree
Showing 5 changed files with 148 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -160,3 +160,10 @@ def secondToTime(s0: int) -> (int, int, int) or str:

def secondToTime(s0: int) -> ((int, int, int) or str):
m, s = divmod(s0, 60)


# Regression test for: https://github.com/astral-sh/ruff/issues/9479
print(f"{a}{b}" or "bar")
print(f"{a}{''}" or "bar")
print(f"{''}{''}" or "bar")
print(f"{1}{''}" or "bar")
Original file line number Diff line number Diff line change
Expand Up @@ -147,3 +147,9 @@

if f(a and [] and False and []): # SIM223
pass

# Regression test for: https://github.com/astral-sh/ruff/issues/9479
print(f"{a}{b}" and "bar")
print(f"{a}{''}" and "bar")
print(f"{''}{''}" and "bar")
print(f"{1}{''}" and "bar")
Original file line number Diff line number Diff line change
Expand Up @@ -1040,5 +1040,25 @@ SIM222.py:161:31: SIM222 [*] Use `(int, int, int)` instead of `(int, int, int) o
161 |-def secondToTime(s0: int) -> ((int, int, int) or str):
161 |+def secondToTime(s0: int) -> ((int, int, int)):
162 162 | m, s = divmod(s0, 60)
163 163 |
164 164 |

SIM222.py:168:7: SIM222 [*] Use `"bar"` instead of `... or "bar"`
|
166 | print(f"{a}{b}" or "bar")
167 | print(f"{a}{''}" or "bar")
168 | print(f"{''}{''}" or "bar")
| ^^^^^^^^^^^^^^^^^^^^ SIM222
169 | print(f"{1}{''}" or "bar")
|
= help: Replace with `"bar"`

Unsafe fix
165 165 | # Regression test for: https://github.com/astral-sh/ruff/issues/9479
166 166 | print(f"{a}{b}" or "bar")
167 167 | print(f"{a}{''}" or "bar")
168 |-print(f"{''}{''}" or "bar")
168 |+print("bar")
169 169 | print(f"{1}{''}" or "bar")


Original file line number Diff line number Diff line change
Expand Up @@ -1003,5 +1003,25 @@ SIM223.py:148:12: SIM223 [*] Use `[]` instead of `[] and ...`
148 |-if f(a and [] and False and []): # SIM223
148 |+if f(a and []): # SIM223
149 149 | pass
150 150 |
151 151 | # Regression test for: https://github.com/astral-sh/ruff/issues/9479

SIM223.py:154:7: SIM223 [*] Use `f"{''}{''}"` instead of `f"{''}{''}" and ...`
|
152 | print(f"{a}{b}" and "bar")
153 | print(f"{a}{''}" and "bar")
154 | print(f"{''}{''}" and "bar")
| ^^^^^^^^^^^^^^^^^^^^^ SIM223
155 | print(f"{1}{''}" and "bar")
|
= help: Replace with `f"{''}{''}"`

Unsafe fix
151 151 | # Regression test for: https://github.com/astral-sh/ruff/issues/9479
152 152 | print(f"{a}{b}" and "bar")
153 153 | print(f"{a}{''}" and "bar")
154 |-print(f"{''}{''}" and "bar")
154 |+print(f"{''}{''}")
155 155 | print(f"{1}{''}" and "bar")


112 changes: 95 additions & 17 deletions crates/ruff_python_ast/src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -308,10 +308,13 @@ pub fn any_over_pattern(pattern: &Pattern, func: &dyn Fn(&Expr) -> bool) -> bool
}
}

pub fn any_over_f_string_element(element: &FStringElement, func: &dyn Fn(&Expr) -> bool) -> bool {
pub fn any_over_f_string_element(
element: &ast::FStringElement,
func: &dyn Fn(&Expr) -> bool,
) -> bool {
match element {
FStringElement::Literal(_) => false,
FStringElement::Expression(ast::FStringExpressionElement {
ast::FStringElement::Literal(_) => false,
ast::FStringElement::Expression(ast::FStringExpressionElement {
expression,
format_spec,
..
Expand Down Expand Up @@ -1171,21 +1174,10 @@ impl Truthiness {
}
Expr::NoneLiteral(_) => Self::Falsey,
Expr::EllipsisLiteral(_) => Self::Truthy,
Expr::FString(ast::ExprFString { value, .. }) => {
if value.iter().all(|part| match part {
ast::FStringPart::Literal(string_literal) => string_literal.is_empty(),
ast::FStringPart::FString(f_string) => f_string.elements.is_empty(),
}) {
Expr::FString(f_string) => {
if is_empty_f_string(&f_string) {
Self::Falsey
} else if value
.elements()
.any(|f_string_element| match f_string_element {
ast::FStringElement::Literal(ast::FStringLiteralElement {
value, ..
}) => !value.is_empty(),
ast::FStringElement::Expression(_) => true,
})
{
} else if is_non_empty_f_string(&f_string) {
Self::Truthy
} else {
Self::Unknown
Expand Down Expand Up @@ -1243,6 +1235,92 @@ impl Truthiness {
}
}

/// Returns `true` if the expression definitely resolves to a non-empty string, when used as an
/// f-string expression, or `false` if the expression may resolve to an empty string.
fn is_non_empty_f_string(expr: &ast::ExprFString) -> bool {
fn inner(expr: &Expr) -> bool {
match expr {
Expr::BoolOp(ast::ExprBoolOp { .. }) => false,
Expr::NamedExpr(ast::ExprNamedExpr { value, .. }) => inner(value),
Expr::BinOp(ast::ExprBinOp { .. }) => false,
Expr::UnaryOp(ast::ExprUnaryOp { .. }) => false,
Expr::Lambda(_) => true,
Expr::IfExp(ast::ExprIfExp { body, orelse, .. }) => inner(body) && inner(orelse),
Expr::Dict(_) => true,
Expr::Set(_) => true,
Expr::ListComp(_) => true,
Expr::SetComp(_) => true,
Expr::DictComp(_) => true,
Expr::GeneratorExp(_) => false,
Expr::Await(_) => false,
Expr::Yield(_) => false,
Expr::YieldFrom(_) => false,
Expr::Compare(_) => true,
Expr::Call(_) => false,
Expr::FString(f_string) => is_non_empty_f_string(f_string),
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => !value.is_empty(),
Expr::BytesLiteral(ast::ExprBytesLiteral { value, .. }) => !value.is_empty(),
Expr::NumberLiteral(_) => true,
Expr::BooleanLiteral(_) => true,
Expr::NoneLiteral(_) => true,
Expr::EllipsisLiteral(_) => true,
Expr::Attribute(_) => false,
Expr::Subscript(_) => false,
Expr::Starred(_) => false,
Expr::Name(_) => false,
Expr::List(_) => true,
Expr::Tuple(_) => true,
Expr::Slice(_) => false,
Expr::IpyEscapeCommand(_) => false,
}
}

expr.value.iter().any(|part| match part {
ast::FStringPart::Literal(string_literal) => !string_literal.is_empty(),
ast::FStringPart::FString(f_string) => {
f_string.elements.iter().all(|element| match element {
FStringElement::Literal(string_literal) => !string_literal.is_empty(),
FStringElement::Expression(f_string) => inner(&f_string.expression),
})
}
})
}

/// Returns `true` if the expression definitely resolves to the empty string, when used as an f-string
/// expression.
fn is_empty_f_string(expr: &ast::ExprFString) -> bool {
fn inner(expr: &Expr) -> bool {
match expr {
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => value.is_empty(),
Expr::BytesLiteral(ast::ExprBytesLiteral { value, .. }) => value.is_empty(),
Expr::FString(ast::ExprFString { value, .. }) => {
value
.elements()
.all(|f_string_element| match f_string_element {
FStringElement::Literal(ast::FStringLiteralElement { value, .. }) => {
value.is_empty()
}
FStringElement::Expression(ast::FStringExpressionElement {
expression,
..
}) => inner(expression),
})
}
_ => false,
}
}

expr.value.iter().all(|part| match part {
ast::FStringPart::Literal(string_literal) => string_literal.is_empty(),
ast::FStringPart::FString(f_string) => {
f_string.elements.iter().all(|element| match element {
FStringElement::Literal(string_literal) => string_literal.is_empty(),
FStringElement::Expression(f_string) => inner(&f_string.expression),
})
}
})
}

pub fn generate_comparison(
left: &Expr,
ops: &[CmpOp],
Expand Down

0 comments on commit 23793d5

Please sign in to comment.