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/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 df4cd05ed8..c72b45d37b 100644 --- a/pyrefly/lib/alt/solve.rs +++ b/pyrefly/lib/alt/solve.rs @@ -3016,6 +3016,40 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { .emit(); } } + BindingExpect::UninitializedAttributeCheck { + name, + range, + class, + is_instance, + initializations, + } => { + let initialized = initializations.iter().any(|initialized| { + initialized.is_initialized(|key| self.get_idx(key).ty().is_never()) + }); + 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, + ErrorKind::PossiblyUninitializedAttribute, + 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..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; @@ -1116,6 +1117,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 +1145,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 +1169,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 +1322,16 @@ pub enum BindingExpect { /// If any don't, the variable may be uninitialized. termination_keys: Vec>, }, + /// Check attribute initialization from the class body or any defining method. + UninitializedAttributeCheck { + name: Name, + range: TextRange, + 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 /// like `int | "str"` will raise a TypeError. @@ -1489,6 +1504,23 @@ impl DisplayWith for BindingExpect { termination_keys ) } + Self::UninitializedAttributeCheck { + name, + range, + class, + is_instance, + initializations, + } => { + write!( + f, + "UninitializedAttributeCheck({}, {}, {:?}, {}, {:?})", + name, + ctx.module().display(range), + class, + is_instance, + initializations + ) + } Self::ForwardRefUnion { left, right, diff --git a/pyrefly/lib/binding/bindings.rs b/pyrefly/lib/binding/bindings.rs index 4ba659de28..7f4b12fbf1 100644 --- a/pyrefly/lib/binding/bindings.rs +++ b/pyrefly/lib/binding/bindings.rs @@ -206,6 +206,107 @@ 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 { + const MAX_DEFERRED_PATHS: usize = 16; + 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(); + 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) + } + } + + 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, + /// 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), +} + +impl From for InitializedInClass { + fn from(initialized: InitializationStatus) -> Self { + match initialized { + InitializationStatus::Initialized => Self::Yes, + InitializationStatus::Uninitialized => Self::IfAny(Vec::new()), + deferred @ InitializationStatus::Deferred(_) => Self::IfAny(vec![deferred]), + } + } +} + pub type BindingEntry = (Index, IndexMap::Value>); table! { @@ -3102,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 13c3e716c5..2d18d3382f 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::InitializedInClass; 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,35 @@ 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 + ); + // Stub files declare attributes without runtime initialization. + if !self.module_info.path().is_interface() + && check_initialization + && let InitializedInClass::IfAny(initializations) = initialized + { + let is_instance = matches!( + &definition, + ClassFieldDefinition::DefinedInMethod { + receiver_kind: MethodSelfKind::Instance, + .. + } + ); + self.insert_binding( + KeyExpect::UninitializedAttributeCheck(range), + BindingExpect::UninitializedAttributeCheck { + name: name.key().clone(), + range, + class: class_indices.class_idx, + is_instance, + initializations, + }, + ); + } + fields.insert_hashed( name.clone(), ClassFieldProperties::new( diff --git a/pyrefly/lib/binding/django.rs b/pyrefly/lib/binding/django.rs index a5ca5b4e97..3b2b67f83e 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::InitializedInClass; 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..0d301f2090 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::InitializationStatus; use crate::binding::bindings::LegacyTParamCollector; use crate::binding::expr::Usage; use crate::binding::pattern::match_case_always_matches; @@ -172,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, } @@ -265,11 +266,14 @@ impl<'a> SelfAttrNames<'a> { .map(|(n, r)| { ( n, - InstanceAttribute( - vec![ExprOrBinding::Binding(Binding::Any(AnyStyle::Implicit))], - None, - r, - MethodSelfKind::Instance, + ( + 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 93e607f54c..44522fdc98 100644 --- a/pyrefly/lib/binding/scope.rs +++ b/pyrefly/lib/binding/scope.rs @@ -70,6 +70,8 @@ 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; use crate::binding::function::SelfAssignments; @@ -630,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 @@ -740,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. @@ -751,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, @@ -769,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, @@ -1048,8 +1064,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, } @@ -1069,7 +1087,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) { @@ -1098,29 +1116,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), + ) + }) }) } } @@ -1176,6 +1208,7 @@ struct ScopeMethod { 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 +1281,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 +2194,42 @@ 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(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, + 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 +2263,21 @@ impl Scopes { x: &ExprAttribute, value: ExprOrBinding, annotation: Option>, + has_value: bool, ) -> bool { + 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 has_value && !in_nested_function { + scope + .flow + .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. attr.0.push(value); @@ -2230,10 +2299,32 @@ impl Scopes { } 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( @@ -2530,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()); } @@ -3070,15 +3162,31 @@ 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 { + method + .return_attribute_initialization + .push(scope.flow.instance_attribute_initialization.clone()); + } + 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 +3248,7 @@ impl Scopes { pub fn finish_class_and_get_field_definitions( &mut self, ) -> ( - SmallMap, + SmallMap, NNModuleRegistrations, ) { let mut field_definitions = SmallMap::new(); @@ -3249,14 +3357,33 @@ impl Scopes { definition: value.idx, }, }; - field_definitions.insert_hashed(name.owned(), (definition, static_info.range)); + let initialized = match &value.style { + _ if value.is_function_def => InitializedInFlow::Yes, + FlowStyle::PossiblyUninitialized => InitializedInFlow::Conditionally, + FlowStyle::MaybeInitialized(keys) => { + InitializedInFlow::DeferredCheck(keys.clone()) + } + _ => InitializedInFlow::Yes, + }; + field_definitions.insert_hashed( + name.owned(), + ( + definition, + static_info.range, + InitializationStatus::from(initialized).into(), + ), + ); } }); // 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,13 +3392,14 @@ impl Scopes { receiver_kind: existing_receiver, }, _, + existing_initialized, )) = field_definitions.get_mut(name.key()) { 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. @@ -3285,6 +3413,19 @@ impl Scopes { *existing_receiver = MethodSelfKind::Class; } } + // Initialization is merged independently of definition priority. + // Any method can establish initialization, so combine their requirements with OR. + match (&mut *existing_initialized, initialized) { + (InitializedInClass::Yes, _) => {} + (_, InitializationStatus::Initialized) => { + *existing_initialized = InitializedInClass::Yes; + } + ( + InitializedInClass::IfAny(groups), + deferred @ InitializationStatus::Deferred(_), + ) => groups.push(deferred), + (InitializedInClass::IfAny(_), InitializationStatus::Uninitialized) => {} + } } else if !field_definitions.contains_key_hashed(name.as_ref()) { field_definitions.insert_hashed( name, @@ -3296,6 +3437,7 @@ impl Scopes { receiver_kind, }, range, + initialized.into(), ), ); } @@ -4010,6 +4152,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. @@ -4052,6 +4195,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. @@ -4136,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, @@ -4157,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, @@ -4226,6 +4372,34 @@ impl<'a> BindingsBuilder<'a> { 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(); @@ -4281,6 +4455,7 @@ impl<'a> BindingsBuilder<'a> { // 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, @@ -4312,6 +4487,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/binding/stmt.rs b/pyrefly/lib/binding/stmt.rs index 914860eac5..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,6 +1204,7 @@ impl<'a> BindingsBuilder<'a> { None, ), ); + let has_value = x.value.is_some(); let value = match x.value { Some(mut assigned) => { self.bind_attr_assign(attr.clone(), &mut assigned, |v, _| { @@ -1217,10 +1221,12 @@ impl<'a> BindingsBuilder<'a> { ExprOrBinding::Binding(Binding::Any(AnyStyle::Implicit)) } }; - 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), + has_value, + ) { self.error( x.range, ErrorKind::BadAssignment, diff --git a/pyrefly/lib/binding/target.rs b/pyrefly/lib/binding/target.rs index c130e644b6..d7384167bc 100644 --- a/pyrefly/lib/binding/target.rs +++ b/pyrefly/lib/binding/target.rs @@ -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, true); } Expr::Subscript(x) => { self.bind_subscript_assign_impl( diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index a7733fafa1..8fc9243b39 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -10,10 +10,542 @@ 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(), + 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 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#" +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` +"#, +); + +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", + r#" +def coin() -> bool: ... +class Base: + if coin(): + value = 1 # E: Attribute `value` may be uninitialized +"#, + ) + .enable_possibly_uninitialized_attribute_error() +} + +testcase!( + test_conditionally_defined_imported_attribute, + env_with_conditional_attribute(), + r#" +from foo import Base +class Child(Base): ... +Base.value +Child.value +"#, +); + +testcase!( + test_conditionally_defined_methods, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + 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, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + 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!( 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: ... @@ -25,8 +557,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 @@ -162,6 +694,56 @@ def f(a: A): "#, ); +testcase!( + test_conditional_attribute_initialized_in_helper, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + 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#" @@ -174,6 +756,208 @@ def f(a: A): "#, ); +testcase!( + test_attribute_initialization_deferred_methods, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), + 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_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(), + 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#" 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 a92759de9a..9390d7979f 100644 --- a/pyrefly/lib/test/scope.rs +++ b/pyrefly/lib/test/scope.rs @@ -1697,18 +1697,88 @@ 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, + TestEnv::new().enable_possibly_uninitialized_attribute_error(), 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 "#, ); 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..b4e1c4ddbe 100644 --- a/website/docs/error-kinds.mdx +++ b/website/docs/error-kinds.mdx @@ -1813,6 +1813,49 @@ 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. + +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 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. +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. +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 +preset = "strict" + +[errors] +possibly-uninitialized-attribute = "ignore" +``` + +This setting does not disable [`missing-attribute`](#missing-attribute). + ## potential-bad-keyword-argument Default severity: `ignore`