Uh oh!
There was an error while loading. Please reload this page.
Enable ruff B023 (function-uses-loop-variable) and fix violations - #70640
Conversation
61c8f4b to
e2509d8Compare
potiuk
left a comment
There was a problem hiding this comment.
Thanks — B023 is a worthwhile rule to turn on; late-bound closures over loop variables are a genuinely nasty bug class because they fail silently and only under specific timing. Adding it to pyproject.toml alongside the fixes is the right way to land it.
I traced the two most interesting cases rather than assuming:
triggers/file.pyis the nicest fix in the set — rewritingrun_sync(lambda: list(os.walk(path)))asrun_sync(lambda p: list(os.walk(p)), path)passes the value explicitly instead of relying on a default-argument trick. Worth using that form elsewhere where the callee accepts arguments; it reads as intent rather than as lint appeasement.- The migration (
0101_3_2_0_ui_improvements_for_deadlines.py) is a false positive in practice:_migrate_dag_deadlinesis defined at line 597 and called at line 688 within the same loop iteration, so late binding was never observable there.
Two changes requested inline before this lands — one about rewriting a shipped migration for no behavioural gain, one about a binding that leaks into a public keyword signature in the secrets-masking path. Neither is hard to address.
These are judgement calls rather than mechanical fixes, so I'd like your own reasoning in reply rather than just an autofix — my review here was AI-assisted and shouldn't be treated as settled.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
e2509d8 to
adfe70dCompareB023 catches the late-binding closure-over-loop-variable footgun where a function defined inside a loop captures the loop variable by reference, so every function in the resulting list sees the same (final) value — a classic silent-bug source in Python (https://docs.python.org/3/faq/programming.html#why-do-lambdas-defined-in-a-loop-with-different-values-all-return-the-same-result). The one user-visible fix is in providers/standard/.../triggers/file.py, where the FileTrigger's os.walk lambda was dispatched to a worker thread via anyio.to_thread.run_sync while the outer glob iteration could advance, potentially walking the wrong path. The rest are pre-existing latent-bug or false-positive sites in a migration script, breeze translation helpers, sphinx extensions, a system-test example DAG, the SMTP OAuth2 auth callback, secrets_masker subclass compat shim, and two provider unit tests — fixed by binding the loop-derived variable as a default argument on the inner function or lambda.
adfe70d to
743fc02Compare
potiuk
left a comment
There was a problem hiding this comment.
Both points addressed, and on the migration you were right to push back on my suggestion.
# noqa: B023 on the def line would indeed have suppressed nothing — ruff flags the reads inside the closure body, not the definition, so it would have taken seven per-line noqas to do what I asked for, in a shipped migration where body noise is exactly what we were trying to avoid. The module-level # ruff: noqa: B023 with the invariant written out, plus the explicit "do not rewrite the body for lint hygiene" note, is a better answer than the one I proposed. Thanks for checking rather than just applying it.
The secrets-masker factory is right too, and your reasoning is the reason it matters: _redact recurses through arbitrary user data, so a **kwargs path that let a caller slip in _f would substitute a different function inside the redaction walk. The comment at the site should stop someone collapsing it back to a default-arg binding.
I have updated the branch from main. The one red check — Postgres 18 / Py3.14 · API…CLI — was from 4 August, ran for six minutes, and produced no pytest summary at all, so it never reached the test stage; that is an infrastructure or expired-image failure rather than anything in this diff. A lint rule plus behaviour-preserving rewrites would not break a single matrix cell of core API/CLI DB tests and nothing else.
Approving on that basis. If the fresh run comes back red on something real, ping me and I will look again rather than leaving you to guess at it.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Uh oh!
There was an error while loading. Please reload this page.
Backport successfully created: v3-3-testNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
|
…olations (#70640) (#72273) B023 catches the late-binding closure-over-loop-variable footgun where a function defined inside a loop captures the loop variable by reference, so every function in the resulting list sees the same (final) value — a classic silent-bug source in Python (https://docs.python.org/3/faq/programming.html#why-do-lambdas-defined-in-a-loop-with-different-values-all-return-the-same-result). The one user-visible fix is in providers/standard/.../triggers/file.py, where the FileTrigger's os.walk lambda was dispatched to a worker thread via anyio.to_thread.run_sync while the outer glob iteration could advance, potentially walking the wrong path. The rest are pre-existing latent-bug or false-positive sites in a migration script, breeze translation helpers, sphinx extensions, a system-test example DAG, the SMTP OAuth2 auth callback, secrets_masker subclass compat shim, and two provider unit tests — fixed by binding the loop-derived variable as a default argument on the inner function or lambda. (cherry picked from commit af0a377) Co-authored-by: Deepak kumar <deepakkumar@meta.com> Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
B023 catches the late-binding closure-over-loop-variable footgun where a function defined inside a loop captures the loop variable by reference, so every function in the resulting list sees the same (final) value — a classic silent-bug source in Python. See the Python FAQ entry.
The one user-visible fix is in
providers/standard/.../triggers/file.py, where theFileTrigger'sos.walklambda was dispatched to a worker thread viaanyio.to_thread.run_syncwhile the outer glob iteration could advance, potentially walking the wrong path.The rest are pre-existing latent-bug or false-positive sites fixed by binding the loop-derived variable as a default argument on the inner function or lambda:
airflow-core/.../0101_3_2_0_ui_improvements_for_deadlines.py(migration; 7 sites in one nested function)dev/breeze/.../ui_commands.py(two i18n helpers)devel-common/src/sphinx_exts/providers_extensions.py(two class-extras lambdas)providers/apache/hive/tests/unit/.../test_s3_to_hive.pyproviders/exasol/tests/unit/exasol/hooks/test_sql.pyproviders/google/tests/system/.../example_cloud_sql_query.pyproviders/smtp/src/airflow/providers/smtp/hooks/smtp.py(OAuth2 auth callback)shared/secrets_masker/.../secrets_masker.py(subclass compat shim)Follows the pattern of #66977 (B015), #66978 (PLE1205), #66979 (B008), and #66960 (trigger init/serialize static check).
Was generative AI tooling used to co-author this PR?