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 autofix for PLR1701 (repeated-isinstance-calls) #4792

Merged
merged 5 commits into from Jun 1, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions crates/ruff/src/rules/pylint/mod.rs
Expand Up @@ -113,6 +113,19 @@ mod tests {
Ok(())
}

#[test]
fn repeated_isinstance_calls() -> Result<()> {
let diagnostics = test_path(
Path::new("pylint/repeated_isinstance_calls.py"),
&Settings {
target_version: PythonVersion::Py39,
..Settings::for_rules(vec![Rule::RepeatedIsinstanceCalls])
},
)?;
assert_messages!(diagnostics);
Ok(())
}

#[test]
fn continue_in_finally() -> Result<()> {
let diagnostics = test_path(
Expand Down
65 changes: 44 additions & 21 deletions crates/ruff/src/rules/pylint/rules/repeated_isinstance_calls.rs
Expand Up @@ -2,11 +2,13 @@ use itertools::Itertools;
use rustc_hash::{FxHashMap, FxHashSet};
use rustpython_parser::ast::{self, Boolop, Expr, Ranged};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::hashable::HashableExpr;

use crate::checkers::ast::Checker;
use crate::registry::AsRule;
use crate::settings::types::PythonVersion;

/// ## What it does
/// Checks for repeated `isinstance` calls on the same object.
Expand Down Expand Up @@ -35,19 +37,22 @@ use crate::checkers::ast::Checker;
/// ```
///
/// ## References
/// - [Python documentation](https://docs.python.org/3/library/functions.html#isinstance)
/// - [Python documentation: `isinstance`](https://docs.python.org/3/library/functions.html#isinstance)
#[violation]
pub struct RepeatedIsinstanceCalls {
obj: String,
types: Vec<String>,
expr: String,
}

impl Violation for RepeatedIsinstanceCalls {
impl AlwaysAutofixableViolation for RepeatedIsinstanceCalls {
#[derive_message_formats]
fn message(&self) -> String {
let RepeatedIsinstanceCalls { obj, types } = self;
let types = types.join(", ");
format!("Merge these isinstance calls: `isinstance({obj}, ({types}))`")
let RepeatedIsinstanceCalls { expr } = self;
format!("Merge `isinstance` calls: `{expr}`")
}

fn autofix_title(&self) -> String {
let RepeatedIsinstanceCalls { expr } = self;
format!("Replace with `{expr}`")
}
}

Expand All @@ -58,7 +63,7 @@ pub(crate) fn repeated_isinstance_calls(
op: Boolop,
values: &[Expr],
) {
if !matches!(op, Boolop::Or) || !checker.semantic_model().is_builtin("isinstance") {
if !op.is_or() {
return;
}

Expand All @@ -74,6 +79,9 @@ pub(crate) fn repeated_isinstance_calls(
let [obj, types] = &args[..] else {
continue;
};
if !checker.semantic_model().is_builtin("isinstance") {
return;
}
Comment on lines +82 to +84
Copy link
Member Author

Choose a reason for hiding this comment

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

@charliermarsh I'm unable to understand why was this moved here 😅

Instead of checking once, this will check for each iteration but I don't think the semantic model changes. Am I missing something here?

Copy link
Member

Choose a reason for hiding this comment

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

It effectively delays the check until we have at least one isinstance call. The result should be the same, since we're returning, but we avoid having to do the is_builtin check if none of the calls are isinstance.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh right, understood. Thanks!

let (num_calls, matches) = obj_to_types
.entry(obj.into())
.or_insert_with(|| (0, FxHashSet::default()));
Expand All @@ -91,18 +99,33 @@ pub(crate) fn repeated_isinstance_calls(

for (obj, (num_calls, types)) in obj_to_types {
if num_calls > 1 && types.len() > 1 {
checker.diagnostics.push(Diagnostic::new(
RepeatedIsinstanceCalls {
obj: checker.generator().expr(obj.as_expr()),
types: types
.iter()
.map(HashableExpr::as_expr)
.map(|expr| checker.generator().expr(expr))
.sorted()
.collect(),
},
expr.range(),
));
let call = merged_isinstance_call(
&checker.generator().expr(obj.as_expr()),
types
.iter()
.map(HashableExpr::as_expr)
.map(|expr| checker.generator().expr(expr))
.sorted(),
checker.settings.target_version,
);
let mut diagnostic =
Diagnostic::new(RepeatedIsinstanceCalls { expr: call.clone() }, expr.range());
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(Fix::automatic(Edit::range_replacement(call, expr.range())));
}
checker.diagnostics.push(diagnostic);
}
}
}

fn merged_isinstance_call(
obj: &str,
types: impl IntoIterator<Item = String>,
target_version: PythonVersion,
) -> String {
if target_version >= PythonVersion::Py310 {
format!("isinstance({}, {})", obj, types.into_iter().join(" | "))
} else {
format!("isinstance({}, ({}))", obj, types.into_iter().join(", "))
}
}
@@ -1,16 +1,27 @@
---
source: crates/ruff/src/rules/pylint/mod.rs
---
repeated_isinstance_calls.py:15:8: PLR1701 Merge these isinstance calls: `isinstance(var[3], (float, int))`
repeated_isinstance_calls.py:15:8: PLR1701 [*] Merge `isinstance` calls: `isinstance(var[3], float | int)`
|
15 | # not merged
16 | if isinstance(var[3], int) or isinstance(var[3], float) or isinstance(var[3], list) and True: # [consider-merging-isinstance]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1701
17 | pass
18 | result = isinstance(var[4], int) or isinstance(var[4], float) or isinstance(var[5], list) and False # [consider-merging-isinstance]
|
= help: Replace with `isinstance(var[3], float | int)`

repeated_isinstance_calls.py:17:14: PLR1701 Merge these isinstance calls: `isinstance(var[4], (float, int))`
ℹ Fix
12 12 | result = isinstance(var[2], (int, float))
13 13 |
14 14 | # not merged
15 |- if isinstance(var[3], int) or isinstance(var[3], float) or isinstance(var[3], list) and True: # [consider-merging-isinstance]
15 |+ if isinstance(var[3], float | int): # [consider-merging-isinstance]
16 16 | pass
17 17 | result = isinstance(var[4], int) or isinstance(var[4], float) or isinstance(var[5], list) and False # [consider-merging-isinstance]
18 18 |

repeated_isinstance_calls.py:17:14: PLR1701 [*] Merge `isinstance` calls: `isinstance(var[4], float | int)`
|
17 | if isinstance(var[3], int) or isinstance(var[3], float) or isinstance(var[3], list) and True: # [consider-merging-isinstance]
18 | pass
Expand All @@ -19,8 +30,19 @@ repeated_isinstance_calls.py:17:14: PLR1701 Merge these isinstance calls: `isins
20 |
21 | result = isinstance(var[5], int) or True or isinstance(var[5], float) # [consider-merging-isinstance]
|
= help: Replace with `isinstance(var[4], float | int)`

ℹ Fix
14 14 | # not merged
15 15 | if isinstance(var[3], int) or isinstance(var[3], float) or isinstance(var[3], list) and True: # [consider-merging-isinstance]
16 16 | pass
17 |- result = isinstance(var[4], int) or isinstance(var[4], float) or isinstance(var[5], list) and False # [consider-merging-isinstance]
17 |+ result = isinstance(var[4], float | int) # [consider-merging-isinstance]
18 18 |
19 19 | result = isinstance(var[5], int) or True or isinstance(var[5], float) # [consider-merging-isinstance]
20 20 |

repeated_isinstance_calls.py:19:14: PLR1701 Merge these isinstance calls: `isinstance(var[5], (float, int))`
repeated_isinstance_calls.py:19:14: PLR1701 [*] Merge `isinstance` calls: `isinstance(var[5], float | int)`
|
19 | result = isinstance(var[4], int) or isinstance(var[4], float) or isinstance(var[5], list) and False # [consider-merging-isinstance]
20 |
Expand All @@ -29,17 +51,39 @@ repeated_isinstance_calls.py:19:14: PLR1701 Merge these isinstance calls: `isins
22 |
23 | inferred_isinstance = isinstance
|
= help: Replace with `isinstance(var[5], float | int)`

ℹ Fix
16 16 | pass
17 17 | result = isinstance(var[4], int) or isinstance(var[4], float) or isinstance(var[5], list) and False # [consider-merging-isinstance]
18 18 |
19 |- result = isinstance(var[5], int) or True or isinstance(var[5], float) # [consider-merging-isinstance]
19 |+ result = isinstance(var[5], float | int) # [consider-merging-isinstance]
20 20 |
21 21 | inferred_isinstance = isinstance
22 22 | result = inferred_isinstance(var[6], int) or inferred_isinstance(var[6], float) or inferred_isinstance(var[6], list) and False # [consider-merging-isinstance]

repeated_isinstance_calls.py:23:14: PLR1701 Merge these isinstance calls: `isinstance(var[10], (list, str))`
repeated_isinstance_calls.py:23:14: PLR1701 [*] Merge `isinstance` calls: `isinstance(var[10], list | str)`
|
23 | inferred_isinstance = isinstance
24 | result = inferred_isinstance(var[6], int) or inferred_isinstance(var[6], float) or inferred_isinstance(var[6], list) and False # [consider-merging-isinstance]
25 | result = isinstance(var[10], str) or isinstance(var[10], int) and var[8] * 14 or isinstance(var[10], float) and var[5] * 14.4 or isinstance(var[10], list) # [consider-merging-isinstance]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1701
26 | result = isinstance(var[11], int) or isinstance(var[11], int) or isinstance(var[11], float) # [consider-merging-isinstance]
|
= help: Replace with `isinstance(var[10], list | str)`

repeated_isinstance_calls.py:24:14: PLR1701 Merge these isinstance calls: `isinstance(var[11], (float, int))`
ℹ Fix
20 20 |
21 21 | inferred_isinstance = isinstance
22 22 | result = inferred_isinstance(var[6], int) or inferred_isinstance(var[6], float) or inferred_isinstance(var[6], list) and False # [consider-merging-isinstance]
23 |- result = isinstance(var[10], str) or isinstance(var[10], int) and var[8] * 14 or isinstance(var[10], float) and var[5] * 14.4 or isinstance(var[10], list) # [consider-merging-isinstance]
23 |+ result = isinstance(var[10], list | str) # [consider-merging-isinstance]
24 24 | result = isinstance(var[11], int) or isinstance(var[11], int) or isinstance(var[11], float) # [consider-merging-isinstance]
25 25 |
26 26 | result = isinstance(var[20])

repeated_isinstance_calls.py:24:14: PLR1701 [*] Merge `isinstance` calls: `isinstance(var[11], float | int)`
|
24 | result = inferred_isinstance(var[6], int) or inferred_isinstance(var[6], float) or inferred_isinstance(var[6], list) and False # [consider-merging-isinstance]
25 | result = isinstance(var[10], str) or isinstance(var[10], int) and var[8] * 14 or isinstance(var[10], float) and var[5] * 14.4 or isinstance(var[10], list) # [consider-merging-isinstance]
Expand All @@ -48,14 +92,36 @@ repeated_isinstance_calls.py:24:14: PLR1701 Merge these isinstance calls: `isins
27 |
28 | result = isinstance(var[20])
|
= help: Replace with `isinstance(var[11], float | int)`

ℹ Fix
21 21 | inferred_isinstance = isinstance
22 22 | result = inferred_isinstance(var[6], int) or inferred_isinstance(var[6], float) or inferred_isinstance(var[6], list) and False # [consider-merging-isinstance]
23 23 | result = isinstance(var[10], str) or isinstance(var[10], int) and var[8] * 14 or isinstance(var[10], float) and var[5] * 14.4 or isinstance(var[10], list) # [consider-merging-isinstance]
24 |- result = isinstance(var[11], int) or isinstance(var[11], int) or isinstance(var[11], float) # [consider-merging-isinstance]
24 |+ result = isinstance(var[11], float | int) # [consider-merging-isinstance]
25 25 |
26 26 | result = isinstance(var[20])
27 27 | result = isinstance()

repeated_isinstance_calls.py:30:14: PLR1701 Merge these isinstance calls: `isinstance(var[12], (float, int, list))`
repeated_isinstance_calls.py:30:14: PLR1701 [*] Merge `isinstance` calls: `isinstance(var[12], float | int | list)`
|
30 | # Combination merged and not merged
31 | result = isinstance(var[12], (int, float)) or isinstance(var[12], list) # [consider-merging-isinstance]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1701
32 |
33 | # not merged but valid
|
= help: Replace with `isinstance(var[12], float | int | list)`

ℹ Fix
27 27 | result = isinstance()
28 28 |
29 29 | # Combination merged and not merged
30 |- result = isinstance(var[12], (int, float)) or isinstance(var[12], list) # [consider-merging-isinstance]
30 |+ result = isinstance(var[12], float | int | list) # [consider-merging-isinstance]
31 31 |
32 32 | # not merged but valid
33 33 | result = isinstance(var[5], int) and var[5] * 14 or isinstance(var[5], float) and var[5] * 14.4