Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add settings for promoting and demoting fixes #7841

Merged
merged 7 commits into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

124 changes: 121 additions & 3 deletions crates/ruff_cli/tests/integration_test.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
#![cfg(not(target_family = "wasm"))]

#[cfg(unix)]
use std::fs;
#[cfg(unix)]
use std::fs::Permissions;
Expand All @@ -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)]
Expand Down Expand Up @@ -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"]
"#,
)?;
Comment on lines +1214 to +1221
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied this testing strategy from the formatter integration tests. Do we have a better place to do this? I'd test more cases (e.g. prefixes, rules in both sets, etc.)


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(())
}
1 change: 1 addition & 0 deletions crates/ruff_diagnostics/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [] }
9 changes: 8 additions & 1 deletion crates/ruff_diagnostics/src/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
}
25 changes: 24 additions & 1 deletion crates/ruff_linter/src/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
zanieb marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}

LinterResult::new((diagnostics, imports), error)
}

Expand Down
4 changes: 4 additions & 0 deletions crates/ruff_linter/src/settings/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
38 changes: 38 additions & 0 deletions crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),

Expand Down Expand Up @@ -494,6 +516,10 @@ pub struct LintConfiguration {
pub rule_selections: Vec<RuleSelection>,
pub explicit_preview_rules: Option<bool>,

// Fix configuration
pub extend_unsafe_fixes: Vec<RuleSelector>,
pub extend_safe_fixes: Vec<RuleSelector>,

// Global lint settings
pub allowed_confusables: Option<Vec<char>>,
pub dummy_variable_rgx: Option<Regex>,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
24 changes: 24 additions & 0 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,30 @@ pub struct LintOptions {
)]
pub ignore: Option<Vec<RuleSelector>>,

/// 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<Vec<RuleSelector>>,

/// 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<Vec<RuleSelector>>,

/// 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
Expand Down
Loading
Loading