-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[ty] Treat Callable dunder members as bound method descriptors
#20860
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
[ty] Treat Callable dunder members as bound method descriptors
#20860
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
missing-argument |
0 | 2 | 0 |
unresolved-attribute |
0 | 2 | 0 |
| Total | 0 | 4 | 0 |
1fa0788 to
e20bb2c
Compare
| return "" | ||
|
|
||
| class A: | ||
| __call__: Callable[[int], str] = call_impl |
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.
The previous example here would have failed at runtime.
e20bb2c to
70378c9
Compare
| Type::Intersection(intersection) => intersection | ||
| .map_positive(db, |element| into_function_like_callable(db, *element)), |
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.
of course, if we just represented function-like callables as FunctionType & <some Callable type>, all we'd need to do here would be to insert FunctionType into the intersection; no need for a new Intersection::map_positive method!
...anyway, sorry for beating this drum, I'll try to shut up about beautiful refactors we could do until the beta's out 🙈
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.
If things wouldn't always turn out to be more complicated than I initially think, I would have already started that project. But I'm almost certain this would reveal at least three issues with our intersection handling, lead to unexpected performance regressions, and probably uncover a new salsa concurrency bug. So it has to wait a little longer 😄.
…able * dcreager/non-inferable-api: just the api parts [ty] Fix further issues in `super()` inference logic (#20843) [ty] Document when a rule was added (#20859) [ty] Treat `Callable` dunder members as bound method descriptors (#20860) [ty] Handle decorators which return unions of `Callable`s (#20858) Fix false negatives in `Truthiness::from_expr` for lambdas, generators, and f-strings (#20704) [ty] Rename Type unwrapping methods (#20857) Update `lint.flake8-type-checking.quoted-annotations` docs (#20765)
Summary
Dunder methods (at least the ones defined in the standard library) always take an instance of the class as the first parameter. So it seems reasonable to generally treat them as bound method descriptors if they are defined via a
Callabletype.This removes just a few false positives from the ecosystem, but solves three user-reported issues:
closes astral-sh/ty#908
closes astral-sh/ty#1143
closes astral-sh/ty#1209
In addition to the change here, I also considered making
ClassVars bound method descriptors. However, there was zero ecosystem impact. So I think we can also close astral-sh/ty#491 with this PR.closes astral-sh/ty#491
Test Plan
Added regression test