-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[ty] Add diagnostics for invalid await
expressions
#19711
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
Conversation
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-08-14 21:31:01.838153846 +0000
+++ new-output.txt 2025-08-14 21:31:01.904154062 +0000
@@ -1,5 +1,5 @@
WARN ty is pre-release software and not ready for production use. Expect to encounter bugs, missing features, and fatal errors.
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/918d35d/src/function/execute.rs:215:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(1543f)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/918d35d/src/function/execute.rs:215:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(6d14)): execute: too many cycle iterations`
_directives_deprecated_library.py:15:31: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int`
_directives_deprecated_library.py:30:26: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `str`
_directives_deprecated_library.py:36:41: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `Self@__add__` |
|
Oops, I think |
Yes, exactly. All dynamic types ( |
Do I just... fn generator_return_type(self, db: &'db dyn Db) -> Option<Type<'db>> {
// -- snip --
match self {
Type::NominalInstance(instance) => {
instance.class.iter_mro(db).find_map(from_class_base)
}
Type::ProtocolInstance(instance) => {
if let Protocol::FromClass(class) = instance.inner {
class.iter_mro(db).find_map(from_class_base)
} else {
None
}
}
+ ty @ Type::Dynamic(_) => Some(ty),
_ => None,
}
} It seems to work, but is there more to it? |
That should be all there is to it 😄. Propagating the dynamic type (instead of returning |
Alright, now class InvalidAwaitReturnImplicit:
def __await__(self):
pass is awaitable as far as the type signature goes. Also, union types don't work. I suppose I have to check every element for being a generator, and construct a union of possible return types if all of them are, in fact, awaitable. |
👍
Ah, yes, you're right. Thank you for fixing these. I think you should be able to use |
Now distinguishes between class AwaitableUnion: # Generator | Unknown, awaitable
if datetime.today().weekday() == 6:
def __await__(self) -> typing.Generator[typing.Any, None, None]:
yield
else:
def __await__(self):
pass
class UnawaitableUnion: # Generator | int, non-awaitable
if datetime.today().weekday() == 6:
def __await__(self) -> typing.Generator[typing.Any, None, None]:
yield
else:
def __await__(self) -> int:
return 5
|
This can happen in unreachable code, for example: a = returns_awaitable()
if sys.version_info >= (3, 12):
# the type of `a` is `Never` here when checking on 3.11 or lower
x = await a
else:
... Now this is obviously a constructed example, but there are valid use cases. |
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.
This is looking pretty good, thank you! Can we add some mdtests (maybe in a new diagnostics/invalid_await.md
) demonstrating cases where this diagnostic is emitted, and snapshotting their output?
); | ||
match self { | ||
Self::Call(CallDunderError::CallError(..)) => { | ||
// TODO: Indicate definition error, not usage error. |
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.
I think it's reasonable to emit a diagnostic at the callsite here. It might also be nice to emit a diagnostic where __await__
is defined with an incorrect signature, but I would see this as in addition, not instead of.
match self { | ||
Self::Call(CallDunderError::CallError(..)) => { | ||
// TODO: Indicate definition error, not usage error. | ||
diag.info( | ||
"`__await__` expects additional arguments and cannot be called implicitly", | ||
); | ||
} | ||
Self::Call(CallDunderError::PossiblyUnbound(..)) => { | ||
diag.info("`__await__` is possibly unbound"); | ||
} | ||
Self::Call(CallDunderError::MethodNotAvailable) => diag.info("`__await__` is missing"), | ||
Self::InvalidReturnType(return_type) => diag.info(format_args!( | ||
"`__await__` returns `{return_type}`, which is not a valid iterator", | ||
return_type = return_type.display(db) | ||
)), | ||
} | ||
} |
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.
Is it possible that in these cases we can highlight the range of the __await__
method, or of the definition of the type that is lacking an __await__
method?
/// await InvalidAwait() # error: TODO: | ||
/// await 42 # error: TODO: |
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.
why are there TODOs here?
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.
Wanted to hear feedback before documenting actual error messages, I think.
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.
I think it's sufficient to include the error code, but not the full message, here. So e.g.
/// await InvalidAwait() # error: TODO: | |
/// await 42 # error: TODO: | |
/// await InvalidAwait() # error: [invalid-await] | |
/// await 42 # error: [invalid-await] |
Added some useful secondary annotations for the lint. Now Added mdtests as proposed. Some other tests that rely on ruff/crates/ty_python_semantic/resources/mdtest/diagnostics/semantic_syntax_errors.md Lines 136 to 138 in 3a542a8
Would expecting error: [invalid-await] here as well break atomicity?
Even though providing the type definition of a non-awaitable object is useful, sometimes diagnostics peek into built-ins:
Also, while fooling around, I discovered that awaiting an instance of this type is perfectly valid, and it isn't considered possibly unbound: class PossiblyUnbound:
if datetime.today().weekday() == 0:
def __await__(self):
yield 1
elif datetime.today().weekday() == 1:
def __await__(self):
yield 2 I wonder why. |
Thanks for this comment! I explored it and found a bug, fixed in #19884 |
## Summary A [passing comment](#19711 (comment)) led me to explore why we didn't report a class attribute as possibly unbound if it was a method and defined in two different conditional branches. I found that the reason was because of our handling of "conflicting declarations" in `place_from_declarations`. It returned a `Result` which would be `Err` in case of conflicting declarations. But we only actually care about conflicting declarations when we are actually doing type inference on that scope and might emit a diagnostic about it. And in all cases (including that one), we want to otherwise proceed with the union of the declared types, as if there was no conflict. In several cases we were failing to handle the union of declared types in the same way as a normal declared type if there was a declared-types conflict. The `Result` return type made this mistake really easy to make, as we'd match on e.g. `Ok(Place::Type(...))` and do one thing, then match on `Err(...)` and do another, even though really both of those cases should be handled the same. This PR refactors `place_from_declarations` to instead return a struct which always represents the declared type we should use in the same way, as well as carrying the conflicting declared types, if any. This struct has a method to allow us to explicitly ignore the declared-types conflict (which is what we want in most cases), as well as a method to get the declared type and the conflict information, in the case where we want to emit a diagnostic on the conflict. ## Test Plan Existing CI; added a test showing that we now understand a multiply-conditionally-defined method as possibly-unbound. This does trigger issues on a couple new fuzzer seeds, but the issues are just new instances of an already-known (and rarely occurring) problem which I already plan to address in a future PR, so I think it's OK to land as-is. I happened to build this initially on top of #19711, which adds invalid-await diagnostics, so I also updated some invalid-syntax tests to not await on an invalid type, since the purpose of those tests is to check the syntactic location of the `await`, not the validity of the awaited type.
Excellent! And thanks for fixing tests for me, I suppose I should merge them from |
All mdtests are now passing. |
I notice that mypy and pyright both implement these diagnostics by checking that anything you It's not clear to me that using the protocol is a better approach to get good diagnostics here, but it's something we could consider in future. For now I think this PR is good. |
@theammir Just for future reference (in case you make future PRs), you may want to note some things I'm fixing in this PR:
|
My entire reasoning behind the design I went with was trying to build from the blocks available, and that one todo comment in That said, pyright's "is not awaitable" diagnostics are thorough, but also deeply nested and somewhat convoluted: Pyright: "InvalidAwaitArgs" is not awaitable
"InvalidAwaitArgs" is incompatible with protocol "Awaitable[_T_co@Awaitable]"
"__await__" is an incompatible type
Type "(value: int) -> Generator[int, Any, None]" is not assignable to type "() -> Generator[Any, Any, _T_co@Awaitable]"
Extra parameter "value" [reportGeneralTypeIssues] All the info about specific extra parameters is there (if you are careful enough to find it on line 4, otherwise read even further), and there's more to the protocol error message than just reporting unassignability anyway.
Thanks! It's been really fun so far, I'd stick around. :) |
* main: [ty] Add diagnostics for invalid `await` expressions (#19711) [ty] Synthesize read-only properties for all declared members on `NamedTuple` classes (#19899) [ty] Remove use of `ClassBase::try_from_type` from `super()` machinery (#19902) [ty] Speedup project file discovery (#19913) [`pyflakes`] Add secondary annotation showing previous definition (`F811`) (#19900) Bump 0.12.9 (#19917) [ty] support `kw_only=True` for `dataclass()` and `field()` (#19677)
Summary
This PR adds a new lint,
invalid-await
, for all sorts of reasons why an object may not beawait
able, as discussed in astral-sh/ty#919.Precisely,
__await__
is guarded against being missing, possibly unbound, or improperly defined (expects additional arguments or doesn't return an iterator).Of course, diagnostics need to be fine-tuned. If
__await__
cannot be called with no extra arguments, it indicates an error (or a quirk?) in the method signature, not at the call site. Without any doubt, such an object is notAwaitable
, but I feel like talking about arguments for an implicit call is a bit leaky.I didn't reference any actual diagnostic messages in the lint definition, because I want to hear feedback first.
Also, there's no mention of the actual required method signature for
__await__
anywhere in the docs. The only reference I had is thetyping
stub. I basically ended up linking[Awaitable]
to "must implement__await__
", which is insufficient on its own.Test Plan
The following code was tested: