From dfd8eaeb32aa20d803be577096ea945f383e54c0 Mon Sep 17 00:00:00 2001 From: Shunsuke Shibayama <45118249+mtshiba@users.noreply.github.com> Date: Mon, 14 Apr 2025 16:23:20 +0900 Subject: [PATCH] [red-knot] detect unreachable attribute assignments (#16852) ## Summary This PR closes #15967. Attribute assignments that are statically known to be unreachable are excluded from consideration for implicit instance attribute type inference. If none of the assignments are found to be reachable, an `unresolved-attribute` error is reported. ## Test Plan [A test case](https://github.com/astral-sh/ruff/blob/main/crates/red_knot_python_semantic/resources/mdtest/attributes.md#attributes-defined-in-statically-known-to-be-false-branches) marked as TODO now work as intended, and new test cases have been added. --------- Co-authored-by: David Peter --- .../resources/mdtest/attributes.md | 112 +++++++- .../src/semantic_index.rs | 39 +-- .../semantic_index/attribute_assignment.rs | 37 --- .../src/semantic_index/builder.rs | 240 ++++++++++------ .../src/semantic_index/definition.rs | 206 ++++++++++---- .../src/semantic_index/use_def.rs | 95 ++++++- crates/red_knot_python_semantic/src/symbol.rs | 4 + crates/red_knot_python_semantic/src/types.rs | 8 + .../src/types/class.rs | 259 +++++++++++++----- .../src/types/infer.rs | 92 +++---- 10 files changed, 764 insertions(+), 328 deletions(-) delete mode 100644 crates/red_knot_python_semantic/src/semantic_index/attribute_assignment.rs diff --git a/crates/red_knot_python_semantic/resources/mdtest/attributes.md b/crates/red_knot_python_semantic/resources/mdtest/attributes.md index 62bd3154d9..14761c79e9 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/attributes.md +++ b/crates/red_knot_python_semantic/resources/mdtest/attributes.md @@ -41,8 +41,7 @@ reveal_type(c_instance.declared_only) # revealed: bytes reveal_type(c_instance.declared_and_bound) # revealed: bool -# We probably don't want to emit a diagnostic for this being possibly undeclared/unbound. -# mypy and pyright do not show an error here. +# error: [possibly-unbound-attribute] reveal_type(c_instance.possibly_undeclared_unbound) # revealed: str # This assignment is fine, as we infer `Unknown | Literal[1, "a"]` for `inferred_from_value`. @@ -339,8 +338,10 @@ class C: for self.z in NonIterable(): pass +# Iterable might be empty +# error: [possibly-unbound-attribute] reveal_type(C().x) # revealed: Unknown | int - +# error: [possibly-unbound-attribute] reveal_type(C().y) # revealed: Unknown | str ``` @@ -409,8 +410,8 @@ reveal_type(c_instance.a) # revealed: Unknown #### Conditionally declared / bound attributes -We currently do not raise a diagnostic or change behavior if an attribute is only conditionally -defined. This is consistent with what mypy and pyright do. +Attributes are possibly unbound if they, or the method to which they are added are conditionally +declared / bound. ```py def flag() -> bool: @@ -428,9 +429,13 @@ class C: c_instance = C() +# error: [possibly-unbound-attribute] reveal_type(c_instance.a1) # revealed: str | None +# error: [possibly-unbound-attribute] reveal_type(c_instance.a2) # revealed: str | None +# error: [possibly-unbound-attribute] reveal_type(c_instance.b1) # revealed: Unknown | Literal[1] +# error: [possibly-unbound-attribute] reveal_type(c_instance.b2) # revealed: Unknown | Literal[1] ``` @@ -539,10 +544,88 @@ class C: if (2 + 3) < 4: self.x: str = "a" -# TODO: Ideally, this would result in a `unresolved-attribute` error. But mypy and pyright -# do not support this either (for conditions that can only be resolved to `False` in type -# inference), so it does not seem to be particularly important. -reveal_type(C().x) # revealed: str +# error: [unresolved-attribute] +reveal_type(C().x) # revealed: Unknown +``` + +```py +class C: + def __init__(self, cond: bool) -> None: + if True: + self.a = 1 + else: + self.a = "a" + + if False: + self.b = 2 + + if cond: + return + + self.c = 3 + + self.d = 4 + self.d = 5 + + def set_c(self, c: str) -> None: + self.c = c + if False: + def set_e(self, e: str) -> None: + self.e = e + +reveal_type(C(True).a) # revealed: Unknown | Literal[1] +# error: [unresolved-attribute] +reveal_type(C(True).b) # revealed: Unknown +reveal_type(C(True).c) # revealed: Unknown | Literal[3] | str +# TODO: this attribute is possibly unbound +reveal_type(C(True).d) # revealed: Unknown | Literal[5] +# error: [unresolved-attribute] +reveal_type(C(True).e) # revealed: Unknown +``` + +#### Attributes considered always bound + +```py +class C: + def __init__(self, cond: bool): + self.x = 1 + if cond: + raise ValueError("Something went wrong") + + # We consider this attribute is always bound. + # This is because, it is not possible to access a partially-initialized object by normal means. + self.y = 2 + +reveal_type(C(False).x) # revealed: Unknown | Literal[1] +reveal_type(C(False).y) # revealed: Unknown | Literal[2] + +class C: + def __init__(self, b: bytes) -> None: + self.b = b + + try: + s = b.decode() + except UnicodeDecodeError: + raise ValueError("Invalid UTF-8 sequence") + + self.s = s + +reveal_type(C(b"abc").b) # revealed: Unknown | bytes +reveal_type(C(b"abc").s) # revealed: Unknown | str + +class C: + def __init__(self, iter) -> None: + self.x = 1 + + for _ in iter: + pass + + # The for-loop may not stop, + # but we consider the subsequent attributes to be definitely-bound. + self.y = 2 + +reveal_type(C([]).x) # revealed: Unknown | Literal[1] +reveal_type(C([]).y) # revealed: Unknown | Literal[2] ``` #### Diagnostics are reported for the right-hand side of attribute assignments @@ -1046,13 +1129,18 @@ def _(flag: bool): def __init(self): if flag: self.x = 1 + self.y = "a" + else: + self.y = "b" - # Emitting a diagnostic in a case like this is not something we support, and it's unclear - # if we ever will (or want to) + # error: [possibly-unbound-attribute] reveal_type(Foo().x) # revealed: Unknown | Literal[1] - # Same here + # error: [possibly-unbound-attribute] Foo().x = 2 + + reveal_type(Foo().y) # revealed: Unknown | Literal["a", "b"] + Foo().y = "c" ``` ### Unions with all paths unbound diff --git a/crates/red_knot_python_semantic/src/semantic_index.rs b/crates/red_knot_python_semantic/src/semantic_index.rs index 7d1b4452f6..5771220788 100644 --- a/crates/red_knot_python_semantic/src/semantic_index.rs +++ b/crates/red_knot_python_semantic/src/semantic_index.rs @@ -13,7 +13,6 @@ use crate::module_name::ModuleName; use crate::node_key::NodeKey; use crate::semantic_index::ast_ids::node_key::ExpressionNodeKey; use crate::semantic_index::ast_ids::AstIds; -use crate::semantic_index::attribute_assignment::AttributeAssignments; use crate::semantic_index::builder::SemanticIndexBuilder; use crate::semantic_index::definition::{Definition, DefinitionNodeKey, Definitions}; use crate::semantic_index::expression::Expression; @@ -24,7 +23,6 @@ use crate::semantic_index::use_def::{EagerBindingsKey, ScopedEagerBindingsId, Us use crate::Db; pub mod ast_ids; -pub mod attribute_assignment; mod builder; pub mod definition; pub mod expression; @@ -98,23 +96,25 @@ pub(crate) fn use_def_map<'db>(db: &'db dyn Db, scope: ScopeId<'db>) -> Arc( +/// Returns all attribute assignments (and their method scope IDs) for a specific class body scope. +/// Only call this when doing type inference on the same file as `class_body_scope`, otherwise it +/// introduces a direct dependency on that file's AST. +pub(crate) fn attribute_assignments<'db, 's>( db: &'db dyn Db, class_body_scope: ScopeId<'db>, -) -> Option>> { + name: &'s str, +) -> impl Iterator, FileScopeId)> + use<'s, 'db> { let file = class_body_scope.file(db); let index = semantic_index(db, file); + let class_scope_id = class_body_scope.file_scope_id(db); - index - .attribute_assignments - .get(&class_body_scope.file_scope_id(db)) - .cloned() + ChildrenIter::new(index, class_scope_id).filter_map(|(file_scope_id, maybe_method)| { + maybe_method.node().as_function()?; + let attribute_table = index.instance_attribute_table(file_scope_id); + let symbol = attribute_table.symbol_id_by_name(name)?; + let use_def = &index.use_def_maps[file_scope_id]; + Some((use_def.instance_attribute_bindings(symbol), file_scope_id)) + }) } /// Returns the module global scope of `file`. @@ -137,6 +137,9 @@ pub(crate) struct SemanticIndex<'db> { /// List of all symbol tables in this file, indexed by scope. symbol_tables: IndexVec>, + /// List of all instance attribute tables in this file, indexed by scope. + instance_attribute_tables: IndexVec, + /// List of all scopes in this file. scopes: IndexVec, @@ -170,10 +173,6 @@ pub(crate) struct SemanticIndex<'db> { /// Flags about the global scope (code usage impacting inference) has_future_annotations: bool, - /// Maps from class body scopes to attribute assignments that were found - /// in methods of that class. - attribute_assignments: FxHashMap>>, - /// Map of all of the eager bindings that appear in this file. eager_bindings: FxHashMap, } @@ -188,6 +187,10 @@ impl<'db> SemanticIndex<'db> { self.symbol_tables[scope_id].clone() } + pub(super) fn instance_attribute_table(&self, scope_id: FileScopeId) -> &SymbolTable { + &self.instance_attribute_tables[scope_id] + } + /// Returns the use-def map for a specific scope. /// /// Use the Salsa cached [`use_def_map()`] query if you only need the diff --git a/crates/red_knot_python_semantic/src/semantic_index/attribute_assignment.rs b/crates/red_knot_python_semantic/src/semantic_index/attribute_assignment.rs deleted file mode 100644 index a5b05db070..0000000000 --- a/crates/red_knot_python_semantic/src/semantic_index/attribute_assignment.rs +++ /dev/null @@ -1,37 +0,0 @@ -use crate::{ - semantic_index::{ast_ids::ScopedExpressionId, expression::Expression}, - unpack::Unpack, -}; - -use ruff_python_ast::name::Name; - -use rustc_hash::FxHashMap; - -/// Describes an (annotated) attribute assignment that we discovered in a method -/// body, typically of the form `self.x: int`, `self.x: int = …` or `self.x = …`. -#[derive(Debug, Clone, PartialEq, Eq, salsa::Update)] -pub(crate) enum AttributeAssignment<'db> { - /// An attribute assignment with an explicit type annotation, either - /// `self.x: ` or `self.x: = …`. - Annotated { annotation: Expression<'db> }, - - /// An attribute assignment without a type annotation, e.g. `self.x = `. - Unannotated { value: Expression<'db> }, - - /// An attribute assignment where the right-hand side is an iterable, for example - /// `for self.x in `. - Iterable { iterable: Expression<'db> }, - - /// An attribute assignment where the expression to be assigned is a context manager, for example - /// `with as self.x`. - ContextManager { context_manager: Expression<'db> }, - - /// An attribute assignment where the left-hand side is an unpacking expression, - /// e.g. `self.x, self.y = `. - Unpack { - attribute_expression_id: ScopedExpressionId, - unpack: Unpack<'db>, - }, -} - -pub(crate) type AttributeAssignments<'db> = FxHashMap>>; 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 4d319a2b89..2a4d10697a 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -16,12 +16,13 @@ use crate::module_resolver::resolve_module; use crate::node_key::NodeKey; use crate::semantic_index::ast_ids::node_key::ExpressionNodeKey; use crate::semantic_index::ast_ids::AstIdsBuilder; -use crate::semantic_index::attribute_assignment::{AttributeAssignment, AttributeAssignments}; use crate::semantic_index::definition::{ - AssignmentDefinitionNodeRef, ComprehensionDefinitionNodeRef, Definition, DefinitionCategory, - DefinitionNodeKey, DefinitionNodeRef, Definitions, ExceptHandlerDefinitionNodeRef, - ForStmtDefinitionNodeRef, ImportDefinitionNodeRef, ImportFromDefinitionNodeRef, - MatchPatternDefinitionNodeRef, StarImportDefinitionNodeRef, WithItemDefinitionNodeRef, + AnnotatedAssignmentDefinitionKind, AnnotatedAssignmentDefinitionNodeRef, + AssignmentDefinitionKind, AssignmentDefinitionNodeRef, ComprehensionDefinitionNodeRef, + Definition, DefinitionCategory, DefinitionKind, DefinitionNodeKey, DefinitionNodeRef, + Definitions, ExceptHandlerDefinitionNodeRef, ForStmtDefinitionKind, ForStmtDefinitionNodeRef, + ImportDefinitionNodeRef, ImportFromDefinitionNodeRef, MatchPatternDefinitionNodeRef, + StarImportDefinitionNodeRef, TargetKind, WithItemDefinitionKind, WithItemDefinitionNodeRef, }; use crate::semantic_index::expression::{Expression, ExpressionKind}; use crate::semantic_index::predicate::{ @@ -87,6 +88,7 @@ pub(super) struct SemanticIndexBuilder<'db> { scopes: IndexVec, scope_ids_by_scope: IndexVec>, symbol_tables: IndexVec, + instance_attribute_tables: IndexVec, ast_ids: IndexVec, use_def_maps: IndexVec>, scopes_by_node: FxHashMap, @@ -94,7 +96,6 @@ pub(super) struct SemanticIndexBuilder<'db> { definitions_by_node: FxHashMap>, expressions_by_node: FxHashMap>, imported_modules: FxHashSet, - attribute_assignments: FxHashMap>, eager_bindings: FxHashMap, } @@ -114,6 +115,7 @@ impl<'db> SemanticIndexBuilder<'db> { scopes: IndexVec::new(), symbol_tables: IndexVec::new(), + instance_attribute_tables: IndexVec::new(), ast_ids: IndexVec::new(), scope_ids_by_scope: IndexVec::new(), use_def_maps: IndexVec::new(), @@ -125,8 +127,6 @@ impl<'db> SemanticIndexBuilder<'db> { imported_modules: FxHashSet::default(), - attribute_assignments: FxHashMap::default(), - eager_bindings: FxHashMap::default(), }; @@ -222,6 +222,8 @@ impl<'db> SemanticIndexBuilder<'db> { let file_scope_id = self.scopes.push(scope); self.symbol_tables.push(SymbolTableBuilder::default()); + self.instance_attribute_tables + .push(SymbolTableBuilder::default()); self.use_def_maps.push(UseDefMapBuilder::default()); let ast_id_scope = self.ast_ids.push(AstIdsBuilder::default()); @@ -315,6 +317,11 @@ impl<'db> SemanticIndexBuilder<'db> { &mut self.symbol_tables[scope_id] } + fn current_attribute_table(&mut self) -> &mut SymbolTableBuilder { + let scope_id = self.current_scope(); + &mut self.instance_attribute_tables[scope_id] + } + fn current_use_def_map_mut(&mut self) -> &mut UseDefMapBuilder<'db> { let scope_id = self.current_scope(); &mut self.use_def_maps[scope_id] @@ -358,6 +365,14 @@ impl<'db> SemanticIndexBuilder<'db> { (symbol_id, added) } + fn add_attribute(&mut self, name: Name) -> ScopedSymbolId { + let (symbol_id, added) = self.current_attribute_table().add_symbol(name); + if added { + self.current_use_def_map_mut().add_attribute(symbol_id); + } + symbol_id + } + fn mark_symbol_bound(&mut self, id: ScopedSymbolId) { self.current_symbol_table().mark_symbol_bound(id); } @@ -458,6 +473,25 @@ impl<'db> SemanticIndexBuilder<'db> { (definition, num_definitions) } + fn add_attribute_definition( + &mut self, + symbol: ScopedSymbolId, + definition_kind: DefinitionKind<'db>, + ) -> Definition { + let definition = Definition::new( + self.db, + self.file, + self.current_scope(), + symbol, + definition_kind, + false, + countme::Count::default(), + ); + self.current_use_def_map_mut() + .record_attribute_binding(symbol, definition); + definition + } + fn record_expression_narrowing_constraint( &mut self, precide_node: &ast::Expr, @@ -626,9 +660,9 @@ impl<'db> SemanticIndexBuilder<'db> { &mut self, object: &ast::Expr, attr: &'db ast::Identifier, - attribute_assignment: AttributeAssignment<'db>, + definition_kind: DefinitionKind<'db>, ) { - if let Some(class_body_scope) = self.is_method_of_class() { + if self.is_method_of_class().is_some() { // We only care about attribute assignments to the first parameter of a method, // i.e. typically `self` or `cls`. let accessed_object_refers_to_first_parameter = @@ -636,12 +670,8 @@ impl<'db> SemanticIndexBuilder<'db> { == self.current_first_parameter_name; if accessed_object_refers_to_first_parameter { - self.attribute_assignments - .entry(class_body_scope) - .or_default() - .entry(attr.id().clone()) - .or_default() - .push(attribute_assignment); + let symbol = self.add_attribute(attr.id().clone()); + self.add_attribute_definition(symbol, definition_kind); } } } @@ -918,18 +948,8 @@ impl<'db> SemanticIndexBuilder<'db> { )); Some(unpackable.as_current_assignment(unpack)) } - ast::Expr::Name(_) => Some(unpackable.as_current_assignment(None)), - ast::Expr::Attribute(ast::ExprAttribute { - value: object, - attr, - .. - }) => { - self.register_attribute_assignment( - object, - attr, - unpackable.as_attribute_assignment(value), - ); - None + ast::Expr::Name(_) | ast::Expr::Attribute(_) => { + Some(unpackable.as_current_assignment(None)) } _ => None, }; @@ -962,6 +982,12 @@ impl<'db> SemanticIndexBuilder<'db> { .map(|builder| Arc::new(builder.finish())) .collect(); + let mut instance_attribute_tables: IndexVec<_, _> = self + .instance_attribute_tables + .into_iter() + .map(SymbolTableBuilder::finish) + .collect(); + let mut use_def_maps: IndexVec<_, _> = self .use_def_maps .into_iter() @@ -976,6 +1002,7 @@ impl<'db> SemanticIndexBuilder<'db> { self.scopes.shrink_to_fit(); symbol_tables.shrink_to_fit(); + instance_attribute_tables.shrink_to_fit(); use_def_maps.shrink_to_fit(); ast_ids.shrink_to_fit(); self.scopes_by_expression.shrink_to_fit(); @@ -987,6 +1014,7 @@ impl<'db> SemanticIndexBuilder<'db> { SemanticIndex { symbol_tables, + instance_attribute_tables, scopes: self.scopes, definitions_by_node: self.definitions_by_node, expressions_by_node: self.expressions_by_node, @@ -997,11 +1025,6 @@ impl<'db> SemanticIndexBuilder<'db> { use_def_maps, imported_modules: Arc::new(self.imported_modules), has_future_annotations: self.has_future_annotations, - attribute_assignments: self - .attribute_assignments - .into_iter() - .map(|(k, v)| (k, Arc::new(v))) - .collect(), eager_bindings: self.eager_bindings, } } @@ -1313,7 +1336,6 @@ where ast::Stmt::AnnAssign(node) => { debug_assert_eq!(&self.current_assignments, &[]); self.visit_expr(&node.annotation); - let annotation = self.add_standalone_type_expression(&node.annotation); if let Some(value) = &node.value { self.visit_expr(value); } @@ -1325,20 +1347,6 @@ where ) { self.push_assignment(node.into()); self.visit_expr(&node.target); - - if let ast::Expr::Attribute(ast::ExprAttribute { - value: object, - attr, - .. - }) = &*node.target - { - self.register_attribute_assignment( - object, - attr, - AttributeAssignment::Annotated { annotation }, - ); - } - self.pop_assignment(); } else { self.visit_expr(&node.target); @@ -1759,7 +1767,7 @@ where fn visit_expr(&mut self, expr: &'ast ast::Expr) { self.scopes_by_expression .insert(expr.into(), self.current_scope()); - let expression_id = self.current_ast_ids().record_expression(expr); + self.current_ast_ids().record_expression(expr); let node_key = NodeKey::from_node(expr); @@ -1792,12 +1800,20 @@ where AssignmentDefinitionNodeRef { unpack, value: &node.value, - name: name_node, + target: expr, }, ); } Some(CurrentAssignment::AnnAssign(ann_assign)) => { - self.add_definition(symbol, ann_assign); + self.add_definition( + symbol, + AnnotatedAssignmentDefinitionNodeRef { + node: ann_assign, + annotation: &ann_assign.annotation, + value: ann_assign.value.as_deref(), + target: expr, + }, + ); } Some(CurrentAssignment::AugAssign(aug_assign)) => { self.add_definition(symbol, aug_assign); @@ -1808,7 +1824,7 @@ where ForStmtDefinitionNodeRef { unpack, iterable: &node.iter, - name: name_node, + target: expr, is_async: node.is_async, }, ); @@ -1840,7 +1856,7 @@ where WithItemDefinitionNodeRef { unpack, context_expr: &item.context_expr, - name: name_node, + target: expr, is_async, }, ); @@ -2026,19 +2042,85 @@ where range: _, }) => { if ctx.is_store() { - if let Some(unpack) = self - .current_assignment() - .as_ref() - .and_then(CurrentAssignment::unpack) - { - self.register_attribute_assignment( - object, - attr, - AttributeAssignment::Unpack { - attribute_expression_id: expression_id, - unpack, - }, - ); + match self.current_assignment() { + Some(CurrentAssignment::Assign { node, unpack, .. }) => { + // SAFETY: `value` and `expr` belong to the `self.module` tree + #[allow(unsafe_code)] + let assignment = AssignmentDefinitionKind::new( + TargetKind::from(unpack), + unsafe { AstNodeRef::new(self.module.clone(), &node.value) }, + unsafe { AstNodeRef::new(self.module.clone(), expr) }, + ); + self.register_attribute_assignment( + object, + attr, + DefinitionKind::Assignment(assignment), + ); + } + Some(CurrentAssignment::AnnAssign(ann_assign)) => { + self.add_standalone_type_expression(&ann_assign.annotation); + // SAFETY: `annotation`, `value` and `expr` belong to the `self.module` tree + #[allow(unsafe_code)] + let assignment = AnnotatedAssignmentDefinitionKind::new( + unsafe { + AstNodeRef::new(self.module.clone(), &ann_assign.annotation) + }, + ann_assign.value.as_deref().map(|value| unsafe { + AstNodeRef::new(self.module.clone(), value) + }), + unsafe { AstNodeRef::new(self.module.clone(), expr) }, + ); + self.register_attribute_assignment( + object, + attr, + DefinitionKind::AnnotatedAssignment(assignment), + ); + } + Some(CurrentAssignment::For { node, unpack, .. }) => { + // // SAFETY: `iter` and `expr` belong to the `self.module` tree + #[allow(unsafe_code)] + let assignment = ForStmtDefinitionKind::new( + TargetKind::from(unpack), + unsafe { AstNodeRef::new(self.module.clone(), &node.iter) }, + unsafe { AstNodeRef::new(self.module.clone(), expr) }, + node.is_async, + ); + self.register_attribute_assignment( + object, + attr, + DefinitionKind::For(assignment), + ); + } + Some(CurrentAssignment::WithItem { + item, + unpack, + is_async, + .. + }) => { + // SAFETY: `context_expr` and `expr` belong to the `self.module` tree + #[allow(unsafe_code)] + let assignment = WithItemDefinitionKind::new( + TargetKind::from(unpack), + unsafe { AstNodeRef::new(self.module.clone(), &item.context_expr) }, + unsafe { AstNodeRef::new(self.module.clone(), expr) }, + is_async, + ); + self.register_attribute_assignment( + object, + attr, + DefinitionKind::WithItem(assignment), + ); + } + Some(CurrentAssignment::Comprehension { .. }) => { + // TODO: + } + Some(CurrentAssignment::AugAssign(_)) => { + // TODO: + } + Some(CurrentAssignment::Named(_)) => { + // TODO: + } + None => {} } } @@ -2138,19 +2220,7 @@ enum CurrentAssignment<'a> { }, } -impl<'a> CurrentAssignment<'a> { - fn unpack(&self) -> Option> { - match self { - Self::Assign { unpack, .. } - | Self::For { unpack, .. } - | Self::WithItem { unpack, .. } => unpack.map(|(_, unpack)| unpack), - Self::AnnAssign(_) - | Self::AugAssign(_) - | Self::Named(_) - | Self::Comprehension { .. } => None, - } - } - +impl CurrentAssignment<'_> { fn unpack_position_mut(&mut self) -> Option<&mut UnpackPosition> { match self { Self::Assign { unpack, .. } @@ -2237,16 +2307,4 @@ impl<'a> Unpackable<'a> { }, } } - - fn as_attribute_assignment(&self, expression: Expression<'a>) -> AttributeAssignment<'a> { - match self { - Unpackable::Assign(_) => AttributeAssignment::Unannotated { value: expression }, - Unpackable::For(_) => AttributeAssignment::Iterable { - iterable: expression, - }, - Unpackable::WithItem { .. } => AttributeAssignment::ContextManager { - context_manager: expression, - }, - } - } } diff --git a/crates/red_knot_python_semantic/src/semantic_index/definition.rs b/crates/red_knot_python_semantic/src/semantic_index/definition.rs index f3f400bc77..9334ac6981 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/definition.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/definition.rs @@ -100,7 +100,7 @@ pub(crate) enum DefinitionNodeRef<'a> { TypeAlias(&'a ast::StmtTypeAlias), NamedExpression(&'a ast::ExprNamed), Assignment(AssignmentDefinitionNodeRef<'a>), - AnnotatedAssignment(&'a ast::StmtAnnAssign), + AnnotatedAssignment(AnnotatedAssignmentDefinitionNodeRef<'a>), AugmentedAssignment(&'a ast::StmtAugAssign), Comprehension(ComprehensionDefinitionNodeRef<'a>), VariadicPositionalParameter(&'a ast::Parameter), @@ -138,12 +138,6 @@ impl<'a> From<&'a ast::ExprNamed> for DefinitionNodeRef<'a> { } } -impl<'a> From<&'a ast::StmtAnnAssign> for DefinitionNodeRef<'a> { - fn from(node: &'a ast::StmtAnnAssign) -> Self { - Self::AnnotatedAssignment(node) - } -} - impl<'a> From<&'a ast::StmtAugAssign> for DefinitionNodeRef<'a> { fn from(node: &'a ast::StmtAugAssign) -> Self { Self::AugmentedAssignment(node) @@ -192,6 +186,12 @@ impl<'a> From> for DefinitionNodeRef<'a> { } } +impl<'a> From> for DefinitionNodeRef<'a> { + fn from(node_ref: AnnotatedAssignmentDefinitionNodeRef<'a>) -> Self { + Self::AnnotatedAssignment(node_ref) + } +} + impl<'a> From> for DefinitionNodeRef<'a> { fn from(node_ref: WithItemDefinitionNodeRef<'a>) -> Self { Self::WithItem(node_ref) @@ -246,14 +246,22 @@ pub(crate) struct ImportFromDefinitionNodeRef<'a> { pub(crate) struct AssignmentDefinitionNodeRef<'a> { pub(crate) unpack: Option<(UnpackPosition, Unpack<'a>)>, pub(crate) value: &'a ast::Expr, - pub(crate) name: &'a ast::ExprName, + pub(crate) target: &'a ast::Expr, +} + +#[derive(Copy, Clone, Debug)] +pub(crate) struct AnnotatedAssignmentDefinitionNodeRef<'a> { + pub(crate) node: &'a ast::StmtAnnAssign, + pub(crate) annotation: &'a ast::Expr, + pub(crate) value: Option<&'a ast::Expr>, + pub(crate) target: &'a ast::Expr, } #[derive(Copy, Clone, Debug)] pub(crate) struct WithItemDefinitionNodeRef<'a> { pub(crate) unpack: Option<(UnpackPosition, Unpack<'a>)>, pub(crate) context_expr: &'a ast::Expr, - pub(crate) name: &'a ast::ExprName, + pub(crate) target: &'a ast::Expr, pub(crate) is_async: bool, } @@ -261,7 +269,7 @@ pub(crate) struct WithItemDefinitionNodeRef<'a> { pub(crate) struct ForStmtDefinitionNodeRef<'a> { pub(crate) unpack: Option<(UnpackPosition, Unpack<'a>)>, pub(crate) iterable: &'a ast::Expr, - pub(crate) name: &'a ast::ExprName, + pub(crate) target: &'a ast::Expr, pub(crate) is_async: bool, } @@ -335,27 +343,34 @@ impl<'db> DefinitionNodeRef<'db> { DefinitionNodeRef::Assignment(AssignmentDefinitionNodeRef { unpack, value, - name, + target, }) => DefinitionKind::Assignment(AssignmentDefinitionKind { - target: TargetKind::from(unpack), + target_kind: TargetKind::from(unpack), value: AstNodeRef::new(parsed.clone(), value), - name: AstNodeRef::new(parsed, name), + target: AstNodeRef::new(parsed, target), + }), + DefinitionNodeRef::AnnotatedAssignment(AnnotatedAssignmentDefinitionNodeRef { + node: _, + annotation, + value, + target, + }) => DefinitionKind::AnnotatedAssignment(AnnotatedAssignmentDefinitionKind { + target: AstNodeRef::new(parsed.clone(), target), + annotation: AstNodeRef::new(parsed.clone(), annotation), + value: value.map(|v| AstNodeRef::new(parsed, v)), }), - DefinitionNodeRef::AnnotatedAssignment(assign) => { - DefinitionKind::AnnotatedAssignment(AstNodeRef::new(parsed, assign)) - } DefinitionNodeRef::AugmentedAssignment(augmented_assignment) => { DefinitionKind::AugmentedAssignment(AstNodeRef::new(parsed, augmented_assignment)) } DefinitionNodeRef::For(ForStmtDefinitionNodeRef { unpack, iterable, - name, + target, is_async, }) => DefinitionKind::For(ForStmtDefinitionKind { - target: TargetKind::from(unpack), + target_kind: TargetKind::from(unpack), iterable: AstNodeRef::new(parsed.clone(), iterable), - name: AstNodeRef::new(parsed, name), + target: AstNodeRef::new(parsed, target), is_async, }), DefinitionNodeRef::Comprehension(ComprehensionDefinitionNodeRef { @@ -381,12 +396,12 @@ impl<'db> DefinitionNodeRef<'db> { DefinitionNodeRef::WithItem(WithItemDefinitionNodeRef { unpack, context_expr, - name, + target, is_async, }) => DefinitionKind::WithItem(WithItemDefinitionKind { - target: TargetKind::from(unpack), + target_kind: TargetKind::from(unpack), context_expr: AstNodeRef::new(parsed.clone(), context_expr), - name: AstNodeRef::new(parsed, name), + target: AstNodeRef::new(parsed, target), is_async, }), DefinitionNodeRef::MatchPattern(MatchPatternDefinitionNodeRef { @@ -449,26 +464,26 @@ impl<'db> DefinitionNodeRef<'db> { Self::Assignment(AssignmentDefinitionNodeRef { value: _, unpack: _, - name, - }) => name.into(), - Self::AnnotatedAssignment(node) => node.into(), + target, + }) => DefinitionNodeKey(NodeKey::from_node(target)), + Self::AnnotatedAssignment(ann_assign) => ann_assign.node.into(), Self::AugmentedAssignment(node) => node.into(), Self::For(ForStmtDefinitionNodeRef { - unpack: _, + target, iterable: _, - name, + unpack: _, is_async: _, - }) => name.into(), + }) => DefinitionNodeKey(NodeKey::from_node(target)), Self::Comprehension(ComprehensionDefinitionNodeRef { target, .. }) => target.into(), Self::VariadicPositionalParameter(node) => node.into(), Self::VariadicKeywordParameter(node) => node.into(), Self::Parameter(node) => node.into(), Self::WithItem(WithItemDefinitionNodeRef { - unpack: _, context_expr: _, + unpack: _, is_async: _, - name, - }) => name.into(), + target, + }) => DefinitionNodeKey(NodeKey::from_node(target)), Self::MatchPattern(MatchPatternDefinitionNodeRef { identifier, .. }) => { identifier.into() } @@ -532,7 +547,7 @@ pub enum DefinitionKind<'db> { TypeAlias(AstNodeRef), NamedExpression(AstNodeRef), Assignment(AssignmentDefinitionKind<'db>), - AnnotatedAssignment(AstNodeRef), + AnnotatedAssignment(AnnotatedAssignmentDefinitionKind), AugmentedAssignment(AstNodeRef), For(ForStmtDefinitionKind<'db>), Comprehension(ComprehensionDefinitionKind), @@ -576,15 +591,15 @@ impl DefinitionKind<'_> { DefinitionKind::Class(class) => class.name.range(), DefinitionKind::TypeAlias(type_alias) => type_alias.name.range(), DefinitionKind::NamedExpression(named) => named.target.range(), - DefinitionKind::Assignment(assignment) => assignment.name().range(), + DefinitionKind::Assignment(assignment) => assignment.target.range(), DefinitionKind::AnnotatedAssignment(assign) => assign.target.range(), DefinitionKind::AugmentedAssignment(aug_assign) => aug_assign.target.range(), - DefinitionKind::For(for_stmt) => for_stmt.name().range(), + DefinitionKind::For(for_stmt) => for_stmt.target.range(), DefinitionKind::Comprehension(comp) => comp.target().range(), DefinitionKind::VariadicPositionalParameter(parameter) => parameter.name.range(), DefinitionKind::VariadicKeywordParameter(parameter) => parameter.name.range(), DefinitionKind::Parameter(parameter) => parameter.parameter.name.range(), - DefinitionKind::WithItem(with_item) => with_item.name().range(), + DefinitionKind::WithItem(with_item) => with_item.target.range(), DefinitionKind::MatchPattern(match_pattern) => match_pattern.identifier.range(), DefinitionKind::ExceptHandler(handler) => handler.node().range(), DefinitionKind::TypeVar(type_var) => type_var.name.range(), @@ -603,15 +618,15 @@ impl DefinitionKind<'_> { DefinitionKind::Class(class) => class.range(), DefinitionKind::TypeAlias(type_alias) => type_alias.range(), DefinitionKind::NamedExpression(named) => named.range(), - DefinitionKind::Assignment(assignment) => assignment.name().range(), - DefinitionKind::AnnotatedAssignment(assign) => assign.range(), + DefinitionKind::Assignment(assignment) => assignment.target.range(), + DefinitionKind::AnnotatedAssignment(assign) => assign.target.range(), DefinitionKind::AugmentedAssignment(aug_assign) => aug_assign.range(), - DefinitionKind::For(for_stmt) => for_stmt.name().range(), + DefinitionKind::For(for_stmt) => for_stmt.target.range(), DefinitionKind::Comprehension(comp) => comp.target().range(), DefinitionKind::VariadicPositionalParameter(parameter) => parameter.range(), DefinitionKind::VariadicKeywordParameter(parameter) => parameter.range(), DefinitionKind::Parameter(parameter) => parameter.parameter.range(), - DefinitionKind::WithItem(with_item) => with_item.name().range(), + DefinitionKind::WithItem(with_item) => with_item.target.range(), DefinitionKind::MatchPattern(match_pattern) => match_pattern.identifier.range(), DefinitionKind::ExceptHandler(handler) => handler.node().range(), DefinitionKind::TypeVar(type_var) => type_var.range(), @@ -674,14 +689,14 @@ impl DefinitionKind<'_> { #[derive(Copy, Clone, Debug, PartialEq, Hash)] pub(crate) enum TargetKind<'db> { Sequence(UnpackPosition, Unpack<'db>), - Name, + NameOrAttribute, } impl<'db> From)>> for TargetKind<'db> { fn from(value: Option<(UnpackPosition, Unpack<'db>)>) -> Self { match value { Some((unpack_position, unpack)) => TargetKind::Sequence(unpack_position, unpack), - None => TargetKind::Name, + None => TargetKind::NameOrAttribute, } } } @@ -803,44 +818,103 @@ impl ImportFromDefinitionKind { #[derive(Clone, Debug)] pub struct AssignmentDefinitionKind<'db> { - target: TargetKind<'db>, + target_kind: TargetKind<'db>, value: AstNodeRef, - name: AstNodeRef, + target: AstNodeRef, } impl<'db> AssignmentDefinitionKind<'db> { - pub(crate) fn target(&self) -> TargetKind<'db> { - self.target + pub(crate) fn new( + target_kind: TargetKind<'db>, + value: AstNodeRef, + target: AstNodeRef, + ) -> Self { + Self { + target_kind, + value, + target, + } + } + + pub(crate) fn target_kind(&self) -> TargetKind<'db> { + self.target_kind } pub(crate) fn value(&self) -> &ast::Expr { self.value.node() } - pub(crate) fn name(&self) -> &ast::ExprName { - self.name.node() + pub(crate) fn target(&self) -> &ast::Expr { + self.target.node() + } +} + +#[derive(Clone, Debug)] +pub struct AnnotatedAssignmentDefinitionKind { + annotation: AstNodeRef, + value: Option>, + target: AstNodeRef, +} + +impl AnnotatedAssignmentDefinitionKind { + pub(crate) fn new( + annotation: AstNodeRef, + value: Option>, + target: AstNodeRef, + ) -> Self { + Self { + annotation, + value, + target, + } + } + + pub(crate) fn value(&self) -> Option<&ast::Expr> { + self.value.as_deref() + } + + pub(crate) fn annotation(&self) -> &ast::Expr { + self.annotation.node() + } + + pub(crate) fn target(&self) -> &ast::Expr { + self.target.node() } } #[derive(Clone, Debug)] pub struct WithItemDefinitionKind<'db> { - target: TargetKind<'db>, + target_kind: TargetKind<'db>, context_expr: AstNodeRef, - name: AstNodeRef, + target: AstNodeRef, is_async: bool, } impl<'db> WithItemDefinitionKind<'db> { + pub(crate) fn new( + target_kind: TargetKind<'db>, + context_expr: AstNodeRef, + target: AstNodeRef, + is_async: bool, + ) -> Self { + Self { + target_kind, + context_expr, + target, + is_async, + } + } + pub(crate) fn context_expr(&self) -> &ast::Expr { self.context_expr.node() } - pub(crate) fn target(&self) -> TargetKind<'db> { - self.target + pub(crate) fn target_kind(&self) -> TargetKind<'db> { + self.target_kind } - pub(crate) fn name(&self) -> &ast::ExprName { - self.name.node() + pub(crate) fn target(&self) -> &ast::Expr { + self.target.node() } pub(crate) const fn is_async(&self) -> bool { @@ -850,23 +924,37 @@ impl<'db> WithItemDefinitionKind<'db> { #[derive(Clone, Debug)] pub struct ForStmtDefinitionKind<'db> { - target: TargetKind<'db>, + target_kind: TargetKind<'db>, iterable: AstNodeRef, - name: AstNodeRef, + target: AstNodeRef, is_async: bool, } impl<'db> ForStmtDefinitionKind<'db> { + pub(crate) fn new( + target_kind: TargetKind<'db>, + iterable: AstNodeRef, + target: AstNodeRef, + is_async: bool, + ) -> Self { + Self { + target_kind, + iterable, + target, + is_async, + } + } + pub(crate) fn iterable(&self) -> &ast::Expr { self.iterable.node() } - pub(crate) fn target(&self) -> TargetKind<'db> { - self.target + pub(crate) fn target_kind(&self) -> TargetKind<'db> { + self.target_kind } - pub(crate) fn name(&self) -> &ast::ExprName { - self.name.node() + pub(crate) fn target(&self) -> &ast::Expr { + self.target.node() } pub(crate) const fn is_async(&self) -> bool { diff --git a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs index a4368cbbe4..ca0c8ec043 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs @@ -259,9 +259,10 @@ use ruff_index::{newtype_index, IndexVec}; use rustc_hash::FxHashMap; +use self::symbol_state::ScopedDefinitionId; use self::symbol_state::{ - LiveBindingsIterator, LiveDeclaration, LiveDeclarationsIterator, ScopedDefinitionId, - SymbolBindings, SymbolDeclarations, SymbolState, + LiveBindingsIterator, LiveDeclaration, LiveDeclarationsIterator, SymbolBindings, + SymbolDeclarations, SymbolState, }; use crate::node_key::NodeKey; use crate::semantic_index::ast_ids::ScopedUseId; @@ -276,6 +277,7 @@ use crate::semantic_index::symbol::{FileScopeId, ScopedSymbolId}; use crate::semantic_index::visibility_constraints::{ ScopedVisibilityConstraintId, VisibilityConstraints, VisibilityConstraintsBuilder, }; +use crate::types::Truthiness; mod symbol_state; @@ -321,6 +323,9 @@ pub(crate) struct UseDefMap<'db> { /// [`SymbolState`] visible at end of scope for each symbol. public_symbols: IndexVec, + /// [`SymbolState`] for each instance attribute. + instance_attributes: IndexVec, + /// Snapshot of bindings in this scope that can be used to resolve a reference in a nested /// eager scope. eager_bindings: EagerBindings, @@ -386,6 +391,13 @@ impl<'db> UseDefMap<'db> { self.bindings_iterator(self.public_symbols[symbol].bindings()) } + pub(crate) fn instance_attribute_bindings( + &self, + symbol: ScopedSymbolId, + ) -> BindingWithConstraintsIterator<'_, 'db> { + self.bindings_iterator(self.instance_attributes[symbol].bindings()) + } + pub(crate) fn eager_bindings( &self, eager_bindings: ScopedEagerBindingsId, @@ -425,6 +437,15 @@ impl<'db> UseDefMap<'db> { .is_always_false() } + pub(crate) fn is_binding_visible( + &self, + db: &dyn crate::Db, + binding: &BindingWithConstraints<'_, 'db>, + ) -> Truthiness { + self.visibility_constraints + .evaluate(db, &self.predicates, binding.visibility_constraint) + } + fn bindings_iterator<'map>( &'map self, bindings: &'map SymbolBindings, @@ -566,6 +587,7 @@ impl std::iter::FusedIterator for DeclarationsIterator<'_, '_> {} #[derive(Clone, Debug)] pub(super) struct FlowSnapshot { symbol_states: IndexVec, + instance_attribute_states: IndexVec, scope_start_visibility: ScopedVisibilityConstraintId, reachability: ScopedVisibilityConstraintId, } @@ -645,6 +667,9 @@ pub(super) struct UseDefMapBuilder<'db> { /// Currently live bindings and declarations for each symbol. symbol_states: IndexVec, + /// Currently live bindings for each instance attribute. + instance_attribute_states: IndexVec, + /// Snapshot of bindings in this scope that can be used to resolve a reference in a nested /// eager scope. eager_bindings: EagerBindings, @@ -665,6 +690,7 @@ impl Default for UseDefMapBuilder<'_> { bindings_by_declaration: FxHashMap::default(), symbol_states: IndexVec::new(), eager_bindings: EagerBindings::default(), + instance_attribute_states: IndexVec::new(), } } } @@ -682,6 +708,13 @@ impl<'db> UseDefMapBuilder<'db> { debug_assert_eq!(symbol, new_symbol); } + pub(super) fn add_attribute(&mut self, symbol: ScopedSymbolId) { + let new_symbol = self + .instance_attribute_states + .push(SymbolState::undefined(self.scope_start_visibility)); + debug_assert_eq!(symbol, new_symbol); + } + pub(super) fn record_binding(&mut self, symbol: ScopedSymbolId, binding: Definition<'db>) { let def_id = self.all_definitions.push(Some(binding)); let symbol_state = &mut self.symbol_states[symbol]; @@ -690,6 +723,18 @@ impl<'db> UseDefMapBuilder<'db> { symbol_state.record_binding(def_id, self.scope_start_visibility); } + pub(super) fn record_attribute_binding( + &mut self, + symbol: ScopedSymbolId, + binding: Definition<'db>, + ) { + let def_id = self.all_definitions.push(Some(binding)); + let attribute_state = &mut self.instance_attribute_states[symbol]; + self.declarations_by_binding + .insert(binding, attribute_state.declarations().clone()); + attribute_state.record_binding(def_id, self.scope_start_visibility); + } + pub(super) fn add_predicate(&mut self, predicate: Predicate<'db>) -> ScopedPredicateId { self.predicates.add_predicate(predicate) } @@ -700,6 +745,10 @@ impl<'db> UseDefMapBuilder<'db> { state .record_narrowing_constraint(&mut self.narrowing_constraints, narrowing_constraint); } + for state in &mut self.instance_attribute_states { + state + .record_narrowing_constraint(&mut self.narrowing_constraints, narrowing_constraint); + } } pub(super) fn record_visibility_constraint( @@ -709,6 +758,9 @@ impl<'db> UseDefMapBuilder<'db> { for state in &mut self.symbol_states { state.record_visibility_constraint(&mut self.visibility_constraints, constraint); } + for state in &mut self.instance_attribute_states { + state.record_visibility_constraint(&mut self.visibility_constraints, constraint); + } self.scope_start_visibility = self .visibility_constraints .add_and_constraint(self.scope_start_visibility, constraint); @@ -762,6 +814,9 @@ impl<'db> UseDefMapBuilder<'db> { /// of it, as the `if`-`elif`-`elif` chain doesn't include any new bindings of `x`. pub(super) fn simplify_visibility_constraints(&mut self, snapshot: FlowSnapshot) { debug_assert!(self.symbol_states.len() >= snapshot.symbol_states.len()); + debug_assert!( + self.instance_attribute_states.len() >= snapshot.instance_attribute_states.len() + ); // If there are any control flow paths that have become unreachable between `snapshot` and // now, then it's not valid to simplify any visibility constraints to `snapshot`. @@ -778,6 +833,13 @@ impl<'db> UseDefMapBuilder<'db> { for (current, snapshot) in self.symbol_states.iter_mut().zip(snapshot.symbol_states) { current.simplify_visibility_constraints(snapshot); } + for (current, snapshot) in self + .instance_attribute_states + .iter_mut() + .zip(snapshot.instance_attribute_states) + { + current.simplify_visibility_constraints(snapshot); + } } pub(super) fn record_reachability_constraint( @@ -849,6 +911,7 @@ impl<'db> UseDefMapBuilder<'db> { pub(super) fn snapshot(&self) -> FlowSnapshot { FlowSnapshot { symbol_states: self.symbol_states.clone(), + instance_attribute_states: self.instance_attribute_states.clone(), scope_start_visibility: self.scope_start_visibility, reachability: self.reachability, } @@ -861,9 +924,12 @@ impl<'db> UseDefMapBuilder<'db> { // greater than the number of known symbols in a previously-taken snapshot. let num_symbols = self.symbol_states.len(); debug_assert!(num_symbols >= snapshot.symbol_states.len()); + let num_attributes = self.instance_attribute_states.len(); + debug_assert!(num_attributes >= snapshot.instance_attribute_states.len()); // Restore the current visible-definitions state to the given snapshot. self.symbol_states = snapshot.symbol_states; + self.instance_attribute_states = snapshot.instance_attribute_states; self.scope_start_visibility = snapshot.scope_start_visibility; self.reachability = snapshot.reachability; @@ -874,6 +940,10 @@ impl<'db> UseDefMapBuilder<'db> { num_symbols, SymbolState::undefined(self.scope_start_visibility), ); + self.instance_attribute_states.resize( + num_attributes, + SymbolState::undefined(self.scope_start_visibility), + ); } /// Merge the given snapshot into the current state, reflecting that we might have taken either @@ -899,6 +969,9 @@ impl<'db> UseDefMapBuilder<'db> { // IDs must line up), so the current number of known symbols must always be equal to or // greater than the number of known symbols in a previously-taken snapshot. debug_assert!(self.symbol_states.len() >= snapshot.symbol_states.len()); + debug_assert!( + self.instance_attribute_states.len() >= snapshot.instance_attribute_states.len() + ); let mut snapshot_definitions_iter = snapshot.symbol_states.into_iter(); for current in &mut self.symbol_states { @@ -917,6 +990,22 @@ impl<'db> UseDefMapBuilder<'db> { // Symbol not present in snapshot, so it's unbound/undeclared from that path. } } + let mut snapshot_definitions_iter = snapshot.instance_attribute_states.into_iter(); + for current in &mut self.instance_attribute_states { + if let Some(snapshot) = snapshot_definitions_iter.next() { + current.merge( + snapshot, + &mut self.narrowing_constraints, + &mut self.visibility_constraints, + ); + } else { + current.merge( + SymbolState::undefined(snapshot.scope_start_visibility), + &mut self.narrowing_constraints, + &mut self.visibility_constraints, + ); + } + } self.scope_start_visibility = self .visibility_constraints @@ -930,6 +1019,7 @@ impl<'db> UseDefMapBuilder<'db> { pub(super) fn finish(mut self) -> UseDefMap<'db> { self.all_definitions.shrink_to_fit(); self.symbol_states.shrink_to_fit(); + self.instance_attribute_states.shrink_to_fit(); self.bindings_by_use.shrink_to_fit(); self.node_reachability.shrink_to_fit(); self.declarations_by_binding.shrink_to_fit(); @@ -944,6 +1034,7 @@ impl<'db> UseDefMapBuilder<'db> { bindings_by_use: self.bindings_by_use, node_reachability: self.node_reachability, public_symbols: self.symbol_states, + instance_attributes: self.instance_attribute_states, declarations_by_binding: self.declarations_by_binding, bindings_by_declaration: self.bindings_by_declaration, eager_bindings: self.eager_bindings, diff --git a/crates/red_knot_python_semantic/src/symbol.rs b/crates/red_knot_python_semantic/src/symbol.rs index 4503264bf9..dae79c3b63 100644 --- a/crates/red_knot_python_semantic/src/symbol.rs +++ b/crates/red_knot_python_semantic/src/symbol.rs @@ -59,6 +59,10 @@ impl<'db> Symbol<'db> { Symbol::Type(ty.into(), Boundness::Bound) } + pub(crate) fn possibly_unbound(ty: impl Into>) -> Self { + Symbol::Type(ty.into(), Boundness::PossiblyUnbound) + } + /// Constructor that creates a [`Symbol`] with a [`crate::types::TodoType`] type /// and boundness [`Boundness::Bound`]. #[allow(unused_variables)] // Only unused in release builds diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index d7a284d8ad..00ac8377be 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -5336,6 +5336,14 @@ impl Truthiness { } } + pub(crate) fn and(self, other: Self) -> Self { + match (self, other) { + (Truthiness::AlwaysTrue, Truthiness::AlwaysTrue) => Truthiness::AlwaysTrue, + (Truthiness::AlwaysFalse, _) | (_, Truthiness::AlwaysFalse) => Truthiness::AlwaysFalse, + _ => Truthiness::Ambiguous, + } + } + fn into_type(self, db: &dyn Db) -> Type { match self { Self::AlwaysTrue => Type::BooleanLiteral(true), diff --git a/crates/red_knot_python_semantic/src/types/class.rs b/crates/red_knot_python_semantic/src/types/class.rs index aca5acd46d..af84291f19 100644 --- a/crates/red_knot_python_semantic/src/types/class.rs +++ b/crates/red_knot_python_semantic/src/types/class.rs @@ -10,8 +10,12 @@ use crate::types::generics::{GenericContext, Specialization}; use crate::{ module_resolver::file_to_module, semantic_index::{ - attribute_assignment::AttributeAssignment, attribute_assignments, semantic_index, - symbol::ScopeId, symbol_table, use_def_map, + ast_ids::HasScopedExpressionId, + attribute_assignments, + definition::{DefinitionKind, TargetKind}, + semantic_index, + symbol::ScopeId, + symbol_table, use_def_map, }, symbol::{ class_symbol, known_module_symbol, symbol_from_bindings, symbol_from_declarations, @@ -853,81 +857,216 @@ impl<'db> ClassLiteralType<'db> { db: &'db dyn Db, class_body_scope: ScopeId<'db>, name: &str, - ) -> Option> { + ) -> Symbol<'db> { // If we do not see any declarations of an attribute, neither in the class body nor in // any method, we build a union of `Unknown` with the inferred types of all bindings of // that attribute. We include `Unknown` in that union to account for the fact that the // attribute might be externally modified. let mut union_of_inferred_types = UnionBuilder::new(db).add(Type::unknown()); - let attribute_assignments = attribute_assignments(db, class_body_scope); + let mut is_attribute_bound = Truthiness::AlwaysFalse; - let attribute_assignments = attribute_assignments - .as_deref() - .and_then(|assignments| assignments.get(name))?; + let file = class_body_scope.file(db); + let index = semantic_index(db, file); + let class_map = use_def_map(db, class_body_scope); + let class_table = symbol_table(db, class_body_scope); - for attribute_assignment in attribute_assignments { - match attribute_assignment { - AttributeAssignment::Annotated { annotation } => { - // We found an annotated assignment of one of the following forms (using 'self' in these - // examples, but we support arbitrary names for the first parameters of methods): - // - // self.name: - // self.name: = … + for (attribute_assignments, method_scope_id) in + attribute_assignments(db, class_body_scope, name) + { + let method_scope = method_scope_id.to_scope_id(db, file); + let method_map = use_def_map(db, method_scope); - let annotation_ty = infer_expression_type(db, *annotation); + // The attribute assignment inherits the visibility of the method which contains it + let is_method_visible = if let Some(method_def) = method_scope.node(db).as_function() { + let method = index.expect_single_definition(method_def); + let method_symbol = class_table.symbol_id_by_name(&method_def.name).unwrap(); + class_map + .public_bindings(method_symbol) + .find_map(|bind| { + (bind.binding == Some(method)) + .then(|| class_map.is_binding_visible(db, &bind)) + }) + .unwrap_or(Truthiness::AlwaysFalse) + } else { + Truthiness::AlwaysFalse + }; + if is_method_visible.is_always_false() { + continue; + } - // TODO: check if there are conflicting declarations - return Some(annotation_ty); + let mut attribute_assignments = attribute_assignments.peekable(); + let unbound_visibility = attribute_assignments + .peek() + .map(|attribute_assignment| { + if attribute_assignment.binding.is_none() { + method_map.is_binding_visible(db, attribute_assignment) + } else { + Truthiness::AlwaysFalse + } + }) + .unwrap_or(Truthiness::AlwaysFalse); + + for attribute_assignment in attribute_assignments { + let Some(binding) = attribute_assignment.binding else { + continue; + }; + match method_map + .is_binding_visible(db, &attribute_assignment) + .and(is_method_visible) + { + Truthiness::AlwaysTrue => { + is_attribute_bound = Truthiness::AlwaysTrue; + } + Truthiness::Ambiguous => { + if is_attribute_bound.is_always_false() { + is_attribute_bound = Truthiness::Ambiguous; + } + } + Truthiness::AlwaysFalse => { + continue; + } } - AttributeAssignment::Unannotated { value } => { - // We found an un-annotated attribute assignment of the form: - // - // self.name = - let inferred_ty = infer_expression_type(db, *value); - - union_of_inferred_types = union_of_inferred_types.add(inferred_ty); + // There is at least one attribute assignment that may be visible, + // so if `unbound_visibility` is always false then this attribute is considered bound. + // TODO: this is incomplete logic since the attributes bound after termination are considered visible. + if unbound_visibility + .negate() + .and(is_method_visible) + .is_always_true() + { + is_attribute_bound = Truthiness::AlwaysTrue; } - AttributeAssignment::Iterable { iterable } => { - // We found an attribute assignment like: - // - // for self.name in : - let iterable_ty = infer_expression_type(db, *iterable); - // TODO: Potential diagnostics resulting from the iterable are currently not reported. - let inferred_ty = iterable_ty.iterate(db); + match binding.kind(db) { + DefinitionKind::AnnotatedAssignment(ann_assign) => { + // We found an annotated assignment of one of the following forms (using 'self' in these + // examples, but we support arbitrary names for the first parameters of methods): + // + // self.name: + // self.name: = … - union_of_inferred_types = union_of_inferred_types.add(inferred_ty); - } - AttributeAssignment::ContextManager { context_manager } => { - // We found an attribute assignment like: - // - // with as self.name: + let annotation_ty = + infer_expression_type(db, index.expression(ann_assign.annotation())); - let context_ty = infer_expression_type(db, *context_manager); - let inferred_ty = context_ty.enter(db); + // TODO: check if there are conflicting declarations + match is_attribute_bound { + Truthiness::AlwaysTrue => { + return Symbol::bound(annotation_ty); + } + Truthiness::Ambiguous => { + return Symbol::possibly_unbound(annotation_ty); + } + Truthiness::AlwaysFalse => unreachable!("If the attribute assignments are all invisible, inference of their types should be skipped"), + } + } + DefinitionKind::Assignment(assign) => { + match assign.target_kind() { + TargetKind::Sequence(_, unpack) => { + // We found an unpacking assignment like: + // + // .., self.name, .. = + // (.., self.name, ..) = + // [.., self.name, ..] = - union_of_inferred_types = union_of_inferred_types.add(inferred_ty); - } - AttributeAssignment::Unpack { - attribute_expression_id, - unpack, - } => { - // We found an unpacking assignment like: - // - // .., self.name, .. = - // (.., self.name, ..) = - // [.., self.name, ..] = + let unpacked = infer_unpack_types(db, unpack); + let target_ast_id = + assign.target().scoped_expression_id(db, method_scope); + let inferred_ty = unpacked.expression_type(target_ast_id); - let inferred_ty = - infer_unpack_types(db, *unpack).expression_type(*attribute_expression_id); - union_of_inferred_types = union_of_inferred_types.add(inferred_ty); + union_of_inferred_types = union_of_inferred_types.add(inferred_ty); + } + TargetKind::NameOrAttribute => { + // We found an un-annotated attribute assignment of the form: + // + // self.name = + + let inferred_ty = + infer_expression_type(db, index.expression(assign.value())); + + union_of_inferred_types = union_of_inferred_types.add(inferred_ty); + } + } + } + DefinitionKind::For(for_stmt) => { + match for_stmt.target_kind() { + TargetKind::Sequence(_, unpack) => { + // We found an unpacking assignment like: + // + // for .., self.name, .. in : + + let unpacked = infer_unpack_types(db, unpack); + let target_ast_id = + for_stmt.target().scoped_expression_id(db, method_scope); + let inferred_ty = unpacked.expression_type(target_ast_id); + + union_of_inferred_types = union_of_inferred_types.add(inferred_ty); + } + TargetKind::NameOrAttribute => { + // We found an attribute assignment like: + // + // for self.name in : + + let iterable_ty = infer_expression_type( + db, + index.expression(for_stmt.iterable()), + ); + // TODO: Potential diagnostics resulting from the iterable are currently not reported. + let inferred_ty = iterable_ty.iterate(db); + + union_of_inferred_types = union_of_inferred_types.add(inferred_ty); + } + } + } + DefinitionKind::WithItem(with_item) => { + match with_item.target_kind() { + TargetKind::Sequence(_, unpack) => { + // We found an unpacking assignment like: + // + // with as .., self.name, ..: + + let unpacked = infer_unpack_types(db, unpack); + let target_ast_id = + with_item.target().scoped_expression_id(db, method_scope); + let inferred_ty = unpacked.expression_type(target_ast_id); + + union_of_inferred_types = union_of_inferred_types.add(inferred_ty); + } + TargetKind::NameOrAttribute => { + // We found an attribute assignment like: + // + // with as self.name: + + let context_ty = infer_expression_type( + db, + index.expression(with_item.context_expr()), + ); + let inferred_ty = context_ty.enter(db); + + union_of_inferred_types = union_of_inferred_types.add(inferred_ty); + } + } + } + DefinitionKind::Comprehension(_) => { + // TODO: + } + DefinitionKind::AugmentedAssignment(_) => { + // TODO: + } + DefinitionKind::NamedExpression(_) => { + // TODO: + } + _ => {} } } } - Some(union_of_inferred_types.build()) + match is_attribute_bound { + Truthiness::AlwaysTrue => Symbol::bound(union_of_inferred_types.build()), + Truthiness::Ambiguous => Symbol::possibly_unbound(union_of_inferred_types.build()), + Truthiness::AlwaysFalse => Symbol::Unbound, + } } /// A helper function for `instance_member` that looks up the `name` attribute only on @@ -961,6 +1100,7 @@ impl<'db> ClassLiteralType<'db> { if let Some(implicit_ty) = Self::implicit_instance_attribute(db, body_scope, name) + .ignore_possibly_unbound() { if declaredness == Boundness::Bound { // If a symbol is definitely declared, and we see @@ -995,6 +1135,7 @@ impl<'db> ClassLiteralType<'db> { } else { if let Some(implicit_ty) = Self::implicit_instance_attribute(db, body_scope, name) + .ignore_possibly_unbound() { Symbol::Type( UnionType::from_elements(db, [declared_ty, implicit_ty]), @@ -1015,9 +1156,7 @@ impl<'db> ClassLiteralType<'db> { // The attribute is not *declared* in the class body. It could still be declared/bound // in a method. - Self::implicit_instance_attribute(db, body_scope, name) - .map_or(Symbol::Unbound, Symbol::bound) - .into() + Self::implicit_instance_attribute(db, body_scope, name).into() } Err((declared, _conflicting_declarations)) => { // There are conflicting declarations for this attribute in the class body. @@ -1028,9 +1167,7 @@ impl<'db> ClassLiteralType<'db> { // This attribute is neither declared nor bound in the class body. // It could still be implicitly defined in a method. - Self::implicit_instance_attribute(db, body_scope, name) - .map_or(Symbol::Unbound, Symbol::bound) - .into() + Self::implicit_instance_attribute(db, body_scope, name).into() } } diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 600ecb7ee8..e793b5389e 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -49,8 +49,9 @@ use crate::module_resolver::resolve_module; use crate::node_key::NodeKey; use crate::semantic_index::ast_ids::{HasScopedExpressionId, HasScopedUseId, ScopedExpressionId}; use crate::semantic_index::definition::{ - AssignmentDefinitionKind, Definition, DefinitionKind, DefinitionNodeKey, - ExceptHandlerDefinitionKind, ForStmtDefinitionKind, TargetKind, WithItemDefinitionKind, + AnnotatedAssignmentDefinitionKind, AssignmentDefinitionKind, Definition, DefinitionKind, + DefinitionNodeKey, ExceptHandlerDefinitionKind, ForStmtDefinitionKind, TargetKind, + WithItemDefinitionKind, }; use crate::semantic_index::expression::{Expression, ExpressionKind}; use crate::semantic_index::symbol::{ @@ -918,7 +919,7 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_assignment_definition(assignment, definition); } DefinitionKind::AnnotatedAssignment(annotated_assignment) => { - self.infer_annotated_assignment_definition(annotated_assignment.node(), definition); + self.infer_annotated_assignment_definition(annotated_assignment, definition); } DefinitionKind::AugmentedAssignment(augmented_assignment) => { self.infer_augment_assignment_definition(augmented_assignment.node(), definition); @@ -1928,23 +1929,23 @@ impl<'db> TypeInferenceBuilder<'db> { definition: Definition<'db>, ) { let context_expr = with_item.context_expr(); - let name = with_item.name(); + let target = with_item.target(); let context_expr_ty = self.infer_standalone_expression(context_expr); let target_ty = if with_item.is_async() { todo_type!("async `with` statement") } else { - match with_item.target() { + match with_item.target_kind() { TargetKind::Sequence(unpack_position, unpack) => { let unpacked = infer_unpack_types(self.db(), unpack); - let name_ast_id = name.scoped_expression_id(self.db(), self.scope()); + let target_ast_id = target.scoped_expression_id(self.db(), self.scope()); if unpack_position == UnpackPosition::First { self.context.extend(unpacked.diagnostics()); } - unpacked.expression_type(name_ast_id) + unpacked.expression_type(target_ast_id) } - TargetKind::Name => self.infer_context_expression( + TargetKind::NameOrAttribute => self.infer_context_expression( context_expr, context_expr_ty, with_item.is_async(), @@ -1952,8 +1953,8 @@ impl<'db> TypeInferenceBuilder<'db> { } }; - self.store_expression_type(name, target_ty); - self.add_binding(name.into(), definition, target_ty); + self.store_expression_type(target, target_ty); + self.add_binding(target.into(), definition, target_ty); } /// Infers the type of a context expression (`with expr`) and returns the target's type @@ -2791,11 +2792,11 @@ impl<'db> TypeInferenceBuilder<'db> { definition: Definition<'db>, ) { let value = assignment.value(); - let name = assignment.name(); + let target = assignment.target(); let value_ty = self.infer_standalone_expression(value); - let mut target_ty = match assignment.target() { + let mut target_ty = match assignment.target_kind() { TargetKind::Sequence(unpack_position, unpack) => { let unpacked = infer_unpack_types(self.db(), unpack); // Only copy the diagnostics if this is the first assignment to avoid duplicating the @@ -2804,22 +2805,19 @@ impl<'db> TypeInferenceBuilder<'db> { self.context.extend(unpacked.diagnostics()); } - let name_ast_id = name.scoped_expression_id(self.db(), self.scope()); - unpacked.expression_type(name_ast_id) + let target_ast_id = target.scoped_expression_id(self.db(), self.scope()); + unpacked.expression_type(target_ast_id) } - TargetKind::Name => { + TargetKind::NameOrAttribute => { // `TYPE_CHECKING` is a special variable that should only be assigned `False` // at runtime, but is always considered `True` in type checking. // See mdtest/known_constants.md#user-defined-type_checking for details. - if &name.id == "TYPE_CHECKING" { + if target.as_name_expr().map(|name| name.id.as_str()) == Some("TYPE_CHECKING") { if !matches!( value.as_boolean_literal_expr(), Some(ast::ExprBooleanLiteral { value: false, .. }) ) { - report_invalid_type_checking_constant( - &self.context, - assignment.name().into(), - ); + report_invalid_type_checking_constant(&self.context, target.into()); } Type::BooleanLiteral(true) } else if self.in_stub() && value.is_ellipsis_literal_expr() { @@ -2830,14 +2828,14 @@ impl<'db> TypeInferenceBuilder<'db> { } }; - if let Some(known_instance) = + if let Some(known_instance) = target.as_name_expr().and_then(|name| { KnownInstanceType::try_from_file_and_name(self.db(), self.file(), &name.id) - { + }) { target_ty = Type::KnownInstance(known_instance); } - self.store_expression_type(name, target_ty); - self.add_binding(name.into(), definition, target_ty); + self.store_expression_type(target, target_ty); + self.add_binding(target.into(), definition, target_ty); } fn infer_annotated_assignment_statement(&mut self, assignment: &ast::StmtAnnAssign) { @@ -2861,16 +2859,12 @@ impl<'db> TypeInferenceBuilder<'db> { /// Infer the types in an annotated assignment definition. fn infer_annotated_assignment_definition( &mut self, - assignment: &ast::StmtAnnAssign, + assignment: &'db AnnotatedAssignmentDefinitionKind, definition: Definition<'db>, ) { - let ast::StmtAnnAssign { - range: _, - target, - annotation, - value, - simple: _, - } = assignment; + let annotation = assignment.annotation(); + let target = assignment.target(); + let value = assignment.value(); let mut declared_ty = self.infer_annotation_expression( annotation, @@ -2886,7 +2880,7 @@ impl<'db> TypeInferenceBuilder<'db> { .is_assignable_to(self.db(), declared_ty.inner_type()) { // annotation not assignable from `bool` is an error - report_invalid_type_checking_constant(&self.context, assignment.into()); + report_invalid_type_checking_constant(&self.context, target.into()); } else if self.in_stub() && value .as_ref() @@ -2900,7 +2894,7 @@ impl<'db> TypeInferenceBuilder<'db> { Some(ast::ExprBooleanLiteral { value: false, .. }) ) { // otherwise, assigning something other than `False` is an error - report_invalid_type_checking_constant(&self.context, assignment.into()); + report_invalid_type_checking_constant(&self.context, target.into()); } declared_ty.inner = Type::BooleanLiteral(true); } @@ -2920,7 +2914,7 @@ impl<'db> TypeInferenceBuilder<'db> { } } - if let Some(value) = value.as_deref() { + if let Some(value) = value { let inferred_ty = self.infer_expression(value); let inferred_ty = if target .as_name_expr() @@ -2933,7 +2927,7 @@ impl<'db> TypeInferenceBuilder<'db> { inferred_ty }; self.add_declaration_with_binding( - assignment.into(), + target.into(), definition, &DeclaredAndInferredType::MightBeDifferent { declared_ty, @@ -2943,12 +2937,12 @@ impl<'db> TypeInferenceBuilder<'db> { } else { if self.in_stub() { self.add_declaration_with_binding( - assignment.into(), + target.into(), definition, &DeclaredAndInferredType::AreTheSame(declared_ty.inner_type()), ); } else { - self.add_declaration(assignment.into(), definition, declared_ty); + self.add_declaration(target.into(), definition, declared_ty); } } @@ -3087,31 +3081,33 @@ impl<'db> TypeInferenceBuilder<'db> { definition: Definition<'db>, ) { let iterable = for_stmt.iterable(); - let name = for_stmt.name(); + let target = for_stmt.target(); let iterable_type = self.infer_standalone_expression(iterable); let loop_var_value_type = if for_stmt.is_async() { todo_type!("async iterables/iterators") } else { - match for_stmt.target() { + match for_stmt.target_kind() { TargetKind::Sequence(unpack_position, unpack) => { let unpacked = infer_unpack_types(self.db(), unpack); if unpack_position == UnpackPosition::First { self.context.extend(unpacked.diagnostics()); } - let name_ast_id = name.scoped_expression_id(self.db(), self.scope()); - unpacked.expression_type(name_ast_id) + let target_ast_id = target.scoped_expression_id(self.db(), self.scope()); + unpacked.expression_type(target_ast_id) + } + TargetKind::NameOrAttribute => { + iterable_type.try_iterate(self.db()).unwrap_or_else(|err| { + err.report_diagnostic(&self.context, iterable_type, iterable.into()); + err.fallback_element_type(self.db()) + }) } - TargetKind::Name => iterable_type.try_iterate(self.db()).unwrap_or_else(|err| { - err.report_diagnostic(&self.context, iterable_type, iterable.into()); - err.fallback_element_type(self.db()) - }), } }; - self.store_expression_type(name, loop_var_value_type); - self.add_binding(name.into(), definition, loop_var_value_type); + self.store_expression_type(target, loop_var_value_type); + self.add_binding(target.into(), definition, loop_var_value_type); } fn infer_while_statement(&mut self, while_statement: &ast::StmtWhile) {