Remove unnecessary LibCST usage in key-in-dict (#6727)

## Summary

We're using LibCST to ensure that we return the full parenthesized range
of an expression, for display purposes. We can just use
`parenthesized_range` which is more efficient and removes one LibCST
dependency.

## Test Plan

`cargo test`
This commit is contained in:
Charlie Marsh 2023-08-21 10:32:09 -04:00 committed by GitHub
parent f017555d53
commit 2405536d03
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 92 additions and 76 deletions

View file

@ -31,6 +31,8 @@ for key in list(obj.keys()):
key in (obj or {}).keys() # SIM118 key in (obj or {}).keys() # SIM118
(key) in (obj or {}).keys() # SIM118
from typing import KeysView from typing import KeysView

View file

@ -7,10 +7,6 @@ use crate::rules::flake8_simplify;
/// Run lint rules over a [`Comprehension`] syntax nodes. /// Run lint rules over a [`Comprehension`] syntax nodes.
pub(crate) fn comprehension(comprehension: &Comprehension, checker: &mut Checker) { pub(crate) fn comprehension(comprehension: &Comprehension, checker: &mut Checker) {
if checker.enabled(Rule::InDictKeys) { if checker.enabled(Rule::InDictKeys) {
flake8_simplify::rules::key_in_dict_for( flake8_simplify::rules::key_in_dict_comprehension(checker, comprehension);
checker,
&comprehension.target,
&comprehension.iter,
);
} }
} }

View file

@ -1178,7 +1178,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
pylint::rules::magic_value_comparison(checker, left, comparators); pylint::rules::magic_value_comparison(checker, left, comparators);
} }
if checker.enabled(Rule::InDictKeys) { if checker.enabled(Rule::InDictKeys) {
flake8_simplify::rules::key_in_dict_compare(checker, expr, left, ops, comparators); flake8_simplify::rules::key_in_dict_compare(checker, compare);
} }
if checker.enabled(Rule::YodaConditions) { if checker.enabled(Rule::YodaConditions) {
flake8_simplify::rules::yoda_conditions(checker, expr, left, ops, comparators); flake8_simplify::rules::yoda_conditions(checker, expr, left, ops, comparators);

View file

@ -1139,7 +1139,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
pygrep_hooks::rules::non_existent_mock_method(checker, test); pygrep_hooks::rules::non_existent_mock_method(checker, test);
} }
} }
Stmt::With(with_ @ ast::StmtWith { items, body, .. }) => { Stmt::With(with_stmt @ ast::StmtWith { items, body, .. }) => {
if checker.enabled(Rule::AssertRaisesException) { if checker.enabled(Rule::AssertRaisesException) {
flake8_bugbear::rules::assert_raises_exception(checker, items); flake8_bugbear::rules::assert_raises_exception(checker, items);
} }
@ -1149,7 +1149,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::MultipleWithStatements) { if checker.enabled(Rule::MultipleWithStatements) {
flake8_simplify::rules::multiple_with_statements( flake8_simplify::rules::multiple_with_statements(
checker, checker,
with_, with_stmt,
checker.semantic.current_statement_parent(), checker.semantic.current_statement_parent(),
); );
} }
@ -1168,13 +1168,16 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
perflint::rules::try_except_in_loop(checker, body); perflint::rules::try_except_in_loop(checker, body);
} }
} }
Stmt::For(ast::StmtFor { Stmt::For(
for_stmt @ ast::StmtFor {
target, target,
body, body,
iter, iter,
orelse, orelse,
is_async,
.. ..
}) => { },
) => {
if checker.any_enabled(&[ if checker.any_enabled(&[
Rule::UnusedLoopControlVariable, Rule::UnusedLoopControlVariable,
Rule::IncorrectDictIterator, Rule::IncorrectDictIterator,
@ -1200,17 +1203,6 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::IterationOverSet) { if checker.enabled(Rule::IterationOverSet) {
pylint::rules::iteration_over_set(checker, iter); pylint::rules::iteration_over_set(checker, iter);
} }
if stmt.is_for_stmt() {
if checker.enabled(Rule::ReimplementedBuiltin) {
flake8_simplify::rules::convert_for_loop_to_any_all(checker, stmt);
}
if checker.enabled(Rule::InDictKeys) {
flake8_simplify::rules::key_in_dict_for(checker, target, iter);
}
if checker.enabled(Rule::TryExceptInLoop) {
perflint::rules::try_except_in_loop(checker, body);
}
}
if checker.enabled(Rule::ManualListComprehension) { if checker.enabled(Rule::ManualListComprehension) {
perflint::rules::manual_list_comprehension(checker, target, body); perflint::rules::manual_list_comprehension(checker, target, body);
} }
@ -1220,6 +1212,17 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::UnnecessaryListCast) { if checker.enabled(Rule::UnnecessaryListCast) {
perflint::rules::unnecessary_list_cast(checker, iter); perflint::rules::unnecessary_list_cast(checker, iter);
} }
if !is_async {
if checker.enabled(Rule::ReimplementedBuiltin) {
flake8_simplify::rules::convert_for_loop_to_any_all(checker, stmt);
}
if checker.enabled(Rule::InDictKeys) {
flake8_simplify::rules::key_in_dict_for(checker, for_stmt);
}
if checker.enabled(Rule::TryExceptInLoop) {
perflint::rules::try_except_in_loop(checker, body);
}
}
} }
Stmt::Try(ast::StmtTry { Stmt::Try(ast::StmtTry {
body, body,

View file

@ -1,16 +1,12 @@
use anyhow::Result;
use ruff_python_ast::{self as ast, Arguments, CmpOp, Expr, Ranged};
use ruff_text_size::TextRange;
use ruff_diagnostics::Edit; use ruff_diagnostics::Edit;
use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix};
use ruff_macros::{derive_message_formats, violation}; use ruff_macros::{derive_message_formats, violation};
use ruff_python_codegen::Stylist; use ruff_python_ast::node::AnyNodeRef;
use ruff_source_file::Locator; use ruff_python_ast::parenthesize::parenthesized_range;
use ruff_python_ast::{self as ast, Arguments, CmpOp, Comprehension, Expr, Ranged};
use ruff_text_size::TextRange;
use crate::autofix::codemods::CodegenStylist;
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
use crate::cst::matchers::{match_attribute, match_call_mut, match_expression};
use crate::registry::AsRule; use crate::registry::AsRule;
/// ## What it does /// ## What it does
@ -67,7 +63,7 @@ fn key_in_dict(
left: &Expr, left: &Expr,
right: &Expr, right: &Expr,
operator: CmpOp, operator: CmpOp,
range: TextRange, parent: AnyNodeRef,
) { ) {
let Expr::Call(ast::ExprCall { let Expr::Call(ast::ExprCall {
func, func,
@ -100,13 +96,16 @@ fn key_in_dict(
return; return;
} }
// Slice exact content to preserve formatting. // Extract the exact range of the left and right expressions.
let left_content = checker.locator().slice(left.range()); let left_range = parenthesized_range(left.into(), parent, checker.locator().contents())
let Ok(value_content) = .unwrap_or(left.range());
value_content_for_key_in_dict(checker.locator(), checker.stylist(), right) let right_range = parenthesized_range(right.into(), parent, checker.locator().contents())
else { .unwrap_or(right.range());
return; let value_range = parenthesized_range(value.into(), parent, checker.locator().contents())
}; .unwrap_or(value.range());
let left_content = checker.locator().slice(left_range);
let value_content = checker.locator().slice(value_range);
let mut diagnostic = Diagnostic::new( let mut diagnostic = Diagnostic::new(
InDictKeys { InDictKeys {
@ -114,37 +113,42 @@ fn key_in_dict(
dict: value_content.to_string(), dict: value_content.to_string(),
operator: operator.as_str().to_string(), operator: operator.as_str().to_string(),
}, },
range, TextRange::new(left_range.start(), right_range.end()),
); );
if checker.patch(diagnostic.kind.rule()) { if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(Fix::suggested(Edit::range_replacement( diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
value_content, value_content.to_string(),
right.range(), right_range,
))); )));
} }
checker.diagnostics.push(diagnostic); checker.diagnostics.push(diagnostic);
} }
/// SIM118 in a for loop /// SIM118 in a `for` loop.
pub(crate) fn key_in_dict_for(checker: &mut Checker, target: &Expr, iter: &Expr) { pub(crate) fn key_in_dict_for(checker: &mut Checker, for_stmt: &ast::StmtFor) {
key_in_dict( key_in_dict(
checker, checker,
target, &for_stmt.target,
iter, &for_stmt.iter,
CmpOp::In, CmpOp::In,
TextRange::new(target.start(), iter.end()), for_stmt.into(),
); );
} }
/// SIM118 in a comparison /// SIM118 in a comprehension.
pub(crate) fn key_in_dict_compare( pub(crate) fn key_in_dict_comprehension(checker: &mut Checker, comprehension: &Comprehension) {
checker: &mut Checker, key_in_dict(
expr: &Expr, checker,
left: &Expr, &comprehension.target,
ops: &[CmpOp], &comprehension.iter,
comparators: &[Expr], CmpOp::In,
) { comprehension.into(),
let [op] = ops else { );
}
/// SIM118 in a comparison.
pub(crate) fn key_in_dict_compare(checker: &mut Checker, compare: &ast::ExprCompare) {
let [op] = compare.ops.as_slice() else {
return; return;
}; };
@ -152,21 +156,9 @@ pub(crate) fn key_in_dict_compare(
return; return;
} }
let [right] = comparators else { let [right] = compare.comparators.as_slice() else {
return; return;
}; };
key_in_dict(checker, left, right, *op, expr.range()); key_in_dict(checker, &compare.left, right, *op, compare.into());
}
fn value_content_for_key_in_dict(
locator: &Locator,
stylist: &Stylist,
expr: &Expr,
) -> Result<String> {
let content = locator.slice(expr.range());
let mut expression = match_expression(content)?;
let call = match_call_mut(&mut expression)?;
let attribute = match_attribute(&mut call.func)?;
Ok(attribute.value.codegen_stylist(stylist))
} }

View file

@ -274,7 +274,7 @@ SIM118.py:32:1: SIM118 [*] Use `key in (obj or {})` instead of `key in (obj or {
32 | key in (obj or {}).keys() # SIM118 32 | key in (obj or {}).keys() # SIM118
| ^^^^^^^^^^^^^^^^^^^^^^^^^ SIM118 | ^^^^^^^^^^^^^^^^^^^^^^^^^ SIM118
33 | 33 |
34 | from typing import KeysView 34 | (key) in (obj or {}).keys() # SIM118
| |
= help: Convert to `key in (obj or {})` = help: Convert to `key in (obj or {})`
@ -285,7 +285,28 @@ SIM118.py:32:1: SIM118 [*] Use `key in (obj or {})` instead of `key in (obj or {
32 |-key in (obj or {}).keys() # SIM118 32 |-key in (obj or {}).keys() # SIM118
32 |+key in (obj or {}) # SIM118 32 |+key in (obj or {}) # SIM118
33 33 | 33 33 |
34 34 | from typing import KeysView 34 34 | (key) in (obj or {}).keys() # SIM118
35 35 | 35 35 |
SIM118.py:34:1: SIM118 [*] Use `(key) in (obj or {})` instead of `(key) in (obj or {}).keys()`
|
32 | key in (obj or {}).keys() # SIM118
33 |
34 | (key) in (obj or {}).keys() # SIM118
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ SIM118
35 |
36 | from typing import KeysView
|
= help: Convert to `(key) in (obj or {})`
Suggested fix
31 31 |
32 32 | key in (obj or {}).keys() # SIM118
33 33 |
34 |-(key) in (obj or {}).keys() # SIM118
34 |+(key) in (obj or {}) # SIM118
35 35 |
36 36 | from typing import KeysView
37 37 |

View file

@ -339,7 +339,9 @@ impl FormatRule<Suite, PyFormatContext<'_>> for FormatSuite {
pub(crate) fn contains_only_an_ellipsis(body: &[Stmt], comments: &Comments) -> bool { pub(crate) fn contains_only_an_ellipsis(body: &[Stmt], comments: &Comments) -> bool {
match body { match body {
[Stmt::Expr(ast::StmtExpr { value, .. })] => { [Stmt::Expr(ast::StmtExpr { value, .. })] => {
let [node] = body else { return false; }; let [node] = body else {
return false;
};
matches!( matches!(
value.as_ref(), value.as_ref(),
Expr::Constant(ast::ExprConstant { Expr::Constant(ast::ExprConstant {