[ty] Fix classmethod + contextmanager + Self#22407
Merged
dcreager merged 6 commits intoastral-sh:mainfrom Jan 13, 2026
Merged
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
eclbg
commented
Jan 5, 2026
|
|
||
| // For classmethod-like callables, bind to the owner class (or instance's meta type). | ||
| // For function-like callables, bind to the instance. | ||
| let self_type = if callable.is_classmethod_like(db) { |
Contributor
Author
There was a problem hiding this comment.
I feel this is somewhat redundant with the change in bind_self in signatures.rs. In both places I'm detecting whether the method is a classmethod in different ways. Open to alternatives, of course.
eclbg
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
eclbg
changed the title
dcreager
reviewed
Jan 8, 2026
| if instance.is_none(db) { | ||
| owner | ||
| } else { | ||
| instance.to_meta_type(db) |
Member
There was a problem hiding this comment.
If you change this to instance, I think you can remove all of the changes in signatures.rs. In other words, bind_self should be given the type that Self should specialize to, not the type of the first parameter.
Doing that, both else branches will then be the same, and so you should be able to simplify this overall to
let self_type = if callable.is_classmethod_like(db) && instance.is_none(db) {
owner
} else {
instance
};
Contributor
Author
There was a problem hiding this comment.
I've been playing around with this and couldn't get the new test to pass without the changes in signatures.rs, even with the change to types.rs suggested in your comment.
Without those, the return type is inferred as <class 'Base'> or <class 'Child'>. This makes sense to me because owner is a SubclassOf(SubclassOfType{subclass_of: Class ..., and I can see why we would need to turn that into the instance type for classmethod-like callables.
Did I understand your suggestion correctly?
I've pushed the change to types.rs and left the original code in signatures.rs to illustrate this. The tests are green like this, and this is how the revealed types mismatch if I remove the changes in signatures.rs:
crates/ty_python_semantic/resources/mdtest/call/methods.md:486 unmatched assertion: revealed: _GeneratorContextManager[Base, None, None]
crates/ty_python_semantic/resources/mdtest/call/methods.md:486 unexpected error: 13 [revealed-type] "Revealed type: `_GeneratorContextManager[<class 'Base'>, None, None]`"
crates/ty_python_semantic/resources/mdtest/call/methods.md:488 unmatched assertion: revealed: Base
crates/ty_python_semantic/resources/mdtest/call/methods.md:488 unexpected error: 17 [revealed-type] "Revealed type: `<class 'Base'>`"
crates/ty_python_semantic/resources/mdtest/call/methods.md:494 unmatched assertion: revealed: _GeneratorContextManager[Child, None, None]
crates/ty_python_semantic/resources/mdtest/call/methods.md:494 unexpected error: 13 [revealed-type] "Revealed type: `_GeneratorContextManager[<class 'Child'>, None, None]`"
crates/ty_python_semantic/resources/mdtest/call/methods.md:496 unmatched assertion: revealed: Child
crates/ty_python_semantic/resources/mdtest/call/methods.md:496 unexpected error: 17 [revealed-type] "Revealed type: `<class 'Child'>`"
Member
There was a problem hiding this comment.
Good catch! So the bind_self needs to take in the type that Self specializes to, but the owner parameter when invoking the descriptor protocol is the class type itself (so an instance of type[Self]). That means we do need the to_instance call to get from the type of owner to the type of Self. I've moved the call from signatures.rs to try_call_dunder_get.
Contributor
Author
There was a problem hiding this comment.
Thanks! Much better :)
|
|
||
| // For classmethod-like callables, bind to the owner class (or instance's meta type). | ||
| // For function-like callables, bind to the instance. | ||
| let self_type = if callable.is_classmethod_like(db) { |
| yield cls() | ||
|
|
||
| class Child(Base): ... | ||
|
|
Member
There was a problem hiding this comment.
Can you also add a couple of additional reveal_types? Doing that in the playground is really what helped me figure out what's happening here:
reveal_type(Base.create)
reveal_type(Base().create)
reveal_type(Child.create)
reveal_type(Child().create)(In particular, doing the member access on both the class and on an instance of the class showed how we were only getting the wrong behavior before this PR for the class access case. The helped zero in on the None descriptor protocol argument being the root cause.)
Contributor
Author
There was a problem hiding this comment.
Thanks a lot for such a detailed review. I'll make the changes tomorrow.
Contributor
Author
There was a problem hiding this comment.
I've added a few more reveal_types
eclbg
force-pushed
the
classmethod-contextmanager-self-fix
branch
from
fbb8594 to
25cd2a3
Compare
dcreager
approved these changes
Jan 13, 2026
Contributor
Author
|
Thanks for the review and for getting this merged! |
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
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.
Summary
The test I've added illustrates the fix. Copying it here too:
Full disclosure: I've used LLMs for this PR, but the result is thoroughly reviewed by me before submitting. I'm excited about my first Rust contribution to Astral tools and will address feedback quickly.
Related to astral-sh/ty#2030, I am working on a fix for the TypeVar case also reported in that issue (by me)
Test Plan
Updated mdtests