[ty] Support implicit imports of submodules in __init__.pyi#20855
[ty] Support implicit imports of submodules in __init__.pyi#20855
__init__.pyi#20855Conversation
Gankra
requested review from
AlexWaygood,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
CodSpeed Performance ReportMerging #20855 will not alter performanceComparing Summary
|
|
AlexWaygood
changed the title
__init__.pyi__init__.pyi
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
unresolved-attribute |
0 | 61 | 1 |
invalid-argument-type |
13 | 0 | 0 |
possibly-missing-attribute |
9 | 0 | 0 |
| Total | 22 | 61 | 1 |
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Nice, this looks like a great step forward in terms of the semantics. Thanks for writing such comprehensive tests!
| In the context of a `.py` we handle this well through our general attempts to faithfully implement | ||
| import side-effects. However for `.pyi` files we are expected to apply |
There was a problem hiding this comment.
nit: it's not really a side effect! Just the direct impact of the import system working as designed: importing any object X (whether that object is a class, function, module, or anything else) into a module M stores X in the global namespace of M and makes it available as an attribute on M when M is imported.
There was a problem hiding this comment.
That sounds like a side-effect to me? As in, state has been mutated.
There was a problem hiding this comment.
But that mutation of state is the primary thing an import statement in Python does, so I don't see it as a side effect 😄 it's the intended purpose, not something that happens by accident as a result of an operation that's intended to do something else
There was a problem hiding this comment.
I think this might reveal an interesting difference in perspective that stems from how dynamic Python is (so much happens at runtime; so little happens at compile time compared to, e.g., Rust).
In Python, I think the statement from x import y in a module z is best conceptualised as an explicit command to the interpreter that says "At runtime, at this point in the global namespace, lookup the symbol y in the x module/package and store a reference to it in the global namespace of z". All other consequences of the import statement, such as it then being available for use elsewhere in the module z, or being available as an attribute on the module z, are downstream from that. from x import y just desugars to something like this, which is executed at runtime just like all other statements or expressions in the module's global namespace:
try:
y = __import__("x.y").y
except ImportError: # not a submodule; look it up as an attribute instead
y = __import__("x").yIf y is loaded into the global namespace of an z/__init__.py file, it will be available from all code in that file and as an attribute on the z package, just like any other symbol that's loaded into the global namespace of that file. So I don't really view the state mutation here as fundamentally different to what happens with any other import in Python, or as a "side effect" of the import system -- this is just how imports work in Python, and it's the primary effect of them IMO
|
|
||
| To that end we define the following extension: | ||
|
|
||
| > If an `__init__.pyi` for `mypackage` contains a `from...import` targetting a submodule, then that |
There was a problem hiding this comment.
| > If an `__init__.pyi` for `mypackage` contains a `from...import` targetting a submodule, then that | |
| > If an `__init__.pyi` for `mypackage` contains a `from...import` targeting a submodule, then that |
crates/ty_python_semantic/resources/mdtest/import/nonstandard_conventions.md
Outdated
Show resolved
Hide resolved
| ## Import of Direct Submodule in (Non-Stub Check) | ||
|
|
||
| `mypackage/__init__.py`: | ||
|
|
||
| ```py | ||
| import mypackage.imported | ||
| ``` | ||
|
|
||
| `mypackage/imported.py`: | ||
|
|
||
| ```py | ||
| X: int = 42 | ||
| ``` | ||
|
|
||
| `main.py`: | ||
|
|
||
| ```py | ||
| import mypackage | ||
|
|
||
| # error: "has no attribute `imported`" | ||
| reveal_type(mypackage.imported.X) # revealed: Unknown | ||
| ``` |
There was a problem hiding this comment.
I think we should try to support this, at least in non-stub files (but possibly not in stubs). This is exactly what the original examples in astral-sh/ty#133 were about, before that issue got kinda hijacked by examples that were really about exemptions we should carve out for the stub file re-export convention 😄
We don't need to add support for this in this PR, but it might be good to add a TODO here, I think?
There was a problem hiding this comment.
Agreed that supporting this would be pretty natural, although I'm a bit nervous about making this extension/idiom "too broad" and undermining the whole idea of re-exports having to be explicit.
There was a problem hiding this comment.
Agreed that supporting this would be pretty natural, although I'm a bit nervous about making this extension/idiom "too broad" and undermining the whole idea of re-exports having to be explicit.
yeah, I do agree with that hesitancy for stub files!
For runtime files, though, I think we should just model the runtime semantics as closely as is reasonable. Failing to be explicit about re-exports in a runtime file is at best something a linter should be concerned with, not a type checker, arguably. In runtime files, a type checker should focus on things that are actually going to cause runtime errors (or are unsound in some type-theoretic way). Things that are in poor taste because they rely on implicit behaviour don't really fall into that bucket IMO.
There was a problem hiding this comment.
I should add some tests for imports at non-global scope (inside functions), which, are a more muddy situation.
crates/ty_python_semantic/resources/mdtest/import/nonstandard_conventions.md
Outdated
Show resolved
Hide resolved
| ## From Import of Non-Submodule (Non-Stub Check) | ||
|
|
||
| `mypackage/__init__.py`: | ||
|
|
||
| ```py | ||
| from .imported import X | ||
| ``` | ||
|
|
||
| `mypackage/imported.py`: | ||
|
|
||
| ```py | ||
| X: int = 42 | ||
| ``` | ||
|
|
||
| `main.py`: | ||
|
|
||
| ```py | ||
| import mypackage | ||
|
|
||
| # error: "has no attribute `imported`" | ||
| reveal_type(mypackage.imported.X) # revealed: Unknown | ||
| ``` |
There was a problem hiding this comment.
Again, I think it would be good to at least support this for non-stub files eventually; it's probably worth adding a TODO here to make it clear that this isn't the desired end state
There was a problem hiding this comment.
Given the rest of our handling of non-stubs I agree that this should work for non-stubs (and only non-stubs).
(Definitely out of scope for this PR though)
| `from mypackage import submodule` from outside the package is not modeled as a side-effect on | ||
| `mypackage`, even in the importing file. |
There was a problem hiding this comment.
hmmmm, we could consider changing this in the future, too
There was a problem hiding this comment.
Agreed, I think fixing this could be in-scope for this PR without too much work.
|
Analysis of ecosystem failures: croniterThis case would be fixed by introducing a rule that non-matching asname aliases disable the submodule init rule. I briefly discussed this with Alex and he said something to the effect of "well adding the submodule even if there's an alias is the runtime behaviour" and then I forgot to add a test for it mentioning it as a distinct case. EDIT: I pushed this change with tests Failure: In validate.py: In typeshed's from croniter import croniter
...
iter = croniter(cron_schedule, starting_from)# import that we now consider as introducing the submodule attr
from . import croniter as croniter_m
# explicit re-export of the class (that now gets shadowed by our "submodule attrs win ties" rule)
from .croniter import croniter as croniterscikitThis case is harder for me to wrap my head around, largely because I can't give a coherent explanation for why it Failure: In from scipy.sparse.linalg import lobpcg
...
_, diffusion_map = lobpcg(laplacian, X, M=M, tol=tol, largest=False)
...In scipy-stubs' # im
from ._eigen import lobpcg
__all__ = [
...
"lobpcg",
...
]In scipy-stubs' # importing the lobpcg function from the submodule of the same name
from .lobpcg import lobpcg
__all__ = [..., "lobpcg", ...] |
Ah, no, |
|
Ah ok I had a nagging feeling you did say that but I was like "no Aria you always want Ok then my guess is that the issue is It is simultaneously a valid import of the |
|
I believe the re-export version of this PR avoids the linalg issue by virtue of it not adding anything to the special set of submodule attributes that gets higher priority (not necessarily an argument for it being a better approach, just noting it). |
|
I've put up a reduced test case for the scipy issue (as reduced as I could get it). Also I think addressed all review comments. |
| /// The set of modules that are imported anywhere within this file. | ||
| maybe_imported_modules: Arc<FxHashSet<MaybeModuleImport>>, |
There was a problem hiding this comment.
Could you update the comment so that it explains how this field is different from imported_modules?
I assume we can't really unify the two (I'm asking because I want to avoid that we store redundant information in the semantic index, which is one of the main contributor to ty's memory usage)
There was a problem hiding this comment.
Whether we can unify the two I believe hinges on whether we actually want from...import and import to ever be treated differently for the purposes of import/re-export idioms.
There was a problem hiding this comment.
Actually I think I could probably unify them still...
| imported_modules: Arc<FxHashSet<ModuleName>>, | ||
|
|
||
| /// The set of modules that are imported anywhere within this file. | ||
| maybe_imported_modules: Arc<FxHashSet<MaybeModuleImport>>, |
There was a problem hiding this comment.
I don't think this needs to be an Arc, as we have no #[salsa::tracked] query that returns the maybe imported modules directly
| maybe_imported_modules: Arc<FxHashSet<MaybeModuleImport>>, | |
| maybe_imported_modules: FxHashSet<MaybeModuleImport>, |
| scopes_by_node: self.scopes_by_node, | ||
| use_def_maps, | ||
| imported_modules: Arc::new(self.imported_modules), | ||
| maybe_imported_modules: Arc::new(self.maybe_imported_modules), |
There was a problem hiding this comment.
Let's call shrink_to_fit before passing the modules to SemanticIndex
There was a problem hiding this comment.
this looks like it's still unaddressed
| }; | ||
|
|
||
| // If there's no alias or a redundant alias, record this as a potential import of a submodule | ||
| if alias.asname.is_none() || is_reexported { |
There was a problem hiding this comment.
Would it be sufficient to only collect the import statements at the module level (skip if we're inside a function or class)? In which case, I even wonder if this has to be inside semantic_index or if it could just be its own salsa query that traverses the top-level statements of a module without traversing into inner blocks.
There was a problem hiding this comment.
I was going off of the fact that we apply no such logic when handling imported_modules. I agree anything but top-level feels a biiit dubious (and I believe pyright agrees with that).
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Nice, I still think this looks great in terms of the semantics! I think we should aim to land this more-or less as-is, notwithstanding some edge cases in scipy
| # error: "has no attribute `imported`" | ||
| reveal_type(mypackage.imported.X) # revealed: Unknown |
There was a problem hiding this comment.
I still think it would be great to add a TODO comment here (#20855 (comment))
Since this is what the original examples in github.com/astral-sh/ty/issues/133 were about, I think this PR arguably doesn't actually close that issue -- rather than being about implicit imports of submodules in __init__.py files (what that issue was originally about), to my mind this PR is really about carving out exemptions to the explicit re-export convention for stub files in __init__.pyi files. It's still a good and important change for us to make! But something slightly different to what github.com/astral-sh/ty/issues/133 was originally about.
| # error: "has no attribute `imported`" | ||
| reveal_type(mypackage.imported.X) # revealed: Unknown |
There was a problem hiding this comment.
I still think a TODO comment here would be great to make it clear that this isn't asserting our intended behaviour, just capturing our current behaviour (#20855 (comment))
| # error: "has no attribute `imported`" | ||
| reveal_type(mypackage.imported.X) # revealed: Unknown |
There was a problem hiding this comment.
I know you signalled this in the prose description, but I think a TODO comment next to this assertion would also be great -- it can be confusing for people working on features if existing "assertions" start "breaking" when they improve semantics. Having a TODO comment right next to the assertion is really helpful for that.
|
What's the status of this PR? Are you waiting on any more input/feedback? Is it blocked on anything? |
This is a second take at the implicit imports approach, allowing `from . import submodule` in an `__init__.pyi` to create the `mypackage.submodule` attribute everyhere. This implementation operates inside of the available_submodule_attributes subsystem instead of as a re-export rule. The upside of this is we are no longer purely syntactic, and absolute from imports that happen to target submodules work (an intentional discussed deviation from pyright which demands a relative from import). Also we don't re-export functions or classes. The downside(?) of this is star imports no longer see these attributes (this may be either good or bad. I believe it's not a huge lift to make it work with star imports but it's some non-trivial reworking). I've also intentionally made `import mypackage.submodule` not trigger this rule although it's trivial to change that. I've tried to cover as many relevant cases as possible for discussion in the new test file I've added (there are some random overlaps with existing tests but trying to add them piecemeal felt confusing and weird, so I just made a dedicated file for this extension to the rules). Fixes #133
Gankra
force-pushed
the
gankra/implort3
branch
from
753abbd to
02f4e5d
Compare
Gankra
force-pushed
the
gankra/implort3
branch
from
46736a5 to
62ac621
Compare
|
Nice, the optimization I wanted to try ended up actually being a fix for the |
| A from import that terminates in a non-submodule should not expose the intermediate submodules as | ||
| attributes. |
There was a problem hiding this comment.
(this could also be changed in the future, as discussed, but no need to do that in this PR of course!)
| scopes_by_node: self.scopes_by_node, | ||
| use_def_maps, | ||
| imported_modules: Arc::new(self.imported_modules), | ||
| maybe_imported_modules: Arc::new(self.maybe_imported_modules), |
There was a problem hiding this comment.
this looks like it's still unaddressed
3 participants
This is a second take at the implicit imports approach, allowing
from . import submodulein an__init__.pyito create themypackage.submoduleattribute everyhere.This implementation operates inside of the available_submodule_attributes subsystem instead of as a re-export rule.
The upside of this is we are no longer purely syntactic, and absolute from imports that happen to target submodules work (an intentional discussed deviation from pyright which demands a relative from import). Also we don't re-export functions or classes.
The downside(?) of this is star imports no longer see these attributes (this may be either good or bad. I believe it's not a huge lift to make it work with star imports but it's some non-trivial reworking).
I've also intentionally made
import mypackage.submodulenot trigger this rule although it's trivial to change that.I've tried to cover as many relevant cases as possible for discussion in the new test file I've added (there are some random overlaps with existing tests but trying to add them piecemeal felt confusing and weird, so I just made a dedicated file for this extension to the rules).
Fixes astral-sh/ty#133
Summary
Test Plan