From 2a3775e5259cae0050e3b866b56819c58ded631a Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Thu, 5 Sep 2024 08:55:00 -0700 Subject: [PATCH] [red-knot] AnnAssign with no RHS is not a Definition (#13247) My plan for handling declared types is to introduce a `Declaration` in addition to `Definition`. A `Declaration` is an annotation of a name with a type; a `Definition` is an actual runtime assignment of a value to a name. A few things (an annotated function parameter, an annotated-assignment with an RHS) are both a `Definition` and a `Declaration`. This more cleanly separates type inference (only cares about `Definition`) from declared types (only impacted by a `Declaration`), and I think it will work out better than trying to squeeze everything into `Definition`. One of the tests in this PR (`annotation_only_assignment_transparent_to_local_inference`) demonstrates one reason why. The statement `x: int` should have no effect on local inference of the type of `x`; whatever the locally inferred type of `x` was before `x: int` should still be the inferred type after `x: int`. This is actually quite hard to do if `x: int` is considered a `Definition`, because a core assumption of the use-def map is that a `Definition` replaces the previous value. To achieve this would require some hackery to effectively treat `x: int` sort of as if it were `x: int = x`, but it's not really even equivalent to that, so this approach gets quite ugly. As a first step in this plan, this PR stops treating AnnAssign with no RHS as a `Definition`, which fixes behavior in a couple added tests. This actually makes things temporarily worse for the ellipsis-type test, since it is defined in typeshed only using annotated assignments with no RHS. This will be fixed properly by the upcoming addition of declarations, which should also treat a declared type as sufficient to import a name, at least from a stub. --- .../src/semantic_index/builder.rs | 30 ++++----- .../src/types/infer.rs | 62 ++++++++++++++++--- crates/ruff_benchmark/benches/red_knot.rs | 1 + 3 files changed, 68 insertions(+), 25 deletions(-) diff --git a/crates/red_knot_python_semantic/src/semantic_index/builder.rs b/crates/red_knot_python_semantic/src/semantic_index/builder.rs index f8e4e34fe2..ff816c0fad 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -498,7 +498,6 @@ where } ast::Stmt::AnnAssign(node) => { debug_assert!(self.current_assignment.is_none()); - // TODO deferred annotation visiting self.visit_expr(&node.annotation); if let Some(value) = &node.value { self.visit_expr(value); @@ -633,21 +632,22 @@ where match expr { ast::Expr::Name(name_node @ ast::ExprName { id, ctx, .. }) => { - let mut flags = match ctx { - ast::ExprContext::Load => SymbolFlags::IS_USED, - ast::ExprContext::Store => SymbolFlags::IS_DEFINED, - ast::ExprContext::Del => SymbolFlags::IS_DEFINED, - ast::ExprContext::Invalid => SymbolFlags::empty(), + let flags = match (ctx, self.current_assignment) { + (ast::ExprContext::Store, Some(CurrentAssignment::AugAssign(_))) => { + // For augmented assignment, the target expression is also used. + SymbolFlags::IS_DEFINED | SymbolFlags::IS_USED + } + (ast::ExprContext::Store, Some(CurrentAssignment::AnnAssign(ann_assign))) + if ann_assign.value.is_none() => + { + // An annotated assignment that doesn't assign a value is not a Definition + SymbolFlags::empty() + } + (ast::ExprContext::Load, _) => SymbolFlags::IS_USED, + (ast::ExprContext::Store, _) => SymbolFlags::IS_DEFINED, + (ast::ExprContext::Del, _) => SymbolFlags::IS_DEFINED, + (ast::ExprContext::Invalid, _) => SymbolFlags::empty(), }; - if matches!( - self.current_assignment, - Some(CurrentAssignment::AugAssign(_)) - ) && !ctx.is_invalid() - { - // For augmented assignment, the target expression is also used, so we should - // record that as a use. - flags |= SymbolFlags::IS_USED; - } let symbol = self.add_or_update_symbol(id.clone(), flags); if flags.contains(SymbolFlags::IS_DEFINED) { match self.current_assignment { diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 359bf757c0..0dcc108531 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -928,10 +928,11 @@ impl<'db> TypeInferenceBuilder<'db> { } fn infer_annotated_assignment_statement(&mut self, assignment: &ast::StmtAnnAssign) { - if let ast::Expr::Name(_) = assignment.target.as_ref() { + // assignments to non-Names are not Definitions, and neither are annotated assignments + // without an RHS + if assignment.value.is_some() && matches!(*assignment.target, ast::Expr::Name(_)) { self.infer_definition(assignment); } else { - // currently we don't consider assignments to non-Names to be Definitions self.infer_annotated_assignment(assignment); } } @@ -941,11 +942,13 @@ impl<'db> TypeInferenceBuilder<'db> { assignment: &ast::StmtAnnAssign, definition: Definition<'db>, ) { - let ty = self.infer_annotated_assignment(assignment); + let ty = self + .infer_annotated_assignment(assignment) + .expect("Only annotated assignments with a RHS should create a Definition"); self.types.definitions.insert(definition, ty); } - fn infer_annotated_assignment(&mut self, assignment: &ast::StmtAnnAssign) -> Type<'db> { + fn infer_annotated_assignment(&mut self, assignment: &ast::StmtAnnAssign) -> Option> { let ast::StmtAnnAssign { range: _, target, @@ -954,13 +957,13 @@ impl<'db> TypeInferenceBuilder<'db> { simple: _, } = assignment; - self.infer_optional_expression(value.as_deref()); + let value_ty = self.infer_optional_expression(value.as_deref()); - let annotation_ty = self.infer_expression(annotation); + self.infer_expression(annotation); self.infer_expression(target); - annotation_ty + value_ty } fn infer_augmented_assignment_statement(&mut self, assignment: &ast::StmtAugAssign) { @@ -1890,8 +1893,6 @@ impl<'db> TypeInferenceBuilder<'db> { let Some(enclosing_symbol) = enclosing_symbol_table.symbol_by_name(name) else { continue; }; - // TODO a "definition" that is just an annotated-assignment with no RHS should not - // count as "is_defined" here. if enclosing_symbol.is_defined() { // We can return early here, because the nearest function-like scope that // defines a name must be the only source for the nonlocal reference (at @@ -2909,7 +2910,7 @@ mod tests { // TODO: update this once `infer_ellipsis_literal_expression` correctly // infers `types.EllipsisType`. - assert_public_ty(&db, "src/a.py", "x", "Unknown | Literal[EllipsisType]"); + assert_public_ty(&db, "src/a.py", "x", "Unbound"); Ok(()) } @@ -3972,6 +3973,47 @@ mod tests { Ok(()) } + #[test] + fn nonlocal_name_reference_skips_annotation_only_assignment() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "/src/a.py", + " + def f(): + x = 1 + def g(): + // it's pretty weird to have an annotated assignment in a function where the + // name is otherwise not defined; maybe should be an error? + x: int + def h(): + y = x + ", + )?; + + assert_scope_ty(&db, "/src/a.py", &["f", "g", "h"], "y", "Literal[1]"); + + Ok(()) + } + + #[test] + fn annotation_only_assignment_transparent_to_local_inference() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "/src/a.py", + " + x = 1 + x: int + y = x + ", + )?; + + assert_public_ty(&db, "/src/a.py", "y", "Literal[1]"); + + Ok(()) + } + // Incremental inference tests fn first_public_def<'db>(db: &'db TestDb, file: File, name: &str) -> Definition<'db> { diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs index 8732042e3d..2adeee80cc 100644 --- a/crates/ruff_benchmark/benches/red_knot.rs +++ b/crates/ruff_benchmark/benches/red_knot.rs @@ -23,6 +23,7 @@ const TOMLLIB_312_URL: &str = "https://raw.githubusercontent.com/python/cpython/ // The failed import from 'collections.abc' is due to lack of support for 'import *'. static EXPECTED_DIAGNOSTICS: &[&str] = &[ + "/src/tomllib/_parser.py:5:24: Module '__future__' has no member 'annotations'", "/src/tomllib/_parser.py:7:29: Module 'collections.abc' has no member 'Iterable'", "Line 69 is too long (89 characters)", "Use double quotes for strings",