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

Rename applicability levels to always, sometimes, and never #7821

Merged
merged 5 commits into from
Oct 5, 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
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
Copy link
Member

Choose a reason for hiding this comment

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

What do you think is the outcome here? Like what language?

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'm not sure! Perhaps what I have now with some additional commentary such as the use of --unsafe-fixes to enable the fix.

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
Loading