[ty] Detect generic Callables in function signatures#22954
Merged
Conversation
dcreager
requested review from
AlexWaygood,
carljm and
sharkdp
as code owners
dcreager
commented
Jan 29, 2026
Comment on lines
+7001
to
+7003
|
|
||
| /// Updates any `Callable` types in a function signature return type to be generic if possible. | ||
| RescopeReturnCallables(&'a FxHashMap<CallableType<'db>, CallableType<'db>>), |
Member
Author
There was a problem hiding this comment.
Breaking up TypeMapping is a new windmill I want to tilt at...
Typing conformance resultsThe percentage of diagnostics emitted that were expected errors decreased from 82.37% to 82.22%. The percentage of expected errors that received a diagnostic increased from 72.62% to 72.71%. Summary
True positives addedDetails
False positives addedDetails
|
dcreager
force-pushed
the
dcreager/callable-genctx
branch
from
2544dc0 to
b7b8516
Compare
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
missing-argument |
831 | 0 | 0 |
invalid-argument-type |
461 | 4 | 7 |
unresolved-attribute |
332 | 0 | 72 |
unused-type-ignore-comment |
10 | 241 | 0 |
possibly-missing-attribute |
173 | 6 | 24 |
unsupported-operator |
198 | 0 | 5 |
no-matching-overload |
123 | 1 | 0 |
invalid-return-type |
57 | 6 | 8 |
not-subscriptable |
61 | 0 | 0 |
unknown-argument |
38 | 0 | 0 |
invalid-assignment |
23 | 2 | 7 |
type-assertion-failure |
0 | 9 | 12 |
invalid-method-override |
15 | 0 | 2 |
call-non-callable |
10 | 5 | 0 |
too-many-positional-arguments |
13 | 0 | 0 |
invalid-context-manager |
11 | 0 | 0 |
not-iterable |
8 | 0 | 0 |
empty-body |
0 | 0 | 6 |
redundant-cast |
4 | 2 | 0 |
invalid-await |
4 | 0 | 0 |
invalid-type-arguments |
1 | 0 | 0 |
invalid-type-form |
0 | 1 | 0 |
parameter-already-assigned |
1 | 0 | 0 |
| Total | 2,374 | 277 | 143 |
Contributor
|
Looks like we need to bump the expected diagnostics on static-frame benchmark. |
Contributor
|
The conformance suite false positives all look like |
Contributor
|
The PyGithub diagnostic at https://github.com/PyGithub/PyGithub/blob/f2540db50423aa124beaeb8c7bfba7098a549c82/github/GithubObject.py#L663 looks like it might deserve investigation? Seems like a false positive. Other new diagnostics I'm seeing in the first few projects look like true positives, or at least unrelated issues that are exposed because we now understand e.g. |
Member
Author
This was a TODO in the new mdtests as well. The cause was that in our |
Member
Author
This was a new false positive in the typing conformance suite, too, which has also been fixed with this latest change. The remaining two new false positives are |
Contributor
|
We now see even more diagnostics on static-frame, so that needs bumping again so we can get benchmark results here. (Usually we bump it to a nearby whole number, not the exact number -- the goal is just to catch any massive unexpected increase. So let's bump it to maybe 1800.) It would be great if we could avoid the conformance suite false positives with Concatenate; we have Todo handling for Concatenate that generally aims to avoid false positives. It's not actually totally clear to me why this PR is causing those false positives. |
carljm
approved these changes
Feb 3, 2026
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This is excellent! Clear code, thorough tests. Thank you!
|
|
||
| # revealed: [T](T, /) -> T | ||
| reveal_type(decorator_factory()) | ||
| # revealed: ty_extensions.GenericContext[T@decorator_factory] |
Contributor
There was a problem hiding this comment.
Is it a problem that we still call this T@decorator_factory? It seems to suggest a binding of the typevar that is not correct. (Though I'm not sure what I'd expect to see displayed there -- it's bound to an anonymous Callable type.)
Member
There was a problem hiding this comment.
T@decorator_factory'return? T@decorator_factory:return?T@<return of decorator_factory>?
Member
Author
There was a problem hiding this comment.
T'return@decorator_factory is probably the easiest modification I could make. The @whatever part comes from the BindingContext, which currently can only come from an actual Definition, which we don't have for the Callable in the return type. But I could synthesize a new TypeVar with the 'return suffix in its name...let me give that a shot
Comment on lines
+2675
to
+2680
| let enclosing_function = | ||
| nearest_enclosing_function(self.db(), self.index, self.scope()) | ||
| .expect("should be in a function body scope"); | ||
| let declared_ty = enclosing_function | ||
| .last_definition_raw_signature(self.db()) | ||
| .return_ty; |
Contributor
There was a problem hiding this comment.
Should we actually infer and store the "adjusted" type as the type of the return-annotation node? That would imply a) that we don't need to do this extra dance here, and b) that I think we'd get more accurate LSP hover types for the return annotation, in this generic-callable special case.
Comment on lines
1834
to
1838
| // If the constraint set is cyclic, we'll hit an infinite expansion when trying to add type | ||
| // mappings for it. | ||
| if constraints.is_cyclic(self.db) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
I assume this was added because you ran into it somewhere -- should we capture that in a test? Or is it already in an existing test?
Member
Author
There was a problem hiding this comment.
The paramspec tests exercise this (and stack overflow without it)
Comment on lines
+654
to
+656
| fn visit_type(&self, db: &'db dyn Db, ty: Type<'db>) { | ||
| walk_type_with_recursion_guard(db, ty, self, &self.recursion_guard); | ||
| } |
Contributor
There was a problem hiding this comment.
A lot of TypeVisitor implementations have this boilerplate -- seems like it should be the default implementation of visit_type maybe?
Member
Author
There was a problem hiding this comment.
I think the issue with that is that recursion_guard can only come from the concrete struct that implements Visitor. A default implementation would not know whether the implementing type has such a field.
Contributor
There was a problem hiding this comment.
Yeah there'd have to be a trait method to return an (optional?) recursion guard, and the default visit would call that. But this can also be a separate change, it's not that important.
Member
There was a problem hiding this comment.
I'm not totally sure that that would really be less boilerplate overall, so I agree looking at that separately seems best
Contributor
There was a problem hiding this comment.
I think it would be strictly less, but not by much. For the common case (which is now up to 4 or 5 visitors) instead of implementing visit you'd implement the method to return your recursion guard. Same number of methods, but a smaller method body. Mainly I see the win as not repeating everywhere "yes, for every type I want to... visit the type". But yeah happy to look at it separately.
Member
There was a problem hiding this comment.
Right, but that's still at least one method that you have to implement every time for the common case, and for the uncommon case you still have to implement visit_type, e.g.
Personally, while I agree the amount of boilerplate is annoying, I like the way the current design gives us symmetry between the common case and the uncommon case — the same method is required in both cases
Member
|
(Looks like there's another benchmark assertion failing, this time on the |
AlexWaygood
reviewed
Feb 4, 2026
CodSpeed Performance ReportMerging this PR will degrade performance by 5.25%Comparing Summary
Performance Changes
|
carljm
pushed a commit
that referenced
this pull request
Feb 10, 2026
…ParamSpec+Concatenate Bisected the issue to commit 4f684db ([ty] Detect generic `Callable`s in function signatures #22954). The heuristic for detecting generic Callables in return position incorrectly interacts with ParamSpec + Concatenate patterns used in decorators like hypothesis.composite. https://claude.ai/code/session_01MiAwMHE7cSZFmdRugzFrE2
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.
When we coerce a generic function or class constructor into a
Callable, the callable type remembers that it is generic:However, there is no easy way to spell a generic
Callabletype in Python. The closest you can get is by defining a generic type alias:To get around this, there is a common heuristic that if a generic function binds a typevar, but that typevar is only mentioned in a
Callablein return position, then it's actually the callable that binds the typevar, not the function. (And if that's the only typevar mentioned in the function signature, the function ends up not being generic at all.) This comes up very often in decorator factories, where the factory function returns a decorator that is generic over one or more typevars:Here,
decorator_factoryshould not be considered generic; itsCallablereturn type is.Note that this is true for PEP-695 typevars, too!
(This is one example where PEP-695 syntax is misleading! We can usually assume that a
[T]binding context makes the item that it's attached to generic — and that we can determine this statically. But in this case we can't! We have to analyze the function signature to see thatdecorator_factoryis not generic, even though it has a PEP-695 binding context.)Closes astral-sh/ty#1136