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 1 commit
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
13 changes: 13 additions & 0 deletions crates/ruff/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
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
58 changes: 39 additions & 19 deletions crates/ruff/src/rules/pylint/rules/repeated_isinstance_calls.rs
Original file line number Diff line number Diff line change
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 @@ -38,16 +40,31 @@ use crate::checkers::ast::Checker;
/// - [Python documentation](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!("Consider merging these isinstance calls to `{expr}`")
}

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

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(", "))
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we use the AST to build this or is it fine to format it directly?

Copy link
Member

Choose a reason for hiding this comment

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

I think either would be reasonable.

}
}

Expand Down Expand Up @@ -91,18 +108,21 @@ 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);
}
}
}
Original file line number Diff line number Diff line change
@@ -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 [*] Consider merging these isinstance calls to `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 [*] Consider merging these isinstance calls to `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 [*] Consider merging these isinstance calls to `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 [*] Consider merging these isinstance calls to `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 [*] Consider merging these isinstance calls to `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 [*] Consider merging these isinstance calls to `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


Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
---
source: crates/ruff/src/rules/pylint/mod.rs
---
repeated_isinstance_calls.py:15:8: PLR1701 [*] Consider merging these isinstance calls to `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))`

ℹ 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 [*] Consider merging these isinstance calls to `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
19 | result = isinstance(var[4], int) or isinstance(var[4], float) or isinstance(var[5], list) and False # [consider-merging-isinstance]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1701
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 [*] Consider merging these isinstance calls to `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 |
21 | result = isinstance(var[5], int) or True or isinstance(var[5], float) # [consider-merging-isinstance]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1701
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 [*] Consider merging these isinstance calls to `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))`

ℹ 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 [*] Consider merging these isinstance calls to `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]
26 | result = isinstance(var[11], int) or isinstance(var[11], int) or isinstance(var[11], float) # [consider-merging-isinstance]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1701
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 [*] Consider merging these isinstance calls to `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