Skip to content

Commit 9f05ebf

Browse files
sharkdpGlyphack
authored andcommitted
[ty] Recognize submodules in self-referential imports (astral-sh#18005)
## 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: <module 'os.path'> ``` closes astral-sh/ty#261 closes astral-sh/ty#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 astral-sh/ty#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
1 parent 16314c6 commit 9f05ebf

File tree

2 files changed

+133
-17
lines changed

2 files changed

+133
-17
lines changed
Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
## Cyclic imports
2+
3+
### Regression tests
4+
5+
#### Issue 261
6+
7+
See: <https://github.com/astral-sh/ty/issues/261>
8+
9+
`main.py`:
10+
11+
```py
12+
from foo import bar
13+
14+
reveal_type(bar) # revealed: <module 'foo.bar'>
15+
```
16+
17+
`foo/__init__.py`:
18+
19+
```py
20+
from foo import bar
21+
22+
__all__ = ["bar"]
23+
```
24+
25+
`foo/bar/__init__.py`:
26+
27+
```py
28+
# empty
29+
```
30+
31+
#### Issue 113
32+
33+
See: <https://github.com/astral-sh/ty/issues/113>
34+
35+
`main.py`:
36+
37+
```py
38+
from pkg.sub import A
39+
40+
# TODO: This should be `<class 'A'>`
41+
reveal_type(A) # revealed: Never
42+
```
43+
44+
`pkg/outer.py`:
45+
46+
```py
47+
class A: ...
48+
```
49+
50+
`pkg/sub/__init__.py`:
51+
52+
```py
53+
from ..outer import *
54+
from .inner import *
55+
```
56+
57+
`pkg/sub/inner.py`:
58+
59+
```py
60+
from pkg.sub import A
61+
```
62+
63+
### Actual cycle
64+
65+
The following example fails at runtime. Ideally, we would emit a diagnostic here. For now, we only
66+
make sure that this does not lead to a module resolution cycle.
67+
68+
`main.py`:
69+
70+
```py
71+
from module import x
72+
73+
reveal_type(x) # revealed: Unknown
74+
```
75+
76+
`module.py`:
77+
78+
```py
79+
# error: [unresolved-import]
80+
from module import x
81+
```
82+
83+
### Normal self-referential import
84+
85+
Some modules like `sys` in typeshed import themselves. Here, we make sure that this does not lead to
86+
cycles or unresolved imports.
87+
88+
`module/__init__.py`:
89+
90+
```py
91+
import module # self-referential import
92+
93+
from module.sub import x
94+
```
95+
96+
`module/sub.py`:
97+
98+
```py
99+
x: int = 1
100+
```
101+
102+
`main.py`:
103+
104+
```py
105+
from module import x
106+
107+
reveal_type(x) # revealed: int
108+
```

crates/ty_python_semantic/src/types/infer.rs

Lines changed: 25 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -4029,26 +4029,34 @@ impl<'db> TypeInferenceBuilder<'db> {
40294029
&alias.name.id
40304030
};
40314031

4032+
// Avoid looking up attributes on a module if a module imports from itself
4033+
// (e.g. `from parent import submodule` inside the `parent` module).
4034+
let import_is_self_referential = module_ty
4035+
.into_module_literal()
4036+
.is_some_and(|module| self.file() == module.module(self.db()).file());
4037+
40324038
// First try loading the requested attribute from the module.
4033-
if let Symbol::Type(ty, boundness) = module_ty.member(self.db(), name).symbol {
4034-
if &alias.name != "*" && boundness == Boundness::PossiblyUnbound {
4035-
// TODO: Consider loading _both_ the attribute and any submodule and unioning them
4036-
// together if the attribute exists but is possibly-unbound.
4037-
if let Some(builder) = self
4038-
.context
4039-
.report_lint(&POSSIBLY_UNBOUND_IMPORT, AnyNodeRef::Alias(alias))
4040-
{
4041-
builder.into_diagnostic(format_args!(
4042-
"Member `{name}` of module `{module_name}` is possibly unbound",
4043-
));
4039+
if !import_is_self_referential {
4040+
if let Symbol::Type(ty, boundness) = module_ty.member(self.db(), name).symbol {
4041+
if &alias.name != "*" && boundness == Boundness::PossiblyUnbound {
4042+
// TODO: Consider loading _both_ the attribute and any submodule and unioning them
4043+
// together if the attribute exists but is possibly-unbound.
4044+
if let Some(builder) = self
4045+
.context
4046+
.report_lint(&POSSIBLY_UNBOUND_IMPORT, AnyNodeRef::Alias(alias))
4047+
{
4048+
builder.into_diagnostic(format_args!(
4049+
"Member `{name}` of module `{module_name}` is possibly unbound",
4050+
));
4051+
}
40444052
}
4053+
self.add_declaration_with_binding(
4054+
alias.into(),
4055+
definition,
4056+
&DeclaredAndInferredType::AreTheSame(ty),
4057+
);
4058+
return;
40454059
}
4046-
self.add_declaration_with_binding(
4047-
alias.into(),
4048-
definition,
4049-
&DeclaredAndInferredType::AreTheSame(ty),
4050-
);
4051-
return;
40524060
}
40534061

40544062
// If the module doesn't bind the symbol, check if it's a submodule. This won't get

0 commit comments

Comments
 (0)