-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[pyflakes
] Fix false positive for __class__
in lambda expressions within class definitions (F821
)
#20564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
TC003 | 1 | 1 | 0 | 0 | 0 |
F401 | 1 | 1 | 0 | 0 | 0 |
F811 | 1 | 1 | 0 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+3 -0 violations, +0 -0 fixes in 2 projects; 53 projects unchanged)
apache/airflow (+1 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL
+ task-sdk/src/airflow/sdk/execution_time/comms.py:56:20: TC003 Move standard library import `socket.socket` into a type-checking block
langchain-ai/langchain (+2 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview
+ libs/langchain/langchain/indexes/_sql_record_manager.py:19:8: F401 [*] `uuid` imported but unused + libs/langchain/langchain/indexes/_sql_record_manager.py:62:5: F811 Redefinition of unused `uuid` from line 19: `uuid` redefined here
Changes by rule (3 rules affected)
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
TC003 | 1 | 1 | 0 | 0 | 0 |
F401 | 1 | 1 | 0 | 0 | 0 |
F811 | 1 | 1 | 0 | 0 | 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! The tests look good, but what do you think about moving the new code to visit_expr
to mirror the function version a bit more closely?
// Here we add the implicit scope surrounding a lambda which allows code in the | ||
// lambda to access `__class__` at runtime when the lambda is defined within a class. | ||
// See the `ScopeKind::DunderClassCell` docs for more information. | ||
let added_dunder_class_scope = if self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was picturing this in visit_stmt
like we did for function definitions rather than in the deferred visit. I guess lambdas aren't statements, so the analogous code would actually fit here in visit_expr
:
ruff/crates/ruff_linter/src/checkers/ast/mod.rs
Lines 1620 to 1632 in e4ac9e9
// Visit the default arguments, but avoid the body, which will be deferred. | |
if let Some(parameters) = parameters { | |
for default in parameters | |
.iter_non_variadic_params() | |
.filter_map(|param| param.default.as_deref()) | |
{ | |
self.visit_expr(default); | |
} | |
} | |
self.semantic.push_scope(ScopeKind::Lambda(lambda)); | |
self.visit.lambdas.push(self.semantic.snapshot()); | |
self.analyze.lambdas.push(self.semantic.snapshot()); |
I think that should work because we push a lambda
snapshot that gets used here like we do for functions. The one downside is that the popping for lambdas happens a bit farther down, unlike functions:
ruff/crates/ruff_linter/src/checkers/ast/mod.rs
Lines 2062 to 2063 in e4ac9e9
self.analyze.scopes.push(self.semantic.scope_id); | |
self.semantic.pop_scope(); |
Yeah, that approach makes sense to me. Thanks for the review! |
Summary
Fixes #20562