Add dedicated method to find typed binding (#8517)

## Summary

We have this pattern in a bunch of places, where we find the _only_
binding to a name (and return `None`) if it's bound multiple times. This
PR DRYs it up into a method on `SemanticModel`.
This commit is contained in:
Charlie Marsh 2023-11-06 08:25:32 -08:00 committed by GitHub
parent 5b3e922050
commit eab8ca4d7e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 82 additions and 79 deletions

View file

@ -43,6 +43,12 @@ def yes_four(x: Dict[int, str]):
del x[:] del x[:]
def yes_five(x: Dict[int, str]):
# FURB131
del x[:]
x = 1
# these should not # these should not
del names["key"] del names["key"]

View file

@ -4,7 +4,6 @@ use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::helpers::any_over_expr; use ruff_python_ast::helpers::any_over_expr;
use ruff_python_ast::{self as ast, Expr, Stmt}; use ruff_python_ast::{self as ast, Expr, Stmt};
use ruff_python_semantic::analyze::typing::is_dict; use ruff_python_semantic::analyze::typing::is_dict;
use ruff_python_semantic::Binding;
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
@ -129,22 +128,16 @@ pub(crate) fn manual_dict_comprehension(checker: &mut Checker, target: &Expr, bo
} }
// Exclude non-dictionary value. // Exclude non-dictionary value.
let Expr::Name(ast::ExprName { let Some(name) = subscript_value.as_name_expr() else {
id: subscript_name, .. return;
}) = subscript_value.as_ref() };
let Some(binding) = checker
.semantic()
.only_binding(name)
.map(|id| checker.semantic().binding(id))
else { else {
return; return;
}; };
let scope = checker.semantic().current_scope();
let bindings: Vec<&Binding> = scope
.get_all(subscript_name)
.map(|binding_id| checker.semantic().binding(binding_id))
.collect();
let [binding] = bindings.as_slice() else {
return;
};
if !is_dict(binding, checker.semantic()) { if !is_dict(binding, checker.semantic()) {
return; return;
} }
@ -165,8 +158,7 @@ pub(crate) fn manual_dict_comprehension(checker: &mut Checker, target: &Expr, bo
// ``` // ```
if if_test.is_some_and(|test| { if if_test.is_some_and(|test| {
any_over_expr(test, &|expr| { any_over_expr(test, &|expr| {
expr.as_name_expr() ComparableExpr::from(expr) == ComparableExpr::from(name)
.is_some_and(|expr| expr.id == *subscript_name)
}) })
}) { }) {
return; return;

View file

@ -5,7 +5,6 @@ use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::helpers::any_over_expr; use ruff_python_ast::helpers::any_over_expr;
use ruff_python_semantic::analyze::typing::is_list; use ruff_python_semantic::analyze::typing::is_list;
use ruff_python_semantic::Binding;
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
@ -144,20 +143,16 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo
} }
// Avoid non-list values. // Avoid non-list values.
let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { let Some(name) = value.as_name_expr() else {
return; return;
}; };
let bindings: Vec<&Binding> = checker let Some(binding) = checker
.semantic() .semantic()
.current_scope() .only_binding(name)
.get_all(id) .map(|id| checker.semantic().binding(id))
.map(|binding_id| checker.semantic().binding(binding_id)) else {
.collect();
let [binding] = bindings.as_slice() else {
return; return;
}; };
if !is_list(binding, checker.semantic()) { if !is_list(binding, checker.semantic()) {
return; return;
} }
@ -176,15 +171,12 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo
// ```python // ```python
// filtered = [x for x in y if x in filtered] // filtered = [x for x in y if x in filtered]
// ``` // ```
if let Some(value_name) = value.as_name_expr() { if if_test.is_some_and(|test| {
if if_test.is_some_and(|test| { any_over_expr(test, &|expr| {
any_over_expr(test, &|expr| { expr.as_name_expr().is_some_and(|expr| expr.id == name.id)
expr.as_name_expr() })
.is_some_and(|expr| expr.id == value_name.id) }) {
}) return;
}) {
return;
}
} }
checker checker

View file

@ -3,7 +3,6 @@ use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::any_over_expr; use ruff_python_ast::helpers::any_over_expr;
use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; use ruff_python_ast::{self as ast, Arguments, Expr, Stmt};
use ruff_python_semantic::analyze::typing::is_list; use ruff_python_semantic::analyze::typing::is_list;
use ruff_python_semantic::Binding;
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
@ -102,20 +101,16 @@ pub(crate) fn manual_list_copy(checker: &mut Checker, target: &Expr, body: &[Stm
} }
// Avoid non-list values. // Avoid non-list values.
let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { let Some(name) = value.as_name_expr() else {
return; return;
}; };
let bindings: Vec<&Binding> = checker let Some(binding) = checker
.semantic() .semantic()
.current_scope() .only_binding(name)
.get_all(id) .map(|id| checker.semantic().binding(id))
.map(|binding_id| checker.semantic().binding(binding_id)) else {
.collect();
let [binding] = bindings.as_slice() else {
return; return;
}; };
if !is_list(binding, checker.semantic()) { if !is_list(binding, checker.semantic()) {
return; return;
} }

View file

@ -97,9 +97,9 @@ pub(crate) fn check_and_remove_from_set(checker: &mut Checker, if_stmt: &ast::St
// Check if what we assume is set is indeed a set. // Check if what we assume is set is indeed a set.
if !checker if !checker
.semantic() .semantic()
.resolve_name(check_set) .only_binding(check_set)
.map(|binding_id| checker.semantic().binding(binding_id)) .map(|id| checker.semantic().binding(id))
.map_or(false, |binding| is_set(binding, checker.semantic())) .is_some_and(|binding| is_set(binding, checker.semantic()))
{ {
return; return;
}; };

View file

@ -2,7 +2,7 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation}; use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr}; use ruff_python_ast::{self as ast, Expr};
use ruff_python_semantic::analyze::typing::{is_dict, is_list}; use ruff_python_semantic::analyze::typing::{is_dict, is_list};
use ruff_python_semantic::{Binding, SemanticModel}; use ruff_python_semantic::SemanticModel;
use ruff_text_size::Ranged; use ruff_text_size::Ranged;
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
@ -70,7 +70,7 @@ pub(crate) fn delete_full_slice(checker: &mut Checker, delete: &ast::StmtDelete)
// Fix is only supported for single-target deletions. // Fix is only supported for single-target deletions.
if delete.targets.len() == 1 { if delete.targets.len() == 1 {
let replacement = generate_method_call(name, "clear", checker.generator()); let replacement = generate_method_call(&name.id, "clear", checker.generator());
diagnostic.set_fix(Fix::unsafe_edit(Edit::replacement( diagnostic.set_fix(Fix::unsafe_edit(Edit::replacement(
replacement, replacement,
delete.start(), delete.start(),
@ -83,7 +83,7 @@ pub(crate) fn delete_full_slice(checker: &mut Checker, delete: &ast::StmtDelete)
} }
/// Match `del expr[:]` where `expr` is a list or a dict. /// Match `del expr[:]` where `expr` is a list or a dict.
fn match_full_slice<'a>(expr: &'a Expr, semantic: &SemanticModel) -> Option<&'a str> { fn match_full_slice<'a>(expr: &'a Expr, semantic: &SemanticModel) -> Option<&'a ast::ExprName> {
// Check that it is `del expr[...]`. // Check that it is `del expr[...]`.
let subscript = expr.as_subscript_expr()?; let subscript = expr.as_subscript_expr()?;
@ -100,22 +100,9 @@ fn match_full_slice<'a>(expr: &'a Expr, semantic: &SemanticModel) -> Option<&'a
return None; return None;
} }
// Check that it is del var[:]
let ast::ExprName { id: name, .. } = subscript.value.as_name_expr()?;
// Let's find definition for var
let scope = semantic.current_scope();
let bindings: Vec<&Binding> = scope
.get_all(name)
.map(|binding_id| semantic.binding(binding_id))
.collect();
// NOTE: Maybe it is too strict of a limitation, but it seems reasonable.
let [binding] = bindings.as_slice() else {
return None;
};
// It should only apply to variables that are known to be lists or dicts. // It should only apply to variables that are known to be lists or dicts.
let name = subscript.value.as_name_expr()?;
let binding = semantic.binding(semantic.only_binding(name)?);
if !(is_dict(binding, semantic) || is_list(binding, semantic)) { if !(is_dict(binding, semantic) || is_list(binding, semantic)) {
return None; return None;
} }

View file

@ -76,7 +76,7 @@ impl Violation for RepeatedAppend {
/// FURB113 /// FURB113
pub(crate) fn repeated_append(checker: &mut Checker, stmt: &Stmt) { pub(crate) fn repeated_append(checker: &mut Checker, stmt: &Stmt) {
let Some(appends) = match_consecutive_appends(checker.semantic(), stmt) else { let Some(appends) = match_consecutive_appends(stmt, checker.semantic()) else {
return; return;
}; };
@ -163,8 +163,8 @@ impl Ranged for AppendGroup<'_> {
/// Match consecutive calls to `append` on list variables starting from the given statement. /// Match consecutive calls to `append` on list variables starting from the given statement.
fn match_consecutive_appends<'a>( fn match_consecutive_appends<'a>(
semantic: &'a SemanticModel,
stmt: &'a Stmt, stmt: &'a Stmt,
semantic: &'a SemanticModel,
) -> Option<Vec<Append<'a>>> { ) -> Option<Vec<Append<'a>>> {
// Match the current statement, to see if it's an append. // Match the current statement, to see if it's an append.
let append = match_append(semantic, stmt)?; let append = match_append(semantic, stmt)?;

View file

@ -6,7 +6,7 @@ use ruff_python_ast as ast;
use ruff_python_ast::{Arguments, Expr, Int}; use ruff_python_ast::{Arguments, Expr, Int};
use ruff_python_codegen::Generator; use ruff_python_codegen::Generator;
use ruff_python_semantic::analyze::typing::{is_dict, is_list, is_set, is_tuple}; use ruff_python_semantic::analyze::typing::{is_dict, is_list, is_set, is_tuple};
use ruff_python_semantic::Binding;
use ruff_text_size::{Ranged, TextRange}; use ruff_text_size::{Ranged, TextRange};
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
@ -114,7 +114,7 @@ pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtF
}; };
// Get the first argument, which is the sequence to iterate over. // Get the first argument, which is the sequence to iterate over.
let Some(Expr::Name(ast::ExprName { id: sequence, .. })) = arguments.args.first() else { let Some(Expr::Name(sequence)) = arguments.args.first() else {
return; return;
}; };
@ -138,7 +138,8 @@ pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtF
); );
// The index is unused, so replace with `for value in sequence`. // The index is unused, so replace with `for value in sequence`.
let replace_iter = Edit::range_replacement(sequence.into(), stmt_for.iter.range()); let replace_iter =
Edit::range_replacement(sequence.id.to_string(), stmt_for.iter.range());
let replace_target = Edit::range_replacement( let replace_target = Edit::range_replacement(
pad( pad(
checker.locator().slice(value).to_string(), checker.locator().slice(value).to_string(),
@ -154,12 +155,11 @@ pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtF
(false, true) => { (false, true) => {
// Ensure the sequence object works with `len`. If it doesn't, the // Ensure the sequence object works with `len`. If it doesn't, the
// fix is unclear. // fix is unclear.
let scope = checker.semantic().current_scope(); let Some(binding) = checker
let bindings: Vec<&Binding> = scope .semantic()
.get_all(sequence) .only_binding(sequence)
.map(|binding_id| checker.semantic().binding(binding_id)) .map(|id| checker.semantic().binding(id))
.collect(); else {
let [binding] = bindings.as_slice() else {
return; return;
}; };
// This will lead to a lot of false negatives, but it is the best // This will lead to a lot of false negatives, but it is the best
@ -193,7 +193,7 @@ pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtF
) )
}) { }) {
let replace_iter = Edit::range_replacement( let replace_iter = Edit::range_replacement(
generate_range_len_call(sequence, checker.generator()), generate_range_len_call(&sequence.id, checker.generator()),
stmt_for.iter.range(), stmt_for.iter.range(),
); );

View file

@ -127,6 +127,27 @@ FURB131.py:43:5: FURB131 [*] Prefer `clear` over deleting a full slice
43 |+ x.clear() 43 |+ x.clear()
44 44 | 44 44 |
45 45 | 45 45 |
46 46 | # these should not 46 46 | def yes_five(x: Dict[int, str]):
FURB131.py:48:5: FURB131 [*] Prefer `clear` over deleting a full slice
|
46 | def yes_five(x: Dict[int, str]):
47 | # FURB131
48 | del x[:]
| ^^^^^^^^ FURB131
49 |
50 | x = 1
|
= help: Replace with `clear()`
Suggested fix
45 45 |
46 46 | def yes_five(x: Dict[int, str]):
47 47 | # FURB131
48 |- del x[:]
48 |+ x.clear()
49 49 |
50 50 | x = 1
51 51 |

View file

@ -616,6 +616,16 @@ impl<'a> SemanticModel<'a> {
self.resolved_names.get(&name.into()).copied() self.resolved_names.get(&name.into()).copied()
} }
/// Resolves the [`ast::ExprName`] to the [`BindingId`] of the symbol it refers to, if it's the
/// only binding to that name in its scope.
pub fn only_binding(&self, name: &ast::ExprName) -> Option<BindingId> {
self.resolve_name(name).filter(|id| {
let binding = self.binding(*id);
let scope = &self.scopes[binding.scope];
scope.shadowed_binding(*id).is_none()
})
}
/// Resolves the [`Expr`] to a fully-qualified symbol-name, if `value` resolves to an imported /// Resolves the [`Expr`] to a fully-qualified symbol-name, if `value` resolves to an imported
/// or builtin symbol. /// or builtin symbol.
/// ///