-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Implement from x.y import z ensuring x.y resolves in the current file
#21583
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
3e0996f to
bb12b0d
Compare
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
85171ad to
59b1221
Compare
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
possibly-missing-attribute |
218 | 379 | 126 |
unresolved-attribute |
16 | 23 | 0 |
invalid-assignment |
6 | 0 | 0 |
unused-ignore-comment |
2 | 4 | 0 |
invalid-argument-type |
3 | 0 | 0 |
unsupported-base |
0 | 1 | 0 |
| Total | 245 | 407 | 126 |
|
everything's great as long as you don't scroll down to the bit of the primer diff where the |
|
Taking a look at the code, the errors on prefect seem completely legitimate? The file doesn't import it and the |
Yeah I agree, I'm not sure why we aren't emitting errors on this on |
|
Also did you ever try not-distinguishing-import-kind? i.e. even making The synthesis of these two issues would be interested in it (but maybe we just use the kind machinery to represent that case anyway): |
I did, but it directly contradicts some longstanding tests that were added in #14946 sooo... I got scared :P I do think it makes sense that if a user has The longstanding tests were also added before we added any kind of ecosystem check, so I do think it would be interesting to see the ecosystem effect from not-distinguishing-import-kind as well. I'd like to try it separately, after I've debugged some of the stranger diagnostics on this branch a bit more. |
4d0ab1d to
ba3d1ed
Compare
8edee57 to
9355fa9
Compare
|
I can't see any "obviously wrong" diagnostics in the ecosystem anymore among the added or changed diagnostics. Nearly all the new diagnostics are on prefect. As noted above, I'm not sure why these aren't emitted on Still TODO is
|
|
Oh dear, rebasing + rerunning has even worse results than last time. Gotta look at what's goin' on... |
|
This is an acceptable regression. |
|
Ok well I can see why you had trouble reproducing the issue... THE ISSUE ONLY APPEARS IF THE CODE CONTAINS YOU DON'T EVEN NEED TO USE IT. WHAT. |
|
More precise statement after messing around with it: |
This comment was marked as outdated.
This comment was marked as outdated.
|
Note: split the uniformly-weak-submodule-attributes out to #21653 |
c7dffd8 to
a2f9e5a
Compare
|
scipy has the same magic lazy imports with |
- [warning] possibly-missing-attribute - Submodule `modin` may not be available as an attribute on module `pandera.typing`
+ [error] unresolved-attribute - Object of type `<module 'pandera.typing'> | <module 'pandera.typing'>` has no attribute `modin`This is an unfortunate but kinda minor degradation (we could potentially add some diagnostic cleaning for it). |
|
I think this is ready for review/discussion. |
Gankra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I actually didn't end up changing the code itself, so, the code changes honestly look like what I'd expect.
|
Thank you so much for picking this up, and in particular for experimenting with #21653 separately! I'm surprised at how small the diff is on #21653 -- it's basically all pwndbg, which is doing very strange and unusual things with its submodule imports ("submodule imports Georg" should be excluded as an outlier, etc.). So I wonder if we should just adopt the rule proposed by #21653, where submodule attributes always have lower priority than symbols defined in the As per #14946 (comment), that would mean that we'd be moving towards mypy's behaviour here and away from pyright's. But empirically, the ecosystem results appear to indicate that mypy's heuristic works better much more of the time than pyright's does. I'm curious for what others think; don't make any changes here until other folks have had a chance to chime in 😅 The disadvantage of adopting the rule in #21653 is that you can get counterintuitive situations where things like this can happen: import a.b
reveal_type(a.b) # revealed: Literal[1]But the ecosystem report suggests that situations like this are rare. And honestly, at some point you just have to say that libraries need to stop shadowing their submodules with variables in |
Summary
Implement
from x.y import zensuringx.yresolves in the current file if ayattribute isn't otherwise available onx. This is implemented by adding these kinds of imports toavailable_submodule_attributesand lowering the priority ofavailable_submodule_attributes.Keeping the priority low ensures that things like
from .y import yin an__init__.pyreliable exposes the function/classyand not the submoduley(an extremely common idiom).Fixes astral-sh/ty#1654
Test Plan
Snapshot tests.