Repository navigation
Add support for __lazy_modules__ - #28459
Conversation
Summary -- The PR is pretty big in total, but it's intended to be reviewed commit-by-commit. As usual, the very first commit is empty for filling the PR summary, so the numbers below refer to the remaining non-empty commits. The first commit adds `__lazy_modules__` tracking to the semantic model. Because the declaration affects later imports and can also be overwritten, we just store whatever was assigned to it last for later lookup when visiting other code. The second commit expands [TID254] to recognize `__lazy_modules__` and includes a few helper methods on the semantic model that are used by later rules as well. The third commit adds `__lazy_modules__` support to [TID255], which also relies on a new `BindingFlags::LAZY` variant. This marks bindings as originating from a lazy import, which is easier than trying to resolve a sequence of potentially shadowed `__lazy_modules__` bindings to check if an import was lazy. For example, we set the `LAZY` flag on `foo` here, which would otherwise require resolve multiple previous bindings: ```py __lazy_modules__ = ["foo"] import foo __lazy_modules__ = [] class C(foo.Something): ... ``` The last four commits make smaller changes to four other rules. As with TID254 and TID255, the primary change is that we withhold a fix that involves `__lazy_modules__` when it would change the module's lazy status. Initially I had hoped to add fixes that modified `__lazy_modules__` (adding to it when we'd otherwise add `lazy`, removing from it when we'd remove `lazy`), but this turned out to be quite complicated. Unlike the `lazy` keyword, `__lazy_modules__` has nonlocal effects that could change the behavior of _any_ import from the same module in the file. Trying to resolve all of that to be sure the fix is reasaonable was a real mess, and reverting Codex's attempt saved almost 700 lines for TID255 alone. Similarly, in case this comes up in someone else's Codex review, my Codex initially tried to handle even more edge cases with `__lazy_modules__` definitions, such as definitions in a loop or redefining it with `*` imports and things like that. I tried to rein it in toward something more like our `__all__` and `from __future__ import annotations` handling, which again seemed like the better tradeoff to me. Closes #25170. [TID254]: https://docs.astral.sh/ruff/rules/lazy-import-mismatch/ [TID255]: https://docs.astral.sh/ruff/rules/lazy-import-immediately-resolved/ Test Plan -- New mdtests
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| lazy-import-immediately-resolved | 18 | 18 | 0 | 0 | 0 |
ee1c521 to
4edd863
Compare
4edd863 to
e30f6a4
Compare
| ### Empty declarations on older Python versions | ||
|
|
||
| An empty declaration enables lazy-import policies even when the target version predates the `lazy` | ||
| keyword. | ||
|
|
||
| ```py | ||
| __lazy_modules__ = [] | ||
| import json # error: [lazy-import-mismatch] | ||
| ``` |
There was a problem hiding this comment.
It's possible that we don't want to do this, but it made sense to me to consider a __lazy_modules__ declaration (plus enabling a lazy-import rule) as enough opt-in to flag these even for a configured Python version before 3.15.
Similarly, I added the "In the future..." note below in analogy to #18502.
| ### Unknown declarations | ||
|
|
||
| An unsupported declaration value does not establish whether the import violates the policy, so no | ||
| diagnostic is emitted. | ||
|
|
||
| ```py | ||
| __lazy_modules__ = configured_modules() | ||
| import json | ||
| ``` |
There was a problem hiding this comment.
One of the relevant sections from the PEP is here:
It is worth noting that the implementation performs membership checks by calling
__contains__on the__lazy_modules__object. Consequently, users requiring wildcard behavior may provide a custom object implementing__contains__to returnTruefor all queries or other desired patterns.
We treat this as an unknown laziness, different from certain laziness or certain eagerness.
|
|
||
| if selector.includes_all() { | ||
| report_all_matching_imports(checker, stmt, policy, selector); | ||
| if checker.lazy_import_context().is_some() |
There was a problem hiding this comment.
Just noting that this is a pretty awkwardly named API for this usage because its previous callers were for semantic syntax error reporting. If the context is Some, it means that we're in an invalid lazy import context:
ruff/crates/ruff_linter/src/checkers/ast/mod.rs
Lines 341 to 362 in 83c3487
I guess I could rename the Checker method at least.
|
|
||
| /// Test exact module membership in the current `__lazy_modules__` declaration. | ||
| /// Returns [`ImportLaziness::Unknown`] when the declaration is not a literal collection of strings. | ||
| pub fn module_laziness(&self, module: &str) -> ImportLaziness { |
There was a problem hiding this comment.
I wondered if we should try to cache this or compute it up front when storing self.lazy_modules, but I guess it only matters if you have defined __lazy_modules__ and probably also if it gets pretty long. If we preprocess, we'd likely end up allocating a Vec<&str> anyway.
MichaReiser
left a comment
There was a problem hiding this comment.
Great write up and the commits made reviewing very easy. Thank you. There's one additional edge case that needs handling (can be a follow up PR) and we should add a test for relative imports.
I think our semantic model already handles relative imports but let's add a test for it, similar to what's in the specification https://docs.python.org/3.15/reference/simple_stmts.html#lazy-imports
I also think we need to extend the rule to handle conditional lazy imports. I don't know how common they are and we can do this as a follow up if you prefer, but what I mean is
if False:
__lazy_modules__ = ["json"]
import json
json.dumps({}) # PR reports an immediately resolved lazy importI think what we need is to track whether a __lazy_modules__ entry is seen on every path or only some. If the module is only in __lazy_modules__ in some branches, then the module's state is Unknown (could be eager or lazy). If the module is in all __lazy_modules__ assignments, then it's lazy.
| /// The rule also recognizes imports made lazy by a literal `__lazy_modules__` | ||
| /// declaration, including when the target version is older than Python 3.15. | ||
| /// This declaration allows a module to use lazy imports on Python 3.15 and later | ||
| /// while retaining eager imports on older versions. | ||
| /// |
There was a problem hiding this comment.
Do we need to document known limitations around dynamic calls?
| pub fn module_laziness(&self, module: &str) -> ImportLaziness { | ||
| let Some(value) = self.lazy_modules else { | ||
| return ImportLaziness::Eager; | ||
| }; | ||
| let (Expr::List(ast::ExprList { elts: elements, .. }) | ||
| | Expr::Tuple(ast::ExprTuple { elts: elements, .. }) | ||
| | Expr::Set(ast::ExprSet { elts: elements, .. })) = value | ||
| else { | ||
| return ImportLaziness::Unknown; | ||
| }; | ||
| if !elements.iter().all(Expr::is_string_literal_expr) { | ||
| return ImportLaziness::Unknown; | ||
| } | ||
| if elements.iter().any(|element| { | ||
| element | ||
| .as_string_literal_expr() | ||
| .is_some_and(|literal| literal.value.to_str() == module) | ||
| }) { | ||
| ImportLaziness::Lazy | ||
| } else { | ||
| ImportLaziness::Eager | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm inclined to parse out the lazy module names when we see the assignment instead of matching on Expr for every use. It's probably a bit more expensive but probably required anyway to support conditional assignments.
| import json | ||
| __lazy_modules__ = ["json"] |
There was a problem hiding this comment.
Is this an idea for another rule that warns about earlier imports that are later marked as lazy (same when using the lazy import syntax?)
Summary -- This is a follow-up to #28459 adding simple support for conditionally defining `__lazy_modules__`. We now track per-module laziness, which allows us to merge definitions like: ```py __lazy_modules__ = ["foo"] if condition: __lazy_modules__ = ["foo", "bar"] ``` into known laziness for `foo` and unknown laziness for `bar`. As in #28459, that unknown laziness is still helpful and distinct from known eagerness because it suppresses diagnostics. The one quirk with the `branch_id` tracking used here is that it doesn't handle exhaustive branches. For example, it would be nice if we noticed that `foo` is always lazy here: ```py if condition: __lazy_modules__ = ["foo", "bar"] else: __lazy_modules__ = ["foo", "baz"] ``` but it's instead merged with the implicit known eagerness of all imports before any `__lazy_modules__` assignment, producing `(foo, Unknown)`. So this is effectively equivalent to the version without an `else` for `foo`. I think this is a decent tradeoff that biases us against false positives, though, and doesn't involve any extra bookkeeping in `if` statement traversal. One of my three Codex reviewers also identified this as a false negative, where the assignment and use are in the same branch, but we've already marked the module as `Unknown`: ```py if condition: __lazy_modules__ = ["json"] import json json.dumps({}) ``` But again we're at least biased in the right direction, and we'd need more bookkeeping to identify this. Test Plan -- New mdtests for TID254 that exercise this behavior
|
Thanks for the review! I addressed the simple stuff here and then opened #28491 for the conditional handling, which also didn't end up being too bad thanks to your suggestions on the approach. |
Summary -- This is a follow-up to #28459 adding simple support for conditionally defining `__lazy_modules__`. We now track per-module laziness, which allows us to merge definitions like: ```py __lazy_modules__ = ["foo"] if condition: __lazy_modules__ = ["foo", "bar"] ``` into known laziness for `foo` and unknown laziness for `bar`. As in #28459, that unknown laziness is still helpful and distinct from known eagerness because it suppresses diagnostics. The one quirk with the `branch_id` tracking used here is that it doesn't handle exhaustive branches. For example, it would be nice if we noticed that `foo` is always lazy here: ```py if condition: __lazy_modules__ = ["foo", "bar"] else: __lazy_modules__ = ["foo", "baz"] ``` but it's instead merged with the implicit known eagerness of all imports before any `__lazy_modules__` assignment, producing `(foo, Unknown)`. So this is effectively equivalent to the version without an `else` for `foo`. I think this is a decent tradeoff that biases us against false positives, though, and doesn't involve any extra bookkeeping in `if` statement traversal. One of my three Codex reviewers also identified this as a false negative, where the assignment and use are in the same branch, but we've already marked the module as `Unknown`: ```py if condition: __lazy_modules__ = ["json"] import json json.dumps({}) ``` But again we're at least biased in the right direction, and we'd need more bookkeeping to identify this, so I was okay accepting that for now. Test Plan -- New mdtests for TID254 that exercise this behavior
…aliases * origin/main: (30 commits) [ty] Respect instance dictionary storage for slotted classes (#27749) Add a GitHub repository threat model for Ruff (#28395) [ty] Preserve wrapped signatures in nominal descriptor checks (#28466) [ty] Avoid rebinding extracted method calls (#28469) Recognize quoted types in `typing.TypeForm` (#28507) Support conditional assignment to `__lazy_modules__` (#28491) Add support for `__lazy_modules__` (#28459) [`pyupgrade`] Stop recommending deprecated `ByteString` aliases (`UP035`) (#28498) [ty] Anchor default exclude patterns at the project root (#28463) [ty] Add playground command to toggle inlay hints (#28517) [ty] Ignore divergent markers when detecting descriptors (#28514) [ty] Reuse rendered union elements when displaying types (#28494) [ty] Fix assignability of bounded typevars to intersection types (#28479) [ty] Retain package listings between resolution steps (#28278) Add accessors for comparison expressions (#28485) [ty] Refactor module resolution to use a `ModuleDirectory` abstraction. (#28418) [ty] Reduce retained AST memory by shrinking expressions (#28335) Bump version to 0.16.7 (#28496) Install rustfmt before linting releases (#28495) ensure prepare release changes pass prek (#28488) ... # Conflicts: # crates/ty_python_semantic/src/types/bool.rs
Summary
For a bit of background,
__lazy_modules__is a backwards compatibility mechanism for lazy imports, introduced in PEP 810 alongside thelazykeyword itself. Instead of declaring an importlazywith the keyword, it can also be listed in a module-level constant named__lazy_modules__:This can be used in projects that support older Python versions, as well as 3.15.
It also supports more dynamic approaches (that this PR doesn't), as noted in one of my inline comments, such as:
As one example from the PEP for emulating wildcard lazy imports.
The PR is pretty big in total, but it's intended to be reviewed commit-by-commit. As usual, the very
first commit is empty for filling the PR summary, so the numbers below refer to the remaining
non-empty commits.
The first commit adds
__lazy_modules__tracking to the semantic model. Because the declarationaffects later imports and can also be overwritten, we just store whatever was assigned to it last
for later lookup when visiting other code.
The second commit expands TID254 to recognize
__lazy_modules__and includes a few helper methodson the semantic model that are used by later rules as well.
The third commit adds
__lazy_modules__support to TID255, which also relies on a newBindingFlags::LAZYvariant. This marks bindings as originating from a lazy import, which is easierthan trying to resolve a sequence of potentially shadowed
__lazy_modules__bindings to check if animport was lazy. For example, we set the
LAZYflag onfoohere, which would otherwise requireresolve multiple previous bindings:
The last four commits make smaller changes to four other rules. As with TID254 and TID255, the
primary change is that we withhold a fix that involves
__lazy_modules__when it would change themodule's lazy status. Initially I had hoped to add fixes that modified
__lazy_modules__(adding toit when we'd otherwise add
lazy, removing from it when we'd removelazy), but this turned out tobe quite complicated. Unlike the
lazykeyword,__lazy_modules__has nonlocal effects that couldchange the behavior of any import from the same module in the file. Trying to resolve all of that
to be sure the fix is reasaonable was a real mess, and reverting Codex's attempt saved almost 700
lines for TID255 alone.
Similarly, in case this comes up in someone else's Codex review, my Codex initially tried to handle
even more edge cases with
__lazy_modules__definitions, such as definitions in a loop orredefining it with
*imports and things like that. I tried to rein it in toward something morelike our
__all__andfrom __future__ import annotationshandling, which again seemed like thebetter tradeoff to me.
Closes #25170.
Test Plan
New mdtests. The ecosystem results all look correct. They are cases where
TID255now fires in modules where__lazy_modules__is defined.