From e323bb015bbaa5bf7f9c5235db964cd59cf3f774 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 29 May 2023 22:09:30 -0400 Subject: [PATCH] Move `fixable` checks into patch blocks (#4721) --- .../flake8_pytest_style/rules/assertion.rs | 46 ++--- .../flake8_simplify/rules/ast_bool_op.rs | 129 +++++++------- .../src/rules/flake8_simplify/rules/ast_if.rs | 157 +++++++++--------- .../rules/flake8_simplify/rules/ast_with.rs | 44 ++--- .../rules/suppressible_exception.rs | 52 +++--- .../pandas_vet/rules/inplace_argument.rs | 46 ++--- .../rules/pylint/rules/manual_import_from.rs | 33 ++-- .../src/rules/pylint/rules/nested_min_max.rs | 27 +-- ...convert_named_tuple_functional_to_class.rs | 22 +-- .../convert_typed_dict_functional_to_class.rs | 30 ++-- .../pyupgrade/rules/use_pep585_annotation.rs | 47 +++--- 11 files changed, 324 insertions(+), 309 deletions(-) diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs index 37fe645c19..aa59cf845d 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs @@ -184,25 +184,27 @@ pub(crate) fn unittest_assertion( match func { Expr::Attribute(ast::ExprAttribute { attr, .. }) => { if let Ok(unittest_assert) = UnittestAssert::try_from(attr.as_str()) { - // We're converting an expression to a statement, so avoid applying the fix if - // the assertion is part of a larger expression. - let fixable = checker.semantic_model().stmt().is_expr_stmt() - && checker.semantic_model().expr_parent().is_none() - && !checker.semantic_model().scope().kind.is_lambda() - && !has_comments_in(expr.range(), checker.locator); let mut diagnostic = Diagnostic::new( PytestUnittestAssertion { assertion: unittest_assert.to_string(), }, func.range(), ); - if fixable && checker.patch(diagnostic.kind.rule()) { - if let Ok(stmt) = unittest_assert.generate_assert(args, keywords) { - #[allow(deprecated)] - diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( - checker.generator().stmt(&stmt), - expr.range(), - ))); + if checker.patch(diagnostic.kind.rule()) { + // We're converting an expression to a statement, so avoid applying the fix if + // the assertion is part of a larger expression. + if checker.semantic_model().stmt().is_expr_stmt() + && checker.semantic_model().expr_parent().is_none() + && !checker.semantic_model().scope().kind.is_lambda() + && !has_comments_in(expr.range(), checker.locator) + { + if let Ok(stmt) = unittest_assert.generate_assert(args, keywords) { + #[allow(deprecated)] + diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( + checker.generator().stmt(&stmt), + expr.range(), + ))); + } } } Some(diagnostic) @@ -440,15 +442,17 @@ pub(crate) fn composite_condition( ) { let composite = is_composite_condition(test); if matches!(composite, CompositionKind::Simple | CompositionKind::Mixed) { - let fixable = matches!(composite, CompositionKind::Simple) - && msg.is_none() - && !has_comments_in(stmt.range(), checker.locator); let mut diagnostic = Diagnostic::new(PytestCompositeAssertion, stmt.range()); - if fixable && checker.patch(diagnostic.kind.rule()) { - #[allow(deprecated)] - diagnostic.try_set_fix_from_edit(|| { - fix_composite_condition(stmt, checker.locator, checker.stylist) - }); + if checker.patch(diagnostic.kind.rule()) { + if matches!(composite, CompositionKind::Simple) + && msg.is_none() + && !has_comments_in(stmt.range(), checker.locator) + { + #[allow(deprecated)] + diagnostic.try_set_fix_from_edit(|| { + fix_composite_condition(stmt, checker.locator, checker.stylist) + }); + } } checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs b/crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs index 2b032172c7..f69d31abeb 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs @@ -293,7 +293,6 @@ pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) { } else { unreachable!("Indices should only contain `isinstance` calls") }; - let fixable = !contains_effect(target, |id| checker.semantic_model().is_builtin(id)); let mut diagnostic = Diagnostic::new( DuplicateIsinstanceCall { name: if let Expr::Name(ast::ExprName { id, .. }) = target { @@ -304,73 +303,75 @@ pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) { }, expr.range(), ); - if fixable && checker.patch(diagnostic.kind.rule()) { - // Grab the types used in each duplicate `isinstance` call (e.g., `int` and `str` - // in `isinstance(obj, int) or isinstance(obj, str)`). - let types: Vec<&Expr> = indices - .iter() - .map(|index| &values[*index]) - .map(|expr| { - let Expr::Call(ast::ExprCall { args, ..}) = expr else { - unreachable!("Indices should only contain `isinstance` calls") - }; - args.get(1).expect("`isinstance` should have two arguments") - }) - .collect(); - - // Generate a single `isinstance` call. - let node = ast::ExprTuple { - // Flatten all the types used across the `isinstance` calls. - elts: types + if checker.patch(diagnostic.kind.rule()) { + if !contains_effect(target, |id| checker.semantic_model().is_builtin(id)) { + // Grab the types used in each duplicate `isinstance` call (e.g., `int` and `str` + // in `isinstance(obj, int) or isinstance(obj, str)`). + let types: Vec<&Expr> = indices .iter() - .flat_map(|value| { - if let Expr::Tuple(ast::ExprTuple { elts, .. }) = value { - Left(elts.iter()) - } else { - Right(iter::once(*value)) - } + .map(|index| &values[*index]) + .map(|expr| { + let Expr::Call(ast::ExprCall { args, .. }) = expr else { + unreachable!("Indices should only contain `isinstance` calls") + }; + args.get(1).expect("`isinstance` should have two arguments") }) - .map(Clone::clone) - .collect(), - ctx: ExprContext::Load, - range: TextRange::default(), - }; - let node1 = ast::ExprName { - id: "isinstance".into(), - ctx: ExprContext::Load, - range: TextRange::default(), - }; - let node2 = ast::ExprCall { - func: Box::new(node1.into()), - args: vec![target.clone(), node.into()], - keywords: vec![], - range: TextRange::default(), - }; - let call = node2.into(); + .collect(); - // Generate the combined `BoolOp`. - let node = ast::ExprBoolOp { - op: Boolop::Or, - values: iter::once(call) - .chain( - values - .iter() - .enumerate() - .filter(|(index, _)| !indices.contains(index)) - .map(|(_, elt)| elt.clone()), - ) - .collect(), - range: TextRange::default(), - }; - let bool_op = node.into(); + // Generate a single `isinstance` call. + let node = ast::ExprTuple { + // Flatten all the types used across the `isinstance` calls. + elts: types + .iter() + .flat_map(|value| { + if let Expr::Tuple(ast::ExprTuple { elts, .. }) = value { + Left(elts.iter()) + } else { + Right(iter::once(*value)) + } + }) + .map(Clone::clone) + .collect(), + ctx: ExprContext::Load, + range: TextRange::default(), + }; + let node1 = ast::ExprName { + id: "isinstance".into(), + ctx: ExprContext::Load, + range: TextRange::default(), + }; + let node2 = ast::ExprCall { + func: Box::new(node1.into()), + args: vec![target.clone(), node.into()], + keywords: vec![], + range: TextRange::default(), + }; + let call = node2.into(); - // Populate the `Fix`. Replace the _entire_ `BoolOp`. Note that if we have - // multiple duplicates, the fixes will conflict. - #[allow(deprecated)] - diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( - checker.generator().expr(&bool_op), - expr.range(), - ))); + // Generate the combined `BoolOp`. + let node = ast::ExprBoolOp { + op: Boolop::Or, + values: iter::once(call) + .chain( + values + .iter() + .enumerate() + .filter(|(index, _)| !indices.contains(index)) + .map(|(_, elt)| elt.clone()), + ) + .collect(), + range: TextRange::default(), + }; + let bool_op = node.into(); + + // Populate the `Fix`. Replace the _entire_ `BoolOp`. Note that if we have + // multiple duplicates, the fixes will conflict. + #[allow(deprecated)] + diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( + checker.generator().expr(&bool_op), + expr.range(), + ))); + } } checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs b/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs index a78d8f6df6..05f3e41516 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs @@ -266,14 +266,6 @@ pub(crate) fn nested_if_statements( checker.locator, ); - // The fixer preserves comments in the nested body, but removes comments between - // the outer and inner if statements. - let nested_if = &body[0]; - let fixable = !has_comments_in( - TextRange::new(stmt.start(), nested_if.start()), - checker.locator, - ); - let mut diagnostic = Diagnostic::new( CollapsibleIf, colon.map_or_else( @@ -281,23 +273,31 @@ pub(crate) fn nested_if_statements( |colon| TextRange::new(stmt.start(), colon.end()), ), ); - if fixable && checker.patch(diagnostic.kind.rule()) { - match fix_if::fix_nested_if_statements(checker.locator, checker.stylist, stmt) { - Ok(edit) => { - if edit - .content() - .unwrap_or_default() - .universal_newlines() - .all(|line| { - LineWidth::new(checker.settings.tab_size).add_str(&line) - <= checker.settings.line_length - }) - { - #[allow(deprecated)] - diagnostic.set_fix(Fix::unspecified(edit)); + if checker.patch(diagnostic.kind.rule()) { + // The fixer preserves comments in the nested body, but removes comments between + // the outer and inner if statements. + let nested_if = &body[0]; + if !has_comments_in( + TextRange::new(stmt.start(), nested_if.start()), + checker.locator, + ) { + match fix_if::fix_nested_if_statements(checker.locator, checker.stylist, stmt) { + Ok(edit) => { + if edit + .content() + .unwrap_or_default() + .universal_newlines() + .all(|line| { + LineWidth::new(checker.settings.tab_size).add_str(&line) + <= checker.settings.line_length + }) + { + #[allow(deprecated)] + diagnostic.set_fix(Fix::unspecified(edit)); + } } + Err(err) => error!("Failed to fix nested if: {err}"), } - Err(err) => error!("Failed to fix nested if: {err}"), } } checker.diagnostics.push(diagnostic); @@ -351,48 +351,49 @@ pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) { } let condition = checker.generator().expr(test); - let fixable = matches!(if_return, Bool::True) - && matches!(else_return, Bool::False) - && !has_comments(stmt, checker.locator) - && (test.is_compare_expr() || checker.semantic_model().is_builtin("bool")); - let mut diagnostic = Diagnostic::new(NeedlessBool { condition }, stmt.range()); - if fixable && checker.patch(diagnostic.kind.rule()) { - if test.is_compare_expr() { - // If the condition is a comparison, we can replace it with the condition. - let node = ast::StmtReturn { - value: Some(test.clone()), - range: TextRange::default(), + if checker.patch(diagnostic.kind.rule()) { + if matches!(if_return, Bool::True) + && matches!(else_return, Bool::False) + && !has_comments(stmt, checker.locator) + && (test.is_compare_expr() || checker.semantic_model().is_builtin("bool")) + { + if test.is_compare_expr() { + // If the condition is a comparison, we can replace it with the condition. + let node = ast::StmtReturn { + value: Some(test.clone()), + range: TextRange::default(), + }; + #[allow(deprecated)] + diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( + checker.generator().stmt(&node.into()), + stmt.range(), + ))); + } else { + // Otherwise, we need to wrap the condition in a call to `bool`. (We've already + // verified, above, that `bool` is a builtin.) + let node = ast::ExprName { + id: "bool".into(), + ctx: ExprContext::Load, + range: TextRange::default(), + }; + let node1 = ast::ExprCall { + func: Box::new(node.into()), + args: vec![(**test).clone()], + keywords: vec![], + range: TextRange::default(), + }; + let node2 = ast::StmtReturn { + value: Some(Box::new(node1.into())), + range: TextRange::default(), + }; + #[allow(deprecated)] + diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( + checker.generator().stmt(&node2.into()), + stmt.range(), + ))); }; - #[allow(deprecated)] - diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( - checker.generator().stmt(&node.into()), - stmt.range(), - ))); - } else { - // Otherwise, we need to wrap the condition in a call to `bool`. (We've already - // verified, above, that `bool` is a builtin.) - let node = ast::ExprName { - id: "bool".into(), - ctx: ExprContext::Load, - range: TextRange::default(), - }; - let node1 = ast::ExprCall { - func: Box::new(node.into()), - args: vec![(**test).clone()], - keywords: vec![], - range: TextRange::default(), - }; - let node2 = ast::StmtReturn { - value: Some(Box::new(node1.into())), - range: TextRange::default(), - }; - #[allow(deprecated)] - diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( - checker.generator().stmt(&node2.into()), - stmt.range(), - ))); - }; + } } checker.diagnostics.push(diagnostic); } @@ -519,19 +520,20 @@ pub(crate) fn use_ternary_operator(checker: &mut Checker, stmt: &Stmt, parent: O return; } - let fixable = !has_comments(stmt, checker.locator); let mut diagnostic = Diagnostic::new( IfElseBlockInsteadOfIfExp { contents: contents.clone(), }, stmt.range(), ); - if fixable && checker.patch(diagnostic.kind.rule()) { - #[allow(deprecated)] - diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( - contents, - stmt.range(), - ))); + if checker.patch(diagnostic.kind.rule()) { + if !has_comments(stmt, checker.locator) { + #[allow(deprecated)] + diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( + contents, + stmt.range(), + ))); + } } checker.diagnostics.push(diagnostic); } @@ -875,19 +877,20 @@ pub(crate) fn use_dict_get_with_default( return; } - let fixable = !has_comments(stmt, checker.locator); let mut diagnostic = Diagnostic::new( IfElseBlockInsteadOfDictGet { contents: contents.clone(), }, stmt.range(), ); - if fixable && checker.patch(diagnostic.kind.rule()) { - #[allow(deprecated)] - diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( - contents, - stmt.range(), - ))); + if checker.patch(diagnostic.kind.rule()) { + if !has_comments(stmt, checker.locator) { + #[allow(deprecated)] + diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( + contents, + stmt.range(), + ))); + } } checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff/src/rules/flake8_simplify/rules/ast_with.rs b/crates/ruff/src/rules/flake8_simplify/rules/ast_with.rs index 5d8f3406d6..35a2bea786 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/ast_with.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/ast_with.rs @@ -89,10 +89,7 @@ pub(crate) fn multiple_with_statements( ), checker.locator, ); - let fixable = !has_comments_in( - TextRange::new(with_stmt.start(), with_body[0].start()), - checker.locator, - ); + let mut diagnostic = Diagnostic::new( MultipleWithStatements, colon.map_or_else( @@ -100,27 +97,32 @@ pub(crate) fn multiple_with_statements( |colon| TextRange::new(with_stmt.start(), colon.end()), ), ); - if fixable && checker.patch(diagnostic.kind.rule()) { - match fix_with::fix_multiple_with_statements( + if checker.patch(diagnostic.kind.rule()) { + if !has_comments_in( + TextRange::new(with_stmt.start(), with_body[0].start()), checker.locator, - checker.stylist, - with_stmt, ) { - Ok(edit) => { - if edit - .content() - .unwrap_or_default() - .universal_newlines() - .all(|line| { - LineWidth::new(checker.settings.tab_size).add_str(&line) - <= checker.settings.line_length - }) - { - #[allow(deprecated)] - diagnostic.set_fix(Fix::unspecified(edit)); + match fix_with::fix_multiple_with_statements( + checker.locator, + checker.stylist, + with_stmt, + ) { + Ok(edit) => { + if edit + .content() + .unwrap_or_default() + .universal_newlines() + .all(|line| { + LineWidth::new(checker.settings.tab_size).add_str(&line) + <= checker.settings.line_length + }) + { + #[allow(deprecated)] + diagnostic.set_fix(Fix::unspecified(edit)); + } } + Err(err) => error!("Failed to fix nested with: {err}"), } - Err(err) => error!("Failed to fix nested with: {err}"), } } checker.diagnostics.push(diagnostic); diff --git a/crates/ruff/src/rules/flake8_simplify/rules/suppressible_exception.rs b/crates/ruff/src/rules/flake8_simplify/rules/suppressible_exception.rs index ee1b45b68a..515ea39dcf 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/suppressible_exception.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/suppressible_exception.rs @@ -13,7 +13,6 @@ use crate::registry::AsRule; #[violation] pub struct SuppressibleException { exception: String, - fixable: bool, } impl Violation for SuppressibleException { @@ -21,12 +20,12 @@ impl Violation for SuppressibleException { #[derive_message_formats] fn message(&self) -> String { - let SuppressibleException { exception, .. } = self; + let SuppressibleException { exception } = self; format!("Use `contextlib.suppress({exception})` instead of `try`-`except`-`pass`") } fn autofix_title(&self) -> Option { - let SuppressibleException { exception, .. } = self; + let SuppressibleException { exception } = self; Some(format!("Replace with `contextlib.suppress({exception})`")) } } @@ -77,37 +76,36 @@ pub(crate) fn suppressible_exception( } else { handler_names.join(", ") }; - let fixable = !has_comments(stmt, checker.locator); + let mut diagnostic = Diagnostic::new( SuppressibleException { exception: exception.clone(), - fixable, }, stmt.range(), ); - - if fixable && checker.patch(diagnostic.kind.rule()) { - diagnostic.try_set_fix(|| { - let (import_edit, binding) = checker.importer.get_or_import_symbol( - "contextlib", - "suppress", - stmt.start(), - checker.semantic_model(), - )?; - let replace_try = Edit::range_replacement( - format!("with {binding}({exception})"), - TextRange::at(stmt.start(), "try".text_len()), - ); - let handler_line_begin = checker.locator.line_start(handler.start()); - let remove_handler = Edit::deletion(handler_line_begin, handler.end()); - #[allow(deprecated)] - Ok(Fix::unspecified_edits( - import_edit, - [replace_try, remove_handler], - )) - }); + if checker.patch(diagnostic.kind.rule()) { + if !has_comments(stmt, checker.locator) { + diagnostic.try_set_fix(|| { + let (import_edit, binding) = checker.importer.get_or_import_symbol( + "contextlib", + "suppress", + stmt.start(), + checker.semantic_model(), + )?; + let replace_try = Edit::range_replacement( + format!("with {binding}({exception})"), + TextRange::at(stmt.start(), "try".text_len()), + ); + let handler_line_begin = checker.locator.line_start(handler.start()); + let remove_handler = Edit::deletion(handler_line_begin, handler.end()); + #[allow(deprecated)] + Ok(Fix::unspecified_edits( + import_edit, + [replace_try, remove_handler], + )) + }); + } } - checker.diagnostics.push(diagnostic); } } diff --git a/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs b/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs index bbb0c3e9a0..4785b5866c 100644 --- a/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs +++ b/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs @@ -92,29 +92,31 @@ pub(crate) fn inplace_argument( _ => false, }; if is_true_literal { - // Avoid applying the fix if: - // 1. The keyword argument is followed by a star argument (we can't be certain that - // the star argument _doesn't_ contain an override). - // 2. The call is part of a larger expression (we're converting an expression to a - // statement, and expressions can't contain statements). - // 3. The call is in a lambda (we can't assign to a variable in a lambda). This - // should be unnecessary, as lambdas are expressions, and so (2) should apply, - // but we don't currently restore expression stacks when parsing deferred nodes, - // and so the parent is lost. - let fixable = !seen_star - && checker.semantic_model().stmt().is_expr_stmt() - && checker.semantic_model().expr_parent().is_none() - && !checker.semantic_model().scope().kind.is_lambda(); let mut diagnostic = Diagnostic::new(PandasUseOfInplaceArgument, keyword.range()); - if fixable && checker.patch(diagnostic.kind.rule()) { - if let Some(fix) = convert_inplace_argument_to_assignment( - checker.locator, - expr, - diagnostic.range(), - args, - keywords, - ) { - diagnostic.set_fix(fix); + if checker.patch(diagnostic.kind.rule()) { + // Avoid applying the fix if: + // 1. The keyword argument is followed by a star argument (we can't be certain that + // the star argument _doesn't_ contain an override). + // 2. The call is part of a larger expression (we're converting an expression to a + // statement, and expressions can't contain statements). + // 3. The call is in a lambda (we can't assign to a variable in a lambda). This + // should be unnecessary, as lambdas are expressions, and so (2) should apply, + // but we don't currently restore expression stacks when parsing deferred nodes, + // and so the parent is lost. + if !seen_star + && checker.semantic_model().stmt().is_expr_stmt() + && checker.semantic_model().expr_parent().is_none() + && !checker.semantic_model().scope().kind.is_lambda() + { + if let Some(fix) = convert_inplace_argument_to_assignment( + checker.locator, + expr, + diagnostic.range(), + args, + keywords, + ) { + diagnostic.set_fix(fix); + } } } diff --git a/crates/ruff/src/rules/pylint/rules/manual_import_from.rs b/crates/ruff/src/rules/pylint/rules/manual_import_from.rs index 2274a3e435..5201bc906d 100644 --- a/crates/ruff/src/rules/pylint/rules/manual_import_from.rs +++ b/crates/ruff/src/rules/pylint/rules/manual_import_from.rs @@ -64,7 +64,6 @@ pub(crate) fn manual_from_import( return; } - let fixable = names.len() == 1; let mut diagnostic = Diagnostic::new( ManualFromImport { module: module.to_string(), @@ -72,22 +71,24 @@ pub(crate) fn manual_from_import( }, alias.range(), ); - if fixable && checker.patch(diagnostic.kind.rule()) { - let node = ast::StmtImportFrom { - module: Some(module.into()), - names: vec![Alias { - name: asname.clone(), - asname: None, + if checker.patch(diagnostic.kind.rule()) { + if names.len() == 1 { + let node = ast::StmtImportFrom { + module: Some(module.into()), + names: vec![Alias { + name: asname.clone(), + asname: None, + range: TextRange::default(), + }], + level: Some(Int::new(0)), range: TextRange::default(), - }], - level: Some(Int::new(0)), - range: TextRange::default(), - }; - #[allow(deprecated)] - diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( - checker.generator().stmt(&node.into()), - stmt.range(), - ))); + }; + #[allow(deprecated)] + diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( + checker.generator().stmt(&node.into()), + stmt.range(), + ))); + } } checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff/src/rules/pylint/rules/nested_min_max.rs b/crates/ruff/src/rules/pylint/rules/nested_min_max.rs index b4b3418cf9..3c5aee7852 100644 --- a/crates/ruff/src/rules/pylint/rules/nested_min_max.rs +++ b/crates/ruff/src/rules/pylint/rules/nested_min_max.rs @@ -145,20 +145,21 @@ pub(crate) fn nested_min_max( MinMax::try_from_call(func.as_ref(), keywords.as_ref(), checker.semantic_model()) == Some(min_max) }) { - let fixable = !has_comments(expr, checker.locator); let mut diagnostic = Diagnostic::new(NestedMinMax { func: min_max }, expr.range()); - if fixable && checker.patch(diagnostic.kind.rule()) { - let flattened_expr = Expr::Call(ast::ExprCall { - func: Box::new(func.clone()), - args: collect_nested_args(checker.semantic_model(), min_max, args), - keywords: keywords.to_owned(), - range: TextRange::default(), - }); - #[allow(deprecated)] - diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( - checker.generator().expr(&flattened_expr), - expr.range(), - ))); + if checker.patch(diagnostic.kind.rule()) { + if !has_comments(expr, checker.locator) { + let flattened_expr = Expr::Call(ast::ExprCall { + func: Box::new(func.clone()), + args: collect_nested_args(checker.semantic_model(), min_max, args), + keywords: keywords.to_owned(), + range: TextRange::default(), + }); + #[allow(deprecated)] + diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( + checker.generator().expr(&flattened_expr), + expr.range(), + ))); + } } checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff/src/rules/pyupgrade/rules/convert_named_tuple_functional_to_class.rs b/crates/ruff/src/rules/pyupgrade/rules/convert_named_tuple_functional_to_class.rs index f5f386492b..d2de8fb2b8 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/convert_named_tuple_functional_to_class.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/convert_named_tuple_functional_to_class.rs @@ -198,22 +198,24 @@ pub(crate) fn convert_named_tuple_functional_to_class( return; } }; - // TODO(charlie): Preserve indentation, to remove the first-column requirement. - let fixable = checker.locator.is_at_start_of_line(stmt.start()); + let mut diagnostic = Diagnostic::new( ConvertNamedTupleFunctionalToClass { name: typename.to_string(), }, stmt.range(), ); - if fixable && checker.patch(diagnostic.kind.rule()) { - diagnostic.set_fix(convert_to_class( - stmt, - typename, - properties, - base_class, - checker.generator(), - )); + if checker.patch(diagnostic.kind.rule()) { + // TODO(charlie): Preserve indentation, to remove the first-column requirement. + if checker.locator.is_at_start_of_line(stmt.start()) { + diagnostic.set_fix(convert_to_class( + stmt, + typename, + properties, + base_class, + checker.generator(), + )); + } } checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs b/crates/ruff/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs index 3e51f00c6c..e1906fa880 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs @@ -15,7 +15,6 @@ use crate::registry::AsRule; #[violation] pub struct ConvertTypedDictFunctionalToClass { name: String, - fixable: bool, } impl Violation for ConvertTypedDictFunctionalToClass { @@ -23,12 +22,12 @@ impl Violation for ConvertTypedDictFunctionalToClass { #[derive_message_formats] fn message(&self) -> String { - let ConvertTypedDictFunctionalToClass { name, .. } = self; + let ConvertTypedDictFunctionalToClass { name } = self; format!("Convert `{name}` from `TypedDict` functional to class syntax") } fn autofix_title(&self) -> Option { - let ConvertTypedDictFunctionalToClass { name, .. } = self; + let ConvertTypedDictFunctionalToClass { name } = self; Some(format!("Convert `{name}` to class syntax")) } } @@ -252,24 +251,25 @@ pub(crate) fn convert_typed_dict_functional_to_class( return; } }; - // TODO(charlie): Preserve indentation, to remove the first-column requirement. - let fixable = checker.locator.is_at_start_of_line(stmt.start()); + let mut diagnostic = Diagnostic::new( ConvertTypedDictFunctionalToClass { name: class_name.to_string(), - fixable, }, stmt.range(), ); - if fixable && checker.patch(diagnostic.kind.rule()) { - diagnostic.set_fix(convert_to_class( - stmt, - class_name, - body, - total_keyword, - base_class, - checker.generator(), - )); + if checker.patch(diagnostic.kind.rule()) { + // TODO(charlie): Preserve indentation, to remove the first-column requirement. + if checker.locator.is_at_start_of_line(stmt.start()) { + diagnostic.set_fix(convert_to_class( + stmt, + class_name, + body, + total_keyword, + base_class, + checker.generator(), + )); + } } checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff/src/rules/pyupgrade/rules/use_pep585_annotation.rs b/crates/ruff/src/rules/pyupgrade/rules/use_pep585_annotation.rs index 4f1703bca9..071b3b3de1 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/use_pep585_annotation.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/use_pep585_annotation.rs @@ -45,30 +45,31 @@ pub(crate) fn use_pep585_annotation( }, expr.range(), ); - let fixable = !checker.semantic_model().in_complex_string_type_definition(); - if fixable && checker.patch(diagnostic.kind.rule()) { - match replacement { - ModuleMember::BuiltIn(name) => { - // Built-in type, like `list`. - if checker.semantic_model().is_builtin(name) { - diagnostic.set_fix(Fix::automatic(Edit::range_replacement( - (*name).to_string(), - expr.range(), - ))); + if checker.patch(diagnostic.kind.rule()) { + if !checker.semantic_model().in_complex_string_type_definition() { + match replacement { + ModuleMember::BuiltIn(name) => { + // Built-in type, like `list`. + if checker.semantic_model().is_builtin(name) { + diagnostic.set_fix(Fix::automatic(Edit::range_replacement( + (*name).to_string(), + expr.range(), + ))); + } + } + ModuleMember::Member(module, member) => { + // Imported type, like `collections.deque`. + diagnostic.try_set_fix(|| { + let (import_edit, binding) = checker.importer.get_or_import_symbol( + module, + member, + expr.start(), + checker.semantic_model(), + )?; + let reference_edit = Edit::range_replacement(binding, expr.range()); + Ok(Fix::suggested_edits(import_edit, [reference_edit])) + }); } - } - ModuleMember::Member(module, member) => { - // Imported type, like `collections.deque`. - diagnostic.try_set_fix(|| { - let (import_edit, binding) = checker.importer.get_or_import_symbol( - module, - member, - expr.start(), - checker.semantic_model(), - )?; - let reference_edit = Edit::range_replacement(binding, expr.range()); - Ok(Fix::suggested_edits(import_edit, [reference_edit])) - }); } } }