Skip to content

Commit

Permalink
Refactor applicability to use always / sometimes / never
Browse files Browse the repository at this point in the history
  • Loading branch information
zanieb committed Oct 4, 2023
1 parent c5ae2f1 commit 180c6e3
Show file tree
Hide file tree
Showing 401 changed files with 1,976 additions and 1,997 deletions.
4 changes: 1 addition & 3 deletions crates/ruff_cli/tests/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,9 +175,7 @@ import os
},
"filename": "-",
"fix": {
"applicability": {
"automatic": "safe"
},
"applicability": "always",
"edits": [
{
"content": "",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ exit_code: 1
},
"filename": "/path/to/F401.py",
"fix": {
"applicability": {
"automatic": "safe"
},
"applicability": "always",
"edits": [
{
"content": "",
Expand Down
61 changes: 26 additions & 35 deletions crates/ruff_diagnostics/src/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,26 +10,17 @@ use crate::edit::Edit;
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "serde", serde(rename_all = "lowercase"))]
pub enum Applicability {
/// The fix can be applied programmatically.
/// The fix is likely to be correct and the resulting code will have valid syntax.
Automatic(Safety),
/// The fix can always be applied.
/// The fix is definitely what the user intended, or it maintains the exact meaning of the code.
Always,

/// The fix can 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 fix should only be manually applied by the user.
/// The fix is likely to be incorrect or the resulting code may have invalid syntax.
Manual,
}

/// Indicates the safety of applying a fix.
#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[serde(rename_all = "lowercase")]
pub enum Safety {
/// The fix is definitely what the user intended, or it maintains the exact meaning of the code.
/// This fix can be automatically applied.
Safe,
/// The fix may be what the user intended, but it is uncertain.
/// The fix can be applied with user opt-in.
Unsafe,
Never,
}

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

impl Fix {
/// Create a new [`Fix`] with [safe applicability](Applicability::Automatic(Safety::Safe)) from an [`Edit`] element.
pub fn automatic_safe(edit: Edit) -> Self {
/// Create a new [`Fix`] with [safe applicability](Applicability::Always) from an [`Edit`] element.
pub fn always_safe(edit: Edit) -> Self {
Self {
edits: vec![edit],
applicability: Applicability::Automatic(Safety::Safe),
applicability: Applicability::Always,
isolation_level: IsolationLevel::default(),
}
}

/// Create a new [`Fix`] with [safe applicability](Applicability::Automatic(Safety::Safe)) from multiple [`Edit`] elements.
pub fn automatic_safe_edits(edit: Edit, rest: impl IntoIterator<Item = Edit>) -> Self {
/// Create a new [`Fix`] with [safe applicability](Applicability::Always) from multiple [`Edit`] elements.
pub fn always_safe_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(Safety::Safe),
applicability: Applicability::Always,
isolation_level: IsolationLevel::default(),
}
}

/// Create a new [`Fix`] with [unsafe applicability](Applicable::Automatic(Safety::Unsafe)) from an [`Edit`] element.
pub fn automatic_unsafe(edit: Edit) -> Self {
/// Create a new [`Fix`] with [unsafe applicability](Applicable::Sometimes) from an [`Edit`] element.
pub fn sometimes_safe(edit: Edit) -> Self {
Self {
edits: vec![edit],
applicability: Applicability::Automatic(Safety::Unsafe),
applicability: Applicability::Sometimes,
isolation_level: IsolationLevel::default(),
}
}

/// Create a new [`Fix`] with [unsafe applicability](Applicability::Automatic(Safety::Unsafe)) from multiple [`Edit`] elements.
pub fn automatic_unsafe_edits(edit: Edit, rest: impl IntoIterator<Item = Edit>) -> Self {
/// Create a new [`Fix`] with [unsafe applicability](Applicability::Sometimes) from multiple [`Edit`] elements.
pub fn sometimes_safe_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(Safety::Unsafe),
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`] with [manual applicability](Applicability::Never) from an [`Edit`] element.
pub fn never_safe(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`] with [manual applicability](Applicability::Never) from multiple [`Edit`] elements.
pub fn never_safe_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_diagnostics/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
pub use diagnostic::{Diagnostic, DiagnosticKind};
pub use edit::Edit;
pub use fix::{Applicability, Fix, IsolationLevel, Safety};
pub use fix::{Applicability, Fix, IsolationLevel};
pub use source_map::{SourceMap, SourceMarker};
pub use violation::{AlwaysFixableViolation, FixAvailability, Violation};

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_safe)
.map(Fix::always_safe)
});
}
checker.diagnostics.push(diagnostic);
Expand Down
6 changes: 3 additions & 3 deletions crates/ruff_linter/src/checkers/noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ 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::automatic_unsafe(delete_noqa(
diagnostic.set_fix(Fix::sometimes_safe(delete_noqa(
directive.range(),
locator,
)));
Expand Down Expand Up @@ -177,12 +177,12 @@ pub(crate) fn check_noqa(
);
if settings.rules.should_fix(diagnostic.kind.rule()) {
if valid_codes.is_empty() {
diagnostic.set_fix(Fix::automatic_unsafe(delete_noqa(
diagnostic.set_fix(Fix::sometimes_safe(delete_noqa(
directive.range(),
locator,
)));
} else {
diagnostic.set_fix(Fix::automatic_unsafe(Edit::range_replacement(
diagnostic.set_fix(Fix::sometimes_safe(Edit::range_replacement(
format!("# noqa: {}", valid_codes.join(", ")),
directive.range(),
)));
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::automatic_safe(edit)),
fix: Some(Fix::always_safe(edit)),
parent: None,
})
.collect()
Expand Down
10 changes: 4 additions & 6 deletions crates/ruff_linter/src/message/diff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use colored::{Color, ColoredString, Colorize, Styles};
use ruff_text_size::{Ranged, TextRange, TextSize};
use similar::{ChangeTag, TextDiff};

use ruff_diagnostics::{Applicability, Fix, Safety};
use ruff_diagnostics::{Applicability, Fix};
use ruff_source_file::{OneIndexed, SourceFile};

use crate::message::Message;
Expand Down Expand Up @@ -53,11 +53,9 @@ impl Display for Diff<'_> {

let message = match self.fix.applicability() {
// TODO(zanieb): Adjust this messaging once it's user-facing
Applicability::Automatic(safety) => match safety {
Safety::Safe => "Fix",
Safety::Unsafe => "Unsafe fix",
},
Applicability::Manual => "Manual fix",
Applicability::Always => "Fix",
Applicability::Sometimes => "Suggested fix",
Applicability::Never => "Possible fix",
};
writeln!(f, "ℹ {}", message.blue())?;

Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/message/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ def fibonacci(n):
},
TextRange::new(TextSize::from(7), TextSize::from(9)),
)
.with_fix(Fix::automatic_unsafe(Edit::range_deletion(TextRange::new(
.with_fix(Fix::sometimes_safe(Edit::range_deletion(TextRange::new(
TextSize::from(0),
TextSize::from(10),
))));
Expand All @@ -193,7 +193,7 @@ def fibonacci(n):
},
TextRange::new(TextSize::from(94), TextSize::from(95)),
)
.with_fix(Fix::automatic_unsafe(Edit::deletion(
.with_fix(Fix::sometimes_safe(Edit::deletion(
TextSize::from(94),
TextSize::from(99),
)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,7 @@ expression: content
},
"filename": "fib.py",
"fix": {
"applicability": {
"automatic": "unsafe"
},
"applicability": "sometimes",
"edits": [
{
"content": "",
Expand Down Expand Up @@ -45,9 +43,7 @@ expression: content
},
"filename": "fib.py",
"fix": {
"applicability": {
"automatic": "unsafe"
},
"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":{"automatic":"unsafe"},"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":{"automatic":"unsafe"},"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_safe(Edit::range_deletion(
locator.full_lines_range(*range),
)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ ERA001.py:1:1: ERA001 [*] Found commented-out code
|
= help: Remove commented-out code

Manual fix
Possible fix
1 |-#import os
2 1 | # from foo import junk
3 2 | #a = 3
Expand All @@ -26,7 +26,7 @@ ERA001.py:2:1: ERA001 [*] Found commented-out code
|
= help: Remove commented-out code

Manual fix
Possible fix
1 1 | #import os
2 |-# from foo import junk
3 2 | #a = 3
Expand All @@ -44,7 +44,7 @@ ERA001.py:3:1: ERA001 [*] Found commented-out code
|
= help: Remove commented-out code

Manual fix
Possible fix
1 1 | #import os
2 2 | # from foo import junk
3 |-#a = 3
Expand All @@ -63,7 +63,7 @@ ERA001.py:5:1: ERA001 [*] Found commented-out code
|
= help: Remove commented-out code

Manual fix
Possible fix
2 2 | # from foo import junk
3 3 | #a = 3
4 4 | a = 4
Expand All @@ -82,7 +82,7 @@ ERA001.py:13:5: ERA001 [*] Found commented-out code
|
= help: Remove commented-out code

Manual fix
Possible fix
10 10 |
11 11 | # This is a real comment.
12 12 | # # This is a (nested) comment.
Expand All @@ -100,7 +100,7 @@ ERA001.py:21:5: ERA001 [*] Found commented-out code
|
= help: Remove commented-out code

Manual fix
Possible fix
18 18 |
19 19 | class A():
20 20 | pass
Expand All @@ -120,7 +120,7 @@ ERA001.py:26:5: ERA001 [*] Found commented-out code
|
= help: Remove commented-out code

Manual fix
Possible fix
23 23 |
24 24 | dictionary = {
25 25 | # "key1": 123, # noqa: ERA001
Expand All @@ -139,7 +139,7 @@ ERA001.py:27:5: ERA001 [*] Found commented-out code
|
= help: Remove commented-out code

Manual fix
Possible fix
24 24 | dictionary = {
25 25 | # "key1": 123, # noqa: ERA001
26 26 | # "key2": 456,
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::automatic_unsafe(Edit::insertion(
diagnostic.set_fix(Fix::sometimes_safe(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::automatic_unsafe(Edit::insertion(
diagnostic.set_fix(Fix::sometimes_safe(Edit::insertion(
format!(" -> {return_type}"),
function.parameters.range().end(),
)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ annotation_presence.py:159:9: ANN204 [*] Missing return type annotation for spec
|
= help: Add `None` return type

Unsafe fix
Suggested fix
156 156 |
157 157 | class Foo:
158 158 | @decorator()
Expand All @@ -272,7 +272,7 @@ annotation_presence.py:165:9: ANN204 [*] Missing return type annotation for spec
|
= help: Add `None` return type

Unsafe fix
Suggested fix
162 162 |
163 163 | # Regression test for: https://github.com/astral-sh/ruff/issues/7711
164 164 | class Class:
Expand Down
Loading

0 comments on commit 180c6e3

Please sign in to comment.