From 739a8aa10e61c9a89e4a2b3b3d1ed06a244fc602 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Tue, 10 Oct 2023 15:04:21 -0500 Subject: [PATCH] Add settings for promoting and demoting fixes (#7841) Adds two configuration-file only settings `extend-safe-fixes` and `extend-unsafe-fixes` which can be used to promote and demote the applicability of fixes for rules. Fixes with `Never` applicability cannot be promoted. --- Cargo.lock | 1 + crates/ruff_cli/tests/integration_test.rs | 124 ++++++++++++++++++++- crates/ruff_diagnostics/Cargo.toml | 1 + crates/ruff_diagnostics/src/fix.rs | 9 +- crates/ruff_linter/src/linter.rs | 25 ++++- crates/ruff_linter/src/settings/mod.rs | 4 + crates/ruff_workspace/src/configuration.rs | 38 +++++++ crates/ruff_workspace/src/options.rs | 24 ++++ ruff.schema.json | 40 +++++++ 9 files changed, 261 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8a0a8f7036563..9b38e0590a155 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2143,6 +2143,7 @@ name = "ruff_diagnostics" version = "0.0.0" dependencies = [ "anyhow", + "is-macro", "log", "ruff_text_size", "serde", diff --git a/crates/ruff_cli/tests/integration_test.rs b/crates/ruff_cli/tests/integration_test.rs index caffc6a97c856..b083aa7a55487 100644 --- a/crates/ruff_cli/tests/integration_test.rs +++ b/crates/ruff_cli/tests/integration_test.rs @@ -1,6 +1,5 @@ #![cfg(not(target_family = "wasm"))] -#[cfg(unix)] use std::fs; #[cfg(unix)] use std::fs::Permissions; @@ -12,13 +11,13 @@ use std::process::Command; use std::str; #[cfg(unix)] -use anyhow::{Context, Result}; +use anyhow::Context; +use anyhow::Result; #[cfg(unix)] use clap::Parser; use insta_cmd::{assert_cmd_snapshot, get_cargo_bin}; #[cfg(unix)] use path_absolutize::path_dedot; -#[cfg(unix)] use tempfile::TempDir; #[cfg(unix)] @@ -1208,3 +1207,122 @@ fn diff_only_unsafe_fixes_available() { "### ); } + +#[test] +fn check_extend_unsafe_fixes() -> Result<()> { + let tempdir = TempDir::new()?; + let ruff_toml = tempdir.path().join("ruff.toml"); + fs::write( + &ruff_toml, + r#" +[lint] +extend-unsafe-fixes = ["UP034"] +"#, + )?; + + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(["check", "--config"]) + .arg(&ruff_toml) + .arg("-") + .args([ + "--output-format", + "text", + "--no-cache", + "--select", + "F601,UP034", + ]) + .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + -:1:14: F601 Dictionary key literal `'a'` repeated + -:2:7: UP034 Avoid extraneous parentheses + Found 2 errors. + 2 hidden fixes can be enabled with the `--unsafe-fixes` option. + + ----- stderr ----- + "###); + + Ok(()) +} + +#[test] +fn check_extend_safe_fixes() -> Result<()> { + let tempdir = TempDir::new()?; + let ruff_toml = tempdir.path().join("ruff.toml"); + fs::write( + &ruff_toml, + r#" +[lint] +extend-safe-fixes = ["F601"] +"#, + )?; + + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(["check", "--config"]) + .arg(&ruff_toml) + .arg("-") + .args([ + "--output-format", + "text", + "--no-cache", + "--select", + "F601,UP034", + ]) + .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + -:1:14: F601 [*] Dictionary key literal `'a'` repeated + -:2:7: UP034 [*] Avoid extraneous parentheses + Found 2 errors. + [*] 2 fixable with the `--fix` option. + + ----- stderr ----- + "###); + + Ok(()) +} + +#[test] +fn check_extend_unsafe_fixes_conflict_with_extend_safe_fixes() -> Result<()> { + // Adding a rule to both options should result in it being treated as unsafe + let tempdir = TempDir::new()?; + let ruff_toml = tempdir.path().join("ruff.toml"); + fs::write( + &ruff_toml, + r#" +[lint] +extend-unsafe-fixes = ["UP034"] +extend-safe-fixes = ["UP034"] +"#, + )?; + + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(["check", "--config"]) + .arg(&ruff_toml) + .arg("-") + .args([ + "--output-format", + "text", + "--no-cache", + "--select", + "F601,UP034", + ]) + .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + -:1:14: F601 Dictionary key literal `'a'` repeated + -:2:7: UP034 Avoid extraneous parentheses + Found 2 errors. + 2 hidden fixes can be enabled with the `--unsafe-fixes` option. + + ----- stderr ----- + "###); + + Ok(()) +} diff --git a/crates/ruff_diagnostics/Cargo.toml b/crates/ruff_diagnostics/Cargo.toml index 4d548f41b6292..9a2e22e2340fb 100644 --- a/crates/ruff_diagnostics/Cargo.toml +++ b/crates/ruff_diagnostics/Cargo.toml @@ -17,4 +17,5 @@ ruff_text_size = { path = "../ruff_text_size" } anyhow = { workspace = true } log = { workspace = true } +is-macro = { workspace = true } serde = { workspace = true, optional = true, features = [] } diff --git a/crates/ruff_diagnostics/src/fix.rs b/crates/ruff_diagnostics/src/fix.rs index 5870ffe8f7591..befb5c1c5650d 100644 --- a/crates/ruff_diagnostics/src/fix.rs +++ b/crates/ruff_diagnostics/src/fix.rs @@ -6,7 +6,7 @@ use ruff_text_size::{Ranged, TextSize}; use crate::edit::Edit; /// Indicates if a fix can be applied. -#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)] +#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord, is_macro::Is)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[cfg_attr(feature = "serde", serde(rename_all = "lowercase"))] pub enum Applicability { @@ -138,4 +138,11 @@ impl Fix { pub fn applies(&self, applicability: Applicability) -> bool { self.applicability >= applicability } + + /// Create a new [`Fix`] with the given [`Applicability`]. + #[must_use] + pub fn with_applicability(mut self, applicability: Applicability) -> Self { + self.applicability = applicability; + self + } } diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 9219bfa8ef774..8f1dd281c1144 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -8,7 +8,7 @@ use itertools::Itertools; use log::error; use rustc_hash::FxHashMap; -use ruff_diagnostics::Diagnostic; +use ruff_diagnostics::{Applicability, Diagnostic}; use ruff_python_ast::imports::ImportMap; use ruff_python_ast::PySourceType; use ruff_python_codegen::Stylist; @@ -260,6 +260,29 @@ pub fn check_path( } } + // Update fix applicability to account for overrides + if !settings.extend_safe_fixes.is_empty() || !settings.extend_unsafe_fixes.is_empty() { + for diagnostic in &mut diagnostics { + if let Some(fix) = diagnostic.fix.take() { + // Enforce demotions over promotions so if someone puts a rule in both we are conservative + if fix.applicability().is_safe() + && settings + .extend_unsafe_fixes + .contains(diagnostic.kind.rule()) + { + diagnostic.set_fix(fix.with_applicability(Applicability::Unsafe)); + } else if fix.applicability().is_unsafe() + && settings.extend_safe_fixes.contains(diagnostic.kind.rule()) + { + diagnostic.set_fix(fix.with_applicability(Applicability::Safe)); + } else { + // Retain the existing fix (will be dropped from `.take()` otherwise) + diagnostic.set_fix(fix); + } + } + } + } + LinterResult::new((diagnostics, imports), error) } diff --git a/crates/ruff_linter/src/settings/mod.rs b/crates/ruff_linter/src/settings/mod.rs index 881611cfc8d52..0d12ad2f76402 100644 --- a/crates/ruff_linter/src/settings/mod.rs +++ b/crates/ruff_linter/src/settings/mod.rs @@ -42,6 +42,8 @@ pub struct LinterSettings { pub rules: RuleTable, pub per_file_ignores: Vec<(GlobMatcher, GlobMatcher, RuleSet)>, + pub extend_unsafe_fixes: RuleSet, + pub extend_safe_fixes: RuleSet, pub target_version: PythonVersion, pub preview: PreviewMode, @@ -139,6 +141,8 @@ impl LinterSettings { namespace_packages: vec![], per_file_ignores: vec![], + extend_safe_fixes: RuleSet::empty(), + extend_unsafe_fixes: RuleSet::empty(), src: vec![path_dedot::CWD.clone()], // Needs duplicating diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 629b2c913bc23..fb72b595246ae 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -226,6 +226,28 @@ impl Configuration { .chain(lint.extend_per_file_ignores) .collect(), )?, + + extend_safe_fixes: lint + .extend_safe_fixes + .iter() + .flat_map(|selector| { + selector.rules(&PreviewOptions { + mode: preview, + require_explicit: false, + }) + }) + .collect(), + extend_unsafe_fixes: lint + .extend_unsafe_fixes + .iter() + .flat_map(|selector| { + selector.rules(&PreviewOptions { + mode: preview, + require_explicit: false, + }) + }) + .collect(), + src: self.src.unwrap_or_else(|| vec![project_root.to_path_buf()]), explicit_preview_rules: lint.explicit_preview_rules.unwrap_or_default(), @@ -494,6 +516,10 @@ pub struct LintConfiguration { pub rule_selections: Vec, pub explicit_preview_rules: Option, + // Fix configuration + pub extend_unsafe_fixes: Vec, + pub extend_safe_fixes: Vec, + // Global lint settings pub allowed_confusables: Option>, pub dummy_variable_rgx: Option, @@ -551,6 +577,8 @@ impl LintConfiguration { .collect(), extend_fixable: options.extend_fixable.unwrap_or_default(), }], + extend_safe_fixes: options.extend_safe_fixes.unwrap_or_default(), + extend_unsafe_fixes: options.extend_unsafe_fixes.unwrap_or_default(), allowed_confusables: options.allowed_confusables, dummy_variable_rgx: options .dummy_variable_rgx @@ -847,6 +875,16 @@ impl LintConfiguration { .into_iter() .chain(self.rule_selections) .collect(), + extend_safe_fixes: config + .extend_safe_fixes + .into_iter() + .chain(self.extend_safe_fixes) + .collect(), + extend_unsafe_fixes: config + .extend_unsafe_fixes + .into_iter() + .chain(self.extend_unsafe_fixes) + .collect(), allowed_confusables: self.allowed_confusables.or(config.allowed_confusables), dummy_variable_rgx: self.dummy_variable_rgx.or(config.dummy_variable_rgx), extend_per_file_ignores: config diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index d90219818cbf0..476d0c7898901 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -523,6 +523,30 @@ pub struct LintOptions { )] pub ignore: Option>, + /// A list of rule codes or prefixes for which unsafe fixes should be considered + /// safe. + #[option( + default = "[]", + value_type = "list[RuleSelector]", + example = r#" + # Allow applying all unsafe fixes in the `E` rules and `F401` without the `--unsafe-fixes` flag + extend_safe_fixes = ["E", "F401"] + "# + )] + pub extend_safe_fixes: Option>, + + /// A list of rule codes or prefixes for which safe fixes should be considered + /// unsafe. + #[option( + default = "[]", + value_type = "list[RuleSelector]", + example = r#" + # Require the `--unsafe-fixes` flag when fixing the `E` rules and `F401` + extend_unsafe_fixes = ["E", "F401"] + "# + )] + pub extend_unsafe_fixes: Option>, + /// Avoid automatically removing unused imports in `__init__.py` files. Such /// imports will still be flagged, but with a dedicated message suggesting /// that the import is either added to the module's `__all__` symbol, or diff --git a/ruff.schema.json b/ruff.schema.json index b5a43347b21e0..df646ec363f9f 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -107,6 +107,16 @@ } } }, + "extend-safe-fixes": { + "description": "A list of rule codes or prefixes for which unsafe fixes should be considered safe.", + "type": [ + "array", + "null" + ], + "items": { + "$ref": "#/definitions/RuleSelector" + } + }, "extend-select": { "description": "A list of rule codes or prefixes to enable, in addition to those specified by `select`.", "type": [ @@ -117,6 +127,16 @@ "$ref": "#/definitions/RuleSelector" } }, + "extend-unsafe-fixes": { + "description": "A list of rule codes or prefixes for which safe fixes should be considered unsafe.", + "type": [ + "array", + "null" + ], + "items": { + "$ref": "#/definitions/RuleSelector" + } + }, "external": { "description": "A list of rule codes that are unsupported by Ruff, but should be preserved when (e.g.) validating `# noqa` directives. Useful for retaining `# noqa` directives that cover plugins not yet implemented by Ruff.", "type": [ @@ -1612,6 +1632,16 @@ } } }, + "extend-safe-fixes": { + "description": "A list of rule codes or prefixes for which unsafe fixes should be considered safe.", + "type": [ + "array", + "null" + ], + "items": { + "$ref": "#/definitions/RuleSelector" + } + }, "extend-select": { "description": "A list of rule codes or prefixes to enable, in addition to those specified by `select`.", "type": [ @@ -1622,6 +1652,16 @@ "$ref": "#/definitions/RuleSelector" } }, + "extend-unsafe-fixes": { + "description": "A list of rule codes or prefixes for which safe fixes should be considered unsafe.", + "type": [ + "array", + "null" + ], + "items": { + "$ref": "#/definitions/RuleSelector" + } + }, "external": { "description": "A list of rule codes that are unsupported by Ruff, but should be preserved when (e.g.) validating `# noqa` directives. Useful for retaining `# noqa` directives that cover plugins not yet implemented by Ruff.", "type": [