[red-knot] Infer Unknown for the loop var in async for loops
#13243
Conversation
|
AlexWaygood
force-pushed
the
alex/redknot-async-for
branch
from
4b482f5 to
90eee6e
Compare
| .iterate(self.db) | ||
| .unwrap_with_diagnostic(iterable.into(), self); | ||
| let loop_var_value_ty = if is_async { | ||
| // TODO(Alex): async iterables/iterators! |
There was a problem hiding this comment.
I haven't brought this up in reviews before, but I would prefer if we don't tag our TODOs with a name. The TODOs belong to the project, and anyone might address them in future. I realize it's intended as a way to take responsibility for the issue, but I think in practice it functions more like cookie-licking (https://communitymgt.fandom.com/wiki/Cookie_Licking), discouraging anyone else from contributing an improvement, while the person who created the TODO may not have time for it yet either.
cc @dhruvmanila
There was a problem hiding this comment.
Heh. Dhruv just this morning told me via DM that we were encouraged to do this at Astral :) he linked this by way of explanation
I don't have a strong preference either way!
There was a problem hiding this comment.
Interesting. I see that rationale, and I am more OK with it if the interpretation is more "this person might have relevant context" and not "you shouldn't work on this unless you check with this person first." But I'm still concerned that the non-cookie-licking interpretation isn't necessarily clear to a new contributor reading the code.
Overall I still think I don't like it. If there's relevant context that's needed to understand the TODO, I'd rather that be in the comment with the TODO, rather than a person's name to go talk to. The case where you really need to go talk to a specific person to understand some code shouldn't be that common, if we are writing reasonably understandable and well-commented code, and when that case arises, git blame exists. (That case can just as well arise for changes in an area of the code that doesn't have a TODO comment, so should we just comment every piece of code we write with our name, because git blame is too hard to use?)
There was a problem hiding this comment.
Opened a convo in Discord about this.
There was a problem hiding this comment.
@charliermarsh may have more context. He prefers adding the name.
I agree that git blame may give you the same context. But it can be difficult if the code went through multiple refactors or was even moved to a different location.
There was a problem hiding this comment.
I don't really have a preference here, though I think it's pretty easy for code to move around enough to make git blame useless at our pace of refactoring and development. I wouldn't mind skipping the inclusion of a name.
There was a problem hiding this comment.
Before I was sent the above link, it was 100% my assumption that the meaning of the name in the TODO comment was "This is a TODO for me, i.e. I am going to fix it" (and thus, implicitly, nobody else should do it without asking me first.)
We can of course clarify this internally, but that doesn't mean it will be clear to the rest of the world.
Summary
async forloops use the asynchronous iteration protocol (involving__aiter__and__anext__) rather than the synchronous iteration protocol (involving__iter__and__next__). Understanding__anext__calls is beyond our capabilities until we can understanding async call expressions (which is probably blocked on generic coroutine types). For now, inferringUnknownfor loop vars inasync forloops is more accurate than erroneously using the synchronous iteration protocol for the types of these definitions.Test Plan
cargo test -p red_knot_python_semantic --lib