Skip to content

Commit fe4ee81

Browse files
carljmAlexWaygood
andauthored
[ty] prefer submodule over module __getattr__ in from-imports (#21260)
Fixes astral-sh/ty#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 astral-sh/ty#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 <Alex.Waygood@Gmail.com>
1 parent 0433526 commit fe4ee81

File tree

3 files changed

+92
-18
lines changed

3 files changed

+92
-18
lines changed

crates/ty_python_semantic/resources/mdtest/import/module_getattr.md

Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -60,11 +60,6 @@ def __getattr__(name: str) -> str:
6060
If a package's `__init__.py` (e.g. `mod/__init__.py`) defines a `__getattr__` function, and there is
6161
also a submodule file present (e.g. `mod/sub.py`), then:
6262

63-
- If you do `import mod` (without importing the submodule directly), accessing `mod.sub` will call
64-
`mod.__getattr__('sub')`, so `reveal_type(mod.sub)` will show the return type of `__getattr__`.
65-
- If you do `import mod.sub` (importing the submodule directly), then `mod.sub` refers to the actual
66-
submodule, so `reveal_type(mod.sub)` will show the type of the submodule itself.
67-
6863
`mod/__init__.py`:
6964

7065
```py
@@ -78,6 +73,9 @@ def __getattr__(name: str) -> str:
7873
value = 42
7974
```
8075

76+
If you `import mod` (without importing the submodule directly), accessing `mod.sub` will call
77+
`mod.__getattr__('sub')`, so `reveal_type(mod.sub)` will show the return type of `__getattr__`.
78+
8179
`test_import_mod.py`:
8280

8381
```py
@@ -86,10 +84,53 @@ import mod
8684
reveal_type(mod.sub) # revealed: str
8785
```
8886

87+
If you `import mod.sub` (importing the submodule directly), then `mod.sub` refers to the actual
88+
submodule, so `reveal_type(mod.sub)` will show the type of the submodule itself.
89+
8990
`test_import_mod_sub.py`:
9091

9192
```py
9293
import mod.sub
9394

9495
reveal_type(mod.sub) # revealed: <module 'mod.sub'>
9596
```
97+
98+
If you `from mod import sub`, at runtime `sub` will be the value returned by the module
99+
`__getattr__`, but other type checkers do not model the precedence this way. They will always prefer
100+
a submodule over a package `__getattr__`, and thus this is the current expectation in the ecosystem.
101+
Effectively, this assumes that a well-implemented package `__getattr__` will always raise
102+
`AttributeError` for a name that also exists as a submodule (and in fact this is the case for many
103+
module `__getattr__` in the ecosystem.)
104+
105+
`test_from_import.py`:
106+
107+
```py
108+
from mod import sub
109+
110+
reveal_type(sub) # revealed: <module 'mod.sub'>
111+
```
112+
113+
## Limiting names handled by `__getattr__`
114+
115+
If a module `__getattr__` is annotated to only accept certain string literals, then the module
116+
`__getattr__` will be ignored for other names. (In principle this could be a more explicit way to
117+
handle the precedence issues discussed above, but it's not currently used in the ecosystem.)
118+
119+
```py
120+
from limited_getattr_module import known_attr
121+
122+
# error: [unresolved-import]
123+
from limited_getattr_module import unknown_attr
124+
125+
reveal_type(known_attr) # revealed: int
126+
reveal_type(unknown_attr) # revealed: Unknown
127+
```
128+
129+
`limited_getattr_module.py`:
130+
131+
```py
132+
from typing import Literal
133+
134+
def __getattr__(name: Literal["known_attr"]) -> int:
135+
return 3
136+
```

crates/ty_python_semantic/src/types.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7909,6 +7909,10 @@ bitflags! {
79097909
/// instance attributes that are only implicitly defined via `self.x = …` in
79107910
/// the body of a class method.
79117911
const IMPLICIT_INSTANCE_ATTRIBUTE = 1 << 6;
7912+
/// A non-standard type qualifier that marks a type returned from a module-level
7913+
/// `__getattr__` function. We need this in order to implement precedence of submodules
7914+
/// over module-level `__getattr__`, for compatibility with other type checkers.
7915+
const FROM_MODULE_GETATTR = 1 << 7;
79127916
}
79137917
}
79147918

@@ -11026,7 +11030,10 @@ impl<'db> ModuleLiteralType<'db> {
1102611030
db,
1102711031
&CallArguments::positional([Type::string_literal(db, name)]),
1102811032
) {
11029-
return Place::Defined(outcome.return_type(db), origin, boundness).into();
11033+
return PlaceAndQualifiers {
11034+
place: Place::Defined(outcome.return_type(db), origin, boundness),
11035+
qualifiers: TypeQualifiers::FROM_MODULE_GETATTR,
11036+
};
1103011037
}
1103111038
}
1103211039
}

crates/ty_python_semantic/src/types/infer/builder.rs

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5347,6 +5347,10 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
53475347
.as_module_literal()
53485348
.is_some_and(|module| Some(self.file()) == module.module(self.db()).file(self.db()));
53495349

5350+
// Although it isn't the runtime semantics, we go to some trouble to prioritize a submodule
5351+
// over module `__getattr__`, because that's what other type checkers do.
5352+
let mut from_module_getattr = None;
5353+
53505354
// First try loading the requested attribute from the module.
53515355
if !import_is_self_referential {
53525356
if let PlaceAndQualifiers {
@@ -5366,19 +5370,23 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
53665370
));
53675371
}
53685372
}
5369-
self.add_declaration_with_binding(
5370-
alias.into(),
5371-
definition,
5372-
&DeclaredAndInferredType::MightBeDifferent {
5373-
declared_ty: TypeAndQualifiers {
5374-
inner: ty,
5375-
origin: TypeOrigin::Declared,
5376-
qualifiers,
5373+
if qualifiers.contains(TypeQualifiers::FROM_MODULE_GETATTR) {
5374+
from_module_getattr = Some((ty, qualifiers));
5375+
} else {
5376+
self.add_declaration_with_binding(
5377+
alias.into(),
5378+
definition,
5379+
&DeclaredAndInferredType::MightBeDifferent {
5380+
declared_ty: TypeAndQualifiers {
5381+
inner: ty,
5382+
origin: TypeOrigin::Declared,
5383+
qualifiers,
5384+
},
5385+
inferred_ty: ty,
53775386
},
5378-
inferred_ty: ty,
5379-
},
5380-
);
5381-
return;
5387+
);
5388+
return;
5389+
}
53825390
}
53835391
}
53845392

@@ -5418,6 +5426,24 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
54185426
return;
54195427
}
54205428

5429+
// We've checked for a submodule, so now we can go ahead and use a type from module
5430+
// `__getattr__`.
5431+
if let Some((ty, qualifiers)) = from_module_getattr {
5432+
self.add_declaration_with_binding(
5433+
alias.into(),
5434+
definition,
5435+
&DeclaredAndInferredType::MightBeDifferent {
5436+
declared_ty: TypeAndQualifiers {
5437+
inner: ty,
5438+
origin: TypeOrigin::Declared,
5439+
qualifiers,
5440+
},
5441+
inferred_ty: ty,
5442+
},
5443+
);
5444+
return;
5445+
}
5446+
54215447
self.add_unknown_declaration_with_binding(alias.into(), definition);
54225448

54235449
if &alias.name == "*" {

0 commit comments

Comments
 (0)