[red-knot] Add symbols for for loop variables
#13075
Conversation
AlexWaygood
force-pushed
the
alex/redknot-for-symbols
branch
2 times, most recently
from
060e167 to
2a7f191
Compare
CodSpeed Performance ReportMerging #13075 will improve performances by 9.06%Comparing Summary
Benchmarks breakdown
|
|
crates/red_knot_python_semantic/src/semantic_index/definition.rs
Outdated
Show resolved
Hide resolved
AlexWaygood
force-pushed
the
alex/redknot-for-symbols
branch
2 times, most recently
from
c81aa5c to
be08b16
Compare
AlexWaygood
force-pushed
the
alex/redknot-for-symbols
branch
from
be08b16 to
3fc3db2
Compare
| // TODO(Alex): only a valid iterable if the *type* of `iterable_ty` has an `__iter__` | ||
| // member (dunders are never looked up on an instance) | ||
| let _dunder_iter_ty = iterable_ty.member(self.db, &ast::name::Name::from("__iter__")); | ||
|
|
||
| // TODO(Alex): | ||
| // - infer the return type of the `__iter__` method, which gives us the iterator | ||
| // - lookup the `__next__` method on the iterator | ||
| // - infer the return type of the iterator's `__next__` method, | ||
| // which gives us the type of the variable being bound here | ||
| // (...or the type of the object being unpacked into multiple definitions, if it's something like | ||
| // `for k, v in d.items(): ...`) | ||
| let loop_var_value_ty = Type::Unknown; |
There was a problem hiding this comment.
I think this is probably not really the right place for this TODO? Since the inferring of the iterator type (and the unpacking of whatever is returned from the iterator's __next__ method) really needs to be done at a higher level? but I'm also not really sure where else to put this TODO?
There was a problem hiding this comment.
In the current architecture, this is the right place for the TODO; we will have to repeat this logic for each definition, in the case of unpacking. We don't currently have another layer available at which to cache it. Caching also has an overhead and sometimes it's better to just repeat some work. But we can also re-evaluate this when we implement unpacking.
There was a problem hiding this comment.
I'm not 100% sure we're on the same page here (but this may also just be me still getting used to this architecture) — anyway, I don't think it's massively important where this TODO lives, so I'll leave this for now!
|
Thanks @dhruvmanila and @carljm! Pushed some changes. Lmk if it's still not quite right. |
| // TODO(Alex): only a valid iterable if the *type* of `iterable_ty` has an `__iter__` | ||
| // member (dunders are never looked up on an instance) | ||
| let _dunder_iter_ty = iterable_ty.member(self.db, &ast::name::Name::from("__iter__")); | ||
|
|
||
| // TODO(Alex): | ||
| // - infer the return type of the `__iter__` method, which gives us the iterator | ||
| // - lookup the `__next__` method on the iterator | ||
| // - infer the return type of the iterator's `__next__` method, | ||
| // which gives us the type of the variable being bound here | ||
| // (...or the type of the object being unpacked into multiple definitions, if it's something like | ||
| // `for k, v in d.items(): ...`) | ||
| let loop_var_value_ty = Type::Unknown; |
There was a problem hiding this comment.
In the current architecture, this is the right place for the TODO; we will have to repeat this logic for each definition, in the case of unpacking. We don't currently have another layer available at which to cache it. Caching also has an overhead and sometimes it's better to just repeat some work. But we can also re-evaluate this when we implement unpacking.
Summary
This PR adds symbols introduced by
forloops to red-knot:xinfor x in range(10): passxandyinfor x, y in d.items(): passa,b,canddinfor [((a,), b), (c, d)] in foo: passThis is the first one of these PRs that I've done, so I hope I haven't missed anything!
Test Plan
Several tests added, and the assertion in the benchmarks has been updated.