From 4c3d02377ba02281ef3ebcd8e5ef690715aba246 Mon Sep 17 00:00:00 2001 From: Asuka Minato Date: Wed, 29 Jul 2026 00:31:45 +0900 Subject: [PATCH 1/9] fix fix test --- pyrefly/lib/alt/solve.rs | 25 +++++ pyrefly/lib/binding/binding.rs | 30 ++++++ pyrefly/lib/binding/class.rs | 37 ++++++- pyrefly/lib/binding/django.rs | 5 +- pyrefly/lib/binding/function.rs | 2 + pyrefly/lib/binding/scope.rs | 179 ++++++++++++++++++++++++++++---- pyrefly/lib/binding/stmt.rs | 25 +++-- pyrefly/lib/binding/target.rs | 11 +- pyrefly/lib/test/attributes.rs | 27 ++++- pyrefly/lib/test/scope.rs | 75 ++++++++++++- 10 files changed, 374 insertions(+), 42 deletions(-) diff --git a/pyrefly/lib/alt/solve.rs b/pyrefly/lib/alt/solve.rs index df4cd05ed8..f84bc2cc74 100644 --- a/pyrefly/lib/alt/solve.rs +++ b/pyrefly/lib/alt/solve.rs @@ -3016,6 +3016,31 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { .emit(); } } + BindingExpect::UninitializedAttributeCheck { + name, + range, + getattr_class, + termination_keys, + } => { + let may_be_uninitialized = termination_keys + .as_ref() + .is_none_or(|keys| !keys.iter().all(|key| self.get_idx(*key).ty().is_never())); + let has_getattr_fallback = getattr_class.is_some_and(|class_idx| { + self.get_idx(class_idx) + .0 + .as_ref() + .is_some_and(|cls| self.get_class_member(cls, &dunder::GETATTR).is_some()) + }); + if may_be_uninitialized && !has_getattr_fallback { + errors + .error_builder( + *range, + ErrorKind::MissingAttribute, + format!("Attribute `{name}` may be uninitialized"), + ) + .emit(); + } + } BindingExpect::ForwardRefUnion { left, right, diff --git a/pyrefly/lib/binding/binding.rs b/pyrefly/lib/binding/binding.rs index 1c71f555f2..d3dc7a6206 100644 --- a/pyrefly/lib/binding/binding.rs +++ b/pyrefly/lib/binding/binding.rs @@ -1116,6 +1116,8 @@ pub enum KeyExpect { PrivateAttributeAccess(TextRange), /// Deferred uninitialized variable check. UninitializedCheck(TextRange), + /// Check that an attribute declaration is initialized on every control-flow path. + UninitializedAttributeCheck(TextRange), /// Forward reference string literal in union type check. ForwardRefUnion(TextRange), /// A name used in annotation position that may be an invalid implicit alias. @@ -1142,6 +1144,7 @@ impl Ranged for KeyExpect { | KeyExpect::ExceptClauseReachability(range) | KeyExpect::PrivateAttributeAccess(range) | KeyExpect::UninitializedCheck(range) + | KeyExpect::UninitializedAttributeCheck(range) | KeyExpect::ForwardRefUnion(range) | KeyExpect::ImplicitAliasCheck(range) | KeyExpect::ValidateImplicitReturn(range) @@ -1165,6 +1168,7 @@ impl DisplayWith for KeyExpect { KeyExpect::ExceptClauseReachability(r) => ("ExceptClauseReachability", r), KeyExpect::PrivateAttributeAccess(r) => ("PrivateAttributeAccess", r), KeyExpect::UninitializedCheck(r) => ("UninitializedCheck", r), + KeyExpect::UninitializedAttributeCheck(r) => ("UninitializedAttributeCheck", r), KeyExpect::ForwardRefUnion(r) => ("ForwardRefUnion", r), KeyExpect::ImplicitAliasCheck(r) => ("ImplicitAliasCheck", r), KeyExpect::ValidateImplicitReturn(r) => ("ValidateImplicitReturn", r), @@ -1317,6 +1321,17 @@ pub enum BindingExpect { /// If any don't, the variable may be uninitialized. termination_keys: Vec>, }, + /// Check that a class attribute is initialized on every control-flow path. + UninitializedAttributeCheck { + name: Name, + range: TextRange, + /// The owning class when this is an instance attribute. Its `__getattr__` may make a + /// conditionally initialized attribute safe to access. + getattr_class: Option>, + /// `None` means binding proved the declaration conditional. Otherwise, initialization + /// depends on whether all of these branches terminate with `Never`. + termination_keys: Option>>, + }, /// Check for forward reference string literal in union type. /// At runtime, `type.__or__` cannot handle string literals, so expressions /// like `int | "str"` will raise a TypeError. @@ -1489,6 +1504,21 @@ impl DisplayWith for BindingExpect { termination_keys ) } + Self::UninitializedAttributeCheck { + name, + range, + getattr_class, + termination_keys, + } => { + write!( + f, + "UninitializedAttributeCheck({}, {}, {:?}, {:?})", + name, + ctx.module().display(range), + getattr_class, + termination_keys + ) + } Self::ForwardRefUnion { left, right, diff --git a/pyrefly/lib/binding/class.rs b/pyrefly/lib/binding/class.rs index 13c3e716c5..2f392e2438 100644 --- a/pyrefly/lib/binding/class.rs +++ b/pyrefly/lib/binding/class.rs @@ -72,8 +72,10 @@ use crate::binding::binding::KeyClassSynthesizedFields; use crate::binding::binding::KeyExpect; use crate::binding::binding::KeyTParams; use crate::binding::binding::KeyVariance; +use crate::binding::binding::MethodSelfKind; use crate::binding::bindings::BindingsBuilder; use crate::binding::bindings::CurrentIdx; +use crate::binding::bindings::InitializedInFlow; use crate::binding::bindings::LegacyTParamCollector; use crate::binding::expr::Usage; use crate::binding::pydantic::PydanticConfigDict; @@ -476,7 +478,8 @@ impl<'a> BindingsBuilder<'a> { let django_field_info = self.extract_django_fields_from_class_body(&field_definitions); let mut fields = SmallMap::with_capacity(field_definitions.len()); let mut django_relation_fields = Vec::new(); - for (name, (definition, range)) in field_definitions.into_iter_hashed() { + + for (name, (definition, range, initialized)) in field_definitions.into_iter_hashed() { if let ClassFieldDefinition::AssignedInBody { value, .. } = &definition && let ExprOrBinding::Expr(e) = value.as_ref() { @@ -500,6 +503,38 @@ impl<'a> BindingsBuilder<'a> { let attrs_field_specifier = self.attrs_field_specifier(&definition, name.key(), range, &attrs_decorators); + let check_initialization = !matches!( + &definition, + ClassFieldDefinition::DefinedInMethod { method, .. } + if !method.recognized_attribute_defining_method + ); + if check_initialization { + let termination_keys = match &initialized { + InitializedInFlow::Yes => None, + InitializedInFlow::Conditionally | InitializedInFlow::No => Some(None), + InitializedInFlow::DeferredCheck(keys) => Some(Some(keys.clone())), + }; + if let Some(termination_keys) = termination_keys { + let getattr_class = matches!( + &definition, + ClassFieldDefinition::DefinedInMethod { + receiver_kind: MethodSelfKind::Instance, + .. + } + ) + .then_some(class_indices.class_idx); + self.insert_binding( + KeyExpect::UninitializedAttributeCheck(range), + BindingExpect::UninitializedAttributeCheck { + name: name.key().clone(), + range, + getattr_class, + termination_keys, + }, + ); + } + } + fields.insert_hashed( name.clone(), ClassFieldProperties::new( diff --git a/pyrefly/lib/binding/django.rs b/pyrefly/lib/binding/django.rs index a5ca5b4e97..f0c87a2c27 100644 --- a/pyrefly/lib/binding/django.rs +++ b/pyrefly/lib/binding/django.rs @@ -13,6 +13,7 @@ use starlark_map::small_map::SmallMap; use crate::binding::binding::ClassFieldDefinition; use crate::binding::binding::ExprOrBinding; use crate::binding::bindings::BindingsBuilder; +use crate::binding::bindings::InitializedInFlow; const PRIMARY_KEY: Name = Name::new_static("primary_key"); const FOREIGN_KEY: Name = Name::new_static("ForeignKey"); @@ -39,13 +40,13 @@ impl<'a> BindingsBuilder<'a> { /// (primary_key, ForeignKey, choices). pub fn extract_django_fields_from_class_body( &self, - field_definitions: &SmallMap, + field_definitions: &SmallMap, ) -> DjangoFieldInfo { let mut primary_key_field = None; let mut foreign_key_like_fields = Vec::new(); let mut relation_fields = Vec::new(); let mut fields_with_choices = Vec::new(); - for (name, (definition, _range)) in field_definitions.iter() { + for (name, (definition, _range, _initialized)) in field_definitions.iter() { if let ClassFieldDefinition::AssignedInBody { value, .. } = definition && let ExprOrBinding::Expr(e) = value.as_ref() && let Some(call) = e.as_call_expr() diff --git a/pyrefly/lib/binding/function.rs b/pyrefly/lib/binding/function.rs index 3378b0eeda..2a0e31c657 100644 --- a/pyrefly/lib/binding/function.rs +++ b/pyrefly/lib/binding/function.rs @@ -70,6 +70,7 @@ use crate::binding::binding::ReturnImplicit; use crate::binding::binding::ReturnType; use crate::binding::binding::ReturnTypeKind; use crate::binding::bindings::BindingsBuilder; +use crate::binding::bindings::InitializedInFlow; use crate::binding::bindings::LegacyTParamCollector; use crate::binding::expr::Usage; use crate::binding::pattern::match_case_always_matches; @@ -270,6 +271,7 @@ impl<'a> SelfAttrNames<'a> { None, r, MethodSelfKind::Instance, + InitializedInFlow::Yes, ), ) }) diff --git a/pyrefly/lib/binding/scope.rs b/pyrefly/lib/binding/scope.rs index 93e607f54c..2b2de92eaa 100644 --- a/pyrefly/lib/binding/scope.rs +++ b/pyrefly/lib/binding/scope.rs @@ -1168,14 +1168,22 @@ pub struct InstanceAttribute( pub Option>, pub TextRange, pub MethodSelfKind, + pub InitializedInFlow, ); +fn instance_attribute_flow_name(name: &Name) -> Name { + // This cannot collide with a Python identifier, but lets attribute initialization reuse + // the same branch-merging machinery as local variables. + Name::new(format!("$self.{name}")) +} + #[derive(Clone, Debug)] struct ScopeMethod { name: Identifier, self_name: Option, instance_attributes: SmallMap, nn_module_registrations: NNModuleRegistrations, + return_attribute_initialization: Vec>, parameters: SmallMap, yields_and_returns: YieldsAndReturns, is_async: bool, @@ -1248,6 +1256,7 @@ impl ScopeMethod { self_name: None, instance_attributes: SmallMap::new(), nn_module_registrations: SmallMap::new(), + return_attribute_initialization: Vec::new(), parameters: SmallMap::new(), yields_and_returns: Default::default(), is_async, @@ -2160,16 +2169,61 @@ impl Scopes { let scope = self.pop(); let unused_variables = Self::collect_unused_variables(scope.variables.clone()); match scope.kind { - ScopeKind::Method(method_scope) => ( - method_scope.yields_and_returns, - Some(SelfAssignments { - method_name: method_scope.name.id, - instance_attributes: method_scope.instance_attributes, - nn_module_registrations: method_scope.nn_module_registrations, - }), - Self::collect_unused_parameters(method_scope.parameters), - unused_variables, - ), + ScopeKind::Method(mut method_scope) => { + for (name, attribute) in method_scope.instance_attributes.iter_mut() { + let mut initialized = if scope.flow.has_terminated { + None + } else { + Some( + scope + .flow + .get_info(&instance_attribute_flow_name(name)) + .map_or(InitializedInFlow::Conditionally, FlowInfo::initialized), + ) + }; + for return_initialization in &method_scope.return_attribute_initialization { + let on_return = return_initialization + .get(name) + .cloned() + .unwrap_or(InitializedInFlow::Conditionally); + initialized = Some(match (initialized, on_return) { + (None, initialized) => initialized, + (Some(InitializedInFlow::Yes), InitializedInFlow::Yes) => { + InitializedInFlow::Yes + } + ( + Some(InitializedInFlow::DeferredCheck(mut existing)), + InitializedInFlow::DeferredCheck(mut new), + ) => { + existing.append(&mut new); + InitializedInFlow::DeferredCheck(existing) + } + ( + Some(InitializedInFlow::Yes), + InitializedInFlow::DeferredCheck(keys), + ) + | ( + Some(InitializedInFlow::DeferredCheck(keys)), + InitializedInFlow::Yes, + ) => InitializedInFlow::DeferredCheck(keys), + _ => InitializedInFlow::Conditionally, + }); + } + // If every path raises or otherwise terminates without returning an instance, + // there is no successfully initialized object missing this attribute. + attribute.4 = initialized.unwrap_or(InitializedInFlow::Yes); + } + ( + method_scope.yields_and_returns, + Some(SelfAssignments { + method_name: method_scope.name.id, + instance_attributes: method_scope.instance_attributes, + nn_module_registrations: method_scope.nn_module_registrations, + }), + Self::collect_unused_parameters(method_scope.parameters), + unused_variables, + ) + } ScopeKind::Function(function_scope) => ( function_scope.yields_and_returns, None, @@ -2203,12 +2257,29 @@ impl Scopes { x: &ExprAttribute, value: ExprOrBinding, annotation: Option>, + idx: Option>, ) -> bool { + let in_loop = self.loop_depth() != 0; for scope in self.iter_rev_mut() { if let ScopeKind::Method(method_scope) = &mut scope.kind && let Some(self_name) = &method_scope.self_name && matches!(&*x.value, Expr::Name(name) if name.id == self_name.id) { + if let Some(idx) = idx { + match scope + .flow + .info + .entry(instance_attribute_flow_name(&x.attr.id)) + { + Entry::Vacant(entry) => { + entry.insert(FlowInfo::new_value(idx, FlowStyle::Other)); + } + Entry::Occupied(mut entry) => { + *entry.get_mut() = + entry.get().updated_value(idx, FlowStyle::Other, in_loop); + } + } + } if let Some(attr) = method_scope.instance_attributes.get_mut(&x.attr.id) { // Accumulate subsequent assignments in the method. attr.0.push(value); @@ -2225,6 +2296,7 @@ impl Scopes { annotation, x.attr.range(), // Keep the range of the first assignment as the definition location. method_scope.receiver_kind, + InitializedInFlow::Yes, ), ); } @@ -3070,15 +3142,43 @@ impl Scopes { x: StmtReturn, is_unreachable: bool, ) -> Result<(), (CurrentIdx, StmtReturn)> { - match self.current_yields_and_returns_mut() { - Some(yields_and_returns) => { - yields_and_returns - .returns - .push((ret.into_idx(), x, is_unreachable)); - Ok(()) + for scope in self.iter_rev_mut() { + match &mut scope.kind { + ScopeKind::Function(function) => { + function + .yields_and_returns + .returns + .push((ret.into_idx(), x, is_unreachable)); + return Ok(()); + } + ScopeKind::Method(method) => { + if !is_unreachable { + let mut initialization = + SmallMap::with_capacity(method.instance_attributes.len()); + for name in method.instance_attributes.keys() { + initialization.insert( + name.clone(), + scope + .flow + .get_info(&instance_attribute_flow_name(name)) + .map_or( + InitializedInFlow::Conditionally, + FlowInfo::initialized, + ), + ); + } + method.return_attribute_initialization.push(initialization); + } + method + .yields_and_returns + .returns + .push((ret.into_idx(), x, is_unreachable)); + return Ok(()); + } + _ => {} } - None => Err((ret, x)), } + Err((ret, x)) } /// Record a yield in the enclosing function body there is one. @@ -3140,7 +3240,7 @@ impl Scopes { pub fn finish_class_and_get_field_definitions( &mut self, ) -> ( - SmallMap, + SmallMap, NNModuleRegistrations, ) { let mut field_definitions = SmallMap::new(); @@ -3249,14 +3349,26 @@ impl Scopes { definition: value.idx, }, }; - field_definitions.insert_hashed(name.owned(), (definition, static_info.range)); + let initialized = match &value.style { + FlowStyle::PossiblyUninitialized => InitializedInFlow::Conditionally, + FlowStyle::MaybeInitialized(keys) => { + InitializedInFlow::DeferredCheck(keys.clone()) + } + _ => InitializedInFlow::Yes, + }; + field_definitions + .insert_hashed(name.owned(), (definition, static_info.range, initialized)); } }); // Merge assignments from different methods. // `method_attrs` yields attributes from recognized constructor methods first (e.g. __init__), // followed by other helper methods. method_attrs.into_iter().for_each( - |(name, method, InstanceAttribute(values, annotation, range, receiver_kind))| { + |( + name, + method, + InstanceAttribute(values, annotation, range, receiver_kind, initialized), + )| { if let Some(( ClassFieldDefinition::DefinedInMethod { values: existing_values, @@ -3265,6 +3377,7 @@ impl Scopes { receiver_kind: existing_receiver, }, _, + existing_initialized, )) = field_definitions.get_mut(name.key()) { if existing_method.recognized_attribute_defining_method @@ -3284,6 +3397,31 @@ impl Scopes { if matches!(receiver_kind, MethodSelfKind::Class) { *existing_receiver = MethodSelfKind::Class; } + match (&mut *existing_initialized, initialized) { + (InitializedInFlow::Yes, _) | (_, InitializedInFlow::Yes) => { + *existing_initialized = InitializedInFlow::Yes; + } + ( + InitializedInFlow::DeferredCheck(existing), + InitializedInFlow::DeferredCheck(mut new), + ) => existing.append(&mut new), + ( + InitializedInFlow::DeferredCheck(_), + InitializedInFlow::Conditionally, + ) + | ( + InitializedInFlow::Conditionally, + InitializedInFlow::DeferredCheck(_), + ) + | ( + InitializedInFlow::Conditionally, + InitializedInFlow::Conditionally, + ) + | (InitializedInFlow::No, _) + | (_, InitializedInFlow::No) => { + *existing_initialized = InitializedInFlow::Conditionally; + } + } } } else if !field_definitions.contains_key_hashed(name.as_ref()) { field_definitions.insert_hashed( @@ -3296,6 +3434,7 @@ impl Scopes { receiver_kind, }, range, + initialized, ), ); } diff --git a/pyrefly/lib/binding/stmt.rs b/pyrefly/lib/binding/stmt.rs index 914860eac5..5868bcc985 100644 --- a/pyrefly/lib/binding/stmt.rs +++ b/pyrefly/lib/binding/stmt.rs @@ -1201,11 +1201,13 @@ impl<'a> BindingsBuilder<'a> { None, ), ); - let value = match x.value { + let (value, attr_idx) = match x.value { Some(mut assigned) => { - self.bind_attr_assign(attr.clone(), &mut assigned, |v, _| { - ExprOrBinding::Expr(v.clone()) - }) + let (value, idx) = + self.bind_attr_assign(attr.clone(), &mut assigned, |v, _| { + ExprOrBinding::Expr(v.clone()) + }); + (value, Some(idx)) } _ => { self.ensure_expr( @@ -1214,13 +1216,18 @@ impl<'a> BindingsBuilder<'a> { is_annotation: false, }, ); - ExprOrBinding::Binding(Binding::Any(AnyStyle::Implicit)) + ( + ExprOrBinding::Binding(Binding::Any(AnyStyle::Implicit)), + None, + ) } }; - if !self - .scopes - .record_self_attr_assign(&attr, value.clone(), Some(ann_key)) - { + if !self.scopes.record_self_attr_assign( + &attr, + value.clone(), + Some(ann_key), + attr_idx, + ) { self.error( x.range, ErrorKind::BadAssignment, diff --git a/pyrefly/lib/binding/target.rs b/pyrefly/lib/binding/target.rs index c130e644b6..4ee12ab8ab 100644 --- a/pyrefly/lib/binding/target.rs +++ b/pyrefly/lib/binding/target.rs @@ -189,7 +189,7 @@ impl<'a> BindingsBuilder<'a> { mut assigned: Option<&mut Expr>, make_assigned_value: impl FnOnce(Option<&Expr>, Option>) -> ExprOrBinding, ensure_assigned: bool, - ) -> ExprOrBinding { + ) -> (ExprOrBinding, Idx) { let narrowing_identifier = identifier_and_chain_prefix_for_expr(&Expr::Attribute(attr.clone())) .map(|(identifier, _)| identifier); @@ -221,7 +221,7 @@ impl<'a> BindingsBuilder<'a> { if let Some(identifier) = narrowing_identifier { self.narrow_if_name_is_defined(identifier, idx); } - value + (value, idx) } pub fn bind_attr_assign( @@ -229,7 +229,7 @@ impl<'a> BindingsBuilder<'a> { attr: ExprAttribute, assigned: &mut Expr, make_assigned_value: impl FnOnce(&Expr, Option>) -> ExprOrBinding, - ) -> ExprOrBinding { + ) -> (ExprOrBinding, Idx) { self.bind_attr_assign_impl( attr, Some(assigned), @@ -321,7 +321,7 @@ impl<'a> BindingsBuilder<'a> { ); } Expr::Attribute(x) => { - let attr_value = self.bind_attr_assign_impl( + let (attr_value, attr_idx) = self.bind_attr_assign_impl( x.clone(), assigned, make_assigned_value, @@ -329,7 +329,8 @@ impl<'a> BindingsBuilder<'a> { ); // If this is a self-assignment, record it because we may use it to infer // the existence of an instance-only attribute. - self.scopes.record_self_attr_assign(x, attr_value, None); + self.scopes + .record_self_attr_assign(x, attr_value, None, Some(attr_idx)); } Expr::Subscript(x) => { self.bind_subscript_assign_impl( diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index a7733fafa1..c81bf1f432 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -10,6 +10,29 @@ use crate::test::util::TestEnv; use crate::testcase; +fn env_with_conditional_attribute() -> TestEnv { + TestEnv::one( + "foo", + r#" +def coin() -> bool: ... +class Base: + if coin(): + value = 1 # E: Attribute `value` may be uninitialized +"#, + ) +} + +testcase!( + test_conditionally_defined_imported_attribute, + env_with_conditional_attribute(), + r#" +from foo import Base +class Child(Base): ... +Base.value +Child.value +"#, +); + // Test case for various edge cases where a name isn't in the flow, and we might // or might not decide an attribute has been defined. testcase!( @@ -25,8 +48,8 @@ class A: c: str # Defined in conditional control flow, with and without annotation if condition(): - d = 42 - e: int = 42 + d = 42 # E: Attribute `d` may be uninitialized + e: int = 42 # E: Attribute `e` may be uninitialized # Defined (with or without annotation) but only in terminating control flow if condition(): f = 42 diff --git a/pyrefly/lib/test/scope.rs b/pyrefly/lib/test/scope.rs index a92759de9a..72c226f6b2 100644 --- a/pyrefly/lib/test/scope.rs +++ b/pyrefly/lib/test/scope.rs @@ -1697,18 +1697,87 @@ def f(): "#, ); -// https://github.com/facebook/pyrefly/issues/2929 +// https://github.com/facebook/pyrefly/issues/4318 testcase!( - bug = "Should detect possibly-missing attribute on conditionally-defined class members", test_conditionally_defined_class_member, r#" def coin() -> bool: return True +from typing import Any, Never + +def stop() -> Never: + raise Exception() + class Config: name: str = "default" if coin(): - debug = True + debug = True # E: Attribute `debug` may be uninitialized + +class Complete: + if coin(): + value = 1 + else: + value = 2 + +class Child(Config): + pass + +class Instance: + def __init__(self) -> None: + if coin(): + self.complete = 1 + else: + self.complete = 2 + if coin(): + self.partial = 1 # E: Attribute `partial` may be uninitialized + +class DynamicInstance: + if coin(): + class_value = 1 # E: Attribute `class_value` may be uninitialized + + def __getattr__(self, name: str) -> Any: ... + + def __init__(self) -> None: + if coin(): + self.value = 1 + +class CompleteWithReturn: + def __init__(self) -> None: + if coin(): + self.value = 1 + return + self.value = 2 + +class PartialWithReturn: + def __init__(self) -> None: + if coin(): + return + self.value = 1 # E: Attribute `value` may be uninitialized + +class BareAnnotation: + def __init__(self) -> None: + self.value: int # E: Attribute `value` may be uninitialized + +class CompleteWithNever: + def __init__(self) -> None: + if coin(): + self.value = 1 + else: + stop() + +Config.name +Config.debug +Config().debug +Child.debug +Child().debug +Complete.value +Instance().complete +Instance().partial +CompleteWithReturn().value +PartialWithReturn().value +BareAnnotation().value +CompleteWithNever().value "#, ); From 4c2a8d190429b0a2b2cf0960d8af75f762caf3f1 Mon Sep 17 00:00:00 2001 From: Asuka Minato Date: Thu, 8 Oct 2026 13:25:38 +0900 Subject: [PATCH 2/9] keep init function's priority, and merge helper --- pyrefly/lib/binding/scope.rs | 45 +++++++++++++------------------ pyrefly/lib/test/attributes.rs | 49 ++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 26 deletions(-) diff --git a/pyrefly/lib/binding/scope.rs b/pyrefly/lib/binding/scope.rs index 2b2de92eaa..4e27b57f97 100644 --- a/pyrefly/lib/binding/scope.rs +++ b/pyrefly/lib/binding/scope.rs @@ -3383,8 +3383,8 @@ impl Scopes { if existing_method.recognized_attribute_defining_method && !method.recognized_attribute_defining_method { - // Prioritization: Existing is from a recognized constructor, new is from an - // unrecognized helper method. The constructor wins, so ignore the new assignment. + // The constructor determines the attribute type, so the helper's values, + // annotation, and receiver kind do not affect the definition. } else { // Merge: Either both are constructors (e.g. __new__ and __init__), or both are // helper methods. We combine all their assignments. @@ -3397,30 +3397,23 @@ impl Scopes { if matches!(receiver_kind, MethodSelfKind::Class) { *existing_receiver = MethodSelfKind::Class; } - match (&mut *existing_initialized, initialized) { - (InitializedInFlow::Yes, _) | (_, InitializedInFlow::Yes) => { - *existing_initialized = InitializedInFlow::Yes; - } - ( - InitializedInFlow::DeferredCheck(existing), - InitializedInFlow::DeferredCheck(mut new), - ) => existing.append(&mut new), - ( - InitializedInFlow::DeferredCheck(_), - InitializedInFlow::Conditionally, - ) - | ( - InitializedInFlow::Conditionally, - InitializedInFlow::DeferredCheck(_), - ) - | ( - InitializedInFlow::Conditionally, - InitializedInFlow::Conditionally, - ) - | (InitializedInFlow::No, _) - | (_, InitializedInFlow::No) => { - *existing_initialized = InitializedInFlow::Conditionally; - } + } + // Initialization is merged independently of definition priority. + // An unconditional helper assignment also counts as initialization. + match (&mut *existing_initialized, initialized) { + (InitializedInFlow::Yes, _) | (_, InitializedInFlow::Yes) => { + *existing_initialized = InitializedInFlow::Yes; + } + ( + InitializedInFlow::DeferredCheck(existing), + InitializedInFlow::DeferredCheck(mut new), + ) => existing.append(&mut new), + (InitializedInFlow::DeferredCheck(_), InitializedInFlow::Conditionally) + | (InitializedInFlow::Conditionally, InitializedInFlow::DeferredCheck(_)) + | (InitializedInFlow::Conditionally, InitializedInFlow::Conditionally) + | (InitializedInFlow::No, _) + | (_, InitializedInFlow::No) => { + *existing_initialized = InitializedInFlow::Conditionally; } } } else if !field_definitions.contains_key_hashed(name.as_ref()) { diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index c81bf1f432..ab42453690 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -185,6 +185,55 @@ def f(a: A): "#, ); +testcase!( + test_conditional_attribute_initialized_in_helper, + r#" +from typing import assert_type + +class DelegatingInit: + def __init__(self, cfg: int | None = None) -> None: + if cfg is None: + self._load_defaults() + else: + self.cfg = cfg + + def _load_defaults(self) -> None: + self.cfg = 0 + +# An unconditional helper assignment suffices even without a call from __init__. +class HelperFirst: + def _load_defaults(self) -> None: + self.cfg = 0 + + def __init__(self, cfg: int | None = None) -> None: + if cfg is not None: + self.cfg = cfg + +class ConditionalHelper: + def __init__(self, cfg: int | None = None) -> None: + if cfg is not None: + self.cfg = cfg # E: Attribute `cfg` may be uninitialized + + def _load_defaults(self, enabled: bool) -> None: + if enabled: + self.cfg = 0 + +# The constructor still determines the attribute type. +class IncompatibleHelper: + def __init__(self, cfg: int | None = None) -> None: + if cfg is not None: + self.cfg = cfg + + def _load_defaults(self) -> None: + self.cfg = "bad" # E: `Literal['bad']` is not assignable to attribute `cfg` with type `int` + +assert_type(DelegatingInit().cfg, int) +assert_type(HelperFirst().cfg, int) +assert_type(ConditionalHelper().cfg, int) +assert_type(IncompatibleHelper().cfg, int) +"#, +); + testcase!( test_self_attribute_in_unrecognized_method_default_disabled, r#" From aedd4c82d35c4771d5581cb3b07f84e65180f89c Mon Sep 17 00:00:00 2001 From: Asuka Minato Date: Thu, 8 Oct 2026 13:32:11 +0900 Subject: [PATCH 3/9] keep is fn def in flow info --- pyrefly/lib/binding/scope.rs | 23 +++++++- pyrefly/lib/test/attributes.rs | 97 ++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 2 deletions(-) diff --git a/pyrefly/lib/binding/scope.rs b/pyrefly/lib/binding/scope.rs index 4e27b57f97..13a22049de 100644 --- a/pyrefly/lib/binding/scope.rs +++ b/pyrefly/lib/binding/scope.rs @@ -740,6 +740,9 @@ struct FlowInfo { struct FlowValue { idx: Idx, style: FlowStyle, + /// True if every reaching value comes from a function definition, including decorated ones. + /// This survives merges that replace `FunctionDef` with an initialization status. + is_function_def: bool, } /// The most recent narrow for a name. @@ -751,7 +754,11 @@ struct FlowNarrow { impl FlowInfo { fn new_value(idx: Idx, style: FlowStyle) -> Self { Self { - value: Some(FlowValue { idx, style }), + value: Some(FlowValue { + idx, + is_function_def: matches!(style, FlowStyle::FunctionDef { .. }), + style, + }), narrow: None, narrow_depth: 0, loop_prior: idx, @@ -769,7 +776,11 @@ impl FlowInfo { fn updated_value(&self, idx: Idx, style: FlowStyle, in_loop: bool) -> Self { Self { - value: Some(FlowValue { idx, style }), + value: Some(FlowValue { + idx, + is_function_def: matches!(style, FlowStyle::FunctionDef { .. }), + style, + }), // Note that any existing narrow is wiped when a new value is bound. narrow: None, narrow_depth: 0, @@ -2602,6 +2613,7 @@ impl Scopes { let scope = self.current_mut(); if let Some(value) = scope.flow.get_value_mut(name) { value.style = FlowStyle::Uninitialized; + value.is_function_def = false; } scope.deleted_names.insert(name.clone()); } @@ -3350,6 +3362,8 @@ impl Scopes { }, }; let initialized = match &value.style { + // Conditional method definitions are exempt from attribute initialization checks. + _ if value.is_function_def => InitializedInFlow::Yes, FlowStyle::PossiblyUninitialized => InitializedInFlow::Conditionally, FlowStyle::MaybeInitialized(keys) => { InitializedInFlow::DeferredCheck(keys.clone()) @@ -4142,6 +4156,7 @@ impl<'a> BindingsBuilder<'a> { let mut branch_idxs = SmallSet::with_capacity(merge_branches.len()); let mut branch_infos = Vec::with_capacity(merge_branches.len()); let mut styles = Vec::with_capacity(merge_branches.len()); + let mut is_function_def = true; let mut n_values = 0; // Collect termination keys from branches that don't define the variable. // These will be used for deferred uninitialized checks at solve time. @@ -4184,6 +4199,7 @@ impl<'a> BindingsBuilder<'a> { } continue; } + is_function_def &= v.is_function_def; if value_idxs.insert(v.idx) { // An invariant in Pyrefly is that we only set style when we // set a value, so duplicate value_idxs always have the same style. @@ -4268,6 +4284,7 @@ impl<'a> BindingsBuilder<'a> { value: Some(FlowValue { idx: *value_idxs.first().unwrap(), style: compute_final_style(styles), + is_function_def, }), narrow: Some(FlowNarrow { idx: merged_idx }), narrow_depth: 1, @@ -4289,6 +4306,7 @@ impl<'a> BindingsBuilder<'a> { value: Some(FlowValue { idx: merged_idx, style: compute_final_style(styles), + is_function_def, }), narrow: None, narrow_depth: 0, @@ -4444,6 +4462,7 @@ impl<'a> BindingsBuilder<'a> { info.value = Some(FlowValue { idx: phi_idx, style: FlowStyle::LoopRecursion, + is_function_def: false, }); info.narrow = None; } diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index ab42453690..24ef631a32 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -33,6 +33,103 @@ Child.value "#, ); +testcase!( + test_conditionally_defined_methods, + r#" +from typing import assert_type + +def enabled() -> bool: ... +def fallback() -> None: ... + +class ConditionalMethods: + if enabled(): + def method(self) -> int: + return 1 + + async def async_method(self) -> int: + return 1 + + @staticmethod + def static_method() -> int: + return 1 + + @classmethod + def class_method(cls) -> int: + return 1 + + @property + def prop(self) -> int: + return 1 + + if enabled(): + def nested(self) -> int: + return 1 + else: + def nested(self) -> int: + return 2 + + while enabled(): + def loop_method(self) -> int: + return 1 + break + + if enabled(): + def deferred(self) -> int: + return 1 + else: + fallback() + + # A later merge must also preserve the method's definition source. + if enabled(): + data = 1 # E: Attribute `data` may be uninitialized + +assert_type(ConditionalMethods().method(), int) +assert_type(ConditionalMethods.static_method(), int) +assert_type(ConditionalMethods.class_method(), int) +assert_type(ConditionalMethods().prop, int) +assert_type(ConditionalMethods().nested(), int) +assert_type(ConditionalMethods().loop_method(), int) +assert_type(ConditionalMethods().deferred(), int) + +async def use_async() -> None: + assert_type(await ConditionalMethods().async_method(), int) +"#, +); + +testcase!( + test_conditional_method_assignment_initialization, + r#" +def enabled() -> bool: ... +def callback() -> int: + return 1 + +class CallableAttribute: + if enabled(): + value = callback # E: Attribute `value` may be uninitialized + +class ReassignedMethod: + if enabled(): + @staticmethod + def value() -> int: ... + value = callback # E: Attribute `value` may be uninitialized + +class MixedDefinitions: + if enabled(): + if enabled(): + @staticmethod + def value() -> int: ... + else: + value = callback # E: Attribute `value` may be uninitialized + +# Local function reads still require a definition on every reachable path. +def local() -> None: + if enabled(): + def value() -> int: + return 1 + value() # E: `value` may be uninitialized +"#, +); + // Test case for various edge cases where a name isn't in the flow, and we might // or might not decide an attribute has been defined. testcase!( From a01848b0a01b1796c4746f17cadbe33696de2db8 Mon Sep 17 00:00:00 2001 From: Asuka Minato Date: Thu, 8 Oct 2026 13:41:19 +0900 Subject: [PATCH 4/9] fix `and` `or` merge --- pyrefly/lib/alt/solve.rs | 8 +-- pyrefly/lib/binding/binding.rs | 12 ++-- pyrefly/lib/binding/bindings.rs | 19 +++++ pyrefly/lib/binding/class.rs | 47 ++++++------ pyrefly/lib/binding/django.rs | 4 +- pyrefly/lib/binding/scope.rs | 37 +++++----- pyrefly/lib/test/attributes.rs | 124 ++++++++++++++++++++++++++++++++ 7 files changed, 196 insertions(+), 55 deletions(-) diff --git a/pyrefly/lib/alt/solve.rs b/pyrefly/lib/alt/solve.rs index f84bc2cc74..89b4ac9a15 100644 --- a/pyrefly/lib/alt/solve.rs +++ b/pyrefly/lib/alt/solve.rs @@ -3020,11 +3020,11 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { name, range, getattr_class, - termination_keys, + termination_key_groups, } => { - let may_be_uninitialized = termination_keys - .as_ref() - .is_none_or(|keys| !keys.iter().all(|key| self.get_idx(*key).ty().is_never())); + let may_be_uninitialized = !termination_key_groups + .iter() + .any(|keys| keys.iter().all(|key| self.get_idx(*key).ty().is_never())); let has_getattr_fallback = getattr_class.is_some_and(|class_idx| { self.get_idx(class_idx) .0 diff --git a/pyrefly/lib/binding/binding.rs b/pyrefly/lib/binding/binding.rs index d3dc7a6206..c9632efd9e 100644 --- a/pyrefly/lib/binding/binding.rs +++ b/pyrefly/lib/binding/binding.rs @@ -1321,16 +1321,16 @@ pub enum BindingExpect { /// If any don't, the variable may be uninitialized. termination_keys: Vec>, }, - /// Check that a class attribute is initialized on every control-flow path. + /// Check attribute initialization from the class body or any defining method. UninitializedAttributeCheck { name: Name, range: TextRange, /// The owning class when this is an instance attribute. Its `__getattr__` may make a /// conditionally initialized attribute safe to access. getattr_class: Option>, - /// `None` means binding proved the declaration conditional. Otherwise, initialization - /// depends on whether all of these branches terminate with `Never`. - termination_keys: Option>>, + /// Initialization requires all keys in at least one group to have Never type. + /// No groups means the attribute may be uninitialized without any deferred conditions. + termination_key_groups: Vec>>, }, /// Check for forward reference string literal in union type. /// At runtime, `type.__or__` cannot handle string literals, so expressions @@ -1508,7 +1508,7 @@ impl DisplayWith for BindingExpect { name, range, getattr_class, - termination_keys, + termination_key_groups, } => { write!( f, @@ -1516,7 +1516,7 @@ impl DisplayWith for BindingExpect { name, ctx.module().display(range), getattr_class, - termination_keys + termination_key_groups ) } Self::ForwardRefUnion { diff --git a/pyrefly/lib/binding/bindings.rs b/pyrefly/lib/binding/bindings.rs index 4ba659de28..2ae601bc97 100644 --- a/pyrefly/lib/binding/bindings.rs +++ b/pyrefly/lib/binding/bindings.rs @@ -206,6 +206,25 @@ impl InitializedInFlow { } } +#[derive(Debug, Clone)] +pub enum InitializedInClass { + Yes, + /// Each group contains the termination keys required by one defining flow. + /// Initialization requires all keys in any one group to have Never type. + /// An empty list of groups means no flow guarantees initialization. + IfAny(Vec>>), +} + +impl From for InitializedInClass { + fn from(initialized: InitializedInFlow) -> Self { + match initialized { + InitializedInFlow::Yes => Self::Yes, + InitializedInFlow::Conditionally | InitializedInFlow::No => Self::IfAny(Vec::new()), + InitializedInFlow::DeferredCheck(keys) => Self::IfAny(vec![keys]), + } + } +} + pub type BindingEntry = (Index, IndexMap::Value>); table! { diff --git a/pyrefly/lib/binding/class.rs b/pyrefly/lib/binding/class.rs index 2f392e2438..103e2e0c0c 100644 --- a/pyrefly/lib/binding/class.rs +++ b/pyrefly/lib/binding/class.rs @@ -75,7 +75,7 @@ use crate::binding::binding::KeyVariance; use crate::binding::binding::MethodSelfKind; use crate::binding::bindings::BindingsBuilder; use crate::binding::bindings::CurrentIdx; -use crate::binding::bindings::InitializedInFlow; +use crate::binding::bindings::InitializedInClass; use crate::binding::bindings::LegacyTParamCollector; use crate::binding::expr::Usage; use crate::binding::pydantic::PydanticConfigDict; @@ -508,31 +508,26 @@ impl<'a> BindingsBuilder<'a> { ClassFieldDefinition::DefinedInMethod { method, .. } if !method.recognized_attribute_defining_method ); - if check_initialization { - let termination_keys = match &initialized { - InitializedInFlow::Yes => None, - InitializedInFlow::Conditionally | InitializedInFlow::No => Some(None), - InitializedInFlow::DeferredCheck(keys) => Some(Some(keys.clone())), - }; - if let Some(termination_keys) = termination_keys { - let getattr_class = matches!( - &definition, - ClassFieldDefinition::DefinedInMethod { - receiver_kind: MethodSelfKind::Instance, - .. - } - ) - .then_some(class_indices.class_idx); - self.insert_binding( - KeyExpect::UninitializedAttributeCheck(range), - BindingExpect::UninitializedAttributeCheck { - name: name.key().clone(), - range, - getattr_class, - termination_keys, - }, - ); - } + if check_initialization + && let InitializedInClass::IfAny(termination_key_groups) = initialized + { + let getattr_class = matches!( + &definition, + ClassFieldDefinition::DefinedInMethod { + receiver_kind: MethodSelfKind::Instance, + .. + } + ) + .then_some(class_indices.class_idx); + self.insert_binding( + KeyExpect::UninitializedAttributeCheck(range), + BindingExpect::UninitializedAttributeCheck { + name: name.key().clone(), + range, + getattr_class, + termination_key_groups, + }, + ); } fields.insert_hashed( diff --git a/pyrefly/lib/binding/django.rs b/pyrefly/lib/binding/django.rs index f0c87a2c27..3b2b67f83e 100644 --- a/pyrefly/lib/binding/django.rs +++ b/pyrefly/lib/binding/django.rs @@ -13,7 +13,7 @@ use starlark_map::small_map::SmallMap; use crate::binding::binding::ClassFieldDefinition; use crate::binding::binding::ExprOrBinding; use crate::binding::bindings::BindingsBuilder; -use crate::binding::bindings::InitializedInFlow; +use crate::binding::bindings::InitializedInClass; const PRIMARY_KEY: Name = Name::new_static("primary_key"); const FOREIGN_KEY: Name = Name::new_static("ForeignKey"); @@ -40,7 +40,7 @@ impl<'a> BindingsBuilder<'a> { /// (primary_key, ForeignKey, choices). pub fn extract_django_fields_from_class_body( &self, - field_definitions: &SmallMap, + field_definitions: &SmallMap, ) -> DjangoFieldInfo { let mut primary_key_field = None; let mut foreign_key_like_fields = Vec::new(); diff --git a/pyrefly/lib/binding/scope.rs b/pyrefly/lib/binding/scope.rs index 13a22049de..4ddc15d185 100644 --- a/pyrefly/lib/binding/scope.rs +++ b/pyrefly/lib/binding/scope.rs @@ -70,6 +70,7 @@ use crate::binding::binding::NarrowUseLocation; use crate::binding::bindings::BindingTable; use crate::binding::bindings::BindingsBuilder; use crate::binding::bindings::CurrentIdx; +use crate::binding::bindings::InitializedInClass; use crate::binding::bindings::InitializedInFlow; use crate::binding::expr::Usage; use crate::binding::function::SelfAssignments; @@ -2206,6 +2207,8 @@ impl Scopes { Some(InitializedInFlow::DeferredCheck(mut existing)), InitializedInFlow::DeferredCheck(mut new), ) => { + // All return paths must initialize the attribute. + // Combine their requirements with AND. existing.append(&mut new); InitializedInFlow::DeferredCheck(existing) } @@ -3252,7 +3255,7 @@ impl Scopes { pub fn finish_class_and_get_field_definitions( &mut self, ) -> ( - SmallMap, + SmallMap, NNModuleRegistrations, ) { let mut field_definitions = SmallMap::new(); @@ -3370,8 +3373,10 @@ impl Scopes { } _ => InitializedInFlow::Yes, }; - field_definitions - .insert_hashed(name.owned(), (definition, static_info.range, initialized)); + field_definitions.insert_hashed( + name.owned(), + (definition, static_info.range, initialized.into()), + ); } }); // Merge assignments from different methods. @@ -3413,22 +3418,20 @@ impl Scopes { } } // Initialization is merged independently of definition priority. - // An unconditional helper assignment also counts as initialization. + // Any method can establish initialization, so combine their requirements with OR. match (&mut *existing_initialized, initialized) { - (InitializedInFlow::Yes, _) | (_, InitializedInFlow::Yes) => { - *existing_initialized = InitializedInFlow::Yes; + (InitializedInClass::Yes, _) => {} + (_, InitializedInFlow::Yes) => { + *existing_initialized = InitializedInClass::Yes; } ( - InitializedInFlow::DeferredCheck(existing), - InitializedInFlow::DeferredCheck(mut new), - ) => existing.append(&mut new), - (InitializedInFlow::DeferredCheck(_), InitializedInFlow::Conditionally) - | (InitializedInFlow::Conditionally, InitializedInFlow::DeferredCheck(_)) - | (InitializedInFlow::Conditionally, InitializedInFlow::Conditionally) - | (InitializedInFlow::No, _) - | (_, InitializedInFlow::No) => { - *existing_initialized = InitializedInFlow::Conditionally; - } + InitializedInClass::IfAny(groups), + InitializedInFlow::DeferredCheck(keys), + ) => groups.push(keys), + ( + InitializedInClass::IfAny(_), + InitializedInFlow::Conditionally | InitializedInFlow::No, + ) => {} } } else if !field_definitions.contains_key_hashed(name.as_ref()) { field_definitions.insert_hashed( @@ -3441,7 +3444,7 @@ impl Scopes { receiver_kind, }, range, - initialized, + initialized.into(), ), ); } diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index 24ef631a32..5c168f6861 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -343,6 +343,130 @@ def f(a: A): "#, ); +testcase!( + test_attribute_initialization_deferred_methods, + r#" +from typing import Never + +def stop() -> Never: + raise RuntimeError() + +def proceed() -> None: ... + +class ConstructorTerminates: + def __init__(self, flag: bool) -> None: + if flag: + self.value = 1 + else: + stop() + + def initialize(self, flag: bool) -> None: + if flag: + self.value = 1 + else: + proceed() + +class HelperTerminates: + def __init__(self, flag: bool) -> None: + if flag: + self.value = 1 + else: + proceed() + + def initialize(self, flag: bool) -> None: + if flag: + self.value = 1 + else: + stop() + +class NeitherTerminates: + def __init__(self, flag: bool) -> None: + if flag: + self.value = 1 # E: Attribute `value` may be uninitialized + else: + proceed() + + def initialize(self, flag: bool) -> None: + if flag: + self.value = 1 + else: + proceed() + +class ConditionalHelper: + def __init__(self, flag: bool) -> None: + if flag: + self.value = 1 + else: + stop() + + def initialize(self, flag: bool) -> None: + if flag: + self.value = 1 + +class ConditionalConstructor: + def __init__(self, flag: bool) -> None: + if flag: + self.value = 1 + + def initialize(self, flag: bool) -> None: + if flag: + self.value = 1 + else: + stop() +"#, +); + +testcase!( + test_attribute_initialization_deferred_return_paths, + r#" +from typing import Never + +def stop() -> Never: + raise RuntimeError() + +def proceed() -> None: ... + +class AllPathsTerminate: + def __init__(self, early: bool, flag: bool) -> None: + if early: + if flag: + self.value = 1 + else: + stop() + return + if flag: + self.value = 1 + else: + stop() + +class EarlyPathContinues: + def __init__(self, early: bool, flag: bool) -> None: + if early: + if flag: + self.value = 1 # E: Attribute `value` may be uninitialized + else: + proceed() + return + if flag: + self.value = 1 + else: + stop() + +class FinalPathContinues: + def __init__(self, early: bool, flag: bool) -> None: + if early: + if flag: + self.value = 1 # E: Attribute `value` may be uninitialized + else: + stop() + return + if flag: + self.value = 1 + else: + proceed() +"#, +); + testcase!( test_inherited_attribute_in_unrecognized_method, r#" From d38b07d5236e9569ce38fb910ddf73cd301b3f6e Mon Sep 17 00:00:00 2001 From: Asuka Minato Date: Thu, 8 Oct 2026 13:47:49 +0900 Subject: [PATCH 5/9] new error msg --- crates/pyrefly_config/src/base.rs | 1 + crates/pyrefly_config/src/config.rs | 32 +++++++++++++++++++++ crates/pyrefly_config/src/error_kind.rs | 3 ++ pyrefly/lib/alt/solve.rs | 2 +- pyrefly/lib/test/attributes.rs | 38 +++++++++++++++++++++++++ pyrefly/lib/test/scope.rs | 1 + pyrefly/lib/test/util.rs | 10 +++++++ scripts/error_presets.json | 1 + website/docs/configuration.mdx | 2 +- website/docs/error-kinds.mdx | 30 +++++++++++++++++++ 10 files changed, 118 insertions(+), 2 deletions(-) diff --git a/crates/pyrefly_config/src/base.rs b/crates/pyrefly_config/src/base.rs index 8bb10b21e9..d8f8c5bfb8 100644 --- a/crates/pyrefly_config/src/base.rs +++ b/crates/pyrefly_config/src/base.rs @@ -200,6 +200,7 @@ impl Preset { (ErrorKind::MethodAssign, Severity::Error), (ErrorKind::MissingOverrideDecorator, Severity::Error), (ErrorKind::OpenUnpacking, Severity::Error), + (ErrorKind::PossiblyUninitializedAttribute, Severity::Error), (ErrorKind::PotentialBadKeywordArgument, Severity::Error), (ErrorKind::UnusedIgnore, Severity::Error), ]); diff --git a/crates/pyrefly_config/src/config.rs b/crates/pyrefly_config/src/config.rs index 7f8dc2a9ae..2de9423b05 100644 --- a/crates/pyrefly_config/src/config.rs +++ b/crates/pyrefly_config/src/config.rs @@ -3943,6 +3943,38 @@ output-format = "omit-errors" assert_eq!(config.root.strict_partial_subtyping, Some(true)); } + #[test] + fn test_possibly_uninitialized_attribute_configuration() { + for (source, initialization_severity, missing_severity) in [ + ("", Severity::Ignore, Severity::Error), + ("preset = 'strict'", Severity::Error, Severity::Error), + ( + "preset = 'strict'\n[errors]\npossibly-uninitialized-attribute = 'ignore'", + Severity::Ignore, + Severity::Error, + ), + ( + "preset = 'strict'\n[errors]\nmissing-attribute = 'ignore'", + Severity::Error, + Severity::Ignore, + ), + ] { + let mut config = ConfigFile::parse_config(source).unwrap(); + config.configure(); + let errors = config.root.errors.as_ref().unwrap(); + assert_eq!( + errors.severity(ErrorKind::PossiblyUninitializedAttribute), + initialization_severity, + "configuration: {source}", + ); + assert_eq!( + errors.severity(ErrorKind::MissingAttribute), + missing_severity, + "configuration: {source}", + ); + } + } + #[test] fn test_preset_off_silences_all_errors() { // The `off` preset silences every error kind and leaves all other diff --git a/crates/pyrefly_config/src/error_kind.rs b/crates/pyrefly_config/src/error_kind.rs index 5a9b46b081..c30d8e9be4 100644 --- a/crates/pyrefly_config/src/error_kind.rs +++ b/crates/pyrefly_config/src/error_kind.rs @@ -367,6 +367,8 @@ pub enum ErrorKind { /// A value whose type is not itself `Any` but contains an implicit `Any`, /// such as `list[Unknown]` or `Unknown | None`. PartiallyUnknownType, + /// A class or instance attribute may be uninitialized on some control-flow paths. + PossiblyUninitializedAttribute, /// A potential conflict between an explicit keyword argument and a NotRequired /// TypedDict field. The field may be absent at runtime, so the conflict is not /// guaranteed. This is a separate error code from BadKeywordArgument to allow @@ -634,6 +636,7 @@ impl ErrorKind { ErrorKind::NonConvergentRecursion => Severity::Warn, ErrorKind::NotRequiredKeyAccess => Severity::Ignore, ErrorKind::OpenUnpacking => Severity::Ignore, + ErrorKind::PossiblyUninitializedAttribute => Severity::Ignore, ErrorKind::PotentialBadKeywordArgument => Severity::Ignore, ErrorKind::PytorchEfficiencyLintCudaCall => Severity::Ignore, ErrorKind::PytorchEfficiencyLintItemCall => Severity::Ignore, diff --git a/pyrefly/lib/alt/solve.rs b/pyrefly/lib/alt/solve.rs index 89b4ac9a15..7a972f7691 100644 --- a/pyrefly/lib/alt/solve.rs +++ b/pyrefly/lib/alt/solve.rs @@ -3035,7 +3035,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { errors .error_builder( *range, - ErrorKind::MissingAttribute, + ErrorKind::PossiblyUninitializedAttribute, format!("Attribute `{name}` may be uninitialized"), ) .emit(); diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index 5c168f6861..d631d32fdc 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -10,6 +10,37 @@ use crate::test::util::TestEnv; use crate::testcase; +testcase!( + test_possibly_uninitialized_attribute_default_disabled, + r#" +def enabled() -> bool: ... + +class C: + if enabled(): + class_value = 1 + + def __init__(self, flag: bool) -> None: + if flag: + self.value = 1 + +C(True).missing # E: Object of class `C` has no attribute `missing` +"#, +); + +testcase!( + test_possibly_uninitialized_attribute_suppression, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + r#" +class C: + def __init__(self, flag: bool) -> None: + if flag: + self.suppressed = 1 # pyrefly: ignore[possibly-uninitialized-attribute] + self.reported = 1 # pyrefly: ignore[missing-attribute] # E: Attribute `reported` may be uninitialized + +C(True).missing # pyrefly: ignore[possibly-uninitialized-attribute] # E: Object of class `C` has no attribute `missing` +"#, +); + fn env_with_conditional_attribute() -> TestEnv { TestEnv::one( "foo", @@ -20,6 +51,7 @@ class Base: value = 1 # E: Attribute `value` may be uninitialized "#, ) + .enable_possibly_uninitialized_attribute_error() } testcase!( @@ -35,6 +67,7 @@ Child.value testcase!( test_conditionally_defined_methods, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), r#" from typing import assert_type @@ -98,6 +131,7 @@ async def use_async() -> None: testcase!( test_conditional_method_assignment_initialization, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), r#" def enabled() -> bool: ... def callback() -> int: @@ -134,6 +168,7 @@ def local() -> None: // or might not decide an attribute has been defined. testcase!( test_semantics_for_when_class_body_defines_attributes, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), r#" from typing import assert_type, Any def condition() -> bool: ... @@ -284,6 +319,7 @@ def f(a: A): testcase!( test_conditional_attribute_initialized_in_helper, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), r#" from typing import assert_type @@ -345,6 +381,7 @@ def f(a: A): testcase!( test_attribute_initialization_deferred_methods, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), r#" from typing import Never @@ -418,6 +455,7 @@ class ConditionalConstructor: testcase!( test_attribute_initialization_deferred_return_paths, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), r#" from typing import Never diff --git a/pyrefly/lib/test/scope.rs b/pyrefly/lib/test/scope.rs index 72c226f6b2..9390d7979f 100644 --- a/pyrefly/lib/test/scope.rs +++ b/pyrefly/lib/test/scope.rs @@ -1700,6 +1700,7 @@ def f(): // https://github.com/facebook/pyrefly/issues/4318 testcase!( test_conditionally_defined_class_member, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), r#" def coin() -> bool: return True diff --git a/pyrefly/lib/test/util.rs b/pyrefly/lib/test/util.rs index 66376f2e42..de6c01d3b3 100644 --- a/pyrefly/lib/test/util.rs +++ b/pyrefly/lib/test/util.rs @@ -258,6 +258,7 @@ pub struct TestEnv { site_package_path: Vec, implicitly_defined_attribute_error: bool, uninitialized_instance_variable_error: bool, + possibly_uninitialized_attribute_error: bool, explicit_any_error: bool, explicit_any_use_error: bool, implicit_any_error: bool, @@ -332,6 +333,7 @@ impl TestEnv { site_package_path: Vec::new(), implicitly_defined_attribute_error: false, uninitialized_instance_variable_error: false, + possibly_uninitialized_attribute_error: false, explicit_any_error: false, explicit_any_use_error: false, implicit_any_error: false, @@ -489,6 +491,11 @@ impl TestEnv { self } + pub fn enable_possibly_uninitialized_attribute_error(mut self) -> Self { + self.possibly_uninitialized_attribute_error = true; + self + } + pub fn enable_explicit_any_error(mut self) -> Self { self.explicit_any_error = true; self @@ -873,6 +880,9 @@ impl TestEnv { if self.uninitialized_instance_variable_error { errors.set_error_severity(ErrorKind::UninitializedInstanceVariable, Severity::Error); } + if self.possibly_uninitialized_attribute_error { + errors.set_error_severity(ErrorKind::PossiblyUninitializedAttribute, Severity::Error); + } if self.explicit_any_error { errors.set_error_severity(ErrorKind::ExplicitAny, Severity::Error); } diff --git a/scripts/error_presets.json b/scripts/error_presets.json index bdd0cb6a7a..120b69a920 100644 --- a/scripts/error_presets.json +++ b/scripts/error_presets.json @@ -109,6 +109,7 @@ "open-unpacking": ["strict", "all"], "parse-error": ["basic", "legacy", "default", "strict", "all"], "partially-unknown-type": ["all"], + "possibly-uninitialized-attribute": ["strict", "all"], "potential-bad-keyword-argument": ["strict", "all"], "protocol-implicitly-defined-attribute": ["legacy", "default", "strict", "all"], "pytorch-efficiency-lint-cuda-call": ["all"], diff --git a/website/docs/configuration.mdx b/website/docs/configuration.mdx index 5fd717ac67..d913f26338 100644 --- a/website/docs/configuration.mdx +++ b/website/docs/configuration.mdx @@ -838,7 +838,7 @@ The default Pyrefly configuration. Equivalent to having no preset at all. Enables additional error codes on top of the default for stricter checking. - Sets [`strict-callable-subtyping`](#strict-callable-subtyping) `= true` -- Enables (as errors): [`direct-abstract-base-instantiation`](./error-kinds.mdx#direct-abstract-base-instantiation), [`implicit-any`](./error-kinds.mdx#implicit-any) (covers every implicit-`Any` sub-kind), [`method-assign`](./error-kinds.mdx#method-assign), [`missing-override-decorator`](./error-kinds.mdx#missing-override-decorator), [`open-unpacking`](./error-kinds.mdx#open-unpacking), [`potential-bad-keyword-argument`](./error-kinds.mdx#potential-bad-keyword-argument), [`unused-ignore`](./error-kinds.mdx#unused-ignore) +- Enables (as errors): [`direct-abstract-base-instantiation`](./error-kinds.mdx#direct-abstract-base-instantiation), [`implicit-any`](./error-kinds.mdx#implicit-any) (covers every implicit-`Any` sub-kind), [`method-assign`](./error-kinds.mdx#method-assign), [`missing-override-decorator`](./error-kinds.mdx#missing-override-decorator), [`open-unpacking`](./error-kinds.mdx#open-unpacking), [`possibly-uninitialized-attribute`](./error-kinds.mdx#possibly-uninitialized-attribute), [`potential-bad-keyword-argument`](./error-kinds.mdx#potential-bad-keyword-argument), [`unused-ignore`](./error-kinds.mdx#unused-ignore) #### Preset: `all` diff --git a/website/docs/error-kinds.mdx b/website/docs/error-kinds.mdx index 183a433f7b..6dc4ad3765 100644 --- a/website/docs/error-kinds.mdx +++ b/website/docs/error-kinds.mdx @@ -1813,6 +1813,36 @@ def untyped(p): # the parameter is an implicit `Any` v = [untyped(1)] # `v` has type `list[Unknown]`: partially unknown ``` +## possibly-uninitialized-attribute + +Default severity: `ignore` + +The `strict` preset enables this check as an error. + +An attribute may be uninitialized after class-body execution or instance initialization. +Pyrefly reports this error at the attribute definition, even if the attribute is never read. +A later `hasattr` guard does not suppress this check. +Conditional method definitions are excluded. + +```python +class C: + def __init__(self, value: int | None) -> None: + if value is not None: + self.value = value # possibly-uninitialized-attribute +``` + +To disable this check while keeping other strict checks, use: + +```toml +preset = "strict" + +[errors] +possibly-uninitialized-attribute = "ignore" +``` + +This setting does not disable [`missing-attribute`](#missing-attribute). +The separate [`uninitialized-instance-variable`](#uninitialized-instance-variable) check reports class-body instance declarations that have no initialization. + ## potential-bad-keyword-argument Default severity: `ignore` From a4058d30f887b5abfaf5d470e61aa4b8e5a64ce6 Mon Sep 17 00:00:00 2001 From: Asuka Minato Date: Thu, 8 Oct 2026 13:50:20 +0900 Subject: [PATCH 6/9] skip pyi check --- pyrefly/lib/binding/class.rs | 4 +++- pyrefly/lib/test/attributes.rs | 43 ++++++++++++++++++++++++++++++++++ website/docs/error-kinds.mdx | 1 + 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/pyrefly/lib/binding/class.rs b/pyrefly/lib/binding/class.rs index 103e2e0c0c..081cfa6ce9 100644 --- a/pyrefly/lib/binding/class.rs +++ b/pyrefly/lib/binding/class.rs @@ -508,7 +508,9 @@ impl<'a> BindingsBuilder<'a> { ClassFieldDefinition::DefinedInMethod { method, .. } if !method.recognized_attribute_defining_method ); - if check_initialization + // Stub files declare attributes without runtime initialization. + if !self.module_info.path().is_interface() + && check_initialization && let InitializedInClass::IfAny(termination_key_groups) = initialized { let getattr_class = matches!( diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index d631d32fdc..76db2cb214 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -41,6 +41,49 @@ C(True).missing # pyrefly: ignore[possibly-uninitialized-attribute] # E: Objec "#, ); +testcase!( + test_possibly_uninitialized_attribute_stub, + TestEnv::one_with_path( + "foo", + "foo.pyi", + r#" +def enabled() -> bool: ... + +class Stub: + if enabled(): + value = 1 + annotated: int = 1 + + def __init__(self, flag: bool) -> None: + if flag: + self.instance_value = 1 +"#, + ) + .enable_possibly_uninitialized_attribute_error(), + r#" +from typing import assert_type +from foo import Stub + +assert_type(Stub.value, int) +assert_type(Stub.annotated, int) +assert_type(Stub(True).instance_value, int) + +class Child(Stub): ... + +assert_type(Child.value, int) + +def enabled() -> bool: ... + +class Implementation: + if enabled(): + value = 1 # E: Attribute `value` may be uninitialized + + def __init__(self, flag: bool) -> None: + if flag: + self.instance_value = 1 # E: Attribute `instance_value` may be uninitialized +"#, +); + fn env_with_conditional_attribute() -> TestEnv { TestEnv::one( "foo", diff --git a/website/docs/error-kinds.mdx b/website/docs/error-kinds.mdx index 6dc4ad3765..19235ad3ed 100644 --- a/website/docs/error-kinds.mdx +++ b/website/docs/error-kinds.mdx @@ -1823,6 +1823,7 @@ An attribute may be uninitialized after class-body execution or instance initial Pyrefly reports this error at the attribute definition, even if the attribute is never read. A later `hasattr` guard does not suppress this check. Conditional method definitions are excluded. +This check does not apply to stub (`.pyi`) files because they do not perform runtime initialization. ```python class C: From 1d506ff90c832d5ec5cecd77cafb78e5b92a43f8 Mon Sep 17 00:00:00 2001 From: Asuka Minato Date: Thu, 8 Oct 2026 14:43:58 +0900 Subject: [PATCH 7/9] add bench, fix some perf fix comments --- pyrefly/Cargo.toml | 4 + pyrefly/benches/README.md | 27 ++ pyrefly/benches/attribute_initialization.rs | 103 +++++ pyrefly/benches/micro.rs | 44 ++ pyrefly/lib/alt/solve.rs | 31 +- pyrefly/lib/binding/binding.rs | 24 +- pyrefly/lib/binding/bindings.rs | 88 +++- pyrefly/lib/binding/class.rs | 48 ++- pyrefly/lib/binding/function.rs | 18 +- pyrefly/lib/binding/scope.rs | 432 ++++++++------------ pyrefly/lib/binding/stmt.rs | 21 +- pyrefly/lib/binding/target.rs | 10 +- pyrefly/lib/test/attributes.rs | 396 ++++++++++++++++++ pyrefly/lib/test/flow_branching.rs | 6 +- pyrefly/lib/test/scope.rs | 2 +- website/docs/error-kinds.mdx | 23 +- 16 files changed, 944 insertions(+), 333 deletions(-) create mode 100644 pyrefly/benches/attribute_initialization.rs diff --git a/pyrefly/Cargo.toml b/pyrefly/Cargo.toml index 964cd7e76c..9115d94889 100644 --- a/pyrefly/Cargo.toml +++ b/pyrefly/Cargo.toml @@ -28,6 +28,10 @@ harness = false name = "micro" harness = false +[[bench]] +name = "attribute_initialization" +harness = false + [[bench]] name = "pytorch" harness = false diff --git a/pyrefly/benches/README.md b/pyrefly/benches/README.md index 313496e339..b595d914ac 100644 --- a/pyrefly/benches/README.md +++ b/pyrefly/benches/README.md @@ -135,6 +135,33 @@ lands at the fbsource repo root `target/criterion/`. It is throwaway — do not commit it, and do not add repo-root ignore entries (it is already gitignored appropriately). +## Attribute initialization + +These synthetic benchmarks measure the cost of tracking instance attribute +initialization. They do not require a PyTorch checkout. + +- `micro` includes the `attribute_initialization` group. It varies the number + of attributes and branch joins, loop joins, or early returns independently. + Each iteration checks an in-memory module with the standard library already loaded. +- `attribute_initialization` drives a warm LSP server with three worker threads. + It edits a file containing a constructor with 64 attributes and 64 loops with branches, + then waits for diagnostics. Each edit alternates between zero and one error + to ensure that the response belongs to the new edit. Initialization checks + are enabled. Server setup and the first check are outside the measured region. + +Build both targets before measuring. Run them sequentially, with no concurrent +builds or tests: + +```bash +cargo bench -p pyrefly --bench micro --bench attribute_initialization --no-run +cargo bench -p pyrefly --bench micro -- attribute_initialization --noplot +cargo bench -p pyrefly --bench attribute_initialization -- --noplot +``` + +Use Criterion's `--save-baseline before` on the original version and +`--baseline before` on the changed version to compare results. Synthetic +results isolate this code path. They do not establish whole-project LSP performance. + ## TSP benchmark `benches/tsp.rs` -- buck `tsp_bench`, cargo bench `tsp`, Criterion id diff --git a/pyrefly/benches/attribute_initialization.rs b/pyrefly/benches/attribute_initialization.rs new file mode 100644 index 0000000000..bbe1f44919 --- /dev/null +++ b/pyrefly/benches/attribute_initialization.rs @@ -0,0 +1,103 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +//! Measure edits to a file containing a constructor with many attributes and joins. +//! Server initialization and the first check are outside the measured region. + +use std::fmt::Write as _; +use std::fs; + +use criterion::Criterion; +use criterion::criterion_group; +use criterion::criterion_main; +use lsp_types::DidChangeTextDocumentNotification; +use lsp_types::Uri; +use pyrefly::commands::lsp::IndexingMode; +use pyrefly::commands::lsp::LspArgs; +use pyrefly_lsp_test::object_model::InitializeSettings; +use pyrefly_lsp_test::object_model::LspInteraction; +use pyrefly_lsp_test::object_model::LspInteractionArgs; +use pyrefly_util::telemetry::NoTelemetry; +use pyrefly_util::thread_pool::TEST_THREAD_COUNT; +use serde_json::json; +use tempfile::tempdir; + +fn attribute_initialization_edit(c: &mut Criterion) { + let dir = tempdir().unwrap(); + fs::write( + dir.path().join("pyrefly.toml"), + "[errors]\npossibly-uninitialized-attribute = 'error'\n", + ) + .unwrap(); + let path = dir.path().join("constructor.py"); + let mut source = + String::from("class C:\n def __init__(self, flag: bool, values: list[int]) -> None:\n"); + for i in 0..64 { + let _ = writeln!(source, " self.attribute_{i}: int = 0"); + } + for i in 0..64 { + let _ = writeln!( + source, + " for value in values:\n if flag:\n self.attribute_{i} = value" + ); + } + let with_error = format!("{source}\nresult: int = 'error'\n"); + fs::write(&path, &source).unwrap(); + let mut interaction = LspInteraction::new_with_args(LspInteractionArgs { + args: LspArgs { + indexing_mode: IndexingMode::None, + workspace_indexing_limit: 50, + build_system_blocking: false, + }, + telemetry: Box::new(NoTelemetry), + thread_count: TEST_THREAD_COUNT, + thrift_remapper: None, + }); + interaction.set_root(dir.path().to_path_buf()); + interaction + .initialize(InitializeSettings { + configuration: Some(Some(json!([ + {"pyrefly": {"displayTypeErrors": "force-on"}} + ]))), + ..Default::default() + }) + .unwrap(); + interaction.client.did_open("constructor.py"); + interaction + .client + .expect_publish_diagnostics_eventual_error_count(path.clone(), 0) + .unwrap(); + + let mut version = 1; + c.bench_function("attribute_initialization/edit_to_diagnostics", |b| { + b.iter(|| { + version += 1; + let has_error = version % 2 == 0; + let text = if has_error { &with_error } else { &source }; + interaction + .client + .send_notification::(json!({ + "textDocument": { + "uri": Uri::from_file_path(&path).unwrap(), + "version": version, + }, + "contentChanges": [{"text": text}], + })); + // Alternating the error count ensures that each edit completes. + interaction + .client + .expect_publish_diagnostics_eventual_error_count( + path.clone(), + usize::from(has_error), + ) + .unwrap(); + }); + }); +} + +criterion_group!(benches, attribute_initialization_edit); +criterion_main!(benches); diff --git a/pyrefly/benches/micro.rs b/pyrefly/benches/micro.rs index 5e76cfac0e..cdb4a1b13e 100644 --- a/pyrefly/benches/micro.rs +++ b/pyrefly/benches/micro.rs @@ -453,6 +453,49 @@ fn measure_module( } /// Smoke benchmark validating the harness end-to-end. +/// Attribute initialization must track branches without constructing unused type joins. +fn attribute_initialization(c: &mut Criterion) { + let mut group = c.benchmark_group("attribute_initialization"); + for (attributes, joins) in [(16, 16), (64, 16), (16, 64), (64, 64)] { + for shape in ["branches", "loops", "returns"] { + let mut source = String::from( + "class C:\n def __init__(self, flag: bool, values: list[int]) -> None:\n", + ); + for i in 0..attributes { + let _ = writeln!(source, " self.attribute_{i}: int = 0"); + } + for i in 0..joins { + let attribute = i % attributes; + match shape { + "branches" => { + let _ = writeln!( + source, + " if flag:\n self.attribute_{attribute} = {i}\n else:\n self.attribute_{attribute} = -{i}" + ); + } + "loops" => { + let _ = writeln!( + source, + " for value in values:\n self.attribute_{attribute} = value" + ); + } + "returns" => { + let _ = writeln!(source, " if flag:\n return"); + } + _ => unreachable!("all benchmark shapes are listed above"), + } + } + let name = format!("{shape}_{attributes}_attributes_{joins}_joins"); + let code = Arc::new(FileContents::from_source(source)); + assert_eq!(check_module(code.dupe(), "bench", BENCH_FILE), 0, "{name}"); + group.bench_function(name, |b| { + b.iter(|| check_module(code.dupe(), "bench", BENCH_FILE)) + }); + } + } + group.finish(); +} + fn smoke(c: &mut Criterion) { measure(c, "smoke", "x: int = 1".to_owned(), 0); } @@ -957,6 +1000,7 @@ fn suggestion_nested(c: &mut Criterion) { criterion_group!( benches, + attribute_initialization, smoke, enum_members, enum_value_access, diff --git a/pyrefly/lib/alt/solve.rs b/pyrefly/lib/alt/solve.rs index 7a972f7691..c72b45d37b 100644 --- a/pyrefly/lib/alt/solve.rs +++ b/pyrefly/lib/alt/solve.rs @@ -3019,19 +3019,28 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { BindingExpect::UninitializedAttributeCheck { name, range, - getattr_class, - termination_key_groups, + class, + is_instance, + initializations, } => { - let may_be_uninitialized = !termination_key_groups - .iter() - .any(|keys| keys.iter().all(|key| self.get_idx(*key).ty().is_never())); - let has_getattr_fallback = getattr_class.is_some_and(|class_idx| { - self.get_idx(class_idx) - .0 - .as_ref() - .is_some_and(|cls| self.get_class_member(cls, &dunder::GETATTR).is_some()) + let initialized = initializations.iter().any(|initialized| { + initialized.is_initialized(|key| self.get_idx(key).ty().is_never()) }); - if may_be_uninitialized && !has_getattr_fallback { + let inherited_or_dynamic = !initialized + && self.get_idx(*class).0.as_ref().is_some_and(|cls| { + (*is_instance && self.get_class_member(cls, &dunder::GETATTR).is_some()) + || self + .get_mro_for_class(cls) + .ancestors_no_object() + .iter() + .any(|base| { + base.class_object() != cls + && self + .get_class_member(base.class_object(), name) + .is_some() + }) + }); + if !initialized && !inherited_or_dynamic { errors .error_builder( *range, diff --git a/pyrefly/lib/binding/binding.rs b/pyrefly/lib/binding/binding.rs index c9632efd9e..ab1346bd61 100644 --- a/pyrefly/lib/binding/binding.rs +++ b/pyrefly/lib/binding/binding.rs @@ -77,6 +77,7 @@ use crate::binding::base_class::BaseClassGeneric; use crate::binding::bindings::BindingEntry; use crate::binding::bindings::BindingTable; use crate::binding::bindings::Bindings; +use crate::binding::bindings::InitializationStatus; use crate::binding::django::DjangoFieldInfo; use crate::binding::narrow::NarrowOp; use crate::binding::narrow::NarrowingSubject; @@ -1325,12 +1326,11 @@ pub enum BindingExpect { UninitializedAttributeCheck { name: Name, range: TextRange, - /// The owning class when this is an instance attribute. Its `__getattr__` may make a - /// conditionally initialized attribute safe to access. - getattr_class: Option>, - /// Initialization requires all keys in at least one group to have Never type. - /// No groups means the attribute may be uninitialized without any deferred conditions. - termination_key_groups: Vec>>, + class: Idx, + /// Instance attributes can also be provided by `__getattr__`. + is_instance: bool, + /// Any defining method can establish initialization on all its returning paths. + initializations: Vec, }, /// Check for forward reference string literal in union type. /// At runtime, `type.__or__` cannot handle string literals, so expressions @@ -1507,16 +1507,18 @@ impl DisplayWith for BindingExpect { Self::UninitializedAttributeCheck { name, range, - getattr_class, - termination_key_groups, + class, + is_instance, + initializations, } => { write!( f, - "UninitializedAttributeCheck({}, {}, {:?}, {:?})", + "UninitializedAttributeCheck({}, {}, {:?}, {}, {:?})", name, ctx.module().display(range), - getattr_class, - termination_key_groups + class, + is_instance, + initializations ) } Self::ForwardRefUnion { diff --git a/pyrefly/lib/binding/bindings.rs b/pyrefly/lib/binding/bindings.rs index 2ae601bc97..3d14f9a60e 100644 --- a/pyrefly/lib/binding/bindings.rs +++ b/pyrefly/lib/binding/bindings.rs @@ -206,21 +206,91 @@ impl InitializedInFlow { } } +/// Initialization on every path through a control-flow merge. +#[derive(Debug, Clone)] +pub enum InitializationStatus { + Initialized, + Uninitialized, + /// Every missing path must terminate. Any key in a path can prove termination. + Deferred(Vec>>), +} + +impl InitializationStatus { + /// Combine paths with AND. A trailing Never expression can terminate a missing path. + pub fn merge<'a>( + paths: impl IntoIterator, Option>)>, + ) -> Self { + let mut conditions = Vec::new(); + for (initialized, termination_key) in paths { + match initialized { + Some(Self::Initialized) => {} + Some(Self::Deferred(paths)) => { + for path in paths { + let mut path = path.clone(); + if let Some(key) = termination_key { + path.push(key); + } + path.sort_unstable(); + path.dedup(); + conditions.push(path); + } + } + None | Some(Self::Uninitialized) => { + if let Some(key) = termination_key { + conditions.push(vec![key]); + } else { + return Self::Uninitialized; + } + } + } + } + if conditions.is_empty() { + Self::Initialized + } else { + // Forks can copy the same requirement into several paths. + conditions.sort_unstable(); + conditions.dedup(); + Self::Deferred(conditions) + } + } + + pub fn is_initialized(&self, is_never: impl Fn(Idx) -> bool) -> bool { + match self { + Self::Initialized => true, + Self::Uninitialized => false, + Self::Deferred(paths) => paths + .iter() + .all(|path| path.iter().any(|key| is_never(*key))), + } + } +} + +impl From for InitializationStatus { + fn from(initialized: InitializedInFlow) -> Self { + match initialized { + InitializedInFlow::Yes => Self::Initialized, + InitializedInFlow::Conditionally | InitializedInFlow::No => Self::Uninitialized, + InitializedInFlow::DeferredCheck(keys) => { + Self::Deferred(keys.into_iter().map(|key| vec![key]).collect()) + } + } + } +} + #[derive(Debug, Clone)] pub enum InitializedInClass { Yes, - /// Each group contains the termination keys required by one defining flow. - /// Initialization requires all keys in any one group to have Never type. - /// An empty list of groups means no flow guarantees initialization. - IfAny(Vec>>), + /// Any defining method or the class body can establish initialization. + /// An empty list means no flow guarantees initialization. + IfAny(Vec), } -impl From for InitializedInClass { - fn from(initialized: InitializedInFlow) -> Self { +impl From for InitializedInClass { + fn from(initialized: InitializationStatus) -> Self { match initialized { - InitializedInFlow::Yes => Self::Yes, - InitializedInFlow::Conditionally | InitializedInFlow::No => Self::IfAny(Vec::new()), - InitializedInFlow::DeferredCheck(keys) => Self::IfAny(vec![keys]), + InitializationStatus::Initialized => Self::Yes, + InitializationStatus::Uninitialized => Self::IfAny(Vec::new()), + deferred @ InitializationStatus::Deferred(_) => Self::IfAny(vec![deferred]), } } } diff --git a/pyrefly/lib/binding/class.rs b/pyrefly/lib/binding/class.rs index 081cfa6ce9..6b05d27e2c 100644 --- a/pyrefly/lib/binding/class.rs +++ b/pyrefly/lib/binding/class.rs @@ -31,6 +31,7 @@ use ruff_python_ast::name::Name; use ruff_text_size::Ranged; use ruff_text_size::TextRange; use starlark_map::small_map::SmallMap; +use starlark_map::small_set::SmallSet; use crate::binding::attrs::AttrsDecoratorMethods; use crate::binding::attrs::collect_attrs_decorator_methods; @@ -508,26 +509,61 @@ impl<'a> BindingsBuilder<'a> { ClassFieldDefinition::DefinedInMethod { method, .. } if !method.recognized_attribute_defining_method ); + // Inspect definition bindings here so method provenance does not occupy every flow value. + let is_conditional_method = + if let ClassFieldDefinition::DefinedWithoutAssign { definition } = &definition { + let mut pending = vec![*definition]; + let mut seen = SmallSet::new(); + let mut found_function = false; + let mut only_functions = true; + while let Some(idx) = pending.pop() { + if !seen.insert(idx) { + continue; + } + match self + .get_original_binding(idx) + .and_then(|(_, binding)| binding) + { + Some(Binding::Function { .. }) => found_function = true, + Some(Binding::Phi(_, branches)) => { + pending.extend(branches.iter().map(|branch| branch.value_key)); + } + Some(Binding::Narrow(prior, _, _)) => pending.push(*prior), + Some(Binding::LoopPhi(phi)) => { + pending.push(phi.0); + pending.extend(phi.1.iter().copied()); + } + _ => { + only_functions = false; + break; + } + } + } + found_function && only_functions + } else { + false + }; // Stub files declare attributes without runtime initialization. if !self.module_info.path().is_interface() && check_initialization - && let InitializedInClass::IfAny(termination_key_groups) = initialized + && !is_conditional_method + && let InitializedInClass::IfAny(initializations) = initialized { - let getattr_class = matches!( + let is_instance = matches!( &definition, ClassFieldDefinition::DefinedInMethod { receiver_kind: MethodSelfKind::Instance, .. } - ) - .then_some(class_indices.class_idx); + ); self.insert_binding( KeyExpect::UninitializedAttributeCheck(range), BindingExpect::UninitializedAttributeCheck { name: name.key().clone(), range, - getattr_class, - termination_key_groups, + class: class_indices.class_idx, + is_instance, + initializations, }, ); } diff --git a/pyrefly/lib/binding/function.rs b/pyrefly/lib/binding/function.rs index 2a0e31c657..0d301f2090 100644 --- a/pyrefly/lib/binding/function.rs +++ b/pyrefly/lib/binding/function.rs @@ -70,7 +70,7 @@ use crate::binding::binding::ReturnImplicit; use crate::binding::binding::ReturnType; use crate::binding::binding::ReturnTypeKind; use crate::binding::bindings::BindingsBuilder; -use crate::binding::bindings::InitializedInFlow; +use crate::binding::bindings::InitializationStatus; use crate::binding::bindings::LegacyTParamCollector; use crate::binding::expr::Usage; use crate::binding::pattern::match_case_always_matches; @@ -173,7 +173,7 @@ impl<'a, 'b> SourceOrderVisitor<'a> for SuperMethodCallFinder<'b> { pub struct SelfAssignments { pub method_name: Name, - pub instance_attributes: SmallMap, + pub instance_attributes: SmallMap, pub nn_module_registrations: NNModuleRegistrations, } @@ -266,12 +266,14 @@ impl<'a> SelfAttrNames<'a> { .map(|(n, r)| { ( n, - InstanceAttribute( - vec![ExprOrBinding::Binding(Binding::Any(AnyStyle::Implicit))], - None, - r, - MethodSelfKind::Instance, - InitializedInFlow::Yes, + ( + InstanceAttribute( + vec![ExprOrBinding::Binding(Binding::Any(AnyStyle::Implicit))], + None, + r, + MethodSelfKind::Instance, + ), + InitializationStatus::Initialized, ), ) }) diff --git a/pyrefly/lib/binding/scope.rs b/pyrefly/lib/binding/scope.rs index 4ddc15d185..2895def640 100644 --- a/pyrefly/lib/binding/scope.rs +++ b/pyrefly/lib/binding/scope.rs @@ -70,6 +70,7 @@ use crate::binding::binding::NarrowUseLocation; use crate::binding::bindings::BindingTable; use crate::binding::bindings::BindingsBuilder; use crate::binding::bindings::CurrentIdx; +use crate::binding::bindings::InitializationStatus; use crate::binding::bindings::InitializedInClass; use crate::binding::bindings::InitializedInFlow; use crate::binding::expr::Usage; @@ -631,6 +632,9 @@ impl TerminationKind { #[derive(Default, Clone, Debug)] pub struct Flow { info: SmallMap, + /// Attribute initialization has no type to merge and needs no Phi bindings. + /// An absent entry means the attribute is not initialized on this path. + instance_attribute_initialization: SmallMap, // Have we seen control flow terminate? // // We continue to analyze the rest of the code after a flow terminates, but @@ -741,9 +745,6 @@ struct FlowInfo { struct FlowValue { idx: Idx, style: FlowStyle, - /// True if every reaching value comes from a function definition, including decorated ones. - /// This survives merges that replace `FunctionDef` with an initialization status. - is_function_def: bool, } /// The most recent narrow for a name. @@ -755,11 +756,7 @@ struct FlowNarrow { impl FlowInfo { fn new_value(idx: Idx, style: FlowStyle) -> Self { Self { - value: Some(FlowValue { - idx, - is_function_def: matches!(style, FlowStyle::FunctionDef { .. }), - style, - }), + value: Some(FlowValue { idx, style }), narrow: None, narrow_depth: 0, loop_prior: idx, @@ -777,11 +774,7 @@ impl FlowInfo { fn updated_value(&self, idx: Idx, style: FlowStyle, in_loop: bool) -> Self { Self { - value: Some(FlowValue { - idx, - is_function_def: matches!(style, FlowStyle::FunctionDef { .. }), - style, - }), + value: Some(FlowValue { idx, style }), // Note that any existing narrow is wiped when a new value is bound. narrow: None, narrow_depth: 0, @@ -1060,8 +1053,10 @@ pub struct ClassIndices { struct ScopeClass { name: Identifier, indices: ClassIndices, - attributes_from_recognized_methods: SmallMap>, - attributes_from_other_methods: SmallMap>, + attributes_from_recognized_methods: + SmallMap>, + attributes_from_other_methods: + SmallMap>, nn_module_registrations: NNModuleRegistrations, has_protocol_base: bool, } @@ -1081,7 +1076,7 @@ impl ScopeClass { pub fn add_attributes_defined_by_method( &mut self, method_name: Name, - attributes: SmallMap, + attributes: SmallMap, nn_module_registrations: NNModuleRegistrations, ) { if is_attribute_defining_method(&method_name, &self.name.id) { @@ -1110,29 +1105,43 @@ impl ScopeClass { /// unrecognized methods, and that constructors take precedence. pub fn method_defined_attributes( self, - ) -> impl Iterator, MethodThatSetsAttr, InstanceAttribute)> { + ) -> impl Iterator< + Item = ( + Hashed, + MethodThatSetsAttr, + (InstanceAttribute, InitializationStatus), + ), + > { Self::iter_attributes(self.attributes_from_recognized_methods, true).chain( Self::iter_attributes(self.attributes_from_other_methods, false), ) } fn iter_attributes( - attrs: SmallMap>, + attrs: SmallMap>, recognized_attribute_defining_method: bool, - ) -> impl Iterator, MethodThatSetsAttr, InstanceAttribute)> { + ) -> impl Iterator< + Item = ( + Hashed, + MethodThatSetsAttr, + (InstanceAttribute, InitializationStatus), + ), + > { { attrs.into_iter().flat_map(move |(method_name, attrs)| { - attrs.into_iter_hashed().map(move |(name, attr)| { - ( - name, - MethodThatSetsAttr { - method_name: method_name.clone(), - recognized_attribute_defining_method, - instance_or_class: attr.3, - }, - attr, - ) - }) + attrs + .into_iter_hashed() + .map(move |(name, (attr, initialized))| { + ( + name, + MethodThatSetsAttr { + method_name: method_name.clone(), + recognized_attribute_defining_method, + instance_or_class: attr.3, + }, + (attr, initialized), + ) + }) }) } } @@ -1180,22 +1189,15 @@ pub struct InstanceAttribute( pub Option>, pub TextRange, pub MethodSelfKind, - pub InitializedInFlow, ); -fn instance_attribute_flow_name(name: &Name) -> Name { - // This cannot collide with a Python identifier, but lets attribute initialization reuse - // the same branch-merging machinery as local variables. - Name::new(format!("$self.{name}")) -} - #[derive(Clone, Debug)] struct ScopeMethod { name: Identifier, self_name: Option, instance_attributes: SmallMap, nn_module_registrations: NNModuleRegistrations, - return_attribute_initialization: Vec>, + return_attribute_initialization: Vec>, parameters: SmallMap, yields_and_returns: YieldsAndReturns, is_async: bool, @@ -2181,57 +2183,36 @@ impl Scopes { let scope = self.pop(); let unused_variables = Self::collect_unused_variables(scope.variables.clone()); match scope.kind { - ScopeKind::Method(mut method_scope) => { - for (name, attribute) in method_scope.instance_attributes.iter_mut() { - let mut initialized = if scope.flow.has_terminated { - None - } else { - Some( - scope - .flow - .get_info(&instance_attribute_flow_name(name)) - .map_or(InitializedInFlow::Conditionally, FlowInfo::initialized), - ) - }; - for return_initialization in &method_scope.return_attribute_initialization { - let on_return = return_initialization - .get(name) - .cloned() - .unwrap_or(InitializedInFlow::Conditionally); - initialized = Some(match (initialized, on_return) { - (None, initialized) => initialized, - (Some(InitializedInFlow::Yes), InitializedInFlow::Yes) => { - InitializedInFlow::Yes - } - ( - Some(InitializedInFlow::DeferredCheck(mut existing)), - InitializedInFlow::DeferredCheck(mut new), - ) => { - // All return paths must initialize the attribute. - // Combine their requirements with AND. - existing.append(&mut new); - InitializedInFlow::DeferredCheck(existing) - } - ( - Some(InitializedInFlow::Yes), - InitializedInFlow::DeferredCheck(keys), - ) - | ( - Some(InitializedInFlow::DeferredCheck(keys)), - InitializedInFlow::Yes, - ) => InitializedInFlow::DeferredCheck(keys), - _ => InitializedInFlow::Conditionally, - }); - } - // If every path raises or otherwise terminates without returning an instance, - // there is no successfully initialized object missing this attribute. - attribute.4 = initialized.unwrap_or(InitializedInFlow::Yes); - } + ScopeKind::Method(method_scope) => { + let instance_attributes = method_scope + .instance_attributes + .into_iter() + .map(|(name, attribute)| { + let final_flow = (!scope.flow.has_terminated).then_some(&scope.flow); + let initialized = InitializationStatus::merge( + final_flow + .into_iter() + .map(|flow| { + ( + flow.instance_attribute_initialization.get(&name), + flow.last_stmt_expr, + ) + }) + .chain( + method_scope + .return_attribute_initialization + .iter() + .map(|flow| (flow.get(&name), None)), + ), + ); + (name, (attribute, initialized)) + }) + .collect(); ( method_scope.yields_and_returns, Some(SelfAssignments { method_name: method_scope.name.id, - instance_attributes: method_scope.instance_attributes, + instance_attributes, nn_module_registrations: method_scope.nn_module_registrations, }), Self::collect_unused_parameters(method_scope.parameters), @@ -2271,28 +2252,20 @@ impl Scopes { x: &ExprAttribute, value: ExprOrBinding, annotation: Option>, - idx: Option>, + has_value: bool, ) -> bool { - let in_loop = self.loop_depth() != 0; + let mut in_nested_function = false; for scope in self.iter_rev_mut() { + in_nested_function |= matches!(scope.kind, ScopeKind::Function(_)); if let ScopeKind::Method(method_scope) = &mut scope.kind && let Some(self_name) = &method_scope.self_name && matches!(&*x.value, Expr::Name(name) if name.id == self_name.id) { - if let Some(idx) = idx { - match scope + if has_value && !in_nested_function { + scope .flow - .info - .entry(instance_attribute_flow_name(&x.attr.id)) - { - Entry::Vacant(entry) => { - entry.insert(FlowInfo::new_value(idx, FlowStyle::Other)); - } - Entry::Occupied(mut entry) => { - *entry.get_mut() = - entry.get().updated_value(idx, FlowStyle::Other, in_loop); - } - } + .instance_attribute_initialization + .insert(x.attr.id.clone(), InitializationStatus::Initialized); } if let Some(attr) = method_scope.instance_attributes.get_mut(&x.attr.id) { // Accumulate subsequent assignments in the method. @@ -2310,16 +2283,37 @@ impl Scopes { annotation, x.attr.range(), // Keep the range of the first assignment as the definition location. method_scope.receiver_kind, - InitializedInFlow::Yes, ), ); } return true; } + in_nested_function |= matches!(scope.kind, ScopeKind::Method(_)); } false } + /// A deletion in the method body removes its initialization guarantee. + pub fn record_self_attr_delete(&mut self, x: &ExprAttribute) { + for scope in self.iter_rev_mut() { + match &scope.kind { + ScopeKind::Function(_) => return, + ScopeKind::Method(method) => { + if method.self_name.as_ref().is_some_and(|self_name| { + matches!(&*x.value, Expr::Name(name) if name.id == self_name.id) + }) { + scope.flow.instance_attribute_initialization.insert( + x.attr.id.clone(), + InitializationStatus::Uninitialized, + ); + } + return; + } + _ => {} + } + } + } + /// Record the value of `self.register_buffer(...)` or `self.register_parameter(...)`. /// The enclosing class is checked for `torch.nn.Module` ancestry during solving. pub fn record_nn_module_registration( @@ -2616,7 +2610,6 @@ impl Scopes { let scope = self.current_mut(); if let Some(value) = scope.flow.get_value_mut(name) { value.style = FlowStyle::Uninitialized; - value.is_function_def = false; } scope.deleted_names.insert(name.clone()); } @@ -3168,21 +3161,9 @@ impl Scopes { } ScopeKind::Method(method) => { if !is_unreachable { - let mut initialization = - SmallMap::with_capacity(method.instance_attributes.len()); - for name in method.instance_attributes.keys() { - initialization.insert( - name.clone(), - scope - .flow - .get_info(&instance_attribute_flow_name(name)) - .map_or( - InitializedInFlow::Conditionally, - FlowInfo::initialized, - ), - ); - } - method.return_attribute_initialization.push(initialization); + method + .return_attribute_initialization + .push(scope.flow.instance_attribute_initialization.clone()); } method .yields_and_returns @@ -3365,8 +3346,6 @@ impl Scopes { }, }; let initialized = match &value.style { - // Conditional method definitions are exempt from attribute initialization checks. - _ if value.is_function_def => InitializedInFlow::Yes, FlowStyle::PossiblyUninitialized => InitializedInFlow::Conditionally, FlowStyle::MaybeInitialized(keys) => { InitializedInFlow::DeferredCheck(keys.clone()) @@ -3375,7 +3354,11 @@ impl Scopes { }; field_definitions.insert_hashed( name.owned(), - (definition, static_info.range, initialized.into()), + ( + definition, + static_info.range, + InitializationStatus::from(initialized).into(), + ), ); } }); @@ -3386,7 +3369,7 @@ impl Scopes { |( name, method, - InstanceAttribute(values, annotation, range, receiver_kind, initialized), + (InstanceAttribute(values, annotation, range, receiver_kind), initialized), )| { if let Some(( ClassFieldDefinition::DefinedInMethod { @@ -3421,17 +3404,14 @@ impl Scopes { // Any method can establish initialization, so combine their requirements with OR. match (&mut *existing_initialized, initialized) { (InitializedInClass::Yes, _) => {} - (_, InitializedInFlow::Yes) => { + (_, InitializationStatus::Initialized) => { *existing_initialized = InitializedInClass::Yes; } ( InitializedInClass::IfAny(groups), - InitializedInFlow::DeferredCheck(keys), - ) => groups.push(keys), - ( - InitializedInClass::IfAny(_), - InitializedInFlow::Conditionally | InitializedInFlow::No, - ) => {} + deferred @ InitializationStatus::Deferred(_), + ) => groups.push(deferred), + (InitializedInClass::IfAny(_), InitializationStatus::Uninitialized) => {} } } else if !field_definitions.contains_key_hashed(name.as_ref()) { field_definitions.insert_hashed( @@ -3972,58 +3952,6 @@ impl MergeStyle { } } -/// The result of analyzing whether a variable is defined after merging branches. -/// This enum captures the three distinct states: -/// - `Defined`: Variable is defined in all non-terminating branches -/// - `DeferredCheck`: Some branches don't define the variable, but they may terminate -/// (have `Never` type). The check is deferred to solve time. -/// - `NotDefined`: Variable is not defined in some branches that may not terminate -enum DefinitionStatus { - /// Variable is defined in all non-terminating branches. - Defined, - /// Variable may be defined if branches with these keys terminate (have `Never` type). - /// The keys are checked at solve time; if all have `Never` type, the variable is - /// considered initialized. - DeferredCheck(Vec>), - /// Variable is not defined in some branches that may not terminate. - NotDefined, -} - -/// Determines the definition status of a variable after a merge. -/// -/// The logic differs slightly for `LoopDefinitelyRuns` (where base having a value -/// or all loop body branches having values is sufficient) vs other merge styles. -fn determine_definition_status( - merge_style: MergeStyle, - base_has_value: bool, - n_values: usize, - n_branches: usize, - n_missing_branches: usize, - n_branches_with_termination_key: usize, - missing_branch_termination_keys: Vec>, -) -> DefinitionStatus { - match merge_style { - MergeStyle::LoopDefinitelyRuns if base_has_value => DefinitionStatus::Defined, - MergeStyle::LoopDefinitelyRuns if n_values == n_branches => DefinitionStatus::Defined, - MergeStyle::LoopDefinitelyRuns if n_missing_branches <= n_branches_with_termination_key => { - if !missing_branch_termination_keys.is_empty() { - DefinitionStatus::DeferredCheck(missing_branch_termination_keys) - } else { - DefinitionStatus::Defined - } - } - _ if n_values == n_branches => DefinitionStatus::Defined, - _ if n_missing_branches <= n_branches_with_termination_key => { - if !missing_branch_termination_keys.is_empty() { - DefinitionStatus::DeferredCheck(missing_branch_termination_keys) - } else { - DefinitionStatus::Defined - } - } - _ => DefinitionStatus::NotDefined, - } -} - /// Information about a single branch being merged, including both the flow info /// for a specific name (if present) and the termination key from the flow this branch came from. struct MergeBranchEntry { @@ -4105,8 +4033,6 @@ impl<'a> BindingsBuilder<'a> { merge_item: MergeItem, phi_key: PhiKey, merge_style: MergeStyle, - n_branches: usize, - n_branches_with_termination_key: usize, ) -> FlowInfo { // Only a promised phi can already appear in a branch's flow. let phi_idx = phi_key.promised; @@ -4159,18 +4085,33 @@ impl<'a> BindingsBuilder<'a> { let mut branch_idxs = SmallSet::with_capacity(merge_branches.len()); let mut branch_infos = Vec::with_capacity(merge_branches.len()); let mut styles = Vec::with_capacity(merge_branches.len()); - let mut is_function_def = true; - let mut n_values = 0; - // Collect termination keys from branches that don't define the variable. - // These will be used for deferred uninitialized checks at solve time. - let mut missing_branch_termination_keys = Vec::new(); - for merge_branch in merge_branches.into_iter() { - // Handle branches that don't have this name at all (flow_info is None) - let Some(flow_info) = merge_branch.flow_info else { - // This branch doesn't have this name - record its termination key if any - if let Some(termination_key) = merge_branch.termination_key { - missing_branch_termination_keys.push(termination_key); + let n_flow_branches = merge_branches.len() - usize::from(added_base_to_merge); + let definition_status = if matches!(merge_style, MergeStyle::LoopDefinitelyRuns) + && base_has_value + { + InitializationStatus::Initialized + } else { + let branches = merge_branches.iter().enumerate().filter_map(|(i, branch)| { + if i == n_flow_branches && matches!(merge_style, MergeStyle::LoopDefinitelyRuns) { + return None; } + let initialized = branch.flow_info.as_ref().is_some_and(|flow| { + flow.value + .as_ref() + .is_some_and(|value| !matches!(value.style, FlowStyle::Uninitialized)) + }); + Some(( + initialized.then_some(&InitializationStatus::Initialized), + branch.termination_key, + )) + }); + // A missing pre-loop definition still represents the zero-iteration path. + let missing_base = (matches!(merge_style, MergeStyle::Loop) && !added_base_to_merge) + .then_some((None, None)); + InitializationStatus::merge(branches.chain(missing_base)) + }; + for merge_branch in merge_branches { + let Some(flow_info) = merge_branch.flow_info else { continue; }; let branch_idx = flow_info.idx(); @@ -4187,67 +4128,30 @@ impl<'a> BindingsBuilder<'a> { } if let Some(v) = flow_info.value { - // A branch with FlowStyle::Uninitialized (e.g., after exception variable - // unbinding via mark_as_deleted) should not count as defining the variable. - // We still track its idx for type inference but don't count it as a value. - let is_uninitialized = matches!(v.style, FlowStyle::Uninitialized); - if !is_uninitialized { - n_values += 1; - } if Some(v.idx) == phi_idx { - // If uninitialized, still track termination key before continuing. - if is_uninitialized && let Some(termination_key) = merge_branch.termination_key - { - missing_branch_termination_keys.push(termination_key); - } continue; } - is_function_def &= v.is_function_def; if value_idxs.insert(v.idx) { // An invariant in Pyrefly is that we only set style when we // set a value, so duplicate value_idxs always have the same style. styles.push(v.style); } - // Treat uninitialized branches like missing branches for termination keys. - if is_uninitialized && let Some(termination_key) = merge_branch.termination_key { - missing_branch_termination_keys.push(termination_key); - } - } else { - // This branch doesn't have a value for the variable. - // If it has a termination key, track it for deferred checking. - if let Some(termination_key) = merge_branch.termination_key { - missing_branch_termination_keys.push(termination_key); - } } branch_idxs.insert(branch_idx); } - // n_total_branches is the actual number of branches we iterated over, which includes - // the base flow for loops (since base is added to merge_branches for type inference). - let n_total_branches = if added_base_to_merge { - n_branches + 1 - } else { - n_branches - }; - let n_missing_branches = n_total_branches - n_values; - let definition_status = determine_definition_status( - merge_style, - base_has_value, - n_values, - n_branches, - n_missing_branches, - n_branches_with_termination_key, - missing_branch_termination_keys, - ); - // Helper to compute the final FlowStyle based on definition status. let compute_final_style = |styles: Vec| -> FlowStyle { match &definition_status { - DefinitionStatus::DeferredCheck(keys) => FlowStyle::MaybeInitialized(keys.clone()), - DefinitionStatus::Defined => { + // Each local branch contributes at most one termination key, so these + // clauses are singletons and can use FlowStyle's existing conjunction. + InitializationStatus::Deferred(paths) => { + FlowStyle::MaybeInitialized(paths.iter().flatten().copied().collect()) + } + InitializationStatus::Initialized => { FlowStyle::merged(true, styles.into_iter(), merge_style) } - DefinitionStatus::NotDefined => { + InitializationStatus::Uninitialized => { FlowStyle::merged(false, styles.into_iter(), merge_style) } } @@ -4287,7 +4191,6 @@ impl<'a> BindingsBuilder<'a> { value: Some(FlowValue { idx: *value_idxs.first().unwrap(), style: compute_final_style(styles), - is_function_def, }), narrow: Some(FlowNarrow { idx: merged_idx }), narrow_depth: 1, @@ -4309,7 +4212,6 @@ impl<'a> BindingsBuilder<'a> { value: Some(FlowValue { idx: merged_idx, style: compute_final_style(styles), - is_function_def, }), narrow: None, narrow_depth: 0, @@ -4361,24 +4263,38 @@ impl<'a> BindingsBuilder<'a> { || base.is_definitely_unreachable) && flows.iter().all(|f| f.is_definitely_unreachable); - // For a regular loop, we merge the base so there's one extra branch being merged. - // For LoopDefinitelyRuns, we don't count the base as an extra branch because we - // know the loop body will definitely execute at least once. - let n_branches = flows.len() - + if matches!(merge_style, MergeStyle::Loop) { - 1 - } else { - 0 - }; - - // Count how many branches have a last_stmt_expr (potential type-based termination) - let n_branches_with_termination_key = - flows.iter().filter(|f| f.last_stmt_expr.is_some()).count(); - // Collect all termination keys from flows (for building dense MergeItems) let all_termination_keys: Vec>> = flows.iter().map(|f| f.last_stmt_expr).collect(); + // Each reachable path must initialize an attribute. A missing assignment can + // instead be justified by a Never-returning call on that path. Ordinary loops + // include the pre-loop path because their bodies may execute zero times. + // If no loop path survives, retain the pre-loop state as the local flow does. + let attribute_flows = flows.iter().map(|flow| (flow, flow.last_stmt_expr)).chain( + (matches!(merge_style, MergeStyle::Loop) + || (merge_style.is_loop() && flows.is_empty())) + .then_some((&base, None)), + ); + let mut attribute_names = SmallSet::new(); + for (flow, _) in attribute_flows.clone() { + for name in flow.instance_attribute_initialization.keys() { + attribute_names.insert(name.clone()); + } + } + let mut instance_attribute_initialization = SmallMap::with_capacity(attribute_names.len()); + for name in attribute_names { + let initialized = InitializationStatus::merge(attribute_flows.clone().map( + |(flow, termination_key)| { + ( + flow.instance_attribute_initialization.get(&name), + termination_key, + ) + }, + )); + instance_attribute_initialization.insert(name, initialized); + } + // Collect all unique names from base + all flows. We need this before we construct merge items // so that we can accurately represent a flow in which some name doesn't appear. let mut all_names: SmallSet = SmallSet::new(); @@ -4421,19 +4337,14 @@ impl<'a> BindingsBuilder<'a> { promised: self.promised_idx(&key), key, }; - let flow_info = self.merged_flow_info( - merge_item, - phi_key, - merge_style, - n_branches, - n_branches_with_termination_key, - ); + let flow_info = self.merged_flow_info(merge_item, phi_key, merge_style); merged_flow_infos.insert_hashed(name, flow_info); } // The resulting flow has terminated only if all branches had terminated. let flow = Flow { info: merged_flow_infos, + instance_attribute_initialization, has_terminated, terminated_by_raise: has_terminated && any_terminated_by_raise, is_definitely_unreachable: all_are_unreachable, @@ -4465,7 +4376,6 @@ impl<'a> BindingsBuilder<'a> { info.value = Some(FlowValue { idx: phi_idx, style: FlowStyle::LoopRecursion, - is_function_def: false, }); info.narrow = None; } diff --git a/pyrefly/lib/binding/stmt.rs b/pyrefly/lib/binding/stmt.rs index 5868bcc985..1b2e37ab6e 100644 --- a/pyrefly/lib/binding/stmt.rs +++ b/pyrefly/lib/binding/stmt.rs @@ -910,6 +910,9 @@ impl<'a> BindingsBuilder<'a> { delete_idx, Binding::Delete(Box::new(target.clone())), ); + if let Expr::Attribute(attribute) = &target { + self.scopes.record_self_attr_delete(attribute); + } if let Expr::Attribute(_) = target && let Some((identifier, _)) = identifier_and_chain_prefix_for_expr(&target) { @@ -1201,13 +1204,12 @@ impl<'a> BindingsBuilder<'a> { None, ), ); - let (value, attr_idx) = match x.value { + let has_value = x.value.is_some(); + let value = match x.value { Some(mut assigned) => { - let (value, idx) = - self.bind_attr_assign(attr.clone(), &mut assigned, |v, _| { - ExprOrBinding::Expr(v.clone()) - }); - (value, Some(idx)) + self.bind_attr_assign(attr.clone(), &mut assigned, |v, _| { + ExprOrBinding::Expr(v.clone()) + }) } _ => { self.ensure_expr( @@ -1216,17 +1218,14 @@ impl<'a> BindingsBuilder<'a> { is_annotation: false, }, ); - ( - ExprOrBinding::Binding(Binding::Any(AnyStyle::Implicit)), - None, - ) + ExprOrBinding::Binding(Binding::Any(AnyStyle::Implicit)) } }; if !self.scopes.record_self_attr_assign( &attr, value.clone(), Some(ann_key), - attr_idx, + has_value, ) { self.error( x.range, diff --git a/pyrefly/lib/binding/target.rs b/pyrefly/lib/binding/target.rs index 4ee12ab8ab..d7384167bc 100644 --- a/pyrefly/lib/binding/target.rs +++ b/pyrefly/lib/binding/target.rs @@ -189,7 +189,7 @@ impl<'a> BindingsBuilder<'a> { mut assigned: Option<&mut Expr>, make_assigned_value: impl FnOnce(Option<&Expr>, Option>) -> ExprOrBinding, ensure_assigned: bool, - ) -> (ExprOrBinding, Idx) { + ) -> ExprOrBinding { let narrowing_identifier = identifier_and_chain_prefix_for_expr(&Expr::Attribute(attr.clone())) .map(|(identifier, _)| identifier); @@ -221,7 +221,7 @@ impl<'a> BindingsBuilder<'a> { if let Some(identifier) = narrowing_identifier { self.narrow_if_name_is_defined(identifier, idx); } - (value, idx) + value } pub fn bind_attr_assign( @@ -229,7 +229,7 @@ impl<'a> BindingsBuilder<'a> { attr: ExprAttribute, assigned: &mut Expr, make_assigned_value: impl FnOnce(&Expr, Option>) -> ExprOrBinding, - ) -> (ExprOrBinding, Idx) { + ) -> ExprOrBinding { self.bind_attr_assign_impl( attr, Some(assigned), @@ -321,7 +321,7 @@ impl<'a> BindingsBuilder<'a> { ); } Expr::Attribute(x) => { - let (attr_value, attr_idx) = self.bind_attr_assign_impl( + let attr_value = self.bind_attr_assign_impl( x.clone(), assigned, make_assigned_value, @@ -330,7 +330,7 @@ impl<'a> BindingsBuilder<'a> { // If this is a self-assignment, record it because we may use it to infer // the existence of an instance-only attribute. self.scopes - .record_self_attr_assign(x, attr_value, None, Some(attr_idx)); + .record_self_attr_assign(x, attr_value, None, true); } Expr::Subscript(x) => { self.bind_subscript_assign_impl( diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index 76db2cb214..31a22a65bc 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -10,6 +10,326 @@ use crate::test::util::TestEnv; use crate::testcase; +testcase!( + test_attribute_initialization_inherited, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + r#" +class Base: + timeout = 10 + def __init__(self) -> None: + self.value = 0 + +class Child(Base): + def __init__(self, t: int | None = None) -> None: + super().__init__() + if t is not None: + self.timeout = t + self.value = t + self.own = t # E: Attribute `own` may be uninitialized + +class Grandchild(Child): + def __init__(self, t: int | None = None) -> None: + if t is not None: + self.timeout = t + self.value = t + +class ClassBody(Base): + if bool(): + timeout = 20 +"#, +); + +testcase!( + test_attribute_initialization_trailing_noreturn, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + r#" +from typing import NoReturn + +def stop() -> NoReturn: + raise ValueError() + +def proceed() -> None: ... + +class TrailingCall: + def __init__(self, flag: bool) -> None: + if flag: + self.value = 1 + stop() + +class TrailingRaise: + def __init__(self, flag: bool) -> None: + if flag: + self.value = 1 + raise ValueError() + +class DeferredThenStop: + def __init__(self, flag: bool) -> None: + if flag: + self.value = 1 + else: + proceed() + stop() + +class DeferredThenProceed: + def __init__(self, flag: bool) -> None: + if flag: + self.value = 1 + else: + stop() + proceed() + +class EarlyReturn: + def __init__(self, flag: bool) -> None: + if flag: + return + self.value = 1 # E: Attribute `value` may be uninitialized + stop() +"#, +); + +testcase!( + test_attribute_initialization_while_true_return, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + r#" +def connect() -> int: ... + +class Retry: + def __init__(self) -> None: + while True: + try: + self.conn = connect() + return + except OSError: + continue + +class EarlyReturn: + def __init__(self, flag: bool) -> None: + while True: + if flag: + return + self.value = 1 # E: Attribute `value` may be uninitialized + return + +class SuppressedLoopRaise: + def __init__(self) -> None: + from contextlib import suppress + with suppress(ValueError): + while True: + raise ValueError() + self.value = 1 + +class ReturnOrBreak: + def __init__(self, flag: bool) -> None: + while True: + if flag: + self.value = 1 # E: Attribute `value` may be uninitialized + return + break +"#, +); + +testcase!( + test_attribute_initialization_nested_function, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + r#" +class NestedOnly: + def __init__(self) -> None: + def later() -> None: + self.value = 1 # E: Attribute `value` may be uninitialized + +class ConditionalOuter: + def __init__(self, flag: bool) -> None: + if flag: + self.value = 1 # E: Attribute `value` may be uninitialized + def later() -> None: + self.value = 2 + +class InitializedOuter: + def __init__(self) -> None: + self.value = 1 + def later() -> None: + del self.value + +class NestedMethod: + def __init__(self) -> None: + class Inner: + def later(inner) -> None: + self.value = 1 # E: Attribute `value` may be uninitialized + +class NestedReturn: + def __init__(self) -> None: + def later() -> None: + return + self.value = 1 +"#, +); + +testcase!( + test_attribute_initialization_control_flow, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + r#" +from contextlib import nullcontext +from typing import Literal + +class TryExcept: + def __init__(self, raw: str) -> None: + try: + self.value = int(raw) + except ValueError: + self.value = 0 + +class TryExceptMissing: + def __init__(self, raw: str) -> None: + try: + self.value = int(raw) # E: Attribute `value` may be uninitialized + except ValueError: + pass + +class Finally: + def __init__(self) -> None: + try: + int("1") + finally: + self.value = 1 + +class With: + def __init__(self) -> None: + with nullcontext(): + self.value = 1 + +class WithConditional: + def __init__(self, flag: bool) -> None: + with nullcontext(): + if flag: + self.value = 1 # E: Attribute `value` may be uninitialized + +class Match: + def __init__(self, kind: Literal["a", "b"]) -> None: + match kind: + case "a": + self.value = 1 + case "b": + self.value = 2 + +class MatchMissing: + def __init__(self, kind: str) -> None: + match kind: + case "a": + self.value = 1 # E: Attribute `value` may be uninitialized + +class Deleted: + def __init__(self) -> None: + self.value = 1 # E: Attribute `value` may be uninitialized + del self.value + +class DeletedConditionally: + def __init__(self, flag: bool) -> None: + self.value = 1 # E: Attribute `value` may be uninitialized + if flag: + del self.value + +class Reassigned: + def __init__(self) -> None: + self.value = 1 + del self.value + self.value = 2 +"#, +); + +testcase!( + test_attribute_initialization_class_conditions_and_hooks, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + r#" +import sys +from dataclasses import dataclass +from typing import TYPE_CHECKING, assert_type + +class Conditional: + if TYPE_CHECKING: + type_only = 1 + if sys.version_info >= (3, 10): + versioned = 1 + +class VersionedLoop: + while sys.version_info >= (3, 0): + pass + value = 1 + +@dataclass +class PostInit: + flag: bool + def __post_init__(self) -> None: + self.value = 1 + +@dataclass +class ConditionalPostInit: + flag: bool + def __post_init__(self) -> None: + if self.flag: + self.value = 1 # E: Attribute `value` may be uninitialized + +class SubclassHook: + def __init_subclass__(cls) -> None: + cls.value = 1 + +class ConditionalSubclassHook: + def __init_subclass__(cls, flag: bool = False) -> None: + if flag: + cls.value = 1 # E: Attribute `value` may be uninitialized + +class ClassLoop: + for value in [1]: + pass + +assert_type(Conditional.type_only, int) +assert_type(Conditional.versioned, int) +assert_type(ClassLoop.value, int) +"#, +); + +testcase!( + bug = "Suppressed exceptions do not merge attribute initialization", + test_attribute_initialization_suppressed_exception, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + r#" +from contextlib import suppress + +class C: + def __init__(self, raw: str) -> None: + with suppress(ValueError): + self.value = int(raw) +"#, +); + +testcase!( + bug = "Return snapshots do not include assignments from finally", + test_attribute_initialization_finally_after_return, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + r#" +class C: + def __init__(self) -> None: + try: + return + finally: + self.value = 1 # E: Attribute `value` may be uninitialized +"#, +); + +testcase!( + bug = "For loop targets are treated as assigned before the first iteration", + test_attribute_initialization_class_loop_target, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + r#" +from typing import assert_type + +class C: + for value in list[int](): + pass + +assert_type(C.value, int) +"#, +); + testcase!( test_possibly_uninitialized_attribute_default_disabled, r#" @@ -496,6 +816,82 @@ class ConditionalConstructor: "#, ); +testcase!( + test_attribute_initialization_loops, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + r#" +from typing import Never, assert_type + +def stop() -> Never: + raise RuntimeError() + +class BeforeLoop: + def __init__(self, values: list[int]) -> None: + self.value = 0 + for value in values: + self.value = value + +class PossiblyEmpty: + def __init__(self, values: list[int]) -> None: + for value in values: + self.value = value # E: Attribute `value` may be uninitialized + +class NonEmpty: + def __init__(self) -> None: + for value in [1]: + self.value = value + +class NoReturningLoop: + def __init__(self) -> None: + self.value = 0 + for value in [1]: + raise RuntimeError() + +class ConditionalContinue: + def __init__(self, flag: bool) -> None: + for value in [1]: + if flag: + continue + self.value = value # E: Attribute `value` may be uninitialized + +class ConditionalBreak: + def __init__(self, flag: bool) -> None: + while True: + if flag: + break + self.value = 1 # E: Attribute `value` may be uninitialized + break + +class LoopElse: + def __init__(self, values: list[int]) -> None: + for value in values: + self.value = value + else: + self.value = 0 + +class DeferredBeforeLoop: + def __init__(self, flag: bool, values: list[int]) -> None: + if flag: + self.value = 1 + else: + stop() + for value in values: + pass + +class DeferredInLoop: + def __init__(self, flag: bool) -> None: + for value in [1]: + if flag: + self.value = value + else: + stop() + +assert_type(BeforeLoop([]).value, int) +assert_type(NonEmpty().value, int) +assert_type(LoopElse([]).value, int) +"#, +); + testcase!( test_attribute_initialization_deferred_return_paths, TestEnv::new().enable_possibly_uninitialized_attribute_error(), diff --git a/pyrefly/lib/test/flow_branching.rs b/pyrefly/lib/test/flow_branching.rs index 51c6344185..52b66f2aaf 100644 --- a/pyrefly/lib/test/flow_branching.rs +++ b/pyrefly/lib/test/flow_branching.rs @@ -2284,7 +2284,6 @@ def f() -> int: // elif condition only executes if the first `if` was False — walrus may not run. testcase!( - bug = "In order to fix false positives, we handled narrows differently in if/elif and introduced a false negative here", test_walrus_in_elif, r#" def condition() -> bool: ... @@ -2293,7 +2292,7 @@ def f() -> bool: pass elif (x := condition()): pass - return x # False negative: x winds up getting applied as if it were in the base flow due to the negative narrow + return x # E: `x` may be uninitialized "#, ); @@ -2381,7 +2380,6 @@ def f() -> None: // the walrus may not execute, so x should be possibly-uninitialized. testcase!( - bug = "Should report x as possibly uninitialized since the if branch does not terminate", test_walrus_in_elif_preceding_if_no_terminate, r#" def foo() -> bool: @@ -2395,7 +2393,7 @@ def f() -> None: pass elif (x := bar()) > 1: pass - print(x) # should be an error: x may be uninitialized + print(x) # E: `x` may be uninitialized "#, ); diff --git a/pyrefly/lib/test/scope.rs b/pyrefly/lib/test/scope.rs index 9390d7979f..c04f56c489 100644 --- a/pyrefly/lib/test/scope.rs +++ b/pyrefly/lib/test/scope.rs @@ -1071,7 +1071,7 @@ def try_except_twice(): except Exception: e3 # E: `e3` is uninitialized - e3 # E: `e3` may be uninitialized + e3 # E: `e3` is uninitialized def try_except_multiple_finally(): try: diff --git a/website/docs/error-kinds.mdx b/website/docs/error-kinds.mdx index 19235ad3ed..68fbc44c75 100644 --- a/website/docs/error-kinds.mdx +++ b/website/docs/error-kinds.mdx @@ -1819,19 +1819,31 @@ Default severity: `ignore` The `strict` preset enables this check as an error. -An attribute may be uninitialized after class-body execution or instance initialization. -Pyrefly reports this error at the attribute definition, even if the attribute is never read. -A later `hasattr` guard does not suppress this check. -Conditional method definitions are excluded. -This check does not apply to stub (`.pyi`) files because they do not perform runtime initialization. +Pyrefly checks whether an assigned attribute exists after the class body or a recognized initialization method completes. +Recognized methods include `__init__`, `__new__`, `__post_init__`, `__init_subclass__`, and supported test setup methods. +Each path that completes normally, including an explicit `return`, must leave the attribute initialized. +Paths that raise, loop forever, or end in a `Never` or `NoReturn` call do not require an assignment. +Deleting an attribute removes its initialization guarantee. ```python class C: def __init__(self, value: int | None) -> None: if value is not None: self.value = value # possibly-uninitialized-attribute + # The path where value is None completes without assigning self.value. ``` +The check permits an inherited attribute when a base class other than `object` defines the same name. +An instance `__getattr__` fallback also suppresses this error for instance attributes. +Pyrefly combines assignments from different methods leniently: one method that initializes the attribute on every completing path suffices. +This includes ordinary helper methods, even when Pyrefly cannot prove that the constructor calls them. +Assignments inside nested functions do not initialize the enclosing method's attributes. + +Pyrefly reports this error at the attribute definition, even if the attribute is never read. +A later `hasattr` guard does not suppress this check. +Conditional method definitions and stub (`.pyi`) files are excluded. +Bare instance annotations use the separate [`uninitialized-instance-variable`](#uninitialized-instance-variable) check. + To disable this check while keeping other strict checks, use: ```toml @@ -1842,7 +1854,6 @@ possibly-uninitialized-attribute = "ignore" ``` This setting does not disable [`missing-attribute`](#missing-attribute). -The separate [`uninitialized-instance-variable`](#uninitialized-instance-variable) check reports class-body instance declarations that have no initialization. ## potential-bad-keyword-argument From 6440b7d730cb5d2056a08cf6bf065e8849d10573 Mon Sep 17 00:00:00 2001 From: Asuka Minato Date: Thu, 22 Oct 2026 12:13:20 +0900 Subject: [PATCH 8/9] Bound attribute initialization proofs and isolate flow changes --- pyrefly/lib/binding/bindings.rs | 67 ++++++++- pyrefly/lib/binding/class.rs | 36 ----- pyrefly/lib/binding/scope.rs | 192 ++++++++++++++++++++------ pyrefly/lib/binding/stmt.rs | 11 -- pyrefly/lib/test/attributes.rs | 33 +++-- pyrefly/lib/test/delayed_inference.rs | 2 +- pyrefly/lib/test/flow_branching.rs | 9 +- pyrefly/lib/test/lsp/hover_type.rs | 2 +- pyrefly/lib/test/paramspec.rs | 2 +- pyrefly/lib/test/perf.rs | 34 +++++ pyrefly/lib/test/scope.rs | 2 +- website/docs/error-kinds.mdx | 3 +- 12 files changed, 283 insertions(+), 110 deletions(-) diff --git a/pyrefly/lib/binding/bindings.rs b/pyrefly/lib/binding/bindings.rs index 3d14f9a60e..7f4b12fbf1 100644 --- a/pyrefly/lib/binding/bindings.rs +++ b/pyrefly/lib/binding/bindings.rs @@ -220,6 +220,7 @@ impl InitializationStatus { pub fn merge<'a>( paths: impl IntoIterator, Option>)>, ) -> Self { + const MAX_DEFERRED_PATHS: usize = 16; let mut conditions = Vec::new(); for (initialized, termination_key) in paths { match initialized { @@ -250,6 +251,16 @@ impl InitializationStatus { // Forks can copy the same requirement into several paths. conditions.sort_unstable(); conditions.dedup(); + if conditions.len() > MAX_DEFERRED_PATHS { + // Retain one key per requirement to prevent exponential growth across joins. + // This strengthens the requirements and can produce false positives above the cap. + // After deduplication, at most one singleton remains per distinct key. + for path in &mut conditions { + path.truncate(1); + } + conditions.sort_unstable(); + conditions.dedup(); + } Self::Deferred(conditions) } } @@ -280,7 +291,8 @@ impl From for InitializationStatus { #[derive(Debug, Clone)] pub enum InitializedInClass { Yes, - /// Any defining method or the class body can establish initialization. + /// Any defining method can establish initialization. + /// Class-body definitions are checked separately and never merge with methods. /// An empty list means no flow guarantees initialization. IfAny(Vec), } @@ -3191,3 +3203,56 @@ impl BindingsBuilder<'_> { ); } } + +#[cfg(test)] +mod initialization_tests { + use pyrefly_graph::index::Idx; + + use super::InitializationStatus; + + #[test] + fn test_attribute_initialization_path_limit() { + let mut status = InitializationStatus::Deferred(vec![vec![Idx::new(0)]]); + for join in 0..5 { + status = InitializationStatus::merge([ + (Some(&status), Some(Idx::new(2 * join + 1))), + (Some(&status), Some(Idx::new(2 * join + 2))), + ]); + if join == 3 { + let InitializationStatus::Deferred(paths) = &status else { + panic!("conditional paths must remain deferred"); + }; + assert_eq!(paths.len(), 16); + assert!(status.is_initialized(|key| key.idx() == 7 || key.idx() == 8)); + } + } + let InitializationStatus::Deferred(paths) = &status else { + panic!("conditional paths must remain deferred"); + }; + assert_eq!(paths.len(), 1); + assert!(!status.is_initialized(|key| key.idx() == 9 || key.idx() == 10)); + assert!(status.is_initialized(|key| key.idx() == 0)); + } + + #[test] + fn test_attribute_initialization_path_growth_is_bounded() { + for initial_paths in [1, 64] { + let mut status = InitializationStatus::Deferred( + (0..initial_paths).map(|key| vec![Idx::new(key)]).collect(), + ); + for join in 0..1000 { + status = InitializationStatus::merge([ + (Some(&status), Some(Idx::new(initial_paths + 2 * join))), + (Some(&status), Some(Idx::new(initial_paths + 2 * join + 1))), + ]); + let InitializationStatus::Deferred(paths) = &status else { + panic!("conditional paths must remain deferred"); + }; + assert!(paths.len() <= initial_paths.max(16)); + assert!(paths.iter().all(|path| path.len() <= 5)); + assert!(!status.is_initialized(|_| false)); + assert!(status.is_initialized(|key| key.idx() < initial_paths)); + } + } + } +} diff --git a/pyrefly/lib/binding/class.rs b/pyrefly/lib/binding/class.rs index 6b05d27e2c..2d18d3382f 100644 --- a/pyrefly/lib/binding/class.rs +++ b/pyrefly/lib/binding/class.rs @@ -31,7 +31,6 @@ use ruff_python_ast::name::Name; use ruff_text_size::Ranged; use ruff_text_size::TextRange; use starlark_map::small_map::SmallMap; -use starlark_map::small_set::SmallSet; use crate::binding::attrs::AttrsDecoratorMethods; use crate::binding::attrs::collect_attrs_decorator_methods; @@ -509,44 +508,9 @@ impl<'a> BindingsBuilder<'a> { ClassFieldDefinition::DefinedInMethod { method, .. } if !method.recognized_attribute_defining_method ); - // Inspect definition bindings here so method provenance does not occupy every flow value. - let is_conditional_method = - if let ClassFieldDefinition::DefinedWithoutAssign { definition } = &definition { - let mut pending = vec![*definition]; - let mut seen = SmallSet::new(); - let mut found_function = false; - let mut only_functions = true; - while let Some(idx) = pending.pop() { - if !seen.insert(idx) { - continue; - } - match self - .get_original_binding(idx) - .and_then(|(_, binding)| binding) - { - Some(Binding::Function { .. }) => found_function = true, - Some(Binding::Phi(_, branches)) => { - pending.extend(branches.iter().map(|branch| branch.value_key)); - } - Some(Binding::Narrow(prior, _, _)) => pending.push(*prior), - Some(Binding::LoopPhi(phi)) => { - pending.push(phi.0); - pending.extend(phi.1.iter().copied()); - } - _ => { - only_functions = false; - break; - } - } - } - found_function && only_functions - } else { - false - }; // Stub files declare attributes without runtime initialization. if !self.module_info.path().is_interface() && check_initialization - && !is_conditional_method && let InitializedInClass::IfAny(initializations) = initialized { let is_instance = matches!( diff --git a/pyrefly/lib/binding/scope.rs b/pyrefly/lib/binding/scope.rs index 2895def640..84f45cec40 100644 --- a/pyrefly/lib/binding/scope.rs +++ b/pyrefly/lib/binding/scope.rs @@ -745,6 +745,9 @@ struct FlowInfo { struct FlowValue { idx: Idx, style: FlowStyle, + /// True when every reaching value comes from a function definition. + /// Branch merges preserve this flag when they replace the flow style. + is_function_def: bool, } /// The most recent narrow for a name. @@ -756,7 +759,11 @@ struct FlowNarrow { impl FlowInfo { fn new_value(idx: Idx, style: FlowStyle) -> Self { Self { - value: Some(FlowValue { idx, style }), + value: Some(FlowValue { + idx, + is_function_def: matches!(style, FlowStyle::FunctionDef { .. }), + style, + }), narrow: None, narrow_depth: 0, loop_prior: idx, @@ -774,7 +781,11 @@ impl FlowInfo { fn updated_value(&self, idx: Idx, style: FlowStyle, in_loop: bool) -> Self { Self { - value: Some(FlowValue { idx, style }), + value: Some(FlowValue { + idx, + is_function_def: matches!(style, FlowStyle::FunctionDef { .. }), + style, + }), // Note that any existing narrow is wiped when a new value is bound. narrow: None, narrow_depth: 0, @@ -2610,6 +2621,7 @@ impl Scopes { let scope = self.current_mut(); if let Some(value) = scope.flow.get_value_mut(name) { value.style = FlowStyle::Uninitialized; + value.is_function_def = false; } scope.deleted_names.insert(name.clone()); } @@ -3346,6 +3358,7 @@ impl Scopes { }, }; let initialized = match &value.style { + _ if value.is_function_def => InitializedInFlow::Yes, FlowStyle::PossiblyUninitialized => InitializedInFlow::Conditionally, FlowStyle::MaybeInitialized(keys) => { InitializedInFlow::DeferredCheck(keys.clone()) @@ -3952,6 +3965,58 @@ impl MergeStyle { } } +/// The result of analyzing whether a variable is defined after merging branches. +/// This enum captures the three distinct states: +/// - `Defined`: Variable is defined in all non-terminating branches +/// - `DeferredCheck`: Some branches don't define the variable, but they may terminate +/// (have `Never` type). The check is deferred to solve time. +/// - `NotDefined`: Variable is not defined in some branches that may not terminate +enum DefinitionStatus { + /// Variable is defined in all non-terminating branches. + Defined, + /// Variable may be defined if branches with these keys terminate (have `Never` type). + /// The keys are checked at solve time; if all have `Never` type, the variable is + /// considered initialized. + DeferredCheck(Vec>), + /// Variable is not defined in some branches that may not terminate. + NotDefined, +} + +/// Determines the definition status of a variable after a merge. +/// +/// The logic differs slightly for `LoopDefinitelyRuns` (where base having a value +/// or all loop body branches having values is sufficient) vs other merge styles. +fn determine_definition_status( + merge_style: MergeStyle, + base_has_value: bool, + n_values: usize, + n_branches: usize, + n_missing_branches: usize, + n_branches_with_termination_key: usize, + missing_branch_termination_keys: Vec>, +) -> DefinitionStatus { + match merge_style { + MergeStyle::LoopDefinitelyRuns if base_has_value => DefinitionStatus::Defined, + MergeStyle::LoopDefinitelyRuns if n_values == n_branches => DefinitionStatus::Defined, + MergeStyle::LoopDefinitelyRuns if n_missing_branches <= n_branches_with_termination_key => { + if !missing_branch_termination_keys.is_empty() { + DefinitionStatus::DeferredCheck(missing_branch_termination_keys) + } else { + DefinitionStatus::Defined + } + } + _ if n_values == n_branches => DefinitionStatus::Defined, + _ if n_missing_branches <= n_branches_with_termination_key => { + if !missing_branch_termination_keys.is_empty() { + DefinitionStatus::DeferredCheck(missing_branch_termination_keys) + } else { + DefinitionStatus::Defined + } + } + _ => DefinitionStatus::NotDefined, + } +} + /// Information about a single branch being merged, including both the flow info /// for a specific name (if present) and the termination key from the flow this branch came from. struct MergeBranchEntry { @@ -4033,6 +4098,8 @@ impl<'a> BindingsBuilder<'a> { merge_item: MergeItem, phi_key: PhiKey, merge_style: MergeStyle, + n_branches: usize, + n_branches_with_termination_key: usize, ) -> FlowInfo { // Only a promised phi can already appear in a branch's flow. let phi_idx = phi_key.promised; @@ -4085,33 +4152,18 @@ impl<'a> BindingsBuilder<'a> { let mut branch_idxs = SmallSet::with_capacity(merge_branches.len()); let mut branch_infos = Vec::with_capacity(merge_branches.len()); let mut styles = Vec::with_capacity(merge_branches.len()); - let n_flow_branches = merge_branches.len() - usize::from(added_base_to_merge); - let definition_status = if matches!(merge_style, MergeStyle::LoopDefinitelyRuns) - && base_has_value - { - InitializationStatus::Initialized - } else { - let branches = merge_branches.iter().enumerate().filter_map(|(i, branch)| { - if i == n_flow_branches && matches!(merge_style, MergeStyle::LoopDefinitelyRuns) { - return None; - } - let initialized = branch.flow_info.as_ref().is_some_and(|flow| { - flow.value - .as_ref() - .is_some_and(|value| !matches!(value.style, FlowStyle::Uninitialized)) - }); - Some(( - initialized.then_some(&InitializationStatus::Initialized), - branch.termination_key, - )) - }); - // A missing pre-loop definition still represents the zero-iteration path. - let missing_base = (matches!(merge_style, MergeStyle::Loop) && !added_base_to_merge) - .then_some((None, None)); - InitializationStatus::merge(branches.chain(missing_base)) - }; - for merge_branch in merge_branches { + let mut is_function_def = true; + let mut n_values = 0; + // Collect termination keys from branches that don't define the variable. + // These will be used for deferred uninitialized checks at solve time. + let mut missing_branch_termination_keys = Vec::new(); + for merge_branch in merge_branches.into_iter() { + // Handle branches that don't have this name at all (flow_info is None) let Some(flow_info) = merge_branch.flow_info else { + // This branch doesn't have this name - record its termination key if any + if let Some(termination_key) = merge_branch.termination_key { + missing_branch_termination_keys.push(termination_key); + } continue; }; let branch_idx = flow_info.idx(); @@ -4128,30 +4180,67 @@ impl<'a> BindingsBuilder<'a> { } if let Some(v) = flow_info.value { + // A branch with FlowStyle::Uninitialized (e.g., after exception variable + // unbinding via mark_as_deleted) should not count as defining the variable. + // We still track its idx for type inference but don't count it as a value. + let is_uninitialized = matches!(v.style, FlowStyle::Uninitialized); + if !is_uninitialized { + n_values += 1; + } if Some(v.idx) == phi_idx { + // If uninitialized, still track termination key before continuing. + if is_uninitialized && let Some(termination_key) = merge_branch.termination_key + { + missing_branch_termination_keys.push(termination_key); + } continue; } + is_function_def &= v.is_function_def; if value_idxs.insert(v.idx) { // An invariant in Pyrefly is that we only set style when we // set a value, so duplicate value_idxs always have the same style. styles.push(v.style); } + // Treat uninitialized branches like missing branches for termination keys. + if is_uninitialized && let Some(termination_key) = merge_branch.termination_key { + missing_branch_termination_keys.push(termination_key); + } + } else { + // This branch doesn't have a value for the variable. + // If it has a termination key, track it for deferred checking. + if let Some(termination_key) = merge_branch.termination_key { + missing_branch_termination_keys.push(termination_key); + } } branch_idxs.insert(branch_idx); } + // n_total_branches is the actual number of branches we iterated over, which includes + // the base flow for loops (since base is added to merge_branches for type inference). + let n_total_branches = if added_base_to_merge { + n_branches + 1 + } else { + n_branches + }; + let n_missing_branches = n_total_branches - n_values; + let definition_status = determine_definition_status( + merge_style, + base_has_value, + n_values, + n_branches, + n_missing_branches, + n_branches_with_termination_key, + missing_branch_termination_keys, + ); + // Helper to compute the final FlowStyle based on definition status. let compute_final_style = |styles: Vec| -> FlowStyle { match &definition_status { - // Each local branch contributes at most one termination key, so these - // clauses are singletons and can use FlowStyle's existing conjunction. - InitializationStatus::Deferred(paths) => { - FlowStyle::MaybeInitialized(paths.iter().flatten().copied().collect()) - } - InitializationStatus::Initialized => { + DefinitionStatus::DeferredCheck(keys) => FlowStyle::MaybeInitialized(keys.clone()), + DefinitionStatus::Defined => { FlowStyle::merged(true, styles.into_iter(), merge_style) } - InitializationStatus::Uninitialized => { + DefinitionStatus::NotDefined => { FlowStyle::merged(false, styles.into_iter(), merge_style) } } @@ -4191,6 +4280,7 @@ impl<'a> BindingsBuilder<'a> { value: Some(FlowValue { idx: *value_idxs.first().unwrap(), style: compute_final_style(styles), + is_function_def, }), narrow: Some(FlowNarrow { idx: merged_idx }), narrow_depth: 1, @@ -4212,6 +4302,7 @@ impl<'a> BindingsBuilder<'a> { value: Some(FlowValue { idx: merged_idx, style: compute_final_style(styles), + is_function_def, }), narrow: None, narrow_depth: 0, @@ -4263,6 +4354,20 @@ impl<'a> BindingsBuilder<'a> { || base.is_definitely_unreachable) && flows.iter().all(|f| f.is_definitely_unreachable); + // For a regular loop, we merge the base so there's one extra branch being merged. + // For LoopDefinitelyRuns, we don't count the base as an extra branch because we + // know the loop body will definitely execute at least once. + let n_branches = flows.len() + + if matches!(merge_style, MergeStyle::Loop) { + 1 + } else { + 0 + }; + + // Count how many branches have a last_stmt_expr (potential type-based termination) + let n_branches_with_termination_key = + flows.iter().filter(|f| f.last_stmt_expr.is_some()).count(); + // Collect all termination keys from flows (for building dense MergeItems) let all_termination_keys: Vec>> = flows.iter().map(|f| f.last_stmt_expr).collect(); @@ -4337,7 +4442,13 @@ impl<'a> BindingsBuilder<'a> { promised: self.promised_idx(&key), key, }; - let flow_info = self.merged_flow_info(merge_item, phi_key, merge_style); + let flow_info = self.merged_flow_info( + merge_item, + phi_key, + merge_style, + n_branches, + n_branches_with_termination_key, + ); merged_flow_infos.insert_hashed(name, flow_info); } @@ -4376,6 +4487,7 @@ impl<'a> BindingsBuilder<'a> { info.value = Some(FlowValue { idx: phi_idx, style: FlowStyle::LoopRecursion, + is_function_def: false, }); info.narrow = None; } @@ -4412,7 +4524,6 @@ impl<'a> BindingsBuilder<'a> { is_while_true: bool, loop_definitely_runs: bool, ) { - let loop_raises = self.scopes.terminated_by_raise(); let finished_loop = self.scopes.finish_loop(); let (breaks, other_exits): (Vec, Vec) = finished_loop .exits @@ -4451,13 +4562,6 @@ impl<'a> BindingsBuilder<'a> { // also know that breaks are the only way to exit, so we drop the current flow, // which is actually unreachable. // - if is_while_true && breaks.is_empty() { - self.scopes.mark_flow_termination(if loop_raises { - TerminationKind::Raise - } else { - TerminationKind::Jump - }); - } if let Some(base) = base_if_breaks { if is_while_true { self.merge_flow(base, breaks, other_range, MergeStyle::Exclusive) diff --git a/pyrefly/lib/binding/stmt.rs b/pyrefly/lib/binding/stmt.rs index 1b2e37ab6e..28e86f21e8 100644 --- a/pyrefly/lib/binding/stmt.rs +++ b/pyrefly/lib/binding/stmt.rs @@ -1407,7 +1407,6 @@ impl<'a> BindingsBuilder<'a> { ); } Stmt::While(mut x) => { - let was_unreachable = self.scopes.is_definitely_unreachable(); self.setup_loop(x.range, &SmallSet::new()); // Note that it is important we ensure *after* we set up the loop, so that both the // narrowing and type checking are aware that the test might be impacted by changes @@ -1458,16 +1457,6 @@ impl<'a> BindingsBuilder<'a> { is_while_true, is_while_true, ); - if is_while_true - && !test_is_environment_independent - && self.scopes.has_terminated() - && !was_unreachable - { - // Another Python environment can skip the loop. - self.scopes.set_definitely_unreachable(false); - self.scopes - .mark_flow_termination(TerminationKind::StaticTest); - } } Stmt::If(mut x) => { let is_definitely_unreachable = self.scopes.is_definitely_unreachable(); diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index 31a22a65bc..1d498f941e 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -10,6 +10,28 @@ use crate::test::util::TestEnv; use crate::testcase; +testcase!( + test_class_body_initialization_is_independent_of_methods, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + r#" +def cond() -> bool: ... + +class Constructor: + if cond(): + x = 1 # E: Attribute `x` may be uninitialized + + def __init__(self) -> None: + self.x = 2 + +class Helper: + if cond(): + x = 1 # E: Attribute `x` may be uninitialized + + def initialize(self) -> None: + self.x = 2 +"#, +); + testcase!( test_attribute_initialization_inherited, TestEnv::new().enable_possibly_uninitialized_attribute_error(), @@ -88,6 +110,7 @@ class EarlyReturn: ); testcase!( + bug = "Retry loops retain a spurious fallthrough path", test_attribute_initialization_while_true_return, TestEnv::new().enable_possibly_uninitialized_attribute_error(), r#" @@ -97,7 +120,7 @@ class Retry: def __init__(self) -> None: while True: try: - self.conn = connect() + self.conn = connect() # E: Attribute `conn` may be uninitialized return except OSError: continue @@ -110,14 +133,6 @@ class EarlyReturn: self.value = 1 # E: Attribute `value` may be uninitialized return -class SuppressedLoopRaise: - def __init__(self) -> None: - from contextlib import suppress - with suppress(ValueError): - while True: - raise ValueError() - self.value = 1 - class ReturnOrBreak: def __init__(self, flag: bool) -> None: while True: diff --git a/pyrefly/lib/test/delayed_inference.rs b/pyrefly/lib/test/delayed_inference.rs index 204dc0a83c..b74bdb36dd 100644 --- a/pyrefly/lib/test/delayed_inference.rs +++ b/pyrefly/lib/test/delayed_inference.rs @@ -45,7 +45,7 @@ from typing import assert_type, Literal x = 5 while True: x = x + 1 -y = x # E: This code is unreachable +y = x assert_type(y, int) "#, ); diff --git a/pyrefly/lib/test/flow_branching.rs b/pyrefly/lib/test/flow_branching.rs index 52b66f2aaf..d7fe0b4e66 100644 --- a/pyrefly/lib/test/flow_branching.rs +++ b/pyrefly/lib/test/flow_branching.rs @@ -432,7 +432,6 @@ c = C() while True: if True: c = C() - break assert_type(c, C) "#, @@ -990,7 +989,7 @@ for x in []: pass # E: Expected an indented block while True: -pass # E: Expected an indented block # E: This code is unreachable +pass # E: Expected an indented block "#, ); @@ -2284,6 +2283,7 @@ def f() -> int: // elif condition only executes if the first `if` was False — walrus may not run. testcase!( + bug = "In order to fix false positives, we handled narrows differently in if/elif and introduced a false negative here", test_walrus_in_elif, r#" def condition() -> bool: ... @@ -2292,7 +2292,7 @@ def f() -> bool: pass elif (x := condition()): pass - return x # E: `x` may be uninitialized + return x # False negative: x winds up getting applied as if it were in the base flow due to the negative narrow "#, ); @@ -2380,6 +2380,7 @@ def f() -> None: // the walrus may not execute, so x should be possibly-uninitialized. testcase!( + bug = "Should report x as possibly uninitialized since the if branch does not terminate", test_walrus_in_elif_preceding_if_no_terminate, r#" def foo() -> bool: @@ -2393,7 +2394,7 @@ def f() -> None: pass elif (x := bar()) > 1: pass - print(x) # E: `x` may be uninitialized + print(x) # should be an error: x may be uninitialized "#, ); diff --git a/pyrefly/lib/test/lsp/hover_type.rs b/pyrefly/lib/test/lsp/hover_type.rs index c512c8e962..fd9bbf7099 100644 --- a/pyrefly/lib/test/lsp/hover_type.rs +++ b/pyrefly/lib/test/lsp/hover_type.rs @@ -478,7 +478,7 @@ while True: y = x # ^ "#; - let report = get_batched_lsp_operations_report_allow_error(&[("main", code)], get_test_report); + let report = get_batched_lsp_operations_report(&[("main", code)], get_test_report); assert_eq!( r#" # main.py diff --git a/pyrefly/lib/test/paramspec.rs b/pyrefly/lib/test/paramspec.rs index e0128661d9..3a2337e5a9 100644 --- a/pyrefly/lib/test/paramspec.rs +++ b/pyrefly/lib/test/paramspec.rs @@ -487,7 +487,7 @@ def run[**P, R](func: Callable[P, R], *args: P.args, **kwargs: P.kwargs) -> R: return func(*args,**kwargs) except Exception: pass - raise NotImplementedError("Unreachable") # E: This code is unreachable + raise NotImplementedError("Unreachable") "#, ); diff --git a/pyrefly/lib/test/perf.rs b/pyrefly/lib/test/perf.rs index 0059cf3648..8e8155973c 100644 --- a/pyrefly/lib/test/perf.rs +++ b/pyrefly/lib/test/perf.rs @@ -302,3 +302,37 @@ fn test_deeply_nested_expression_is_rejected() { errors[0].msg() ); } + +// Deferred attribute proofs must remain bounded even when the diagnostic is disabled. +#[test] +fn test_attribute_initialization_branch_growth() { + let mut source = String::from("def proceed() -> None: ...\nclass C:\n"); + for method in ["__init__", "helper"] { + source.push_str(&format!(" def {method}(self, flag: bool) -> None:\n")); + for attribute in 0..16 { + source.push_str(&format!( + " if flag:\n self.{method}_value_{attribute} = 1\n else:\n proceed()\n", + )); + } + for _ in 0..64 { + source.push_str( + " if flag:\n proceed()\n else:\n proceed()\n", + ); + } + } + for enabled in [false, true] { + let env = TestEnv::one("main", &source); + let env = if enabled { + env.enable_possibly_uninitialized_attribute_error() + } else { + env + }; + let (state, handle) = env.to_state(); + let errors = state + .transaction() + .get_errors([&handle("main")]) + .collect_errors() + .ordinary; + assert_eq!(errors.len(), if enabled { 16 } else { 0 }, "{errors:#?}"); + } +} diff --git a/pyrefly/lib/test/scope.rs b/pyrefly/lib/test/scope.rs index c04f56c489..9390d7979f 100644 --- a/pyrefly/lib/test/scope.rs +++ b/pyrefly/lib/test/scope.rs @@ -1071,7 +1071,7 @@ def try_except_twice(): except Exception: e3 # E: `e3` is uninitialized - e3 # E: `e3` is uninitialized + e3 # E: `e3` may be uninitialized def try_except_multiple_finally(): try: diff --git a/website/docs/error-kinds.mdx b/website/docs/error-kinds.mdx index 68fbc44c75..b4e1c4ddbe 100644 --- a/website/docs/error-kinds.mdx +++ b/website/docs/error-kinds.mdx @@ -1822,7 +1822,7 @@ The `strict` preset enables this check as an error. Pyrefly checks whether an assigned attribute exists after the class body or a recognized initialization method completes. Recognized methods include `__init__`, `__new__`, `__post_init__`, `__init_subclass__`, and supported test setup methods. Each path that completes normally, including an explicit `return`, must leave the attribute initialized. -Paths that raise, loop forever, or end in a `Never` or `NoReturn` call do not require an assignment. +Paths that raise or end in a `Never` or `NoReturn` call do not require an assignment. Deleting an attribute removes its initialization guarantee. ```python @@ -1837,6 +1837,7 @@ The check permits an inherited attribute when a base class other than `object` d An instance `__getattr__` fallback also suppresses this error for instance attributes. Pyrefly combines assignments from different methods leniently: one method that initializes the attribute on every completing path suffices. This includes ordinary helper methods, even when Pyrefly cannot prove that the constructor calls them. +Class-body definitions are checked separately, even when a method assigns the same name. Assignments inside nested functions do not initialize the enclosing method's attributes. Pyrefly reports this error at the attribute definition, even if the attribute is never read. From cc54c046afdf6b3a0704a0012b93e3854f3cbcea Mon Sep 17 00:00:00 2001 From: Asuka Minato Date: Fri, 23 Oct 2026 10:33:38 +0900 Subject: [PATCH 9/9] . --- pyrefly/lib/binding/scope.rs | 8 ++++++++ pyrefly/lib/binding/stmt.rs | 11 +++++++++++ pyrefly/lib/test/attributes.rs | 3 +-- pyrefly/lib/test/delayed_inference.rs | 2 +- pyrefly/lib/test/flow_branching.rs | 3 ++- pyrefly/lib/test/lsp/hover_type.rs | 2 +- pyrefly/lib/test/paramspec.rs | 2 +- 7 files changed, 25 insertions(+), 6 deletions(-) diff --git a/pyrefly/lib/binding/scope.rs b/pyrefly/lib/binding/scope.rs index 84f45cec40..44522fdc98 100644 --- a/pyrefly/lib/binding/scope.rs +++ b/pyrefly/lib/binding/scope.rs @@ -4524,6 +4524,7 @@ impl<'a> BindingsBuilder<'a> { is_while_true: bool, loop_definitely_runs: bool, ) { + let loop_raises = self.scopes.terminated_by_raise(); let finished_loop = self.scopes.finish_loop(); let (breaks, other_exits): (Vec, Vec) = finished_loop .exits @@ -4562,6 +4563,13 @@ impl<'a> BindingsBuilder<'a> { // also know that breaks are the only way to exit, so we drop the current flow, // which is actually unreachable. // + if is_while_true && breaks.is_empty() { + self.scopes.mark_flow_termination(if loop_raises { + TerminationKind::Raise + } else { + TerminationKind::Jump + }); + } if let Some(base) = base_if_breaks { if is_while_true { self.merge_flow(base, breaks, other_range, MergeStyle::Exclusive) diff --git a/pyrefly/lib/binding/stmt.rs b/pyrefly/lib/binding/stmt.rs index 28e86f21e8..1b2e37ab6e 100644 --- a/pyrefly/lib/binding/stmt.rs +++ b/pyrefly/lib/binding/stmt.rs @@ -1407,6 +1407,7 @@ impl<'a> BindingsBuilder<'a> { ); } Stmt::While(mut x) => { + let was_unreachable = self.scopes.is_definitely_unreachable(); self.setup_loop(x.range, &SmallSet::new()); // Note that it is important we ensure *after* we set up the loop, so that both the // narrowing and type checking are aware that the test might be impacted by changes @@ -1457,6 +1458,16 @@ impl<'a> BindingsBuilder<'a> { is_while_true, is_while_true, ); + if is_while_true + && !test_is_environment_independent + && self.scopes.has_terminated() + && !was_unreachable + { + // Another Python environment can skip the loop. + self.scopes.set_definitely_unreachable(false); + self.scopes + .mark_flow_termination(TerminationKind::StaticTest); + } } Stmt::If(mut x) => { let is_definitely_unreachable = self.scopes.is_definitely_unreachable(); diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index 1d498f941e..8fc9243b39 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -110,7 +110,6 @@ class EarlyReturn: ); testcase!( - bug = "Retry loops retain a spurious fallthrough path", test_attribute_initialization_while_true_return, TestEnv::new().enable_possibly_uninitialized_attribute_error(), r#" @@ -120,7 +119,7 @@ class Retry: def __init__(self) -> None: while True: try: - self.conn = connect() # E: Attribute `conn` may be uninitialized + self.conn = connect() return except OSError: continue diff --git a/pyrefly/lib/test/delayed_inference.rs b/pyrefly/lib/test/delayed_inference.rs index b74bdb36dd..204dc0a83c 100644 --- a/pyrefly/lib/test/delayed_inference.rs +++ b/pyrefly/lib/test/delayed_inference.rs @@ -45,7 +45,7 @@ from typing import assert_type, Literal x = 5 while True: x = x + 1 -y = x +y = x # E: This code is unreachable assert_type(y, int) "#, ); diff --git a/pyrefly/lib/test/flow_branching.rs b/pyrefly/lib/test/flow_branching.rs index d7fe0b4e66..51c6344185 100644 --- a/pyrefly/lib/test/flow_branching.rs +++ b/pyrefly/lib/test/flow_branching.rs @@ -432,6 +432,7 @@ c = C() while True: if True: c = C() + break assert_type(c, C) "#, @@ -989,7 +990,7 @@ for x in []: pass # E: Expected an indented block while True: -pass # E: Expected an indented block +pass # E: Expected an indented block # E: This code is unreachable "#, ); diff --git a/pyrefly/lib/test/lsp/hover_type.rs b/pyrefly/lib/test/lsp/hover_type.rs index fd9bbf7099..c512c8e962 100644 --- a/pyrefly/lib/test/lsp/hover_type.rs +++ b/pyrefly/lib/test/lsp/hover_type.rs @@ -478,7 +478,7 @@ while True: y = x # ^ "#; - let report = get_batched_lsp_operations_report(&[("main", code)], get_test_report); + let report = get_batched_lsp_operations_report_allow_error(&[("main", code)], get_test_report); assert_eq!( r#" # main.py diff --git a/pyrefly/lib/test/paramspec.rs b/pyrefly/lib/test/paramspec.rs index 3a2337e5a9..e0128661d9 100644 --- a/pyrefly/lib/test/paramspec.rs +++ b/pyrefly/lib/test/paramspec.rs @@ -487,7 +487,7 @@ def run[**P, R](func: Callable[P, R], *args: P.args, **kwargs: P.kwargs) -> R: return func(*args,**kwargs) except Exception: pass - raise NotImplementedError("Unreachable") + raise NotImplementedError("Unreachable") # E: This code is unreachable "#, );