[ty] Make signature return and parameter types non-optional#22425
[ty] Make signature return and parameter types non-optional#22425
Conversation
22f5aa2 to
5a35fd2
Compare
5a35fd2 to
6aad0a3
Compare
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
6aad0a3 to
e7d8ad0
Compare
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
30 | 0 | 5 |
unresolved-attribute |
4 | 0 | 22 |
invalid-assignment |
1 | 0 | 15 |
call-non-callable |
12 | 0 | 0 |
invalid-return-type |
2 | 0 | 10 |
no-matching-overload |
9 | 0 | 0 |
possibly-missing-attribute |
4 | 0 | 1 |
invalid-method-override |
3 | 0 | 0 |
missing-argument |
3 | 0 | 0 |
invalid-await |
2 | 0 | 0 |
not-subscriptable |
2 | 0 | 0 |
unsupported-operator |
2 | 0 | 0 |
unused-ignore-comment |
1 | 1 | 0 |
not-iterable |
1 | 0 | 0 |
redundant-cast |
0 | 1 | 0 |
too-many-positional-arguments |
1 | 0 | 0 |
unknown-argument |
1 | 0 | 0 |
| Total | 78 | 2 | 53 |
e7d8ad0 to
b78fe87
Compare
|
The new diagnostics on aioredis and asynq are true positives. Aioredis is because we now have a better idea of the return type of an un-annotated |
|
Ok, the root cause of the asynq changes is that this PR fixed ParamSpec inference in cases where the callable we are inferring the ParamSpec from had an un-annotated return type. Adding a test for this. The root cause of the home-assistant core changes appears to be the same, but in this case it tickles what looks like a pre-existing bug (most likely astral-sh/ty#2131 ) which causes a wrong specialization of a typevar to |
|
The new tornado diagnostic is a true positive, it also come from understanding that an |
|
The new scipy diagnostics are interesting. They are true positives (barring some improvement we might want to make in astral-sh/ty#1248 for untyped heterogenous dicts); the question is why they show up newly in this PR. It seems to have something to do with having lambdas (which are inferred as callables with no return type) in the value type of the dictionary; this previously caused us to short-circuit typevar solving somehow and just return The other ecosystem hits all look like variations on the things already mentioned. So I think this PR might slightly raise priority on astral-sh/ty#2131, but I'm not seeing anything blocking in the ecosystem report. |
Gankra
left a comment
There was a problem hiding this comment.
Seems like a great simplification. I don't think I ever even saw the Coroutine change, I assume it fell out of not early-returning on a None somewhere?
crates/ty_python_semantic/src/types/property_tests/type_generation.rs
Outdated
Show resolved
Hide resolved
2c2ff5b to
264090c
Compare
Summary
Fixes astral-sh/ty#2363
Fixes astral-sh/ty#2013
And several other bugs with the same root cause. And makes any similar bugs impossible by construction.
Previously we distinguished "no annotation" (Rust
None) from "explicitly annotated with something of typeUnknown" (which is not an error, and results in the annotation being of Rust typeSome(Type::DynamicType(Unknown))), even though semantically these should be treated the same.This was a bit of a bug magnet, because it was easy to forget to make this
None->Unknowntranslation everywhere we needed to. And in fact we did fail to do it in the case of materializing a callable, leading to a top-materialized callable still having (rust)Nonereturn type, which should have instead materialized toobject.This also fixes several other bugs related to not handling un-annotated return types correctly:
async defto beUnknown, where it should beCoroutineType[Any, Any, Unknown].Unknownfromsome_dict.get("key", None)if the value type ofsome_dictincluded a callable type with un-annotated return type.We now make signature return types and annotated parameter types required, and we eagerly insert
Unknownif there's no annotation. Most of the diff is just a bunch of mechanical code changes where we construct these types, and simplifications where we use them.One exception is type display: when a callable type has un-annotated parameters, we want to display them as un-annotated, but if it has a parameter explicitly annotated with something of
Unknowntype, we want to display that parameter asx: Unknown(it would be confusing if it looked like your annotation just disappeared entirely).Fortunately, we already have a mechanism in place for handling this: the
inferred_annotationflag, which suppresses display of an annotation. Previously we used it only forselfandclsparameters with an inferred annotated type -- but we now also set it for any un-annotated parameter, for which we inferUnknowntype.We also need to normalize
inferred_annotation, since it's display-only and shouldn't impact type equivalence. (This is technically a previously-existing bug, it just never came up when it only affected self types -- now it comes up because we have tests asserting thatdef f(x)anddef g(x: Unknown)are equivalent.)Test Plan
Added mdtests.