mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-08-04 10:48:32 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Track conditional deletions in the semantic model (#10415)

Browse source 
## Summary

Given `del X`, we'll typically add a `BindingKind::Deletion` to `X` to
shadow the current binding. However, if the deletion is inside of a
conditional operation, we _won't_, as in:

```python
def f():
    global X

    if X > 0:
        del X
```

We will, however, track it as a reference to the binding. This PR adds
the expression context to those resolved references, so that we can
detect that the `X` in `global X` was "assigned to".

Closes https://github.com/astral-sh/ruff/issues/10397.
This commit is contained in:
Charlie Marsh 2024-03-14 17:45:46 -07:00 • committed by GitHub
parent a8e50a7f40
commit 10ace88e9a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 133 additions and 45 deletions
Download patch file Download diff file Expand all files Collapse all files

7
crates/ruff_linter/resources/test/fixtures/pylint/global_variable_not_assigned.py vendored
View file

@ -11,6 +11,13 @@ def f():
print(X)
def f():
global X
if X > 0:
del X
###
# Non-errors.
###

30
crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs
View file

@ -1,6 +1,6 @@
use ruff_diagnostics::{Diagnostic, Fix};
use ruff_python_semantic::analyze::visibility;
use ruff_python_semantic::{Binding, BindingKind, Imported, ScopeKind};
use ruff_python_semantic::{Binding, BindingKind, Imported, ResolvedReference, ScopeKind};
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
@ -91,13 +91,29 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
if checker.enabled(Rule::GlobalVariableNotAssigned) {
for (name, binding_id) in scope.bindings() {
let binding = checker.semantic.binding(binding_id);
// If the binding is a `global`, then it's a top-level `global` that was never
// assigned in the current scope. If it were assigned, the `global` would be
// shadowed by the assignment.
if binding.kind.is_global() {
diagnostics.push(Diagnostic::new(
pylint::rules::GlobalVariableNotAssigned {
name: (*name).to_string(),
},
binding.range(),
));
// If the binding was conditionally deleted, it will include a reference within
// a `Del` context, but won't be shadowed by a `BindingKind::Deletion`, as in:
// ```python
// if condition:
// del var
// ```
if binding
.references
.iter()
.map(|id| checker.semantic.reference(*id))
.all(ResolvedReference::is_load)
{
diagnostics.push(Diagnostic::new(
pylint::rules::GlobalVariableNotAssigned {
name: (*name).to_string(),
},
binding.range(),
));
}
}
}
}

9
crates/ruff_linter/src/checkers/ast/mod.rs
View file

@ -540,7 +540,11 @@ impl<'a> Visitor<'a> for Checker<'a> {
for name in names {
if let Some((scope_id, binding_id)) = self.semantic.nonlocal(name) {
// Mark the binding as "used".
self.semantic.add_local_reference(binding_id, name.range());
self.semantic.add_local_reference(
binding_id,
ExprContext::Load,
name.range(),
);
// Mark the binding in the enclosing scope as "rebound" in the current
// scope.
@ -2113,7 +2117,8 @@ impl<'a> Checker<'a> {
// Mark anything referenced in `__all__` as used.
// TODO(charlie): `range` here should be the range of the name in `__all__`, not
// the range of `__all__` itself.
self.semantic.add_global_reference(binding_id, range);
self.semantic
.add_global_reference(binding_id, ExprContext::Load, range);
} else {
if self.semantic.global_scope().uses_star_imports() {
if self.enabled(Rule::UndefinedLocalWithImportStarUsage) {

1
crates/ruff_linter/src/renamer.rs
View file

@ -255,6 +255,7 @@ impl Renamer {
| BindingKind::ClassDefinition(_)
| BindingKind::FunctionDefinition(_)
| BindingKind::Deletion
| BindingKind::ConditionalDeletion(_)
| BindingKind::UnboundException(_) => {
Some(Edit::range_replacement(target.to_string(), binding.range()))
}

8
crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs
View file

@ -121,8 +121,7 @@ pub(crate) fn runtime_import_in_type_checking_block(
checker
.semantic()
.reference(reference_id)
.context()
.is_runtime()
.in_runtime_context()
})
{
let Some(node_id) = binding.source else {
@ -155,8 +154,7 @@ pub(crate) fn runtime_import_in_type_checking_block(
if checker.settings.flake8_type_checking.quote_annotations
&& binding.references().all(|reference_id| {
let reference = checker.semantic().reference(reference_id);
reference.context().is_typing()
|| reference.in_runtime_evaluated_annotation()
reference.in_typing_context() || reference.in_runtime_evaluated_annotation()
})
{
actions
@ -268,7 +266,7 @@ fn quote_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding])
.flat_map(|ImportBinding { binding, .. }| {
binding.references.iter().filter_map(|reference_id| {
let reference = checker.semantic().reference(*reference_id);
if reference.context().is_runtime() {
if reference.in_runtime_context() {
Some(quote_annotation(
reference.expression_id()?,
checker.semantic(),

2
crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs
View file

@ -499,7 +499,7 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) ->
.flat_map(|ImportBinding { binding, .. }| {
binding.references.iter().filter_map(|reference_id| {
let reference = checker.semantic().reference(*reference_id);
if reference.context().is_runtime() {
if reference.in_runtime_context() {
Some(quote_annotation(
reference.expression_id()?,
checker.semantic(),

1
crates/ruff_linter/src/rules/pylint/rules/non_ascii_name.rs
View file

@ -67,6 +67,7 @@ pub(crate) fn non_ascii_name(binding: &Binding, locator: &Locator) -> Option<Dia
| BindingKind::FromImport(_)
| BindingKind::SubmoduleImport(_)
| BindingKind::Deletion
| BindingKind::ConditionalDeletion(_)
| BindingKind::UnboundException(_) => {
return None;
}

34
crates/ruff_python_semantic/src/binding.rs
View file

@ -75,6 +75,11 @@ impl<'a> Binding<'a> {
self.flags.intersects(BindingFlags::GLOBAL)
}
/// Return `true` if this [`Binding`] was deleted.
pub const fn is_deleted(&self) -> bool {
self.flags.intersects(BindingFlags::DELETED)
}
/// Return `true` if this [`Binding`] represents an assignment to `__all__` with an invalid
/// value (e.g., `__all__ = "Foo"`).
pub const fn is_invalid_all_format(&self) -> bool {
@ -165,6 +170,7 @@ impl<'a> Binding<'a> {
// Deletions, annotations, `__future__` imports, and builtins are never considered
// redefinitions.
BindingKind::Deletion
| BindingKind::ConditionalDeletion(_)
| BindingKind::Annotation
| BindingKind::FutureImport
| BindingKind::Builtin => {
@ -265,6 +271,19 @@ bitflags! {
/// ```
const GLOBAL = 1 << 4;
/// The binding was deleted (i.e., the target of a `del` statement).
///
/// For example, the binding could be `x` in:
/// ```python
/// del x
/// ```
///
/// The semantic model will typically shadow a deleted binding via an additional binding
/// with [`BindingKind::Deletion`]; however, conditional deletions (e.g.,
/// `if condition: del x`) do _not_ generate a shadow binding. This flag is thus used to
/// detect whether a binding was _ever_ deleted, even conditionally.
const DELETED = 1 << 5;
/// The binding represents an export via `__all__`, but the assigned value uses an invalid
/// expression (i.e., a non-container type).
///
@ -272,7 +291,7 @@ bitflags! {
/// ```python
/// __all__ = 1
/// ```
const INVALID_ALL_FORMAT = 1 << 5;
const INVALID_ALL_FORMAT = 1 << 6;
/// The binding represents an export via `__all__`, but the assigned value contains an
/// invalid member (i.e., a non-string).
@ -281,7 +300,7 @@ bitflags! {
/// ```python
/// __all__ = [1]
/// ```
const INVALID_ALL_OBJECT = 1 << 6;
const INVALID_ALL_OBJECT = 1 << 7;
/// The binding represents a private declaration.
///
@ -289,7 +308,7 @@ bitflags! {
/// ```python
/// _T = "This is a private variable"
/// ```
const PRIVATE_DECLARATION = 1 << 7;
const PRIVATE_DECLARATION = 1 << 8;
/// The binding represents an unpacked assignment.
///
@ -297,7 +316,7 @@ bitflags! {
/// ```python
/// (x, y) = 1, 2
/// ```
const UNPACKED_ASSIGNMENT = 1 << 8;
const UNPACKED_ASSIGNMENT = 1 << 9;
}
}
@ -512,6 +531,13 @@ pub enum BindingKind<'a> {
/// ```
Deletion,
/// A binding for a deletion, like `x` in:
/// ```python
/// if x > 0:
/// del x
/// ```
ConditionalDeletion(BindingId),
/// A binding to bind an exception to a local variable, like `x` in:
/// ```python
/// try:

52
crates/ruff_python_semantic/src/model.rs
View file

@ -5,7 +5,7 @@ use rustc_hash::FxHashMap;
use ruff_python_ast::helpers::from_relative_import;
use ruff_python_ast::name::{QualifiedName, UnqualifiedName};
use ruff_python_ast::{self as ast, Expr, Operator, Stmt};
use ruff_python_ast::{self as ast, Expr, ExprContext, Operator, Stmt};
use ruff_python_stdlib::path::is_python_stub_file;
use ruff_text_size::{Ranged, TextRange, TextSize};
@ -271,7 +271,7 @@ impl<'a> SemanticModel<'a> {
.get(symbol)
.map_or(true, |binding_id| {
// Treat the deletion of a name as a reference to that name.
self.add_local_reference(binding_id, range);
self.add_local_reference(binding_id, ExprContext::Del, range);
self.bindings[binding_id].is_unbound()
});
@ -296,8 +296,9 @@ impl<'a> SemanticModel<'a> {
let reference_id = self.resolved_references.push(
ScopeId::global(),
self.node_id,
name.range,
ExprContext::Load,
self.flags,
name.range,
);
self.bindings[binding_id].references.push(reference_id);
@ -308,8 +309,9 @@ impl<'a> SemanticModel<'a> {
let reference_id = self.resolved_references.push(
ScopeId::global(),
self.node_id,
name.range,
ExprContext::Load,
self.flags,
name.range,
);
self.bindings[binding_id].references.push(reference_id);
}
@ -365,8 +367,9 @@ impl<'a> SemanticModel<'a> {
let reference_id = self.resolved_references.push(
self.scope_id,
self.node_id,
name.range,
ExprContext::Load,
self.flags,
name.range,
);
self.bindings[binding_id].references.push(reference_id);
@ -377,8 +380,9 @@ impl<'a> SemanticModel<'a> {
let reference_id = self.resolved_references.push(
self.scope_id,
self.node_id,
name.range,
ExprContext::Load,
self.flags,
name.range,
);
self.bindings[binding_id].references.push(reference_id);
}
@ -426,6 +430,15 @@ impl<'a> SemanticModel<'a> {
return ReadResult::UnboundLocal(binding_id);
}
BindingKind::ConditionalDeletion(binding_id) => {
self.unresolved_references.push(
name.range,
self.exceptions(),
UnresolvedReferenceFlags::empty(),
);
return ReadResult::UnboundLocal(binding_id);
}
// If we hit an unbound exception that shadowed a bound name, resole to the
// bound name. For example, given:
//
@ -446,8 +459,9 @@ impl<'a> SemanticModel<'a> {
let reference_id = self.resolved_references.push(
self.scope_id,
self.node_id,
name.range,
ExprContext::Load,
self.flags,
name.range,
);
self.bindings[binding_id].references.push(reference_id);
@ -458,8 +472,9 @@ impl<'a> SemanticModel<'a> {
let reference_id = self.resolved_references.push(
self.scope_id,
self.node_id,
name.range,
ExprContext::Load,
self.flags,
name.range,
);
self.bindings[binding_id].references.push(reference_id);
}
@ -548,6 +563,7 @@ impl<'a> SemanticModel<'a> {
match self.bindings[binding_id].kind {
BindingKind::Annotation => continue,
BindingKind::Deletion | BindingKind::UnboundException(None) => return None,
BindingKind::ConditionalDeletion(binding_id) => return Some(binding_id),
BindingKind::UnboundException(Some(binding_id)) => return Some(binding_id),
_ => return Some(binding_id),
}
@ -1315,18 +1331,28 @@ impl<'a> SemanticModel<'a> {
}
/// Add a reference to the given [`BindingId`] in the local scope.
pub fn add_local_reference(&mut self, binding_id: BindingId, range: TextRange) {
pub fn add_local_reference(
&mut self,
binding_id: BindingId,
ctx: ExprContext,
range: TextRange,
) {
let reference_id =
self.resolved_references
.push(self.scope_id, self.node_id, range, self.flags);
.push(self.scope_id, self.node_id, ctx, self.flags, range);
self.bindings[binding_id].references.push(reference_id);
}
/// Add a reference to the given [`BindingId`] in the global scope.
pub fn add_global_reference(&mut self, binding_id: BindingId, range: TextRange) {
pub fn add_global_reference(
&mut self,
binding_id: BindingId,
ctx: ExprContext,
range: TextRange,
) {
let reference_id =
self.resolved_references
.push(ScopeId::global(), self.node_id, range, self.flags);
.push(ScopeId::global(), self.node_id, ctx, self.flags, range);
self.bindings[binding_id].references.push(reference_id);
}
@ -1700,7 +1726,6 @@ bitflags! {
/// only required by the Python interpreter, but by runtime type checkers too.
const RUNTIME_REQUIRED_ANNOTATION = 1 << 2;
/// The model is in a type definition.
///
/// For example, the model could be visiting `int` in:
@ -1886,7 +1911,6 @@ bitflags! {
/// ```
const COMPREHENSION_ASSIGNMENT = 1 << 19;
/// The model is in a module / class / function docstring.
///
/// For example, the model could be visiting either the module, class,

34
crates/ruff_python_semantic/src/reference.rs
View file

@ -3,10 +3,10 @@ use std::ops::Deref;
use bitflags::bitflags;
use ruff_index::{newtype_index, IndexSlice, IndexVec};
use ruff_python_ast::ExprContext;
use ruff_source_file::Locator;
use ruff_text_size::{Ranged, TextRange};
use crate::context::ExecutionContext;
use crate::scope::ScopeId;
use crate::{Exceptions, NodeId, SemanticModelFlags};
@ -18,10 +18,12 @@ pub struct ResolvedReference {
node_id: Option<NodeId>,
/// The scope in which the reference is defined.
scope_id: ScopeId,
/// The range of the reference in the source code.
range: TextRange,
/// The expression context in which the reference occurs (e.g., `Load`, `Store`, `Del`).
ctx: ExprContext,
/// The model state in which the reference occurs.
flags: SemanticModelFlags,
/// The range of the reference in the source code.
range: TextRange,
}
impl ResolvedReference {
@ -35,13 +37,19 @@ impl ResolvedReference {
self.scope_id
}
/// The [`ExecutionContext`] of the reference.
pub const fn context(&self) -> ExecutionContext {
if self.flags.intersects(SemanticModelFlags::TYPING_CONTEXT) {
ExecutionContext::Typing
} else {
ExecutionContext::Runtime
}
/// Return `true` if the reference occurred in a `Load` operation.
pub const fn is_load(&self) -> bool {
self.ctx.is_load()
}
/// Return `true` if the context is in a typing context.
pub const fn in_typing_context(&self) -> bool {
self.flags.intersects(SemanticModelFlags::TYPING_CONTEXT)
}
/// Return `true` if the context is in a runtime context.
pub const fn in_runtime_context(&self) -> bool {
!self.flags.intersects(SemanticModelFlags::TYPING_CONTEXT)
}
/// Return `true` if the context is in a typing-only type annotation.
@ -108,14 +116,16 @@ impl ResolvedReferences {
&mut self,
scope_id: ScopeId,
node_id: Option<NodeId>,
range: TextRange,
ctx: ExprContext,
flags: SemanticModelFlags,
range: TextRange,
) -> ResolvedReferenceId {
self.0.push(ResolvedReference {
node_id,
scope_id,
range,
ctx,
flags,
range,
})
}
}