From 142c1bc7609b76dd651dcbbf8fa184261a910686 Mon Sep 17 00:00:00 2001 From: David Peter Date: Tue, 13 May 2025 16:59:11 +0200 Subject: [PATCH] [ty] Recognize submodules in self-referential imports (#18005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fix the lookup of `submodule`s in cases where the `parent` module has a self-referential import like `from parent import submodule`. This allows us to infer proper types for many symbols where we previously inferred `Never`. This leads to many new false (and true) positives across the ecosystem because the fact that we previously inferred `Never` shadowed a lot of problems. For example, we inferred `Never` for `os.path`, which is why we now see a lot of new diagnostics related to `os.path.abspath` and similar. ```py import os reveal_type(os.path) # previously: Never, now: ``` closes https://github.com/astral-sh/ty/issues/261 closes https://github.com/astral-sh/ty/issues/307 ## Ecosystem analysis ``` ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━┳━━━━━━━┳━━━━━━━━━━━━┓ ┃ Diagnostic ID ┃ Severity ┃ Removed ┃ Added ┃ Net Change ┃ ┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━╇━━━━━━━╇━━━━━━━━━━━━┩ │ call-non-callable │ error │ 1 │ 5 │ +4 │ │ call-possibly-unbound-method │ warning │ 6 │ 26 │ +20 │ │ invalid-argument-type │ error │ 26 │ 94 │ +68 │ │ invalid-assignment │ error │ 18 │ 46 │ +28 │ │ invalid-context-manager │ error │ 9 │ 4 │ -5 │ │ invalid-raise │ error │ 1 │ 1 │ 0 │ │ invalid-return-type │ error │ 3 │ 20 │ +17 │ │ invalid-super-argument │ error │ 4 │ 0 │ -4 │ │ invalid-type-form │ error │ 573 │ 0 │ -573 │ │ missing-argument │ error │ 2 │ 10 │ +8 │ │ no-matching-overload │ error │ 0 │ 715 │ +715 │ │ non-subscriptable │ error │ 0 │ 35 │ +35 │ │ not-iterable │ error │ 6 │ 7 │ +1 │ │ possibly-unbound-attribute │ warning │ 14 │ 31 │ +17 │ │ possibly-unbound-import │ warning │ 13 │ 0 │ -13 │ │ possibly-unresolved-reference │ warning │ 0 │ 8 │ +8 │ │ redundant-cast │ warning │ 1 │ 0 │ -1 │ │ too-many-positional-arguments │ error │ 2 │ 0 │ -2 │ │ unknown-argument │ error │ 2 │ 0 │ -2 │ │ unresolved-attribute │ error │ 583 │ 304 │ -279 │ │ unresolved-import │ error │ 0 │ 96 │ +96 │ │ unsupported-operator │ error │ 0 │ 17 │ +17 │ │ unused-ignore-comment │ warning │ 29 │ 2 │ -27 │ ├───────────────────────────────┼──────────┼─────────┼───────┼────────────┤ │ TOTAL │ │ 1293 │ 1421 │ +128 │ └───────────────────────────────┴──────────┴─────────┴───────┴────────────┘ Analysis complete. Found 23 unique diagnostic IDs. Total diagnostics removed: 1293 Total diagnostics added: 1421 Net change: +128 ``` * We see a lot of new errors (`no-matching-overload`) related to `os.path.dirname` and other `os.path` operations because we infer `str | None` for `__file__`, but many projects use something like `os.path.dirname(__file__)`. * We also see many new `unresolved-attribute` errors related to the fact that we now infer proper module types for some imports (e.g. `import kornia.augmentation as K`), but we don't allow implicit imports (e.g. accessing `K.auto.operations` without also importing `K.auto`). See https://github.com/astral-sh/ty/issues/133. * Many false positive `invalid-type-form` are removed because we now infer the correct type for some type expression instead of `Never`, which is not valid in a type annotation/expression context. ## Test Plan Added new Markdown tests --- .../resources/mdtest/import/cyclic.md | 108 ++++++++++++++++++ crates/ty_python_semantic/src/types/infer.rs | 42 ++++--- 2 files changed, 133 insertions(+), 17 deletions(-) create mode 100644 crates/ty_python_semantic/resources/mdtest/import/cyclic.md diff --git a/crates/ty_python_semantic/resources/mdtest/import/cyclic.md b/crates/ty_python_semantic/resources/mdtest/import/cyclic.md new file mode 100644 index 0000000000..4af714c7de --- /dev/null +++ b/crates/ty_python_semantic/resources/mdtest/import/cyclic.md @@ -0,0 +1,108 @@ +## Cyclic imports + +### Regression tests + +#### Issue 261 + +See: + +`main.py`: + +```py +from foo import bar + +reveal_type(bar) # revealed: +``` + +`foo/__init__.py`: + +```py +from foo import bar + +__all__ = ["bar"] +``` + +`foo/bar/__init__.py`: + +```py +# empty +``` + +#### Issue 113 + +See: + +`main.py`: + +```py +from pkg.sub import A + +# TODO: This should be `` +reveal_type(A) # revealed: Never +``` + +`pkg/outer.py`: + +```py +class A: ... +``` + +`pkg/sub/__init__.py`: + +```py +from ..outer import * +from .inner import * +``` + +`pkg/sub/inner.py`: + +```py +from pkg.sub import A +``` + +### Actual cycle + +The following example fails at runtime. Ideally, we would emit a diagnostic here. For now, we only +make sure that this does not lead to a module resolution cycle. + +`main.py`: + +```py +from module import x + +reveal_type(x) # revealed: Unknown +``` + +`module.py`: + +```py +# error: [unresolved-import] +from module import x +``` + +### Normal self-referential import + +Some modules like `sys` in typeshed import themselves. Here, we make sure that this does not lead to +cycles or unresolved imports. + +`module/__init__.py`: + +```py +import module # self-referential import + +from module.sub import x +``` + +`module/sub.py`: + +```py +x: int = 1 +``` + +`main.py`: + +```py +from module import x + +reveal_type(x) # revealed: int +``` diff --git a/crates/ty_python_semantic/src/types/infer.rs b/crates/ty_python_semantic/src/types/infer.rs index 7d85d5dd52..16ebb5b830 100644 --- a/crates/ty_python_semantic/src/types/infer.rs +++ b/crates/ty_python_semantic/src/types/infer.rs @@ -3946,26 +3946,34 @@ impl<'db> TypeInferenceBuilder<'db> { &alias.name.id }; + // Avoid looking up attributes on a module if a module imports from itself + // (e.g. `from parent import submodule` inside the `parent` module). + let import_is_self_referential = module_ty + .into_module_literal() + .is_some_and(|module| self.file() == module.module(self.db()).file()); + // First try loading the requested attribute from the module. - if let Symbol::Type(ty, boundness) = module_ty.member(self.db(), name).symbol { - if &alias.name != "*" && boundness == Boundness::PossiblyUnbound { - // TODO: Consider loading _both_ the attribute and any submodule and unioning them - // together if the attribute exists but is possibly-unbound. - if let Some(builder) = self - .context - .report_lint(&POSSIBLY_UNBOUND_IMPORT, AnyNodeRef::Alias(alias)) - { - builder.into_diagnostic(format_args!( - "Member `{name}` of module `{module_name}` is possibly unbound", - )); + if !import_is_self_referential { + if let Symbol::Type(ty, boundness) = module_ty.member(self.db(), name).symbol { + if &alias.name != "*" && boundness == Boundness::PossiblyUnbound { + // TODO: Consider loading _both_ the attribute and any submodule and unioning them + // together if the attribute exists but is possibly-unbound. + if let Some(builder) = self + .context + .report_lint(&POSSIBLY_UNBOUND_IMPORT, AnyNodeRef::Alias(alias)) + { + builder.into_diagnostic(format_args!( + "Member `{name}` of module `{module_name}` is possibly unbound", + )); + } } + self.add_declaration_with_binding( + alias.into(), + definition, + &DeclaredAndInferredType::AreTheSame(ty), + ); + return; } - self.add_declaration_with_binding( - alias.into(), - definition, - &DeclaredAndInferredType::AreTheSame(ty), - ); - return; } // If the module doesn't bind the symbol, check if it's a submodule. This won't get