Skip to content

Commit

Permalink
Rename applicability levels to always, sometimes, and never (#7821)
Browse files Browse the repository at this point in the history
Following much discussion for #4181 at
#5119,
#5476, #7769,
#7819, and in
[Discord](https://discord.com/channels/1039017663004942429/1082324250112823306/1159144114231709746),
this pull request changes `Applicability` from using `Automatic`,
`Suggested`, and `Manual` to `Always`, `Sometimes`, and `Never`.

Also removes `Applicability::Unspecified` (replacing #7792).
  • Loading branch information
zanieb authored Oct 5, 2023
1 parent 7dc9887 commit b64f403
Show file tree
Hide file tree
Showing 187 changed files with 386 additions and 381 deletions.
2 changes: 1 addition & 1 deletion crates/ruff_cli/tests/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ import os
},
"filename": "-",
"fix": {
"applicability": "Automatic",
"applicability": "always",
"edits": [
{
"content": "",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ exit_code: 1
},
"filename": "/path/to/F401.py",
"fix": {
"applicability": "Automatic",
"applicability": "always",
"edits": [
{
"content": "",
Expand Down
89 changes: 30 additions & 59 deletions crates/ruff_diagnostics/src/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,22 @@ use ruff_text_size::{Ranged, TextSize};

use crate::edit::Edit;

/// Indicates confidence in the correctness of a suggested fix.
#[derive(Default, Copy, Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)]
/// Indicates if a fix can be applied.
#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "serde", serde(rename_all = "lowercase"))]
pub enum Applicability {
/// The fix is definitely what the user intended, or maintains the exact meaning of the code.
/// This fix should be automatically applied.
Automatic,

/// The fix may be what the user intended, but it is uncertain.
/// The fix should result in valid code if it is applied.
/// The fix can be applied with user opt-in.
Suggested,
/// The fix is safe and can always be applied.
/// The fix is definitely what the user intended, or it maintains the exact meaning of the code.
Always,

/// The fix has a good chance of being incorrect or the code be incomplete.
/// The fix may result in invalid code if it is applied.
/// The fix should only be manually applied by the user.
Manual,
/// The fix is unsafe and should only be applied with user opt-in.
/// The fix may be what the user intended, but it is uncertain; the resulting code will have valid syntax.
Sometimes,

/// The applicability of the fix is unknown.
#[default]
Unspecified,
/// The fix is unsafe and should only be manually applied by the user.
/// The fix is likely to be incorrect or the resulting code may have invalid syntax.
Never,
}

/// Indicates the level of isolation required to apply a fix.
Expand All @@ -52,86 +47,62 @@ pub struct Fix {
}

impl Fix {
/// Create a new [`Fix`] with an unspecified applicability from an [`Edit`] element.
#[deprecated(
note = "Use `Fix::automatic`, `Fix::suggested`, or `Fix::manual` instead to specify an applicability."
)]
pub fn unspecified(edit: Edit) -> Self {
Self {
edits: vec![edit],
applicability: Applicability::Unspecified,
isolation_level: IsolationLevel::default(),
}
}

/// Create a new [`Fix`] with an unspecified applicability from multiple [`Edit`] elements.
#[deprecated(
note = "Use `Fix::automatic_edits`, `Fix::suggested_edits`, or `Fix::manual_edits` instead to specify an applicability."
)]
pub fn unspecified_edits(edit: Edit, rest: impl IntoIterator<Item = Edit>) -> Self {
Self {
edits: std::iter::once(edit).chain(rest).collect(),
applicability: Applicability::Unspecified,
isolation_level: IsolationLevel::default(),
}
}

/// Create a new [`Fix`] with [automatic applicability](Applicability::Automatic) from an [`Edit`] element.
pub fn automatic(edit: Edit) -> Self {
/// Create a new [`Fix`] that can [always](Applicability::Always) be applied from an [`Edit`] element.
pub fn always_applies(edit: Edit) -> Self {
Self {
edits: vec![edit],
applicability: Applicability::Automatic,
applicability: Applicability::Always,
isolation_level: IsolationLevel::default(),
}
}

/// Create a new [`Fix`] with [automatic applicability](Applicability::Automatic) from multiple [`Edit`] elements.
pub fn automatic_edits(edit: Edit, rest: impl IntoIterator<Item = Edit>) -> Self {
/// Create a new [`Fix`] that can [always](Applicability::Always) be applied from multiple [`Edit`] elements.
pub fn always_applies_edits(edit: Edit, rest: impl IntoIterator<Item = Edit>) -> Self {
let mut edits: Vec<Edit> = std::iter::once(edit).chain(rest).collect();
edits.sort_by_key(|edit| (edit.start(), edit.end()));
Self {
edits,
applicability: Applicability::Automatic,
applicability: Applicability::Always,
isolation_level: IsolationLevel::default(),
}
}

/// Create a new [`Fix`] with [suggested applicability](Applicability::Suggested) from an [`Edit`] element.
pub fn suggested(edit: Edit) -> Self {
/// Create a new [`Fix`] that can [sometimes](Applicability::Sometimes) be applied from an [`Edit`] element.
pub fn sometimes_applies(edit: Edit) -> Self {
Self {
edits: vec![edit],
applicability: Applicability::Suggested,
applicability: Applicability::Sometimes,
isolation_level: IsolationLevel::default(),
}
}

/// Create a new [`Fix`] with [suggested applicability](Applicability::Suggested) from multiple [`Edit`] elements.
pub fn suggested_edits(edit: Edit, rest: impl IntoIterator<Item = Edit>) -> Self {
/// Create a new [`Fix`] that can [sometimes](Applicability::Sometimes) be applied from multiple [`Edit`] elements.
pub fn sometimes_applies_edits(edit: Edit, rest: impl IntoIterator<Item = Edit>) -> Self {
let mut edits: Vec<Edit> = std::iter::once(edit).chain(rest).collect();
edits.sort_by_key(|edit| (edit.start(), edit.end()));
Self {
edits,
applicability: Applicability::Suggested,
applicability: Applicability::Sometimes,
isolation_level: IsolationLevel::default(),
}
}

/// Create a new [`Fix`] with [manual applicability](Applicability::Manual) from an [`Edit`] element.
pub fn manual(edit: Edit) -> Self {
/// Create a new [`Fix`] that should [never](Applicability::Never) be applied from an [`Edit`] element .
pub fn never_applies(edit: Edit) -> Self {
Self {
edits: vec![edit],
applicability: Applicability::Manual,
applicability: Applicability::Never,
isolation_level: IsolationLevel::default(),
}
}

/// Create a new [`Fix`] with [manual applicability](Applicability::Manual) from multiple [`Edit`] elements.
pub fn manual_edits(edit: Edit, rest: impl IntoIterator<Item = Edit>) -> Self {
/// Create a new [`Fix`] that should [never](Applicability::Never) be applied from multiple [`Edit`] elements.
pub fn never_applies_edits(edit: Edit, rest: impl IntoIterator<Item = Edit>) -> Self {
let mut edits: Vec<Edit> = std::iter::once(edit).chain(rest).collect();
edits.sort_by_key(|edit| (edit.start(), edit.end()));
Self {
edits,
applicability: Applicability::Manual,
applicability: Applicability::Never,
isolation_level: IsolationLevel::default(),
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ pub(crate) fn bindings(checker: &mut Checker) {
binding,
checker.locator,
)
.map(Fix::automatic)
.map(Fix::always_applies)
});
}
checker.diagnostics.push(diagnostic);
Expand Down
18 changes: 11 additions & 7 deletions crates/ruff_linter/src/checkers/noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,10 @@ pub(crate) fn check_noqa(
let mut diagnostic =
Diagnostic::new(UnusedNOQA { codes: None }, directive.range());
if settings.rules.should_fix(diagnostic.kind.rule()) {
diagnostic
.set_fix(Fix::suggested(delete_noqa(directive.range(), locator)));
diagnostic.set_fix(Fix::sometimes_applies(delete_noqa(
directive.range(),
locator,
)));
}
diagnostics.push(diagnostic);
}
Expand Down Expand Up @@ -175,15 +177,17 @@ pub(crate) fn check_noqa(
);
if settings.rules.should_fix(diagnostic.kind.rule()) {
if valid_codes.is_empty() {
diagnostic.set_fix(Fix::suggested(delete_noqa(
diagnostic.set_fix(Fix::sometimes_applies(delete_noqa(
directive.range(),
locator,
)));
} else {
diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
format!("# noqa: {}", valid_codes.join(", ")),
directive.range(),
)));
diagnostic.set_fix(Fix::sometimes_applies(
Edit::range_replacement(
format!("# noqa: {}", valid_codes.join(", ")),
directive.range(),
),
));
}
}
diagnostics.push(diagnostic);
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/fix/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ mod tests {
// The choice of rule here is arbitrary.
kind: MissingNewlineAtEndOfFile.into(),
range: edit.range(),
fix: Some(Fix::unspecified(edit)),
fix: Some(Fix::always_applies(edit)),
parent: None,
})
.collect()
Expand Down
8 changes: 4 additions & 4 deletions crates/ruff_linter/src/message/diff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,10 @@ impl Display for Diff<'_> {
let diff = TextDiff::from_lines(self.source_code.source_text(), &output);

let message = match self.fix.applicability() {
Applicability::Automatic => "Fix",
Applicability::Suggested => "Suggested fix",
Applicability::Manual => "Possible fix",
Applicability::Unspecified => "Suggested fix", /* For backwards compatibility, unspecified fixes are 'suggested' */
// TODO(zanieb): Adjust this messaging once it's user-facing
Applicability::Always => "Fix",
Applicability::Sometimes => "Suggested fix",
Applicability::Never => "Possible fix",
};
writeln!(f, "ℹ {}", message.blue())?;

Expand Down
9 changes: 4 additions & 5 deletions crates/ruff_linter/src/message/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,9 @@ def fibonacci(n):
},
TextRange::new(TextSize::from(7), TextSize::from(9)),
)
.with_fix(Fix::suggested(Edit::range_deletion(TextRange::new(
TextSize::from(0),
TextSize::from(10),
))));
.with_fix(Fix::sometimes_applies(Edit::range_deletion(
TextRange::new(TextSize::from(0), TextSize::from(10)),
)));

let fib_source = SourceFileBuilder::new("fib.py", fib).finish();

Expand All @@ -193,7 +192,7 @@ def fibonacci(n):
},
TextRange::new(TextSize::from(94), TextSize::from(95)),
)
.with_fix(Fix::suggested(Edit::deletion(
.with_fix(Fix::sometimes_applies(Edit::deletion(
TextSize::from(94),
TextSize::from(99),
)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ expression: content
},
"filename": "fib.py",
"fix": {
"applicability": "Suggested",
"applicability": "sometimes",
"edits": [
{
"content": "",
Expand Down Expand Up @@ -43,7 +43,7 @@ expression: content
},
"filename": "fib.py",
"fix": {
"applicability": "Suggested",
"applicability": "sometimes",
"edits": [
{
"content": "",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
source: crates/ruff_linter/src/message/json_lines.rs
expression: content
---
{"code":"F401","end_location":{"column":10,"row":1},"filename":"fib.py","fix":{"applicability":"Suggested","edits":[{"content":"","end_location":{"column":1,"row":2},"location":{"column":1,"row":1}}],"message":"Remove unused import: `os`"},"location":{"column":8,"row":1},"message":"`os` imported but unused","noqa_row":1,"url":"https://docs.astral.sh/ruff/rules/unused-import"}
{"code":"F841","end_location":{"column":6,"row":6},"filename":"fib.py","fix":{"applicability":"Suggested","edits":[{"content":"","end_location":{"column":10,"row":6},"location":{"column":5,"row":6}}],"message":"Remove assignment to unused variable `x`"},"location":{"column":5,"row":6},"message":"Local variable `x` is assigned to but never used","noqa_row":6,"url":"https://docs.astral.sh/ruff/rules/unused-variable"}
{"code":"F401","end_location":{"column":10,"row":1},"filename":"fib.py","fix":{"applicability":"sometimes","edits":[{"content":"","end_location":{"column":1,"row":2},"location":{"column":1,"row":1}}],"message":"Remove unused import: `os`"},"location":{"column":8,"row":1},"message":"`os` imported but unused","noqa_row":1,"url":"https://docs.astral.sh/ruff/rules/unused-import"}
{"code":"F841","end_location":{"column":6,"row":6},"filename":"fib.py","fix":{"applicability":"sometimes","edits":[{"content":"","end_location":{"column":10,"row":6},"location":{"column":5,"row":6}}],"message":"Remove assignment to unused variable `x`"},"location":{"column":5,"row":6},"message":"Local variable `x` is assigned to but never used","noqa_row":6,"url":"https://docs.astral.sh/ruff/rules/unused-variable"}
{"code":"F821","end_location":{"column":5,"row":1},"filename":"undef.py","fix":null,"location":{"column":4,"row":1},"message":"Undefined name `a`","noqa_row":1,"url":"https://docs.astral.sh/ruff/rules/undefined-name"}

Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ pub(crate) fn commented_out_code(
let mut diagnostic = Diagnostic::new(CommentedOutCode, *range);

if settings.rules.should_fix(Rule::CommentedOutCode) {
diagnostic.set_fix(Fix::manual(Edit::range_deletion(
diagnostic.set_fix(Fix::never_applies(Edit::range_deletion(
locator.full_lines_range(*range),
)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,7 @@ pub(crate) fn definition(
function.identifier(),
);
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(Fix::suggested(Edit::insertion(
diagnostic.set_fix(Fix::sometimes_applies(Edit::insertion(
" -> None".to_string(),
function.parameters.range().end(),
)));
Expand All @@ -721,7 +721,7 @@ pub(crate) fn definition(
);
if checker.patch(diagnostic.kind.rule()) {
if let Some(return_type) = simple_magic_return_type(name) {
diagnostic.set_fix(Fix::suggested(Edit::insertion(
diagnostic.set_fix(Fix::sometimes_applies(Edit::insertion(
format!(" -> {return_type}"),
function.parameters.range().end(),
)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub(crate) fn assert_false(checker: &mut Checker, stmt: &Stmt, test: &Expr, msg:

let mut diagnostic = Diagnostic::new(AssertFalse, test.range());
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
diagnostic.set_fix(Fix::sometimes_applies(Edit::range_replacement(
checker.generator().stmt(&assertion_error(msg)),
stmt.range(),
)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ fn duplicate_handler_exceptions<'a>(
expr.range(),
);
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
diagnostic.set_fix(Fix::always_applies(Edit::range_replacement(
// Single exceptions don't require parentheses, but since we're _removing_
// parentheses, insert whitespace as needed.
if let [elt] = unique_elts.as_slice() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ pub(crate) fn getattr_with_constant(

let mut diagnostic = Diagnostic::new(GetAttrWithConstant, expr.range());
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
diagnostic.set_fix(Fix::sometimes_applies(Edit::range_replacement(
pad(
if matches!(
obj,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,5 +200,8 @@ fn move_initialization(
}

let initialization_edit = Edit::insertion(content, pos);
Some(Fix::manual_edits(default_edit, [initialization_edit]))
Some(Fix::never_applies_edits(
default_edit,
[initialization_edit],
))
}
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ pub(crate) fn redundant_tuple_in_exception_handler(
// ```
// Otherwise, the output will be invalid syntax, since we're removing a set of
// parentheses.
diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
diagnostic.set_fix(Fix::always_applies(Edit::range_replacement(
pad(
checker.generator().expr(elt),
type_.range(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ pub(crate) fn setattr_with_constant(
if expr == child.as_ref() {
let mut diagnostic = Diagnostic::new(SetAttrWithConstant, expr.range());
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
diagnostic.set_fix(Fix::sometimes_applies(Edit::range_replacement(
assignment(obj, name, value, checker.generator()),
expr.range(),
)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ pub(crate) fn unreliable_callable_check(
if checker.patch(diagnostic.kind.rule()) {
if id == "hasattr" {
if checker.semantic().is_builtin("callable") {
diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
diagnostic.set_fix(Fix::always_applies(Edit::range_replacement(
format!("callable({})", checker.locator().slice(obj)),
expr.range(),
)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ pub(crate) fn unused_loop_control_variable(checker: &mut Checker, stmt_for: &ast
.filter(|binding| binding.start() >= expr.start())
.all(|binding| !binding.is_used())
{
diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
diagnostic.set_fix(Fix::sometimes_applies(Edit::range_replacement(
rename,
expr.range(),
)));
Expand Down
Loading

0 comments on commit b64f403

Please sign in to comment.