From e917a61016107ac7aa9e90f8b08a0d144e0897b5 Mon Sep 17 00:00:00 2001 From: David Szotten Date: Sat, 5 Aug 2023 22:29:45 +0100 Subject: [PATCH] wip fixing ruff rules --- .../src/rules/flake8_comprehensions/fixes.rs | 2 +- .../flake8_pytest_style/rules/helpers.rs | 11 +++++- crates/ruff/src/rules/flynt/helpers.rs | 36 ++++++++++--------- .../flynt/rules/static_join_to_fstring.rs | 2 +- .../rules/f_string_missing_placeholders.rs | 10 ++++-- .../pylint/rules/assert_on_string_literal.rs | 20 +++++------ .../explicit_f_string_type_conversion.rs | 14 ++++---- .../tryceratops/rules/raise_vanilla_args.rs | 6 ++-- 8 files changed, 58 insertions(+), 43 deletions(-) diff --git a/crates/ruff/src/rules/flake8_comprehensions/fixes.rs b/crates/ruff/src/rules/flake8_comprehensions/fixes.rs index e366c4a2de0632..b1bae547ff5ebb 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/fixes.rs +++ b/crates/ruff/src/rules/flake8_comprehensions/fixes.rs @@ -1003,7 +1003,7 @@ pub(crate) fn fix_unnecessary_map( // If the expression is embedded in an f-string, surround it with spaces to avoid // syntax errors. if matches!(object_type, ObjectType::Set | ObjectType::Dict) { - if parent.is_some_and(Expr::is_formatted_value_expr) { + if parent.is_some_and(Expr::is_f_string_expr) { content = format!(" {content} "); } } diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs index df35d5a44fd08c..64bba6d4d3fd6b 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs @@ -64,12 +64,21 @@ pub(super) fn is_empty_or_null_string(expr: &Expr) -> bool { }) => string.is_empty(), Expr::Constant(constant) if constant.value.is_none() => true, Expr::FString(ast::ExprFString { values, range: _ }) => { - values.iter().all(is_empty_or_null_string) + values.iter().all(is_empty_or_null_string_part) } _ => false, } } +fn is_empty_or_null_string_part(part: &ast::FStringPart) -> bool { + match part { + ast::FStringPart::String(ast::StringTodoName { value, .. }) => value.is_empty(), + ast::FStringPart::FormattedValue(ast::FormattedValue { expression, .. }) => { + is_empty_or_null_string(expression) + } + } +} + pub(super) fn split_names(names: &str) -> Vec<&str> { // Match the following pytest code: // [x.strip() for x in argnames.split(",") if x.strip()] diff --git a/crates/ruff/src/rules/flynt/helpers.rs b/crates/ruff/src/rules/flynt/helpers.rs index 31368a0fd929cd..58260e6f302a33 100644 --- a/crates/ruff/src/rules/flynt/helpers.rs +++ b/crates/ruff/src/rules/flynt/helpers.rs @@ -2,25 +2,22 @@ use ruff_python_ast::{self as ast, Arguments, Constant, ConversionFlag, Expr}; use ruff_text_size::TextRange; /// Wrap an expression in a `FormattedValue` with no special formatting. -fn to_formatted_value_expr(inner: &Expr) -> Expr { - let node = ast::ExprFormattedValue { - value: Box::new(inner.clone()), +fn to_formatted_value_expr(inner: &Expr) -> ast::FStringPart { + ast::FStringPart::FormattedValue(ast::FormattedValue { + expression: Box::new(inner.clone()), debug_text: None, conversion: ConversionFlag::None, - format_spec: None, + format_spec: vec![], range: TextRange::default(), - }; - node.into() + }) } /// Convert a string to a constant string expression. -pub(super) fn to_constant_string(s: &str) -> Expr { - let node = ast::ExprConstant { - value: Constant::Str(s.to_owned()), - kind: None, +pub(super) fn to_constant_string(s: &str) -> ast::FStringPart { + ast::FStringPart::String(ast::StringTodoName { + value: s.to_owned(), range: TextRange::default(), - }; - node.into() + }) } /// Figure out if `expr` represents a "simple" call @@ -52,15 +49,20 @@ fn is_simple_callee(func: &Expr) -> bool { } /// Convert an expression to a f-string element (if it looks like a good idea). -pub(super) fn to_f_string_element(expr: &Expr) -> Option { +pub(super) fn to_fstring_element(expr: &Expr) -> Option { match expr { // These are directly handled by `unparse_f_string_element`: Expr::Constant(ast::ExprConstant { - value: Constant::Str(_), + value: Constant::Str(value), + range, .. - }) - | Expr::FString(_) - | Expr::FormattedValue(_) => Some(expr.clone()), + }) => Some(ast::FStringPart::String(ast::StringTodoName { + value: value.to_string(), + range: *range, + })), + // TODO (but "we don't know how to handle these right now" when called so not needed?) + // | Expr::FString(_) + // These should be pretty safe to wrap in a formatted value. Expr::Constant(ast::ExprConstant { value: diff --git a/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs b/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs index f1290ee8be1e31..e274f9dec31eea 100644 --- a/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs +++ b/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs @@ -95,7 +95,7 @@ fn build_fstring(joiner: &str, joinees: &[Expr]) -> Option { if !std::mem::take(&mut first) { fstring_elems.push(helpers::to_constant_string(joiner)); } - fstring_elems.push(helpers::to_f_string_element(expr)?); + fstring_elems.push(helpers::to_fstring_element(expr)?); } let node = ast::ExprFString { diff --git a/crates/ruff/src/rules/pyflakes/rules/f_string_missing_placeholders.rs b/crates/ruff/src/rules/pyflakes/rules/f_string_missing_placeholders.rs index fd2982c11a5191..a33f60d10a922c 100644 --- a/crates/ruff/src/rules/pyflakes/rules/f_string_missing_placeholders.rs +++ b/crates/ruff/src/rules/pyflakes/rules/f_string_missing_placeholders.rs @@ -1,4 +1,4 @@ -use ruff_python_ast::{Expr, PySourceType, Ranged}; +use ruff_python_ast::{Expr, FStringPart, PySourceType, Ranged}; use ruff_python_parser::{lexer, AsMode, StringKind, Tok}; use ruff_text_size::{TextRange, TextSize}; @@ -81,10 +81,14 @@ fn find_useless_f_strings<'a>( } /// F541 -pub(crate) fn f_string_missing_placeholders(expr: &Expr, values: &[Expr], checker: &mut Checker) { +pub(crate) fn f_string_missing_placeholders( + expr: &Expr, + values: &[FStringPart], + checker: &mut Checker, +) { if !values .iter() - .any(|value| matches!(value, Expr::FormattedValue(_))) + .any(|value| matches!(value, FStringPart::FormattedValue(_))) { for (prefix_range, tok_range) in find_useless_f_strings(expr, checker.locator(), checker.source_type) diff --git a/crates/ruff/src/rules/pylint/rules/assert_on_string_literal.rs b/crates/ruff/src/rules/pylint/rules/assert_on_string_literal.rs index 0dcde075d13f1c..42cfe80c431c70 100644 --- a/crates/ruff/src/rules/pylint/rules/assert_on_string_literal.rs +++ b/crates/ruff/src/rules/pylint/rules/assert_on_string_literal.rs @@ -75,21 +75,17 @@ pub(crate) fn assert_on_string_literal(checker: &mut Checker, test: &Expr) { checker.diagnostics.push(Diagnostic::new( AssertOnStringLiteral { kind: if values.iter().all(|value| match value { - Expr::Constant(ast::ExprConstant { value, .. }) => match value { - Constant::Str(value, ..) => value.is_empty(), - Constant::Bytes(value) => value.is_empty(), - _ => false, - }, - _ => false, + ast::FStringPart::String(ast::StringTodoName { value, .. }) => { + value.is_empty() + } + ast::FStringPart::FormattedValue(_) => false, }) { Kind::Empty } else if values.iter().any(|value| match value { - Expr::Constant(ast::ExprConstant { value, .. }) => match value { - Constant::Str(value, ..) => !value.is_empty(), - Constant::Bytes(value) => !value.is_empty(), - _ => false, - }, - _ => false, + ast::FStringPart::String(ast::StringTodoName { value, .. }) => { + !value.is_empty() + } + ast::FStringPart::FormattedValue(_) => false, }) { Kind::NonEmpty } else { diff --git a/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs b/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs index 4739395116ec18..86c65f3c527c26 100644 --- a/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs +++ b/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs @@ -56,12 +56,12 @@ impl AlwaysAutofixableViolation for ExplicitFStringTypeConversion { pub(crate) fn explicit_f_string_type_conversion( checker: &mut Checker, expr: &Expr, - values: &[Expr], + values: &[ast::FStringPart], ) { for (index, formatted_value) in values .iter() .filter_map(|expr| { - if let Expr::FormattedValue(expr) = &expr { + if let ast::FStringPart::FormattedValue(expr) = &expr { Some(expr) } else { None @@ -69,8 +69,10 @@ pub(crate) fn explicit_f_string_type_conversion( }) .enumerate() { - let ast::ExprFormattedValue { - value, conversion, .. + let ast::FormattedValue { + expression, + conversion, + .. } = formatted_value; // Skip if there's already a conversion flag. @@ -87,7 +89,7 @@ pub(crate) fn explicit_f_string_type_conversion( range: _, }, .. - }) = value.as_ref() + }) = expression.as_ref() else { continue; }; @@ -122,7 +124,7 @@ pub(crate) fn explicit_f_string_type_conversion( continue; } - let mut diagnostic = Diagnostic::new(ExplicitFStringTypeConversion, value.range()); + let mut diagnostic = Diagnostic::new(ExplicitFStringTypeConversion, expression.range()); if checker.patch(diagnostic.kind.rule()) { diagnostic.try_set_fix(|| { convert_call_to_conversion_flag(expr, index, checker.locator(), checker.stylist()) diff --git a/crates/ruff/src/rules/tryceratops/rules/raise_vanilla_args.rs b/crates/ruff/src/rules/tryceratops/rules/raise_vanilla_args.rs index ad2efa5ba56250..197cefda56ca2b 100644 --- a/crates/ruff/src/rules/tryceratops/rules/raise_vanilla_args.rs +++ b/crates/ruff/src/rules/tryceratops/rules/raise_vanilla_args.rs @@ -56,8 +56,10 @@ where match expr { Expr::FString(ast::ExprFString { values, range: _ }) => { for value in values { - if any_string(value, predicate) { - return true; + if let ast::FStringPart::String(ast::StringTodoName { value, .. }) = value { + if predicate(value.as_str()) { + return true; + } } } }