[ty] Infer types for key-based access on TypedDicts #19763
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-key |
37 | 0 | 0 |
invalid-argument-type |
13 | 0 | 4 |
unused-ignore-comment |
0 | 7 | 0 |
possibly-unbound-attribute |
2 | 2 | 2 |
non-subscriptable |
3 | 0 | 0 |
invalid-assignment |
2 | 0 | 0 |
possibly-unresolved-reference |
2 | 0 | 0 |
unresolved-attribute |
2 | 0 | 0 |
unsupported-operator |
1 | 0 | 0 |
| Total | 62 | 9 | 6 |
| memchr = { workspace = true } | ||
| strum = { workspace = true } | ||
| strum_macros = { workspace = true } | ||
| strsim = "0.11.1" |
There was a problem hiding this comment.
I know that we had a custom Levenshtein implementation in #18705, but that was removed again. And strsim is already a transitive dependency for the CLI version of ty at least — via clap.
Happy to replace that with something else though.
There was a problem hiding this comment.
I have no opinions here -- I'm in favor of whatever works and is implemented :)
There was a problem hiding this comment.
The advantage of going with the custom implementation in #18705 (which I think it would be fairly easy to bring back -- it's quite isolated as a module) is that Brent and I based it directly on the CPython implementation of this feature, and we brought in most of CPython's tests for this feature. CPython's implementation of this feature is very battle-tested at this point: it's been present in several stable releases of Python and initially received a large number of bug reports (which have since been fixed) regarding bad "Did you mean?" suggestions. So at this point I think we can be pretty confident that CPython's implementation is very well tuned for giving good suggestions for typos in Python code specifically.
Having said that, it's obviously nice for us to have to maintain less code, and exactly which Levenshtein implementation we go with probably isn't the most important issue for us right now :-)
| } | ||
|
|
||
| /// Suggest a name from `existing_names` that is similar to `wrong_name`. | ||
| pub(super) fn did_you_mean<S: AsRef<str>, T: AsRef<str>>( |
There was a problem hiding this comment.
I copied this function more or less 1:1 from another project of mine. It's not like I have a huge amount of experience with the heuristic here, but I did some iterations on it and it usually gives decent results.
This comment was marked as resolved.
This comment was marked as resolved.
sharkdp
force-pushed
the
david/typeddict-getitem
branch
from
ff702fb to
574bff2
Compare
sharkdp
requested review from
AlexWaygood,
MichaReiser,
carljm and
dcreager
as code owners
| reveal_type(alice["age"]) # revealed: Unknown | ||
|
|
||
| # TODO: this should reveal `Unknown`, and it should emit an error | ||
| reveal_type(alice["non_existing"]) # revealed: Unknown |
There was a problem hiding this comment.
So.. all of these still don't work, because the type of alice here is simply dict[Unknown, Unknown]. See below for the real tests for this feature.
| memchr = { workspace = true } | ||
| strum = { workspace = true } | ||
| strum_macros = { workspace = true } | ||
| strsim = "0.11.1" |
There was a problem hiding this comment.
I have no opinions here -- I'm in favor of whatever works and is implemented :)
crates/ty_python_semantic/resources/mdtest/narrow/assignment.md
Outdated
Show resolved
Hide resolved
| let overloads = fields.iter().map(|(name, field)| { | ||
| let key_type = Type::StringLiteral(StringLiteralType::new(db, name.as_str())); | ||
|
|
||
| Signature::new( | ||
| Parameters::new([ | ||
| Parameter::positional_only(Some(Name::new_static("self"))) | ||
| .with_annotated_type(instance_ty), | ||
| Parameter::positional_only(Some(Name::new_static("key"))) | ||
| .with_annotated_type(key_type), | ||
| ]), | ||
| Some(field.declared_ty), | ||
| ) | ||
| }); |
There was a problem hiding this comment.
I like the synthesized-overloads approach a lot. I seem to remember @erictraut mentioning before that neither pyright nor mypy implement TypedDict __getitem__ or __setitem__ by synthesizing overloads. I'm curious if you've looked at the more advanced TypedDict features we have yet to implement, and considered whether this approach will be able to handle those features also?
There was a problem hiding this comment.
I think that the other methods here (get, update, etc.) can be handled in a similar way to __getitem__.
For validating writes to attributes, we should be able to synthesize a similar overload set for __setitem__.
I do not yet have a good understanding of all TypedDict features to come (I merely read the docs/spec, and wrote a list of all tasks). So there might very well be things that can not be handled this way. But I currently don't see why that would be a reason not to solve those basic features with synthesized overloads.
My biggest worry is that this approach here does not scale well to large TypedDicts. Synthesizing N overloads is probably not that bad, but resolving calls to these overloads probably scales superlinear in N?
There was a problem hiding this comment.
My biggest worry is that this approach here does not scale well to large
TypedDicts. Synthesizing N overloads is probably not that bad, but resolving calls to these overloads probably scales superlinear in N?
I think we just need to see how this goes. We can always change the approach later if it proves a problem in practice.
For tuple.__getitem__, I attempted to mitigate this problem by synthesizing the minimum number of overloads. This is done by combining them where possible. E.g. for tuple[int, str, int], we "only" synthesize these overloads -- the overloads for the first element and the third element are combined:
@overload
def __getitem__(self, index: Literal[0, 2, -1, -3], /) -> int: ...
@overload
def __getitem__(self, index: Literal[1, -2], /) -> str: ...
@overload
def __getitem__(self, index: SupportsIndex, /) -> int | str: ...
@overload
def __getitem__(self, index: slice, /) -> tuple[int | str, ...]: ...You could do a similar thing for TypedDict __getitem__ methods: for Foo in the following example, it looks like we synthesize two overloads, but really only one is required (they can be combined, since the value type is the same for both keys:
from typing import TypedDict
class Foo(TypedDict):
x: str
y: strThere was a problem hiding this comment.
Yeah, that's a great optimization idea. And potentially more impactful than for tuples (as I would assume TypedDicts to have more items, on average). There's probably also a lot of TypedDicts out there where the majority of value types are just int or str.
What's not immediately clear to me is if this is an optimization at all? We'll spend more time when synthesizing the overloads, and we have to allocate an additional hashmap to group the items. And it's not obvious to me that this will even lead to faster overload resolution? It might be that creating / iterating over these union types is more expensive than a larger number of overloads with simple Literal["key"] argument types? I guess we'll need a micro-benchmark with a large TypedDict.
I wrote down two tasks in astral-sh/ty#154
|
Looked again at almost all ecosystem changes and they're basically all true positives (a few known limitations unrelated to TypedDicts)! |
| let overloads = fields.iter().map(|(name, field)| { | ||
| let key_type = Type::StringLiteral(StringLiteralType::new(db, name.as_str())); | ||
|
|
||
| Signature::new( | ||
| Parameters::new([ | ||
| Parameter::positional_only(Some(Name::new_static("self"))) | ||
| .with_annotated_type(instance_ty), | ||
| Parameter::positional_only(Some(Name::new_static("key"))) | ||
| .with_annotated_type(key_type), | ||
| ]), | ||
| Some(field.declared_ty), | ||
| ) | ||
| }); |
There was a problem hiding this comment.
My biggest worry is that this approach here does not scale well to large
TypedDicts. Synthesizing N overloads is probably not that bad, but resolving calls to these overloads probably scales superlinear in N?
I think we just need to see how this goes. We can always change the approach later if it proves a problem in practice.
For tuple.__getitem__, I attempted to mitigate this problem by synthesizing the minimum number of overloads. This is done by combining them where possible. E.g. for tuple[int, str, int], we "only" synthesize these overloads -- the overloads for the first element and the third element are combined:
@overload
def __getitem__(self, index: Literal[0, 2, -1, -3], /) -> int: ...
@overload
def __getitem__(self, index: Literal[1, -2], /) -> str: ...
@overload
def __getitem__(self, index: SupportsIndex, /) -> int | str: ...
@overload
def __getitem__(self, index: slice, /) -> tuple[int | str, ...]: ...You could do a similar thing for TypedDict __getitem__ methods: for Foo in the following example, it looks like we synthesize two overloads, but really only one is required (they can be combined, since the value type is the same for both keys:
from typing import TypedDict
class Foo(TypedDict):
x: str
y: str| memchr = { workspace = true } | ||
| strum = { workspace = true } | ||
| strum_macros = { workspace = true } | ||
| strsim = "0.11.1" |
There was a problem hiding this comment.
The advantage of going with the custom implementation in #18705 (which I think it would be fairly easy to bring back -- it's quite isolated as a module) is that Brent and I based it directly on the CPython implementation of this feature, and we brought in most of CPython's tests for this feature. CPython's implementation of this feature is very battle-tested at this point: it's been present in several stable releases of Python and initially received a large number of bug reports (which have since been fixed) regarding bad "Did you mean?" suggestions. So at this point I think we can be pretty confident that CPython's implementation is very well tuned for giving good suggestions for typos in Python code specifically.
Having said that, it's obviously nice for us to have to maintain less code, and exactly which Levenshtein implementation we go with probably isn't the most important issue for us right now :-)
Summary
This PR adds type inference for key-based access on
TypedDicts and a new diagnostic for invalid subscript accesses:And when you try to access a non-existing key:
part of astral-sh/ty#154
Test Plan
Updated Markdown tests