[ty] Implement module-level __getattr__ support
#19791
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PrettyWood
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
PrettyWood
changed the title
__getattr__ support
Contributor
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
Contributor
|
carljm
requested changes
Aug 6, 2025
crates/ty_python_semantic/resources/mdtest/import/module_getattr.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/import/module_getattr.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/import/module_getattr.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/import/module_getattr.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/import/module_getattr.md
Outdated
Show resolved
Hide resolved
Contributor
Author
|
Thanks a lot for the detailed review @carljm. Really appreciate. I’ve taken all your feedback into account, and it’s ready for another look. The ecosystem you're building with uv, ruff, and now ty is genuinely impressive. It's raising the bar for the entire python community. Huge respect for the work you're doing. |
PrettyWood
commented
Aug 7, 2025
| value = 42 | ||
| ``` | ||
|
|
||
| ## Precedence: submodules vs `__getattr__` - Case 2 |
Contributor
Author
There was a problem hiding this comment.
If you know how I can merge cases 1 and 2 together, feel free to update directly the PR but since we are doing static analysis, we can't play with sys.modules or anything like that
Contributor
There was a problem hiding this comment.
You could share the definitions of mod/__init__.py and mod/sub.py and then have two different named files (one that does import mod and the other that does import mod.sub), all in the same test.
carljm
reviewed
Aug 8, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This looks good, thank you!
I realized we are missing the case of from mod import foo, we only handle import mod; mod.foo in this PR.
| value = 42 | ||
| ``` | ||
|
|
||
| ## Precedence: submodules vs `__getattr__` - Case 2 |
Contributor
There was a problem hiding this comment.
You could share the definitions of mod/__init__.py and mod/sub.py and then have two different named files (one that does import mod and the other that does import mod.sub), all in the same test.
| @@ -0,0 +1,84 @@ | |||
| # Module-level `__getattr__` | |||
Contributor
There was a problem hiding this comment.
I thought of one other case we are missing. We don't necessarily need to add support for it in this PR (though if you are up for it, that's also fine!), but I think we should at least add a test for it with TODO comment to help us keep track of the fact that it's missing.
At runtime if module mod.py has __getattr__ implementation, you can also do from mod import whatever and it will exercise the __getattr__. Currently this PR doesn't implement that, it only implements attribute access. To do it for imports as well, we'd need to add a similar case in the imported_symbol function (in place.rs) as a fallback. We would probably want to refactor your try_module_getattr method to be a standalone function instead that takes a &Db and a File, so we can call it also from imported_symbol.
Contributor
Author
There was a problem hiding this comment.
Maybe I didn't understand properly what you meant.
The flow is already from mod import whatever → infer_import_from_definition() → module_ty.member() → module.static_member() → try_module_getattr() (if normal lookup failed)
I added a test that passes that does what you wrote (again I may have misunderstood)
Contributor
There was a problem hiding this comment.
Oh! My bad, I thought I had tested this and it wasn't working, but I must have done something wrong. That's great that we actually only need to do this in one place :) The new test looks excellent, thank you!
carljm
approved these changes
Aug 8, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Excellent work, thank you!!
| @@ -0,0 +1,84 @@ | |||
| # Module-level `__getattr__` | |||
Contributor
There was a problem hiding this comment.
Oh! My bad, I thought I had tested this and it wasn't working, but I must have done something wrong. That's great that we actually only need to do this in one place :) The new test looks excellent, thank you!
dcreager
added a commit
that referenced
this pull request
Aug 11, 2025
* main: (31 commits) Add AIR301 rule (#17707) Avoid underflow in default ranges before a BOM (#19839) Update actions/download-artifact digest to de96f46 (#19852) Update docker/login-action action to v3.5.0 (#19860) Update rui314/setup-mold digest to 7344740 (#19853) Update cargo-bins/cargo-binstall action to v1.14.4 (#19855) Update actions/cache action to v4.2.4 (#19854) Update Rust crate hashbrown to v0.15.5 (#19858) Update Rust crate camino to v1.1.11 (#19857) Update Rust crate proc-macro2 to v1.0.96 (#19859) Update dependency ruff to v0.12.8 (#19856) SIM905: Fix handling of U+001C..U+001F whitespace (#19849) RUF064: offer a safe fix for multi-digit zeros (#19847) Clean up unused rendering code in `ruff_linter` (#19832) [ty] Add Salsa caching to `TupleType::to_class_type` (#19840) [ty] Handle cycles when finding implicit attributes (#19833) [ty] fix goto-definition on imports (#19834) [ty] Implement stdlib stub mapping (#19529) [`flake8-comprehensions`] Fix false positive for `C420` with attribute, subscript, or slice assignment targets (#19513) [ty] Implement module-level `__getattr__` support (#19791) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix astral-sh/ty#943
Summary
Add module-level
__getattr__support for ty's type checker, fixing issue astral-sh/ty#943.Module-level
__getattr__functions (PEP 562) are now respected when resolving dynamic attributes, matching the behavior of mypy and pyright.Implementation
Thanks @sharkdp for the guidance in astral-sh/ty#943 (comment)
__getattr__logic into a reusable helper function__getattr__resolution inModuleLiteral.static_member()__getattr____getattr__methodsTest Plan
(run
cargo nextest run -p ty_python_semantic mdtest__import_module_getattr)__getattr__is called correctly and returns proper types