[ty] Support calls to intersection types#22469
Conversation
Typing conformance resultsNo changes detected ✅ |
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
call-top-callable |
94 | 0 | 0 |
unresolved-attribute |
30 | 0 | 16 |
invalid-argument-type |
25 | 0 | 14 |
unused-type-ignore-comment |
0 | 25 | 0 |
call-non-callable |
16 | 1 | 0 |
invalid-return-type |
3 | 0 | 14 |
unknown-argument |
15 | 0 | 0 |
missing-argument |
12 | 0 | 0 |
invalid-assignment |
6 | 0 | 1 |
index-out-of-bounds |
5 | 0 | 0 |
too-many-positional-arguments |
5 | 0 | 0 |
unsupported-operator |
1 | 0 | 3 |
no-matching-overload |
3 | 0 | 0 |
| Total | 215 | 26 | 48 |
Implement proper handling for calling intersection types. Previously, calling an intersection type would return a `@Todo` type that suppressed errors but provided no useful type information. Now, when calling an intersection type: - We try to call each positive element with the given arguments - Elements where the call fails (wrong arguments, not callable, etc.) are discarded - If at least one element succeeds, the call is valid - The return type is the intersection of return types from successful elements - If all elements fail, an appropriate error is reported This approach means that if an intersection contains both a callable with a known signature and a `Top[Callable[..., object]]` (from narrowing by `callable()`), the call will succeed using the element with the known signature, avoiding spurious `call-top-callable` errors. Fixes astral-sh/ty#1858
3dae02a to
f39370a
Compare
When calling an intersection type where all callable elements reject the specific call arguments, we now show an error for each failing element (similar to how unions work) instead of a single generic "not callable" error. This is achieved by: - Not filtering out failed elements when ALL elements fail - Returning BindingError instead of NotCallable to trigger individual diagnostics - Adding IntersectionDiagnostic struct and CompoundDiagnostic trait - Modifying report_diagnostics to iterate over intersection elements
More diagnostics are now emitted due to showing individual errors when all intersection elements fail a call.
When all intersection elements fail a call, we now use a priority hierarchy to determine which errors to show: 1. NotCallable (lowest) - object has no __call__ method 2. TopCallable - object is a top callable with unknown signature 3. BindingError (highest) - specific errors like invalid-argument-type Only errors from the highest priority category are shown. This prevents noise from less-specific errors when more informative errors are available. For example, if an intersection has one element that fails with invalid-argument-type and another that's not callable, we only show the invalid-argument-type error since it's more specific and actionable.
Refactors the `Bindings` structure to use a two-level representation: - Outer level: union elements (each can be a single callable or an intersection) - Inner level: bindings within an intersection element This enables proper handling of types like `(A & B) | C` when calling, where `A & B` is an intersection that was narrowed by `callable()`. Key changes: - Add `BindingsElement` struct to represent a single union element (which may contain multiple bindings for intersections) - Update `from_union` to preserve intersection structure instead of flattening - Update `from_intersection` to create a single element with multiple bindings - Update `return_type`, `check_types_impl`, and `report_diagnostics` to handle the two-level structure - Add test for union-of-intersections case The priority hierarchy for intersection call errors is preserved: when all bindings in an intersection fail, only the highest-priority error type is reported (BindingError > TopCallable > NotCallable).
Resolves conflicts between the two-level Bindings structure and main's new Type match cases for property descriptors, dataclass transformers, and other features.
06c217d to
f72e874
Compare
This PR fixes false `invalid-await` errors for patterns like:
```python
if inspect.isawaitable(x):
await x # x has type Unknown & Awaitable[object]
```
Changes:
1. Add intersection handling to `generator_return_type()` so that await
operations on intersection types work correctly. When awaiting an
intersection, we iterate over positive elements, collect return types
from awaitable elements, and intersect them together. Non-awaitable
elements are skipped; only if all elements fail to be awaitable do we
report an error.
2. Add intersection handling to `try_call_dunder_with_policy()` to call
dunder methods on each intersection element separately and combine the
results. This avoids intersecting bound methods (which often collapses
to Never) and instead intersects the return types.
Added tests for:
- Awaiting `Unknown & Awaitable[object]` returns `Unknown`
- Awaiting intersection of two Coroutine types
- Awaiting intersection with non-awaitable elements skips those elements
- Awaiting intersection where all elements are non-awaitable produces error
f72e874 to
01b1937
Compare
When multiple bindings in an intersection (or multiple elements in a union) return argument forms from type checking (e.g., during argument type expansion), we should merge them to properly detect conflicting forms, rather than overwriting with the last one.
Use error priority to select the appropriate error kind when all bindings fail, rather than handling intersection and single-binding cases separately. This addresses review comments 2 and 3 from PR #22469.
Update the doc comment to clarify that Bindings represents a union of callables (possibly size one), where each element is an intersection (possibly size one). This addresses review comment 5 from PR #22469.
…allable() - Add is_callable() method to BindingsElement for clearer element-level checks - Update report_diagnostics to iterate over elements instead of flattened bindings - Add documentation to iter() and iter_mut() clarifying they flatten the structure This addresses review comments 6 and 7 from PR #22469.
Simplify the call site by moving the is_intersection() and as_result().is_ok() checks inside retain_successful(). This addresses review comment 8 from PR #22469.
When an intersection fails inside a union, report errors with both union and intersection context: - Add LayeredDiagnostic that combines both contexts - Show the correct intersection type (not the full union) - Include test with snapshot diagnostics demonstrating the layered output This addresses review comment 9 from PR #22469.
AlexWaygood
left a comment
There was a problem hiding this comment.
Awesome!
This is a test-only review so far. There are a few tests that don't test what they say they're testing.
crates/ty_python_semantic/resources/mdtest/dataclasses/dataclass_transform.md
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/intersection_types.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/intersection_types.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/intersection_types.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/intersection_types.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/intersection_types.md
Outdated
Show resolved
Hide resolved
AlexWaygood
left a comment
There was a problem hiding this comment.
(still not a complete review -- will pick it back up tomorrow!)
carljm
left a comment
There was a problem hiding this comment.
Thanks for the review!
| # Diagnostics | ||
|
|
||
| ``` | ||
| error[invalid-argument-type]: Argument to bound method `__call__` is incorrect |
There was a problem hiding this comment.
This looks verbose in this contrived example, but I think in real code it's pretty unlikely to call a union containing an intersection where all elements of the union and the intersection all fail the call, and all elements of the intersection have the same "priority" error. And if that did happen, you might well want to know the details on each failed element.
crates/ty_python_semantic/resources/mdtest/dataclasses/dataclass_transform.md
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/intersection_types.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/intersection_types.md
Outdated
Show resolved
Hide resolved
AlexWaygood
left a comment
There was a problem hiding this comment.
Nice!
I feel like @dhruvmanila and @dcreager probably have more expertise than me on our call-binding machinery, so it would be great to have one of them take a look at this -- but I don't think that needs to block landing this.
Great work!
| Binding::single(self, Signature::todo("Type::Intersection.call")).into() | ||
| Type::Intersection(intersection) => { | ||
| // For intersections, we try to call each positive element. | ||
| // Elements where the call fails are discarded. |
There was a problem hiding this comment.
This second sentence doesn't seem strictly true? All elements fail in this call, but we do not "discard" the elements on your branch -- we emit two diagnostics, but we correctly infer call as evaluating to A & B:
from ty_extensions import Intersection
class A: ...
class B: ...
class C:
def __call__(self, arg: str) -> A:
return A()
class D:
def __call__(self, arg: str) -> B:
return B()
def f(cd: Intersection[C, D]):
reveal_type(cd(42))Would it be more accurate to say that "non-callable elements are discarded"?
There was a problem hiding this comment.
If some elements succeed and others fail, the return type is based only on the successful calls. This is tested in the "Three or more elements with partial success" mdtest:
from ty_extensions import Intersection
class RetA: ...
class RetB: ...
class A:
def __call__(self) -> RetA:
return RetA()
class B:
def __call__(self) -> RetB:
return RetB()
class C:
def __call__(self, x: int) -> int:
return 1
def _(x: Intersection[A, B, C]) -> None:
# A() succeeds, B() succeeds, C() fails (needs int arg) -> discarded
reveal_type(x()) # revealed: RetA & RetBBut if all elements fail (as in the test you showed), we don't want to give the return type as Never, so we give as our best guess the intersection of all of their return types.
I just deleted this comment here, as it's an overly-simplified description of logic that doesn't even live here.
There was a problem hiding this comment.
Oh, there is one more wrinkle around top-callables. In their case, the call "fails" because the arguments might be wrong -- but we know by the nature of a top-callable that somewhere in that infinite union, there is a callable subtype which does accept the current call arguments. So we still intersect the return type of the top-callable into the return type, even though calling the top-callable "failed". This is important to not lose narrowing information in some cases.
| /// Trait for adding context about compound types (unions/intersections) to diagnostics. | ||
| trait CompoundDiagnostic { | ||
| /// Adds context about any relevant compound type function types to the given diagnostic. | ||
| fn add_context(&self, db: &dyn Db, diag: &mut Diagnostic); | ||
| } |
There was a problem hiding this comment.
I have somewhat mixed feelings about the way we try to attach all contextual subdiagnostics inside our call-binding infrastructure. I feel like I'd prefer it if we instead propagated that information out of the call-binding infrastucture for callers to do with what they wish. It would make issues like astral-sh/ty#2482 easier to fix -- it's very hard to attach a specific subdiagnostic right now at only one callsite if the diagnostic came from our generalised call-binding machinery. And this PR feels like it's doubling down on the current way of doing things even more.
Still, this shouldn't block this important PR from being merged. It's fine to ignore this grumble for now :-)
There was a problem hiding this comment.
That's reasonable! I feel pretty confident this isn't adding a lot more difficulty to that potential refactor over what we have today, though.
* main: (43 commits) [`ruff`] Suppress diagnostic for strings with backslashes in interpolations before Python 3.12 (`RUF027`) (#21069) [flake8-bugbear] Fix B023 false positive for immediately-invoked lambdas (#23294) [ty] Add `Final` mdtests for loops and redeclaration (#23331) [`flake8-pyi`] Also check string annotations (`PYI041`) (#19023) Remove AlexWaygood as a flake8-pyi codeowner (#23347) [ty] Add comments to clarify the purpose of `NominalInstanceType::class_name` and `NominalInstanceType::class_module_name` (#23339) Add attestations for release artifacts and Docker images (#23111) [ty] Fix `assert_type` diagnostic messages (#23342) [ty] Force-update all insta snapshots (#23343) Add Q004 to the list of conflicting rules (#23340) [ty] Fix `invalid-match-pattern` false positives (#23338) [ty] new diagnostic called-match-pattern-must-be-a-type (#22939) [ty] Update flaky projects (#23337) [ty] Increase timeout for ecosystem report to 40 min (#23336) Bump ecosystem-analyzer pin (#23335) [ty] Replace `strsim` with CPython-based Levenshtein implementation (#23291) [ty] Add mdtest for staticmethod assigned in class body (#23330) [ty] fix inferring type variable from string literal argument (#23326) [ty] bytes literal is a sequence of integers (#23329) Update rand and getrandom (#23333) ...
|
Thanks for the review! Given the pain of resolving merge conflicts here, I'm going to go ahead and merge with your review, but very open to post-land comments from @dhruvmanila or @dcreager (or anyone). |
|
(With apologies to everyone else I'm certainly causing merge conflicts for, cc @charliermarsh) |
|
I love merge conflicts if it means progress. |
| if is_async_callable(fn): | ||
| reveal_type(fn) # revealed: ((int, /) -> int | Awaitable[int]) & Top[(...) -> Awaitable[object]] | ||
| result = fn(1) | ||
| # This includes `int & Awaitable[object]`: an `int` subtype could define `__await__`. | ||
| reveal_type(result) # revealed: (int & Awaitable[object]) | Awaitable[int] |
There was a problem hiding this comment.
Is this reveal type based on the fact that (A | B) & C gets distributed as (A & C) | (B & C) where Awaitable[int] & Awaitable[object] is simplified to Awaitable[int]?
There was a problem hiding this comment.
Yes, that's right. This is another one of those cases where we'll handle narrowing more pedantically than other type checkers. People will expect the await below to yield int, but we'll yield object because we account for this (int & Awaitable[object]) -- the possibility of an unknown subclass of int which adds awaitability.
|
Overall, LGTM! (Oops, this comment didn't go through.) |
Fixes astral-sh/ty#1858
Handle calls to intersection types. Previously, calling an intersection type would return a
@Todotype that suppressed errors but provided no useful type information.Now, when calling an intersection type:
Ecosystem review only revealed existing known issues or correct behaviors newly exposed by actually processing intersection calls and getting precise return types instead of Todo types