From f3156ad8049e242fdaad5af8dbdfd1438bd07d4e Mon Sep 17 00:00:00 2001 From: Robin Caloudis Date: Sun, 25 Feb 2024 12:46:59 +0100 Subject: [PATCH 1/2] Fix false-positive in RUF027 --- .../resources/test/fixtures/ruff/RUF027_1.py | 3 ++ .../ruff/rules/missing_fstring_syntax.rs | 41 ++++++++++++++++--- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py index 3684f77a39de2..ac8fcf11a0227 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py @@ -25,6 +25,9 @@ def negative_cases(): json3 = "{ 'positive': 'false' }" alternative_formatter("{a}", a=5) formatted = "{a}".fmt(a=7) + partial = "partial sentence" + a = _("formatting of {partial} in a translation string is bad practice") + _("formatting of {partial} in a translation string is bad practice") print(do_nothing("{a}".format(a=3))) print(do_nothing(alternative_formatter("{a}", a=5))) print(format(do_nothing("{a}"), a=5)) diff --git a/crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs b/crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs index 3e11577e8b972..4f461ee4a43e8 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs @@ -61,13 +61,28 @@ pub(crate) fn missing_fstring_syntax( locator: &Locator, semantic: &SemanticModel, ) { - // we want to avoid statement expressions that are just a string literal. - // there's no reason to have standalone f-strings and this lets us avoid docstrings too - if let ast::Stmt::Expr(ast::StmtExpr { value, .. }) = semantic.current_statement() { - match value.as_ref() { - ast::Expr::StringLiteral(_) | ast::Expr::FString(_) => return, - _ => {} + match semantic.current_statement() { + ast::Stmt::Expr(ast::StmtExpr { value, .. }) => { + match value.as_ref() { + // We want to avoid statement expressions that are just a string literal. + // There's no reason to have standalone f-strings and this lets us avoid docstrings too. + ast::Expr::StringLiteral(_) | ast::Expr::FString(_) => return, + ast::Expr::Call(ast::ExprCall { func, .. }) => { + if is_translation_string(func) { + return; + } + } + _ => {} + } } + ast::Stmt::Assign(ast::StmtAssign { value, .. }) => { + if let ast::Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() { + if is_translation_string(func) { + return; + } + } + } + _ => {} } if should_be_fstring(literal, locator, semantic) { @@ -77,6 +92,20 @@ pub(crate) fn missing_fstring_syntax( } } +// It is a convention to use the `_()` alias for `gettext()`. We want to avoid +// statement expressions and assignments related to aliases of the gettext API. +// See https://docs.python.org/3/library/gettext.html for details. When one +// uses `_() to mark a string for translation, the tools look for these markers +// and replace the original string with its translated counterpart. If the +// string contains variable placeholders or formatting, it can complicate the +// translation process, lead to errors or incorrect translations. +fn is_translation_string(func: &ast::Expr) -> bool { + match func { + ast::Expr::Name(ast::ExprName { id, .. }) => id == "_", + _ => false, + } +} + /// Returns `true` if `literal` is likely an f-string with a missing `f` prefix. /// See [`MissingFStringSyntax`] for the validation criteria. fn should_be_fstring( From 17693ee8c3dc9974a1ba2c13581d254ff93acab9 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 25 Feb 2024 18:02:21 -0500 Subject: [PATCH 2/2] Detect along path --- .../resources/test/fixtures/ruff/RUF027_1.py | 1 + .../ruff/rules/missing_fstring_syntax.rs | 62 +++++++++---------- 2 files changed, 30 insertions(+), 33 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py index ac8fcf11a0227..964147300ba11 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py @@ -28,6 +28,7 @@ def negative_cases(): partial = "partial sentence" a = _("formatting of {partial} in a translation string is bad practice") _("formatting of {partial} in a translation string is bad practice") + print(_("formatting of {partial} in a translation string is bad practice")) print(do_nothing("{a}".format(a=3))) print(do_nothing(alternative_formatter("{a}", a=5))) print(format(do_nothing("{a}"), a=5)) diff --git a/crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs b/crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs index 4f461ee4a43e8..583a51947398d 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs @@ -61,28 +61,18 @@ pub(crate) fn missing_fstring_syntax( locator: &Locator, semantic: &SemanticModel, ) { - match semantic.current_statement() { - ast::Stmt::Expr(ast::StmtExpr { value, .. }) => { - match value.as_ref() { - // We want to avoid statement expressions that are just a string literal. - // There's no reason to have standalone f-strings and this lets us avoid docstrings too. - ast::Expr::StringLiteral(_) | ast::Expr::FString(_) => return, - ast::Expr::Call(ast::ExprCall { func, .. }) => { - if is_translation_string(func) { - return; - } - } - _ => {} - } - } - ast::Stmt::Assign(ast::StmtAssign { value, .. }) => { - if let ast::Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() { - if is_translation_string(func) { - return; - } - } + // we want to avoid statement expressions that are just a string literal. + // there's no reason to have standalone f-strings and this lets us avoid docstrings too + if let ast::Stmt::Expr(ast::StmtExpr { value, .. }) = semantic.current_statement() { + match value.as_ref() { + ast::Expr::StringLiteral(_) | ast::Expr::FString(_) => return, + _ => {} } - _ => {} + } + + // We also want to avoid expressions that are intended to be translated. + if semantic.current_expressions().any(is_gettext) { + return; } if should_be_fstring(literal, locator, semantic) { @@ -92,18 +82,24 @@ pub(crate) fn missing_fstring_syntax( } } -// It is a convention to use the `_()` alias for `gettext()`. We want to avoid -// statement expressions and assignments related to aliases of the gettext API. -// See https://docs.python.org/3/library/gettext.html for details. When one -// uses `_() to mark a string for translation, the tools look for these markers -// and replace the original string with its translated counterpart. If the -// string contains variable placeholders or formatting, it can complicate the -// translation process, lead to errors or incorrect translations. -fn is_translation_string(func: &ast::Expr) -> bool { - match func { - ast::Expr::Name(ast::ExprName { id, .. }) => id == "_", - _ => false, - } +/// Returns `true` if an expression appears to be a `gettext` call. +/// +/// We want to avoid statement expressions and assignments related to aliases +/// of the gettext API. +/// +/// See for details. When one +/// uses `_` to mark a string for translation, the tools look for these markers +/// and replace the original string with its translated counterpart. If the +/// string contains variable placeholders or formatting, it can complicate the +/// translation process, lead to errors or incorrect translations. +fn is_gettext(expr: &ast::Expr) -> bool { + let ast::Expr::Call(ast::ExprCall { func, .. }) = expr else { + return false; + }; + let ast::Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { + return false; + }; + matches!(id.as_str(), "_" | "gettext" | "ngettext") } /// Returns `true` if `literal` is likely an f-string with a missing `f` prefix.