From 706d87f239cb3af6562581d221bf6cd1f1636b66 Mon Sep 17 00:00:00 2001 From: Dylan <53534755+dylwil3@users.noreply.github.com> Date: Fri, 3 Jan 2025 08:50:13 -0600 Subject: [PATCH] Show errors for attempted fixes only when passed `--verbose` (#15237) The default logging level for diagnostics includes logs written using the `log` crate with level `error`, `warn`, and `info`. An unsuccessful fix attached to a diagnostic via `try_set_fix` or `try_set_optional_fix` was logged at level `error`. Note that the user would see these messages even without passing `--fix`, and possibly also on lines with `noqa` comments. This PR changes the logging level here to a `debug`. We also found ad-hoc instances of error logging in the implementations of several rules, and have replaced those with either a `debug` or call to `try_set{_optional}_fix`. Closes #15229 --- crates/ruff/tests/integration_test.rs | 64 +++++++++++++++++++ crates/ruff_diagnostics/src/diagnostic.rs | 6 +- .../rules/flake8_simplify/rules/ast_with.rs | 42 ++++++------ .../flake8_simplify/rules/collapsible_if.rs | 33 +++++----- .../rules/invalid_literal_comparisons.rs | 39 ++++++----- .../pyupgrade/rules/deprecated_mock_import.rs | 4 +- 6 files changed, 132 insertions(+), 56 deletions(-) diff --git a/crates/ruff/tests/integration_test.rs b/crates/ruff/tests/integration_test.rs index bae45dd22f78a..6001cc90452bd 100644 --- a/crates/ruff/tests/integration_test.rs +++ b/crates/ruff/tests/integration_test.rs @@ -2126,3 +2126,67 @@ unfixable = ["RUF"] Ok(()) } + +#[test] +fn verbose_show_failed_fix_errors() { + let mut cmd = RuffCheck::default() + .args(["--select", "UP006", "--preview", "-v"]) + .build(); + + insta::with_settings!( + { + // the logs have timestamps we need to remove + filters => vec![( + r"\[[\d:-]+]", + "" + )] + },{ + assert_cmd_snapshot!(cmd + .pass_stdin("import typing\nCallable = 'abc'\ndef g() -> typing.Callable: ..."), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + -:3:12: UP006 Use `collections.abc.Callable` instead of `typing.Callable` for type annotation + | + 1 | import typing + 2 | Callable = 'abc' + 3 | def g() -> typing.Callable: ... + | ^^^^^^^^^^^^^^^ UP006 + | + = help: Replace with `collections.abc.Callable` + + Found 1 error. + + ----- stderr ----- + [ruff::resolve][DEBUG] Isolated mode, not reading any pyproject.toml + [ruff_diagnostics::diagnostic][DEBUG] Failed to create fix for NonPEP585Annotation: Unable to insert `Callable` into scope due to name conflict + "###); } + ); +} + +#[test] +fn no_verbose_hide_failed_fix_errors() { + let mut cmd = RuffCheck::default() + .args(["--select", "UP006", "--preview"]) + .build(); + assert_cmd_snapshot!(cmd + .pass_stdin("import typing\nCallable = 'abc'\ndef g() -> typing.Callable: ..."), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + -:3:12: UP006 Use `collections.abc.Callable` instead of `typing.Callable` for type annotation + | + 1 | import typing + 2 | Callable = 'abc' + 3 | def g() -> typing.Callable: ... + | ^^^^^^^^^^^^^^^ UP006 + | + = help: Replace with `collections.abc.Callable` + + Found 1 error. + + ----- stderr ----- + "###); +} diff --git a/crates/ruff_diagnostics/src/diagnostic.rs b/crates/ruff_diagnostics/src/diagnostic.rs index 84da5f3904607..be54a8de0e247 100644 --- a/crates/ruff_diagnostics/src/diagnostic.rs +++ b/crates/ruff_diagnostics/src/diagnostic.rs @@ -1,5 +1,5 @@ use anyhow::Result; -use log::error; +use log::debug; #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; @@ -56,7 +56,7 @@ impl Diagnostic { pub fn try_set_fix(&mut self, func: impl FnOnce() -> Result) { match func() { Ok(fix) => self.fix = Some(fix), - Err(err) => error!("Failed to create fix for {}: {}", self.kind.name, err), + Err(err) => debug!("Failed to create fix for {}: {}", self.kind.name, err), } } @@ -67,7 +67,7 @@ impl Diagnostic { match func() { Ok(None) => {} Ok(Some(fix)) => self.fix = Some(fix), - Err(err) => error!("Failed to create fix for {}: {}", self.kind.name, err), + Err(err) => debug!("Failed to create fix for {}: {}", self.kind.name, err), } } diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs index 7bfe65c9295b6..906aff0c25a53 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs @@ -1,5 +1,5 @@ +use anyhow::bail; use ast::Expr; -use log::error; use ruff_diagnostics::{Diagnostic, Fix}; use ruff_diagnostics::{FixAvailability, Violation}; @@ -170,26 +170,30 @@ pub(crate) fn multiple_with_statements( .comment_ranges() .intersects(TextRange::new(with_stmt.start(), with_stmt.body[0].start())) { - match fix_with::fix_multiple_with_statements( - checker.locator(), - checker.stylist(), - with_stmt, - ) { - Ok(edit) => { - if edit.content().map_or(true, |content| { - fits( - content, - with_stmt.into(), - checker.locator(), - checker.settings.pycodestyle.max_line_length, - checker.settings.tab_size, - ) - }) { - diagnostic.set_fix(Fix::unsafe_edit(edit)); + diagnostic.try_set_optional_fix(|| { + match fix_with::fix_multiple_with_statements( + checker.locator(), + checker.stylist(), + with_stmt, + ) { + Ok(edit) => { + if edit.content().map_or(true, |content| { + fits( + content, + with_stmt.into(), + checker.locator(), + checker.settings.pycodestyle.max_line_length, + checker.settings.tab_size, + ) + }) { + Ok(Some(Fix::unsafe_edit(edit))) + } else { + Ok(None) + } } + Err(err) => bail!("Failed to collapse `with`: {err}"), } - Err(err) => error!("Failed to fix nested with: {err}"), - } + }); } checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/collapsible_if.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/collapsible_if.rs index c32ced436b524..6ede86a536585 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/collapsible_if.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/collapsible_if.rs @@ -2,7 +2,6 @@ use std::borrow::Cow; use anyhow::{bail, Result}; use libcst_native::ParenthesizedNode; -use log::error; use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; @@ -118,22 +117,26 @@ pub(crate) fn nested_if_statements( nested_if.start(), nested_if.body()[0].start(), )) { - match collapse_nested_if(checker.locator(), checker.stylist(), nested_if) { - Ok(edit) => { - if edit.content().map_or(true, |content| { - fits( - content, - (&nested_if).into(), - checker.locator(), - checker.settings.pycodestyle.max_line_length, - checker.settings.tab_size, - ) - }) { - diagnostic.set_fix(Fix::unsafe_edit(edit)); + diagnostic.try_set_optional_fix(|| { + match collapse_nested_if(checker.locator(), checker.stylist(), nested_if) { + Ok(edit) => { + if edit.content().map_or(true, |content| { + fits( + content, + (&nested_if).into(), + checker.locator(), + checker.settings.pycodestyle.max_line_length, + checker.settings.tab_size, + ) + }) { + Ok(Some(Fix::unsafe_edit(edit))) + } else { + Ok(None) + } } + Err(err) => bail!("Failed to collapse `if`: {err}"), } - Err(err) => error!("Failed to fix nested if: {err}"), - } + }); } checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/invalid_literal_comparisons.rs b/crates/ruff_linter/src/rules/pyflakes/rules/invalid_literal_comparisons.rs index 955def2f9a892..71c10a8adf76d 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/invalid_literal_comparisons.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/invalid_literal_comparisons.rs @@ -1,4 +1,4 @@ -use log::error; +use anyhow::{bail, Error}; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, ViolationMetadata}; @@ -94,24 +94,29 @@ pub(crate) fn invalid_literal_comparison( if lazy_located.is_none() { lazy_located = Some(locate_cmp_ops(expr, checker.tokens())); } - if let Some(located_op) = lazy_located.as_ref().and_then(|located| located.get(index)) { - assert_eq!(located_op.op, *op); - if let Some(content) = match located_op.op { - CmpOp::Is => Some("==".to_string()), - CmpOp::IsNot => Some("!=".to_string()), - node => { - error!("Failed to fix invalid comparison: {node:?}"); - None + diagnostic.try_set_optional_fix(|| { + if let Some(located_op) = + lazy_located.as_ref().and_then(|located| located.get(index)) + { + assert_eq!(located_op.op, *op); + if let Ok(content) = match located_op.op { + CmpOp::Is => Ok::("==".to_string()), + CmpOp::IsNot => Ok("!=".to_string()), + node => { + bail!("Failed to fix invalid comparison: {node:?}") + } + } { + Ok(Some(Fix::safe_edit(Edit::range_replacement( + content, + located_op.range, + )))) + } else { + Ok(None) } - } { - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( - content, - located_op.range, - ))); + } else { + bail!("Failed to fix invalid comparison due to missing op") } - } else { - error!("Failed to fix invalid comparison due to missing op"); - } + }); checker.diagnostics.push(diagnostic); } left = right; diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_mock_import.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_mock_import.rs index 4a0c6998e3eac..92268c8ee7702 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_mock_import.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_mock_import.rs @@ -3,7 +3,7 @@ use libcst_native::{ AsName, AssignTargetExpression, Attribute, Dot, Expression, Import, ImportAlias, ImportFrom, ImportNames, Name, NameOrAttribute, ParenthesizableWhitespace, }; -use log::error; +use log::debug; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, ViolationMetadata}; @@ -286,7 +286,7 @@ pub(crate) fn deprecated_mock_import(checker: &mut Checker, stmt: &Stmt) { match format_import(stmt, indent, checker.locator(), checker.stylist()) { Ok(content) => Some(content), Err(e) => { - error!("Failed to rewrite `mock` import: {e}"); + debug!("Failed to rewrite `mock` import: {e}"); None } }