From 23793d57a474a963c2f6dfb0784cc967dfdfd64d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 11 Jan 2024 20:46:52 -0500 Subject: [PATCH] Account for possibly-empty f-string values in truthiness logic --- .../test/fixtures/flake8_simplify/SIM222.py | 7 ++ .../test/fixtures/flake8_simplify/SIM223.py | 6 + ...ke8_simplify__tests__SIM222_SIM222.py.snap | 20 ++++ ...ke8_simplify__tests__SIM223_SIM223.py.snap | 20 ++++ crates/ruff_python_ast/src/helpers.rs | 112 +++++++++++++++--- 5 files changed, 148 insertions(+), 17 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM222.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM222.py index dcf730f4a75ec8..bd1282ce0f1b85 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM222.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM222.py @@ -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") diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM223.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM223.py index 98ad6d6e9e645a..1842f06994c1f5 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM223.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM223.py @@ -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") diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM222_SIM222.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM222_SIM222.py.snap index 585be6c06eabbd..ddf4598e13b596 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM222_SIM222.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM222_SIM222.py.snap @@ -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") diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM223_SIM223.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM223_SIM223.py.snap index d1d2fae78d113d..855b2091823cb8 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM223_SIM223.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM223_SIM223.py.snap @@ -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") diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index bb463c95b3f451..2102094477bd75 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -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, .. @@ -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 @@ -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],