Skip to content

Commit

Permalink
Add dedicated method to find typed binding (#8517)
Browse files Browse the repository at this point in the history
## 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`.
  • Loading branch information
charliermarsh authored Nov 6, 2023
1 parent 5b3e922 commit eab8ca4
Show file tree
Hide file tree
Showing 10 changed files with 81 additions and 78 deletions.
6 changes: 6 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB131.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ def yes_four(x: Dict[int, str]):
del x[:]


def yes_five(x: Dict[int, str]):
# FURB131
del x[:]

x = 1

# these should not

del names["key"]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::helpers::any_over_expr;
use ruff_python_ast::{self as ast, Expr, Stmt};
use ruff_python_semantic::analyze::typing::is_dict;
use ruff_python_semantic::Binding;

use crate::checkers::ast::Checker;

Expand Down Expand Up @@ -129,22 +128,16 @@ pub(crate) fn manual_dict_comprehension(checker: &mut Checker, target: &Expr, bo
}

// Exclude non-dictionary value.
let Expr::Name(ast::ExprName {
id: subscript_name, ..
}) = subscript_value.as_ref()
else {
let Some(name) = subscript_value.as_name_expr() else {
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 {
let Some(binding) = checker
.semantic()
.only_binding(name)
.map(|id| checker.semantic().binding(id))
else {
return;
};

if !is_dict(binding, checker.semantic()) {
return;
}
Expand All @@ -165,8 +158,7 @@ pub(crate) fn manual_dict_comprehension(checker: &mut Checker, target: &Expr, bo
// ```
if if_test.is_some_and(|test| {
any_over_expr(test, &|expr| {
expr.as_name_expr()
.is_some_and(|expr| expr.id == *subscript_name)
ComparableExpr::from(expr) == ComparableExpr::from(name)
})
}) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::helpers::any_over_expr;
use ruff_python_semantic::analyze::typing::is_list;
use ruff_python_semantic::Binding;

use crate::checkers::ast::Checker;

Expand Down Expand Up @@ -144,20 +143,16 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo
}

// Avoid non-list values.
let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else {
let Some(name) = value.as_name_expr() else {
return;
};
let bindings: Vec<&Binding> = checker
let Some(binding) = checker
.semantic()
.current_scope()
.get_all(id)
.map(|binding_id| checker.semantic().binding(binding_id))
.collect();

let [binding] = bindings.as_slice() else {
.only_binding(name)
.map(|id| checker.semantic().binding(id))
else {
return;
};

if !is_list(binding, checker.semantic()) {
return;
}
Expand All @@ -176,15 +171,12 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo
// ```python
// 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| {
any_over_expr(test, &|expr| {
expr.as_name_expr()
.is_some_and(|expr| expr.id == value_name.id)
})
}) {
return;
}
if if_test.is_some_and(|test| {
any_over_expr(test, &|expr| {
expr.as_name_expr().is_some_and(|expr| expr.id == name.id)
})
}) {
return;
}

checker
Expand Down
15 changes: 5 additions & 10 deletions crates/ruff_linter/src/rules/perflint/rules/manual_list_copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::any_over_expr;
use ruff_python_ast::{self as ast, Arguments, Expr, Stmt};
use ruff_python_semantic::analyze::typing::is_list;
use ruff_python_semantic::Binding;

use crate::checkers::ast::Checker;

Expand Down Expand Up @@ -102,20 +101,16 @@ pub(crate) fn manual_list_copy(checker: &mut Checker, target: &Expr, body: &[Stm
}

// Avoid non-list values.
let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else {
let Some(name) = value.as_name_expr() else {
return;
};
let bindings: Vec<&Binding> = checker
let Some(binding) = checker
.semantic()
.current_scope()
.get_all(id)
.map(|binding_id| checker.semantic().binding(binding_id))
.collect();

let [binding] = bindings.as_slice() else {
.only_binding(name)
.map(|id| checker.semantic().binding(id))
else {
return;
};

if !is_list(binding, checker.semantic()) {
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
if !checker
.semantic()
.resolve_name(check_set)
.map(|binding_id| checker.semantic().binding(binding_id))
.map_or(false, |binding| is_set(binding, checker.semantic()))
.only_binding(check_set)
.map(|id| checker.semantic().binding(id))
.is_some_and(|binding| is_set(binding, checker.semantic()))
{
return;
};
Expand Down
23 changes: 5 additions & 18 deletions crates/ruff_linter/src/rules/refurb/rules/delete_full_slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr};
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 crate::checkers::ast::Checker;
Expand Down Expand Up @@ -70,7 +70,7 @@ pub(crate) fn delete_full_slice(checker: &mut Checker, delete: &ast::StmtDelete)

// Fix is only supported for single-target deletions.
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(
replacement,
delete.start(),
Expand All @@ -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.
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[...]`.
let subscript = expr.as_subscript_expr()?;

Expand All @@ -100,22 +100,9 @@ fn match_full_slice<'a>(expr: &'a Expr, semantic: &SemanticModel) -> Option<&'a
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.
let name = subscript.value.as_name_expr()?;
let binding = semantic.binding(semantic.only_binding(name)?);
if !(is_dict(binding, semantic) || is_list(binding, semantic)) {
return None;
}
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/refurb/rules/repeated_append.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ impl Violation for RepeatedAppend {

/// FURB113
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;
};

Expand Down Expand Up @@ -163,8 +163,8 @@ impl Ranged for AppendGroup<'_> {

/// Match consecutive calls to `append` on list variables starting from the given statement.
fn match_consecutive_appends<'a>(
semantic: &'a SemanticModel,
stmt: &'a Stmt,
semantic: &'a SemanticModel,
) -> Option<Vec<Append<'a>>> {
// Match the current statement, to see if it's an append.
let append = match_append(semantic, stmt)?;
Expand Down
20 changes: 10 additions & 10 deletions crates/ruff_linter/src/rules/refurb/rules/unnecessary_enumerate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use ruff_python_ast as ast;
use ruff_python_ast::{Arguments, Expr, Int};
use ruff_python_codegen::Generator;
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 crate::checkers::ast::Checker;
Expand Down Expand Up @@ -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.
let Some(Expr::Name(ast::ExprName { id: sequence, .. })) = arguments.args.first() else {
let Some(Expr::Name(sequence)) = arguments.args.first() else {
return;
};

Expand All @@ -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`.
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(
pad(
checker.locator().slice(value).to_string(),
Expand All @@ -154,12 +155,11 @@ pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtF
(false, true) => {
// Ensure the sequence object works with `len`. If it doesn't, the
// fix is unclear.
let scope = checker.semantic().current_scope();
let bindings: Vec<&Binding> = scope
.get_all(sequence)
.map(|binding_id| checker.semantic().binding(binding_id))
.collect();
let [binding] = bindings.as_slice() else {
let Some(binding) = checker
.semantic()
.only_binding(sequence)
.map(|id| checker.semantic().binding(id))
else {
return;
};
// This will lead to a lot of false negatives, but it is the best
Expand Down Expand Up @@ -193,7 +193,7 @@ pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtF
)
}) {
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(),
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,27 @@ FURB131.py:43:5: FURB131 [*] Prefer `clear` over deleting a full slice
43 |+ x.clear()
44 44 |
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 |


10 changes: 10 additions & 0 deletions crates/ruff_python_semantic/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,16 @@ impl<'a> SemanticModel<'a> {
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
/// or builtin symbol.
///
Expand Down

0 comments on commit eab8ca4

Please sign in to comment.