Python
/
ruff
mirror of https://github.com/astral-sh/ruff
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Track overridden bindings within each scope (#2511)

This commit is contained in:
Charlie Marsh 2023-02-02 22:31:46 -05:00 committed by GitHub
parent a074625121
commit d4cef9305a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 148 additions and 68 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
resources/test/fixtures/flake8_bugbear/B007.py vendored
View File

@ -64,9 +64,19 @@ def f():
def f():
# Fixable, but not fixed in practice, since we can't tell if `bar` is used.
# Fixable.
for foo, bar, baz in (["1", "2", "3"],):
if foo or baz:
break
bar = 1
def f():
# Fixable.
for foo, bar, baz in (["1", "2", "3"],):
if foo or baz:
break
bar = 1
print(bar)

2
src/ast/operations.rs
View File

@ -97,7 +97,7 @@ pub fn extract_all_names(
// Grab the existing bound __all__ values.
if let StmtKind::AugAssign { .. } = &stmt.node {
if let Some(index) = scope.values.get("__all__") {
if let Some(index) = scope.bindings.get("__all__") {
if let BindingKind::Export(existing) = &checker.bindings[*index].kind {
names.extend_from_slice(existing);
}

10
src/ast/types.rs
View File

@ -87,8 +87,11 @@ pub struct Scope<'a> {
pub kind: ScopeKind<'a>,
pub import_starred: bool,
pub uses_locals: bool,
/// A map from bound name to binding index.
pub values: FxHashMap<&'a str, usize>,
/// A map from bound name to binding index, for live bindings.
pub bindings: FxHashMap<&'a str, usize>,
/// A map from bound name to binding index, for bindings that were created in the scope but
/// rebound (and thus overridden) later on in the same scope.
pub rebounds: FxHashMap<&'a str, Vec<usize>>,
}
impl<'a> Scope<'a> {
@ -98,7 +101,8 @@ impl<'a> Scope<'a> {
kind,
import_starred: false,
uses_locals: false,
values: FxHashMap::default(),
bindings: FxHashMap::default(),
rebounds: FxHashMap::default(),
}
}
}

84
src/checkers/ast.rs
View File

@ -74,7 +74,9 @@ pub struct Checker<'a> {
pub(crate) parents: Vec<RefEquality<'a, Stmt>>,
pub(crate) depths: FxHashMap<RefEquality<'a, Stmt>, usize>,
pub(crate) child_to_parent: FxHashMap<RefEquality<'a, Stmt>, RefEquality<'a, Stmt>>,
// A stack of all bindings created in any scope, at any point in execution.
pub(crate) bindings: Vec<Binding<'a>>,
// Map from binding index to indexes of bindings that redefine it in other scopes.
pub(crate) redefinitions: IntMap<usize, Vec<usize>>,
pub(crate) exprs: Vec<RefEquality<'a, Expr>>,
pub(crate) scopes: Vec<Scope<'a>>,
@ -212,7 +214,7 @@ impl<'a> Checker<'a> {
/// Return the current `Binding` for a given `name`.
pub fn find_binding(&self, member: &str) -> Option<&Binding> {
self.current_scopes()
.find_map(|scope| scope.values.get(member))
.find_map(|scope| scope.bindings.get(member))
.map(|index| &self.bindings[*index])
}
@ -352,7 +354,7 @@ where
source: Some(RefEquality(stmt)),
context,
});
scope.values.insert(name, index);
scope.bindings.insert(name, index);
}
}
@ -382,7 +384,7 @@ where
source: Some(RefEquality(stmt)),
context,
});
scope.values.insert(name, index);
scope.bindings.insert(name, index);
}
// Mark the binding in the defining scopes as used too. (Skip the global scope
@ -390,7 +392,7 @@ where
for (name, range) in names.iter().zip(ranges.iter()) {
let mut exists = false;
for index in self.scope_stack.iter().skip(1).rev().skip(1) {
if let Some(index) = self.scopes[*index].values.get(&name.as_str()) {
if let Some(index) = self.scopes[*index].bindings.get(&name.as_str()) {
exists = true;
self.bindings[*index].runtime_usage = usage;
}
@ -1768,7 +1770,7 @@ where
let globals = operations::extract_globals(body);
for (name, stmt) in operations::extract_globals(body) {
if self.scopes[GLOBAL_SCOPE_INDEX]
.values
.bindings
.get(name)
.map_or(true, |index| {
matches!(self.bindings[*index].kind, BindingKind::Annotation)
@ -1784,7 +1786,7 @@ where
source: Some(RefEquality(stmt)),
context: self.execution_context(),
});
self.scopes[GLOBAL_SCOPE_INDEX].values.insert(name, index);
self.scopes[GLOBAL_SCOPE_INDEX].bindings.insert(name, index);
}
}
@ -1832,7 +1834,7 @@ where
let globals = operations::extract_globals(body);
for (name, stmt) in &globals {
if self.scopes[GLOBAL_SCOPE_INDEX]
.values
.bindings
.get(name)
.map_or(true, |index| {
matches!(self.bindings[*index].kind, BindingKind::Annotation)
@ -1848,7 +1850,7 @@ where
source: Some(RefEquality(stmt)),
context: self.execution_context(),
});
self.scopes[GLOBAL_SCOPE_INDEX].values.insert(name, index);
self.scopes[GLOBAL_SCOPE_INDEX].bindings.insert(name, index);
}
}
@ -3481,7 +3483,7 @@ where
let name_range =
helpers::excepthandler_name_range(excepthandler, self.locator).unwrap();
if self.current_scope().values.contains_key(&name.as_str()) {
if self.current_scope().bindings.contains_key(&name.as_str()) {
self.handle_node_store(
name,
&Expr::new(
@ -3495,7 +3497,7 @@ where
);
}
let definition = self.current_scope().values.get(&name.as_str()).copied();
let definition = self.current_scope().bindings.get(&name.as_str()).copied();
self.handle_node_store(
name,
&Expr::new(
@ -3513,7 +3515,7 @@ where
if let Some(index) = {
let scope = &mut self.scopes
[*(self.scope_stack.last().expect("No current scope found"))];
&scope.values.remove(&name.as_str())
&scope.bindings.remove(&name.as_str())
} {
if !self.bindings[*index].used() {
if self.settings.rules.enabled(&Rule::UnusedVariable) {
@ -3548,7 +3550,7 @@ where
if let Some(index) = definition {
let scope = &mut self.scopes
[*(self.scope_stack.last().expect("No current scope found"))];
scope.values.insert(name, index);
scope.bindings.insert(name, index);
}
}
None => walk_excepthandler(self, excepthandler),
@ -3743,7 +3745,7 @@ impl<'a> Checker<'a> {
source: None,
context: ExecutionContext::Runtime,
});
scope.values.insert(builtin, index);
scope.bindings.insert(builtin, index);
}
}
@ -3810,9 +3812,9 @@ impl<'a> Checker<'a> {
.iter()
.rev()
.enumerate()
.find(|(_, scope_index)| self.scopes[**scope_index].values.contains_key(&name))
.find(|(_, scope_index)| self.scopes[**scope_index].bindings.contains_key(&name))
{
let existing_binding_index = self.scopes[*scope_index].values.get(&name).unwrap();
let existing_binding_index = self.scopes[*scope_index].bindings.get(&name).unwrap();
let existing = &self.bindings[*existing_binding_index];
let in_current_scope = stack_index == 0;
if !matches!(existing.kind, BindingKind::Builtin)
@ -3875,7 +3877,7 @@ impl<'a> Checker<'a> {
}
let scope = self.current_scope();
let binding = if let Some(index) = scope.values.get(&name) {
let binding = if let Some(index) = scope.bindings.get(&name) {
if matches!(self.bindings[*index].kind, BindingKind::Builtin) {
// Avoid overriding builtins.
binding
@ -3914,8 +3916,14 @@ impl<'a> Checker<'a> {
// Don't treat annotations as assignments if there is an existing value
// in scope.
let scope = &mut self.scopes[*(self.scope_stack.last().expect("No current scope found"))];
if !(matches!(binding.kind, BindingKind::Annotation) && scope.values.contains_key(name)) {
scope.values.insert(name, binding_index);
if !(matches!(binding.kind, BindingKind::Annotation) && scope.bindings.contains_key(name)) {
if let Some(rebound_index) = scope.bindings.insert(name, binding_index) {
scope
.rebounds
.entry(name)
.or_insert_with(Vec::new)
.push(rebound_index);
}
}
self.bindings.push(binding);
@ -3940,7 +3948,7 @@ impl<'a> Checker<'a> {
}
}
if let Some(index) = scope.values.get(&id.as_str()) {
if let Some(index) = scope.bindings.get(&id.as_str()) {
// Mark the binding as used.
let context = self.execution_context();
self.bindings[*index].mark_used(scope_id, Range::from_located(expr), context);
@ -3970,7 +3978,7 @@ impl<'a> Checker<'a> {
.unwrap_or_default();
if has_alias {
// Mark the sub-importation as used.
if let Some(index) = scope.values.get(full_name) {
if let Some(index) = scope.bindings.get(full_name) {
self.bindings[*index].mark_used(
scope_id,
Range::from_located(expr),
@ -3987,7 +3995,7 @@ impl<'a> Checker<'a> {
.unwrap_or_default();
if has_alias {
// Mark the sub-importation as used.
if let Some(index) = scope.values.get(full_name.as_str()) {
if let Some(index) = scope.bindings.get(full_name.as_str()) {
self.bindings[*index].mark_used(
scope_id,
Range::from_located(expr),
@ -4012,7 +4020,7 @@ impl<'a> Checker<'a> {
let mut from_list = vec![];
for scope_index in self.scope_stack.iter().rev() {
let scope = &self.scopes[*scope_index];
for binding in scope.values.values().map(|index| &self.bindings[*index]) {
for binding in scope.bindings.values().map(|index| &self.bindings[*index]) {
if let BindingKind::StarImportation(level, module) = &binding.kind {
from_list.push(helpers::format_import_from(
level.as_ref(),
@ -4090,9 +4098,14 @@ impl<'a> Checker<'a> {
{
if matches!(self.current_scope().kind, ScopeKind::Function(..)) {
// Ignore globals.
if !self.current_scope().values.get(id).map_or(false, |index| {
matches!(self.bindings[*index].kind, BindingKind::Global)
}) {
if !self
.current_scope()
.bindings
.get(id)
.map_or(false, |index| {
matches!(self.bindings[*index].kind, BindingKind::Global)
})
{
pep8_naming::rules::non_lowercase_variable_in_function(self, expr, parent, id);
}
}
@ -4261,7 +4274,7 @@ impl<'a> Checker<'a> {
let scope =
&mut self.scopes[*(self.scope_stack.last().expect("No current scope found"))];
if scope.values.remove(&id.as_str()).is_none()
if scope.bindings.remove(&id.as_str()).is_none()
&& self.settings.rules.enabled(&Rule::UndefinedName)
{
self.diagnostics.push(Diagnostic::new(
@ -4500,7 +4513,7 @@ impl<'a> Checker<'a> {
.iter()
.map(|scope| {
scope
.values
.bindings
.values()
.map(|index| &self.bindings[*index])
.filter(|binding| {
@ -4523,7 +4536,7 @@ impl<'a> Checker<'a> {
.rules
.enabled(&Rule::GlobalVariableNotAssigned)
{
for (name, index) in &scope.values {
for (name, index) in &scope.bindings {
let binding = &self.bindings[*index];
if matches!(binding.kind, BindingKind::Global) {
if let Some(stmt) = &binding.source {
@ -4546,7 +4559,7 @@ impl<'a> Checker<'a> {
}
let all_binding: Option<&Binding> = scope
.values
.bindings
.get("__all__")
.map(|index| &self.bindings[*index]);
let all_names: Option<Vec<&str>> =
@ -4560,7 +4573,7 @@ impl<'a> Checker<'a> {
if let Some(all_binding) = all_binding {
if let Some(names) = &all_names {
for &name in names {
if !scope.values.contains_key(name) {
if !scope.bindings.contains_key(name) {
diagnostics.push(Diagnostic::new(
pyflakes::rules::UndefinedExport {
name: name.to_string(),
@ -4578,7 +4591,7 @@ impl<'a> Checker<'a> {
// unused. Note that we only store references in `redefinitions` if
// the bindings are in different scopes.
if self.settings.rules.enabled(&Rule::RedefinedWhileUnused) {
for (name, index) in &scope.values {
for (name, index) in &scope.bindings {
let binding = &self.bindings[*index];
if matches!(
@ -4619,7 +4632,8 @@ impl<'a> Checker<'a> {
if let Some(all_binding) = all_binding {
if let Some(names) = &all_names {
let mut from_list = vec![];
for binding in scope.values.values().map(|index| &self.bindings[*index])
for binding in
scope.bindings.values().map(|index| &self.bindings[*index])
{
if let BindingKind::StarImportation(level, module) = &binding.kind {
from_list.push(helpers::format_import_from(
@ -4631,7 +4645,7 @@ impl<'a> Checker<'a> {
from_list.sort();
for &name in names {
if !scope.values.contains_key(name) {
if !scope.bindings.contains_key(name) {
diagnostics.push(Diagnostic::new(
pyflakes::rules::ImportStarUsage {
name: name.to_string(),
@ -4673,7 +4687,7 @@ impl<'a> Checker<'a> {
.copied()
.collect()
};
for (.., index) in &scope.values {
for (.., index) in &scope.bindings {
let binding = &self.bindings[*index];
if let Some(diagnostic) =
@ -4707,7 +4721,7 @@ impl<'a> Checker<'a> {
let mut ignored: FxHashMap<BindingContext, Vec<UnusedImport>> =
FxHashMap::default();
for (name, index) in &scope.values {
for (name, index) in &scope.bindings {
let binding = &self.bindings[*index];
let full_name = match &binding.kind {

44
src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs
View File

@ -21,6 +21,7 @@
use rustc_hash::FxHashMap;
use rustpython_ast::{Expr, ExprKind, Stmt};
use serde::{Deserialize, Serialize};
use std::iter;
use ruff_macros::derive_message_formats;
@ -145,22 +146,37 @@ pub fn unused_loop_control_variable(
Range::from_located(expr),
);
if matches!(certainty, Certainty::Certain) && checker.patch(diagnostic.kind.rule()) {
// Guard against cases in which the loop variable is used later on in the scope.
// TODO(charlie): This avoids fixing cases in which the loop variable is overridden (but
// unused) later on in the scope.
if let Some(binding) = checker.find_binding(name) {
// Find the `BindingKind::LoopVar` corresponding to the name.
let scope = checker.current_scope();
if let Some(binding) = iter::once(scope.bindings.get(name))
.flatten()
.chain(
iter::once(scope.rebounds.get(name))
.flatten()
.into_iter()
.flatten(),
)
.find_map(|index| {
let binding = &checker.bindings[*index];
if let Some(source) = &binding.source {
if source == &RefEquality(stmt) {
Some(binding)
} else {
None
}
} else {
None
}
})
{
if matches!(binding.kind, BindingKind::LoopVar) {
if !binding.used() {
if let Some(source) = &binding.source {
if source == &RefEquality(stmt) {
// Prefix the variable name with an underscore.
diagnostic.amend(Fix::replacement(
format!("_{name}"),
expr.location,
expr.end_location.unwrap(),
));
}
}
// Prefix the variable name with an underscore.
diagnostic.amend(Fix::replacement(
format!("_{name}"),
expr.location,
expr.end_location.unwrap(),
));
}
}
}

40
src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B007_B007.py.snap
View File

@ -24,7 +24,15 @@ expression: diagnostics
end_location:
row: 18
column: 13
fix: ~
fix:
content:
- _k
location:
row: 18
column: 12
end_location:
row: 18
column: 13
parent: ~
- kind:
UnusedLoopControlVariable:
@ -148,6 +156,34 @@ expression: diagnostics
end_location:
row: 68
column: 16
fix: ~
fix:
content:
- _bar
location:
row: 68
column: 13
end_location:
row: 68
column: 16
parent: ~
- kind:
UnusedLoopControlVariable:
name: bar
certainty: Certain
location:
row: 77
column: 13
end_location:
row: 77
column: 16
fix:
content:
- _bar
location:
row: 77
column: 13
end_location:
row: 77
column: 16
parent: ~

10
src/rules/flake8_unused_arguments/rules.rs
View File

@ -135,7 +135,7 @@ pub fn unused_arguments(
function(
&Argumentable::Function,
args,
&scope.values,
&scope.bindings,
bindings,
&checker.settings.dummy_variable_rgx,
checker
@ -164,7 +164,7 @@ pub fn unused_arguments(
method(
&Argumentable::Method,
args,
&scope.values,
&scope.bindings,
bindings,
&checker.settings.dummy_variable_rgx,
checker
@ -193,7 +193,7 @@ pub fn unused_arguments(
method(
&Argumentable::ClassMethod,
args,
&scope.values,
&scope.bindings,
bindings,
&checker.settings.dummy_variable_rgx,
checker
@ -222,7 +222,7 @@ pub fn unused_arguments(
function(
&Argumentable::StaticMethod,
args,
&scope.values,
&scope.bindings,
bindings,
&checker.settings.dummy_variable_rgx,
checker
@ -245,7 +245,7 @@ pub fn unused_arguments(
function(
&Argumentable::Lambda,
args,
&scope.values,
&scope.bindings,
bindings,
&checker.settings.dummy_variable_rgx,
checker

4
src/rules/pyflakes/rules/undefined_local.rs
View File

@ -24,10 +24,10 @@ impl Violation for UndefinedLocal {
/// F821
pub fn undefined_local(name: &str, scopes: &[&Scope], bindings: &[Binding]) -> Option<Diagnostic> {
let current = &scopes.last().expect("No current scope found");
if matches!(current.kind, ScopeKind::Function(_)) && !current.values.contains_key(name) {
if matches!(current.kind, ScopeKind::Function(_)) && !current.bindings.contains_key(name) {
for scope in scopes.iter().rev().skip(1) {
if matches!(scope.kind, ScopeKind::Function(_) | ScopeKind::Module) {
if let Some(binding) = scope.values.get(name).map(|index| &bindings[*index]) {
if let Some(binding) = scope.bindings.get(name).map(|index| &bindings[*index]) {
if let Some((scope_id, location)) = binding.runtime_usage {
if scope_id == current.id {
return Some(Diagnostic::new(

2
src/rules/pyflakes/rules/unused_annotation.rs
View File

@ -23,7 +23,7 @@ impl Violation for UnusedAnnotation {
pub fn unused_annotation(checker: &mut Checker, scope: usize) {
let scope = &checker.scopes[scope];
for (name, binding) in scope
.values
.bindings
.iter()
.map(|(name, index)| (name, &checker.bindings[*index]))
{

2
src/rules/pyflakes/rules/unused_variable.rs
View File

@ -182,7 +182,7 @@ pub fn unused_variable(checker: &mut Checker, scope: usize) {
}
for (name, binding) in scope
.values
.bindings
.iter()
.map(|(name, index)| (name, &checker.bindings[*index]))
{

4
src/rules/pylint/rules/use_sys_exit.rs
View File

@ -30,7 +30,7 @@ impl Violation for UseSysExit {
/// sys import *`).
fn is_module_star_imported(checker: &Checker, module: &str) -> bool {
checker.current_scopes().any(|scope| {
scope.values.values().any(|index| {
scope.bindings.values().any(|index| {
if let BindingKind::StarImportation(_, name) = &checker.bindings[*index].kind {
name.as_ref().map(|name| name == module).unwrap_or_default()
} else {
@ -45,7 +45,7 @@ fn is_module_star_imported(checker: &Checker, module: &str) -> bool {
fn get_member_import_name_alias(checker: &Checker, module: &str, member: &str) -> Option<String> {
checker.current_scopes().find_map(|scope| {
scope
.values
.bindings
.values()
.find_map(|index| match &checker.bindings[*index].kind {
// e.g. module=sys object=exit

2
src/rules/pyupgrade/rules/useless_object_inheritance.rs
View File

@ -35,7 +35,7 @@ fn rule(name: &str, bases: &[Expr], scope: &Scope, bindings: &[Binding]) -> Opti
}
if !matches!(
scope
.values
.bindings
.get(&id.as_str())
.map(|index| &bindings[*index]),
None | Some(Binding {