Skip to content
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.

Sign up for GitHub

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

[red-knot] Infer Unknown for the loop var in async for loops #13243

Merged
merged 1 commit into from
Sep 4, 2024
+83 −9
Merged

[red-knot] Infer Unknown for the loop var in async for loops #13243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/red_knot_python_semantic/src/semantic_index/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,7 @@ where
ForStmtDefinitionNodeRef {
iterable: &node.iter,
target: name_node,
is_async: node.is_async,
},
);
}
Expand Down
22 changes: 16 additions & 6 deletions crates/red_knot_python_semantic/src/semantic_index/definition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ pub(crate) struct WithItemDefinitionNodeRef<'a> {
pub(crate) struct ForStmtDefinitionNodeRef<'a> {
pub(crate) iterable: &'a ast::Expr,
pub(crate) target: &'a ast::ExprName,
pub(crate) is_async: bool,
}

#[derive(Copy, Clone, Debug)]
Expand Down Expand Up @@ -206,12 +207,15 @@ impl DefinitionNodeRef<'_> {
DefinitionNodeRef::AugmentedAssignment(augmented_assignment) => {
DefinitionKind::AugmentedAssignment(AstNodeRef::new(parsed, augmented_assignment))
}
DefinitionNodeRef::For(ForStmtDefinitionNodeRef { iterable, target }) => {
DefinitionKind::For(ForStmtDefinitionKind {
iterable: AstNodeRef::new(parsed.clone(), iterable),
target: AstNodeRef::new(parsed, target),
})
}
DefinitionNodeRef::For(ForStmtDefinitionNodeRef {
iterable,
target,
is_async,
}) => DefinitionKind::For(ForStmtDefinitionKind {
iterable: AstNodeRef::new(parsed.clone(), iterable),
target: AstNodeRef::new(parsed, target),
is_async,
}),
DefinitionNodeRef::Comprehension(ComprehensionDefinitionNodeRef {
iterable,
target,
Expand Down Expand Up @@ -265,6 +269,7 @@ impl DefinitionNodeRef<'_> {
Self::For(ForStmtDefinitionNodeRef {
iterable: _,
target,
is_async: _,
}) => target.into(),
Self::Comprehension(ComprehensionDefinitionNodeRef { target, .. }) => target.into(),
Self::Parameter(node) => match node {
Expand Down Expand Up @@ -388,6 +393,7 @@ impl WithItemDefinitionKind {
pub struct ForStmtDefinitionKind {
iterable: AstNodeRef<ast::Expr>,
target: AstNodeRef<ast::ExprName>,
is_async: bool,
}

impl ForStmtDefinitionKind {
Expand All @@ -398,6 +404,10 @@ impl ForStmtDefinitionKind {
pub(crate) fn target(&self) -> &ast::ExprName {
self.target.node()
}

pub(crate) fn is_async(&self) -> bool {
self.is_async
}
}

#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]
Expand Down
69 changes: 66 additions & 3 deletions crates/red_knot_python_semantic/src/types/infer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,7 @@ impl<'db> TypeInferenceBuilder<'db> {
self.infer_for_statement_definition(
for_statement_definition.target(),
for_statement_definition.iterable(),
for_statement_definition.is_async(),
definition,
);
}
Expand Down Expand Up @@ -1045,6 +1046,7 @@ impl<'db> TypeInferenceBuilder<'db> {
&mut self,
target: &ast::ExprName,
iterable: &ast::Expr,
is_async: bool,
definition: Definition<'db>,
) {
let expression = self.index.expression(iterable);
Expand All @@ -1054,9 +1056,14 @@ impl<'db> TypeInferenceBuilder<'db> {
.types
.expression_ty(iterable.scoped_ast_id(self.db, self.scope));

let loop_var_value_ty = iterable_ty
.iterate(self.db)
.unwrap_with_diagnostic(iterable.into(), self);
let loop_var_value_ty = if is_async {
// TODO(Alex): async iterables/iterators!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened a convo in Discord about this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Type::Unknown
} else {
iterable_ty
.iterate(self.db)
.unwrap_with_diagnostic(iterable.into(), self)
};

self.types
.expressions
Expand Down Expand Up @@ -3026,6 +3033,62 @@ mod tests {
Ok(())
}

/// This tests that we understand that `async` for loops
/// do not work according to the synchronous iteration protocol
#[test]
fn invalid_async_for_loop() -> anyhow::Result<()> {
let mut db = setup_db();

db.write_dedented(
"src/a.py",
"
async def foo():
class Iterator:
def __next__(self) -> int:
return 42

class Iterable:
def __iter__(self) -> Iterator:
return Iterator()

async for x in Iterator():
pass
",
)?;

// TODO(Alex) async iterables/iterators!
assert_scope_ty(&db, "src/a.py", &["foo"], "x", "Unknown");

Ok(())
}

#[test]
fn basic_async_for_loop() -> anyhow::Result<()> {
let mut db = setup_db();

db.write_dedented(
"src/a.py",
"
async def foo():
class IntAsyncIterator:
async def __anext__(self) -> int:
return 42

class IntAsyncIterable:
def __aiter__(self) -> IntAsyncIterator:
return IntAsyncIterator()

async for x in IntAsyncIterable():
pass
",
)?;

// TODO(Alex) async iterables/iterators!
assert_scope_ty(&db, "src/a.py", &["foo"], "x", "Unknown");

Ok(())
}

#[test]
fn class_constructor_call_expression() -> anyhow::Result<()> {
let mut db = setup_db();
Expand Down
Loading