From fe4ee81b9749bd326845aec30e694af8568549e6 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Mon, 3 Nov 2025 15:24:01 -0500 Subject: [PATCH] [ty] prefer submodule over module __getattr__ in from-imports (#21260) Fixes https://github.com/astral-sh/ty/issues/1053 ## Summary Other type checkers prioritize a submodule over a package `__getattr__` in `from mod import sub`, even though the runtime precedence is the other direction. In effect, this is making an implicit assumption that a module `__getattr__` will not handle (that is, will raise `AttributeError`) for names that are also actual submodules, rather than shadowing them. In practice this seems like a realistic assumption in the ecosystem? Or at least the ecosystem has adapted to it, and we need to adapt this precedence also, for ecosystem compatibility. The implementation is a bit ugly, precisely because it departs from the runtime semantics, and our implementation is oriented toward modeling runtime semantics accurately. That is, `__getattr__` is modeled within the member-lookup code, so it's hard to split "member lookup result from module `__getattr__`" apart from other member lookup results. I did this via a synthetic `TypeQualifier::FROM_MODULE_GETATTR` that we attach to a type resulting from a member lookup, which isn't beautiful but it works well and doesn't introduce inefficiency (e.g. redundant member lookups). ## Test Plan Updated mdtests. Also added a related mdtest formalizing our support for a module `__getattr__` that is explicitly annotated to accept a limited set of names. In principle this could be an alternative (more explicit) way to handle the precedence problem without departing from runtime semantics, if the ecosystem would adopt it. ### Ecosystem analysis Lots of removed diagnostics which are an improvement because we now infer the expected submodule. Added diagnostics are mostly unrelated issues surfaced now because we previously had an earlier attribute error resulting in `Unknown`; now we correctly resolve the module so that earlier attribute error goes away, we get an actual type instead of `Unknown`, and that triggers a new error. In scipy and sklearn, the module `__getattr__` which we were respecting previously is un-annotated so returned a forgiving `Unknown`; now we correctly see the actual module, which reveals some cases of https://github.com/astral-sh/ty/issues/133 that were previously hidden (`scipy/optimize/__init__.py` [imports `from ._tnc`](https://github.com/scipy/scipy/blob/eff82ca575668d2d7a4bc12b6afba98daaf6d5d0/scipy/optimize/__init__.py#L429).) --------- Co-authored-by: Alex Waygood --- .../resources/mdtest/import/module_getattr.md | 51 +++++++++++++++++-- crates/ty_python_semantic/src/types.rs | 9 +++- .../src/types/infer/builder.rs | 50 +++++++++++++----- 3 files changed, 92 insertions(+), 18 deletions(-) diff --git a/crates/ty_python_semantic/resources/mdtest/import/module_getattr.md b/crates/ty_python_semantic/resources/mdtest/import/module_getattr.md index 4c493f4b74..79cce812fe 100644 --- a/crates/ty_python_semantic/resources/mdtest/import/module_getattr.md +++ b/crates/ty_python_semantic/resources/mdtest/import/module_getattr.md @@ -60,11 +60,6 @@ def __getattr__(name: str) -> str: If a package's `__init__.py` (e.g. `mod/__init__.py`) defines a `__getattr__` function, and there is also a submodule file present (e.g. `mod/sub.py`), then: -- If you do `import mod` (without importing the submodule directly), accessing `mod.sub` will call - `mod.__getattr__('sub')`, so `reveal_type(mod.sub)` will show the return type of `__getattr__`. -- If you do `import mod.sub` (importing the submodule directly), then `mod.sub` refers to the actual - submodule, so `reveal_type(mod.sub)` will show the type of the submodule itself. - `mod/__init__.py`: ```py @@ -78,6 +73,9 @@ def __getattr__(name: str) -> str: value = 42 ``` +If you `import mod` (without importing the submodule directly), accessing `mod.sub` will call +`mod.__getattr__('sub')`, so `reveal_type(mod.sub)` will show the return type of `__getattr__`. + `test_import_mod.py`: ```py @@ -86,6 +84,9 @@ import mod reveal_type(mod.sub) # revealed: str ``` +If you `import mod.sub` (importing the submodule directly), then `mod.sub` refers to the actual +submodule, so `reveal_type(mod.sub)` will show the type of the submodule itself. + `test_import_mod_sub.py`: ```py @@ -93,3 +94,43 @@ import mod.sub reveal_type(mod.sub) # revealed: ``` + +If you `from mod import sub`, at runtime `sub` will be the value returned by the module +`__getattr__`, but other type checkers do not model the precedence this way. They will always prefer +a submodule over a package `__getattr__`, and thus this is the current expectation in the ecosystem. +Effectively, this assumes that a well-implemented package `__getattr__` will always raise +`AttributeError` for a name that also exists as a submodule (and in fact this is the case for many +module `__getattr__` in the ecosystem.) + +`test_from_import.py`: + +```py +from mod import sub + +reveal_type(sub) # revealed: +``` + +## Limiting names handled by `__getattr__` + +If a module `__getattr__` is annotated to only accept certain string literals, then the module +`__getattr__` will be ignored for other names. (In principle this could be a more explicit way to +handle the precedence issues discussed above, but it's not currently used in the ecosystem.) + +```py +from limited_getattr_module import known_attr + +# error: [unresolved-import] +from limited_getattr_module import unknown_attr + +reveal_type(known_attr) # revealed: int +reveal_type(unknown_attr) # revealed: Unknown +``` + +`limited_getattr_module.py`: + +```py +from typing import Literal + +def __getattr__(name: Literal["known_attr"]) -> int: + return 3 +``` diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index e79282a35d..3e3c241925 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -7909,6 +7909,10 @@ bitflags! { /// instance attributes that are only implicitly defined via `self.x = …` in /// the body of a class method. const IMPLICIT_INSTANCE_ATTRIBUTE = 1 << 6; + /// A non-standard type qualifier that marks a type returned from a module-level + /// `__getattr__` function. We need this in order to implement precedence of submodules + /// over module-level `__getattr__`, for compatibility with other type checkers. + const FROM_MODULE_GETATTR = 1 << 7; } } @@ -11026,7 +11030,10 @@ impl<'db> ModuleLiteralType<'db> { db, &CallArguments::positional([Type::string_literal(db, name)]), ) { - return Place::Defined(outcome.return_type(db), origin, boundness).into(); + return PlaceAndQualifiers { + place: Place::Defined(outcome.return_type(db), origin, boundness), + qualifiers: TypeQualifiers::FROM_MODULE_GETATTR, + }; } } } diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs index 078d49fe5e..cbb2fe8236 100644 --- a/crates/ty_python_semantic/src/types/infer/builder.rs +++ b/crates/ty_python_semantic/src/types/infer/builder.rs @@ -5347,6 +5347,10 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { .as_module_literal() .is_some_and(|module| Some(self.file()) == module.module(self.db()).file(self.db())); + // Although it isn't the runtime semantics, we go to some trouble to prioritize a submodule + // over module `__getattr__`, because that's what other type checkers do. + let mut from_module_getattr = None; + // First try loading the requested attribute from the module. if !import_is_self_referential { if let PlaceAndQualifiers { @@ -5366,19 +5370,23 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { )); } } - self.add_declaration_with_binding( - alias.into(), - definition, - &DeclaredAndInferredType::MightBeDifferent { - declared_ty: TypeAndQualifiers { - inner: ty, - origin: TypeOrigin::Declared, - qualifiers, + if qualifiers.contains(TypeQualifiers::FROM_MODULE_GETATTR) { + from_module_getattr = Some((ty, qualifiers)); + } else { + self.add_declaration_with_binding( + alias.into(), + definition, + &DeclaredAndInferredType::MightBeDifferent { + declared_ty: TypeAndQualifiers { + inner: ty, + origin: TypeOrigin::Declared, + qualifiers, + }, + inferred_ty: ty, }, - inferred_ty: ty, - }, - ); - return; + ); + return; + } } } @@ -5418,6 +5426,24 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { return; } + // We've checked for a submodule, so now we can go ahead and use a type from module + // `__getattr__`. + if let Some((ty, qualifiers)) = from_module_getattr { + self.add_declaration_with_binding( + alias.into(), + definition, + &DeclaredAndInferredType::MightBeDifferent { + declared_ty: TypeAndQualifiers { + inner: ty, + origin: TypeOrigin::Declared, + qualifiers, + }, + inferred_ty: ty, + }, + ); + return; + } + self.add_unknown_declaration_with_binding(alias.into(), definition); if &alias.name == "*" {