Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/airflow/AIR201.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,26 @@ def extract_data():
bash_command="{{ ti.xcom_pull(task_ids='unknown_task') }}", # AIR201 (no fix)
)

# Referencing a builtin does not imply that a task variable is visible (no fix)
task_26 = PythonOperator(
task_id="task_26",
op_args="{{ ti.xcom_pull(task_ids='len') }}", # AIR201 (no fix)
)

# The same is true if the builtin was materialized by an earlier load
print(max)
task_27 = PythonOperator(
task_id="task_27",
op_args="{{ ti.xcom_pull(task_ids='max') }}", # AIR201 (no fix)
)

# A variable that shadows a builtin is still eligible for a fix
len = PythonOperator(task_id="len", python_callable=my_callable)
task_28 = PythonOperator(
task_id="task_28",
op_args="{{ ti.xcom_pull(task_ids='len') }}", # AIR201 (fix: len.output)
)


# Cases that should NOT trigger AIR201:

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,11 @@

# regression test for https://github.com/astral-sh/ruff/pull/18805
range((0), 42)


# A pre-scanned global declaration should not shadow the builtin.
def f():
global range


range(0, 3)
63 changes: 23 additions & 40 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ use ruff_python_semantic::{
Import, Module, ModuleKind, ModuleSource, NodeId, ScopeId, ScopeKind, SemanticModel,
SemanticModelFlags, StarImport, SubmoduleImport,
};
use ruff_python_stdlib::builtins::{python_builtins, python_magic_globals};
use ruff_python_trivia::CommentRanges;
use ruff_source_file::{OneIndexed, SourceFile, SourceFileBuilder, SourceRow};
use ruff_text_size::{Ranged, TextRange, TextSize};
Expand Down Expand Up @@ -272,7 +271,14 @@ impl<'a> Checker<'a> {
target_version: TargetVersion,
context: &'a LintContext<'a>,
) -> Self {
let semantic = SemanticModel::new(&settings.typing_modules, path, module);
let semantic = SemanticModel::new(
&settings.typing_modules,
&settings.builtins,
target_version.linter_version(),
source_type,
path,
module,
);
Self {
parsed,
parsed_type_annotation: None,
Expand Down Expand Up @@ -1184,7 +1190,7 @@ impl<'a> Visitor<'a> for Checker<'a> {
node_index: _,
}) if !self.semantic.scope_id.is_global() => {
for name in names {
let binding_id = self.semantic.global_scope().get(name);
let binding_id = self.semantic.global_binding(name);

// Mark the binding in the global scope as "rebound" in the current scope.
if let Some(binding_id) = binding_id {
Expand All @@ -1194,6 +1200,7 @@ impl<'a> Visitor<'a> for Checker<'a> {

// Add a binding to the current scope.
let binding_id = self.semantic.push_binding(
name,
name.range(),
BindingKind::Global(binding_id),
BindingFlags::GLOBAL,
Expand Down Expand Up @@ -1224,6 +1231,7 @@ impl<'a> Visitor<'a> for Checker<'a> {

// Add a binding to the current scope.
let binding_id = self.semantic.push_binding(
name,
name.range(),
BindingKind::Nonlocal(binding_id, scope_id),
BindingFlags::NONLOCAL,
Expand Down Expand Up @@ -1292,6 +1300,7 @@ impl<'a> Visitor<'a> for Checker<'a> {
let added_dunder_class_scope = if self.semantic.current_scope().kind.is_class() {
self.semantic.push_scope(ScopeKind::DunderClassCell);
let binding_id = self.semantic.push_binding(
"__class__",
TextRange::default(),
BindingKind::DunderClassCell,
BindingFlags::empty(),
Expand Down Expand Up @@ -2285,7 +2294,7 @@ impl<'a> Visitor<'a> for Checker<'a> {
}) => {
if let Some(name) = name {
// Store the existing binding, if any.
let binding_id = self.semantic.lookup_symbol(name.as_str());
let binding_id = self.semantic.lookup_binding(name.as_str());

// Add the bound exception name to the scope.
self.add_binding(
Expand Down Expand Up @@ -2694,7 +2703,7 @@ impl<'a> Checker<'a> {
}

// Create the `Binding`.
let binding_id = self.semantic.push_binding(range, kind, flags);
let binding_id = self.semantic.push_binding(name, range, kind, flags);

// If the name is private, mark is as such.
if name.starts_with('_') {
Expand Down Expand Up @@ -2754,35 +2763,7 @@ impl<'a> Checker<'a> {
binding_id
}

fn bind_builtins(&mut self) {
let target_version = self.target_version();
let settings = self.settings();
let builtin_count = python_builtins(target_version.minor, self.source_type.is_ipynb())
.count()
+ python_magic_globals(target_version.minor).count()
+ settings.builtins.len();

self.semantic.reserve_builtin_bindings(builtin_count);

let mut bind_builtin = |builtin| {
// Add the builtin to the scope.
let binding_id = self.semantic.push_builtin();
let scope = self.semantic.global_scope_mut();
scope.add(builtin, binding_id);
};
let standard_builtins = python_builtins(target_version.minor, self.source_type.is_ipynb());
for builtin in standard_builtins {
bind_builtin(builtin);
}
for builtin in python_magic_globals(target_version.minor) {
bind_builtin(builtin);
}
for builtin in &settings.builtins {
bind_builtin(builtin);
}
}

fn handle_node_load(&mut self, expr: &Expr) {
fn handle_node_load(&mut self, expr: &'a Expr) {
let Expr::Name(expr) = expr else {
return;
};
Expand Down Expand Up @@ -2920,9 +2901,12 @@ impl<'a> Checker<'a> {
}

// Create a binding to model the deletion.
let binding_id =
self.semantic
.push_binding(expr.range(), BindingKind::Deletion, BindingFlags::empty());
let binding_id = self.semantic.push_binding(
id,
expr.range(),
BindingKind::Deletion,
BindingFlags::empty(),
);
let scope = self.semantic.current_scope_mut();
scope.add(id, binding_id);
}
Expand Down Expand Up @@ -3203,6 +3187,7 @@ impl<'a> Checker<'a> {
{
self.semantic.push_scope(ScopeKind::DunderClassCell);
let binding_id = self.semantic.push_binding(
"__class__",
TextRange::default(),
BindingKind::DunderClassCell,
BindingFlags::empty(),
Expand Down Expand Up @@ -3260,7 +3245,7 @@ impl<'a> Checker<'a> {
for definition in definitions {
for export in definition.names() {
let (name, range) = (export.name(), export.range());
if let Some(binding_id) = self.semantic.global_scope().get(name) {
if let Some(binding_id) = self.semantic.global_binding(name) {
self.semantic.flags |= SemanticModelFlags::DUNDER_ALL_DEFINITION;
// Mark anything referenced in `__all__` as used.
self.semantic
Expand Down Expand Up @@ -3394,8 +3379,6 @@ pub(crate) fn check_ast(
target_version,
context,
);
checker.bind_builtins();

// Iterate over the AST.
checker.visit_module(parsed.suite());
checker.visit_body(parsed.suite());
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/airflow/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ pub(crate) fn is_guarded_by_try_except(
try_block_contains_undeprecated_attribute(try_node, module, name, semantic)
}
Expr::Name(ExprName { id, .. }) => {
let Some(binding_id) = semantic.lookup_symbol(id.as_str()) else {
let Some(binding_id) = semantic.lookup_symbol(id.as_str()).binding_id() else {
return false;
};
let binding = semantic.binding(binding_id);
Expand Down Expand Up @@ -247,7 +247,7 @@ pub(crate) fn generate_remove_and_runtime_import_edit(
let semantic = checker.semantic();
let binding = semantic
.resolve_name(head)
.or_else(|| checker.semantic().lookup_symbol(&head.id))
.or_else(|| checker.semantic().lookup_symbol(&head.id).binding_id())
.map(|id| checker.semantic().binding(id))?;
let stmt = binding.statement(semantic)?;
let remove_edit = remove_unused_imports(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ fn annotation_is_typed_dict_subclass(annotation: &Expr, semantic: &SemanticModel
};
let Some(binding_id) = semantic
.resolve_name(name)
.or_else(|| semantic.lookup_symbol(&name.id))
.or_else(|| semantic.lookup_symbol(&name.id).binding_id())
else {
return false;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,12 @@ pub(crate) fn xcom_pull_in_template_string(checker: &Checker, call: &ast::ExprCa

// If the task_id matches a variable in scope, provide an unsafe fix
// replacing the template string with `<variable>.output`.
if checker.semantic().lookup_symbol(&task_id).is_some() {
if checker
.semantic()
.lookup_symbol(&task_id)
.binding_id()
.is_some_and(|binding_id| !checker.semantic().binding(binding_id).kind.is_builtin())
{
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
format!("{task_id}.output"),
arg_value.range(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,3 +281,43 @@ AIR201 Use the `.output` attribute on the task object for "unknown_task" instead
115 | )
|
help: Replace with `unknown_task.output`

AIR201 Use the `.output` attribute on the task object for "len" instead of `xcom_pull` in a template string
--> AIR201.py:120:13
|
118 | task_26 = PythonOperator(
119 | task_id="task_26",
120 | op_args="{{ ti.xcom_pull(task_ids='len') }}", # AIR201 (no fix)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
121 | )
|
help: Replace with `len.output`

AIR201 Use the `.output` attribute on the task object for "max" instead of `xcom_pull` in a template string
--> AIR201.py:127:13
|
125 | task_27 = PythonOperator(
126 | task_id="task_27",
127 | op_args="{{ ti.xcom_pull(task_ids='max') }}", # AIR201 (no fix)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
128 | )
|
help: Replace with `max.output`

AIR201 [*] Use the `.output` attribute on the task object for "len" instead of `xcom_pull` in a template string
--> AIR201.py:134:13
|
132 | task_28 = PythonOperator(
133 | task_id="task_28",
134 | op_args="{{ ti.xcom_pull(task_ids='len') }}", # AIR201 (fix: len.output)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
135 | )
|
help: Replace with `len.output`
|
133 | task_id="task_28",
- op_args="{{ ti.xcom_pull(task_ids='len') }}", # AIR201 (fix: len.output)
134 + op_args=len.output, # AIR201 (fix: len.output)
135 | )
|
note: This is an unsafe fix and may change runtime behavior
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ fn is_bound_to_tuple(arg: &Expr, semantic: &SemanticModel) -> bool {
return false;
};

let Some(binding_id) = semantic.lookup_symbol(id.as_str()) else {
let Some(binding_id) = semantic.lookup_symbol(id.as_str()).binding_id() else {
return false;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,18 @@ help: Remove `start` argument
18 | # regression test for https://github.com/astral-sh/ruff/pull/18805
- range((0), 42)
19 + range(42)
20 |
|

PIE808 [*] Unnecessary `start` argument in `range`
--> PIE808.py:27:7
|
27 | range(0, 3)
| ^
|
help: Remove `start` argument
|
26 |
- range(0, 3)
27 + range(3)
|
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ pub(crate) fn runtime_import_in_type_checking_block(checker: &Checker, scope: &S
let ignore_dunder_all_references = checker
.semantic()
.lookup_symbol_in_scope("__getattr__", ScopeId::global(), false)
.is_some();
.is_bound();

for binding_id in scope.binding_ids() {
let binding = checker.semantic().binding(binding_id);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ fn is_guarded_by_try_except(
try_block_contains_undeprecated_attribute(try_node, &replacement.details, semantic)
}
Expr::Name(ast::ExprName { id, .. }) => {
let Some(binding_id) = semantic.lookup_symbol(id.as_str()) else {
let Some(binding_id) = semantic.lookup_symbol(id.as_str()).binding_id() else {
return false;
};
let binding = semantic.binding(binding_id);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ pub(crate) fn non_lowercase_variable_in_function(checker: &Checker, range: TextR
if checker
.semantic()
.lookup_symbol(name)
.binding_id()
.is_some_and(|id| checker.semantic().binding(id).is_global())
{
return;
Expand Down
15 changes: 15 additions & 0 deletions crates/ruff_linter/src/rules/pyflakes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1231,6 +1231,21 @@ mod tests {
flakes("__builtins__", &[]);
}

#[test]
fn builtin_after_exception_target_cleanup() {
flakes(
r"
try:
pass
except Exception as len:
pass

print(len)
",
&[Rule::UnusedVariable],
);
}

#[test]
fn magic_globals_name() {
// Use of the C{__name__} magic global should not emit an undefined name
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,7 @@ fn has_simple_shadowed_bindings(scope: &Scope, id: BindingId, semantic: &Semanti
fn symbol_used_in_dunder_all(semantic: &SemanticModel<'_>, binding: &ImportBinding) -> bool {
semantic
.lookup_symbol_in_scope(binding.symbol_stored_in_outer_scope(), binding.scope, false)
.binding_id()
.is_some_and(|bdg| {
semantic
.binding(bdg)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ pub(crate) fn unnecessary_lambda(checker: &Checker, lambda: &ExprLambda) {
// Suppress the fix if the assignment expression target shadows one of the lambda's parameters.
// This is necessary to avoid introducing a change in the behavior of the program.
for name in names {
if let Some(binding_id) = checker.semantic().lookup_symbol(name.id()) {
if let Some(binding_id) = checker.semantic().lookup_symbol(name.id()).binding_id() {
let binding = checker.semantic().binding(binding_id);
if checker
.semantic()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ fn is_custom_exception(
let Some(symbol) = qualified_name.segments().last() else {
return false;
};
let Some(binding_id) = semantic.lookup_symbol(symbol) else {
let Some(binding_id) = semantic.lookup_symbol(symbol).binding_id() else {
return false;
};
let binding = semantic.binding(binding_id);
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pyupgrade/rules/pep695/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ pub(crate) fn expr_name_to_type_var<'a>(
) -> Option<TypeVar<'a>> {
let StmtAssign { value, .. } = semantic
.lookup_symbol(name.id.as_str())
.binding_id()
.and_then(|binding_id| semantic.binding(binding_id).source)
.map(|node_id| semantic.statement(node_id))?
.as_assign_stmt()?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ pub(crate) fn unnecessary_builtin_import(
if &alias.name == "*" {
return true;
}
let Some(binding_id) = semantic.lookup_symbol(alias.name.as_str()) else {
let Some(binding_id) = semantic.lookup_symbol(alias.name.as_str()).binding_id() else {
return false;
};
let binding = semantic.binding(binding_id);
Expand Down
Loading
Loading