From ef06ddd3d802c7553fca24bdc8ee1c8a806a85ad Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Thu, 23 Apr 2026 16:41:15 -0700 Subject: [PATCH] Add laziness test suite for type checking dependencies (#3219) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Pull Request resolved: https://github.com/facebook/pyrefly/pull/3219 Adds a snapshot-based test suite documenting how much of a file's dependencies get type-checked when the file itself is checked. The laziness goal is that dependencies should stop at the earliest step (Load/Ast/Exports/Answers/Solutions) sufficient to resolve what callers actually need — e.g. a transitively-imported module used only as an annotation should not have its body inferred. Each test sets up a small module graph, checks one entry-point module, and records in a markdown snapshot the highest step each module reached plus the tree of cross-module demands observed. The snapshots capture current behavior — some document good laziness properties, others document opportunities where dependencies are computed further than callers need. Snapshots regenerate with `UPDATE_SNAPSHOTS=1`. Supporting infrastructure: - `DemandCollector` (`pyrefly_util::demand_tree`) records cross-module `LookupExport`/`LookupAnswer` events into a tree. Scoped to a single `Transaction` via `set_demand_collector` so parallel checks don't interfere. `enter()` returns an RAII span guard so the per-thread nesting stack stays balanced even when an Answer lookup panics. - `pyrefly check --report-demand-tree ` emits a JSON document (`{ module_steps, demand_tree }`). - The test harness renders the structured tree into a flat indented form for snapshot comparison, and aggregates pervasive `builtins`/`typing` demands into a single count so test output stays readable. Differential Revision: D102239243 --- crates/pyrefly_util/src/demand_tree.rs | 152 ++++++ crates/pyrefly_util/src/lib.rs | 1 + pyrefly/Cargo.toml | 7 +- pyrefly/lib/alt/answers.rs | 21 + pyrefly/lib/binding/bindings.rs | 1 + pyrefly/lib/commands/check.rs | 78 ++- pyrefly/lib/state/state.rs | 55 ++ pyrefly/lib/state/subscriber.rs | 58 ++- pyrefly/test_laziness/OPPORTUNITIES.md | 230 +++++++++ pyrefly/test_laziness/build.rs | 70 +++ pyrefly/test_laziness/mod.rs | 483 ++++++++++++++++++ .../test_annotated_return_breaks_cascade.md | 57 +++ .../test_laziness/test_attribute_inherited.md | 76 +++ .../test_attribute_on_class_itself.md | 78 +++ .../test_bare_import_forces_exports.md | 44 ++ .../test_deprecated_forces_exports.md | 50 ++ .../test_export_exists_forces_exports.md | 52 ++ .../test_import_class_as_annotation.md | 46 ++ .../test_import_class_instantiated.md | 47 ++ .../test_import_function_called.md | 35 ++ .../test_import_function_unused.md | 34 ++ .../test_import_star_forces_exports.md | 51 ++ .../test_is_final_forces_exports.md | 48 ++ ...ltiple_inheritance_solves_unique_fields.md | 73 +++ .../test_special_export_forces_exports.md | 51 ++ .../test_transitive_import_annotated.md | 47 ++ .../test_transitive_import_unannotated.md | 58 +++ .../test_unused_import_from_same_module.md | 53 ++ 28 files changed, 2041 insertions(+), 15 deletions(-) create mode 100644 crates/pyrefly_util/src/demand_tree.rs create mode 100644 pyrefly/test_laziness/OPPORTUNITIES.md create mode 100644 pyrefly/test_laziness/build.rs create mode 100644 pyrefly/test_laziness/mod.rs create mode 100644 pyrefly/test_laziness/test_annotated_return_breaks_cascade.md create mode 100644 pyrefly/test_laziness/test_attribute_inherited.md create mode 100644 pyrefly/test_laziness/test_attribute_on_class_itself.md create mode 100644 pyrefly/test_laziness/test_bare_import_forces_exports.md create mode 100644 pyrefly/test_laziness/test_deprecated_forces_exports.md create mode 100644 pyrefly/test_laziness/test_export_exists_forces_exports.md create mode 100644 pyrefly/test_laziness/test_import_class_as_annotation.md create mode 100644 pyrefly/test_laziness/test_import_class_instantiated.md create mode 100644 pyrefly/test_laziness/test_import_function_called.md create mode 100644 pyrefly/test_laziness/test_import_function_unused.md create mode 100644 pyrefly/test_laziness/test_import_star_forces_exports.md create mode 100644 pyrefly/test_laziness/test_is_final_forces_exports.md create mode 100644 pyrefly/test_laziness/test_multiple_inheritance_solves_unique_fields.md create mode 100644 pyrefly/test_laziness/test_special_export_forces_exports.md create mode 100644 pyrefly/test_laziness/test_transitive_import_annotated.md create mode 100644 pyrefly/test_laziness/test_transitive_import_unannotated.md create mode 100644 pyrefly/test_laziness/test_unused_import_from_same_module.md diff --git a/crates/pyrefly_util/src/demand_tree.rs b/crates/pyrefly_util/src/demand_tree.rs new file mode 100644 index 0000000000..31a005cf66 --- /dev/null +++ b/crates/pyrefly_util/src/demand_tree.rs @@ -0,0 +1,152 @@ +/* + * 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. + */ + +//! Demand tree collection for debugging and testing laziness. +//! +//! A [`DemandCollector`] records cross-module demand calls into a tree. A +//! collector is usually owned by a `Transaction`, scoping collection to a +//! single check run — so parallel checks don't interfere with one another. +//! Parent/child nesting is tracked via a per-thread scratch stack. +//! +//! The tree is machine-readable via serde. `#[serde(default)]` on optional +//! fields lets the schema grow without breaking existing consumers. + +use std::cell::RefCell; +use std::fmt; +use std::sync::Arc; +use std::sync::Mutex; + +use serde::Deserialize; +use serde::Serialize; + +/// What kind of cross-module demand a node represents. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(tag = "type", rename_all = "snake_case")] +pub enum DemandKind { + /// A leaf event for an Exports-level demand (e.g. `module_exists`, + /// `export_exists`). Carries the reason string identifying which + /// `LookupExport` method triggered the demand. + Exports { reason: String }, + /// A cross-module Answer lookup span. May have children if the + /// computation recursively demanded data from other modules. + /// `key` is the `Debug`-formatted lookup key. + Answer { key: String }, +} + +/// A node in the demand tree. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct DemandNode { + /// The module that made the demand. + pub from: String, + /// The module the demand was made against. + pub target: String, + /// What kind of demand this was. + pub kind: DemandKind, + /// Nested demands made while computing this one. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub children: Vec, +} + +thread_local! { + /// Per-thread stack of in-flight parent spans, used to nest closed demand + /// nodes under their caller. This is inherently per-thread scratch space: + /// enter and exit always happen on the same thread, and the stack empties + /// itself as long as every enter has a matching exit. + static STACK: RefCell> = const { RefCell::new(Vec::new()) }; +} + +/// A demand-tree collection session. Cloning produces another handle to the +/// same underlying roots (the collector is reference-counted internally). +#[derive(Clone, Default)] +pub struct DemandCollector { + roots: Arc>>, +} + +impl DemandCollector { + pub fn new() -> Self { + Self::default() + } + + /// Open a demand span for a cross-module Answer lookup. Hold the returned + /// guard for the duration of the demand — dropping the guard (including + /// on unwind) closes the span and attaches it to its parent, keeping + /// enter/exit balanced even across panics. `key` is the lookup key, + /// formatted via `Debug`. + #[inline] + pub fn enter( + &self, + from: impl fmt::Display, + target: impl fmt::Display, + key: impl fmt::Debug, + ) -> DemandSpan<'_> { + STACK.with(|stack| { + stack.borrow_mut().push(DemandNode { + from: from.to_string(), + target: target.to_string(), + kind: DemandKind::Answer { + key: format!("{key:?}"), + }, + children: Vec::new(), + }); + }); + DemandSpan { collector: self } + } + + /// Record a leaf event for an Exports-level demand. `reason` identifies + /// which `LookupExport` method triggered the demand. + #[inline] + pub fn exports_event(&self, from: impl fmt::Display, target: impl fmt::Display, reason: &str) { + self.attach(DemandNode { + from: from.to_string(), + target: target.to_string(), + kind: DemandKind::Exports { + reason: reason.to_owned(), + }, + children: Vec::new(), + }); + } + + /// Attach a completed node either to the current parent on the stack or, + /// if no parent is in flight, to the collector's shared root list. + fn attach(&self, node: DemandNode) { + let leftover = STACK.with(|stack| { + let mut stack = stack.borrow_mut(); + match stack.last_mut() { + Some(parent) => { + parent.children.push(node); + None + } + None => Some(node), + } + }); + if let Some(node) = leftover { + self.roots.lock().unwrap().push(node); + } + } + + /// Take the collected tree roots, leaving the collector empty. + pub fn take_roots(&self) -> Vec { + std::mem::take(&mut *self.roots.lock().unwrap()) + } +} + +/// RAII guard for an in-flight Answer demand span. Dropping the guard — +/// whether via normal control flow or unwind — pops the span off the +/// per-thread stack and attaches it to its parent (or to the collector's +/// roots if no parent is in flight). +#[must_use = "demand span is closed when the guard is dropped; hold it for the duration of the demand"] +pub struct DemandSpan<'a> { + collector: &'a DemandCollector, +} + +impl Drop for DemandSpan<'_> { + fn drop(&mut self) { + if let Some(node) = STACK.with(|stack| stack.borrow_mut().pop()) { + self.collector.attach(node); + } + } +} diff --git a/crates/pyrefly_util/src/lib.rs b/crates/pyrefly_util/src/lib.rs index 1b79e8abae..4865f52ebe 100644 --- a/crates/pyrefly_util/src/lib.rs +++ b/crates/pyrefly_util/src/lib.rs @@ -31,6 +31,7 @@ pub mod absolutize; pub mod arc_id; pub mod args; pub mod assert_size; +pub mod demand_tree; pub mod display; pub mod events; pub mod forgetter; diff --git a/pyrefly/Cargo.toml b/pyrefly/Cargo.toml index d21221e500..4e182259bb 100644 --- a/pyrefly/Cargo.toml +++ b/pyrefly/Cargo.toml @@ -1,4 +1,4 @@ -# @generated by autocargo from //pyrefly/pyrefly:[pyrefly,pyrefly_library,pyrefly_lsp_interaction_tests] +# @generated by autocargo from //pyrefly/pyrefly:[pyrefly,pyrefly_laziness_tests,pyrefly_library,pyrefly_lsp_interaction_tests] [package] name = "pyrefly" @@ -7,6 +7,7 @@ authors = ["Meta"] edition = "2024" repository = "https://github.com/facebook/pyrefly" license = "MIT" +build = "test_laziness/build.rs" [lib] path = "lib/lib.rs" @@ -15,6 +16,10 @@ path = "lib/lib.rs" name = "pyrefly" path = "bin/main.rs" +[[test]] +name = "pyrefly_laziness_tests" +path = "test_laziness/mod.rs" + [[test]] name = "pyrefly_lsp_interaction_tests" path = "lib/test/lsp/lsp_interaction/mod.rs" diff --git a/pyrefly/lib/alt/answers.rs b/pyrefly/lib/alt/answers.rs index 579694aad1..1de5ebe6f3 100644 --- a/pyrefly/lib/alt/answers.rs +++ b/pyrefly/lib/alt/answers.rs @@ -37,6 +37,27 @@ use crate::alt::traits::Solve; use crate::binding::binding::AnyIdx; use crate::binding::binding::Exported; use crate::binding::binding::Key; +use crate::binding::binding::KeyAbstractClassCheck; +use crate::binding::binding::KeyAnnotation; +use crate::binding::binding::KeyClass; +use crate::binding::binding::KeyClassBaseType; +use crate::binding::binding::KeyClassField; +use crate::binding::binding::KeyClassMetadata; +use crate::binding::binding::KeyClassMro; +use crate::binding::binding::KeyClassSynthesizedFields; +use crate::binding::binding::KeyConsistentOverrideCheck; +use crate::binding::binding::KeyDecoratedFunction; +use crate::binding::binding::KeyDecorator; +use crate::binding::binding::KeyExpect; +use crate::binding::binding::KeyExport; +use crate::binding::binding::KeyLegacyTypeParam; +use crate::binding::binding::KeyTParams; +use crate::binding::binding::KeyTypeAlias; +use crate::binding::binding::KeyUndecoratedFunction; +use crate::binding::binding::KeyVariance; +use crate::binding::binding::KeyVarianceCheck; +use crate::binding::binding::KeyYield; +use crate::binding::binding::KeyYieldFrom; use crate::binding::binding::Keyed; use crate::binding::bindings::BindingEntry; use crate::binding::bindings::BindingTable; diff --git a/pyrefly/lib/binding/bindings.rs b/pyrefly/lib/binding/bindings.rs index 67e86e7210..eda033c628 100644 --- a/pyrefly/lib/binding/bindings.rs +++ b/pyrefly/lib/binding/bindings.rs @@ -52,6 +52,7 @@ use vec1::Vec1; use vec1::vec1; use crate::binding::binding::AnnotationTarget; +use crate::binding::binding::AnyIdx; use crate::binding::binding::Binding; use crate::binding::binding::BindingAnnotation; use crate::binding::binding::BindingExport; diff --git a/pyrefly/lib/commands/check.rs b/pyrefly/lib/commands/check.rs index a5a75cabe7..973bc32a07 100644 --- a/pyrefly/lib/commands/check.rs +++ b/pyrefly/lib/commands/check.rs @@ -76,6 +76,7 @@ use crate::state::require::RequireLevels; use crate::state::state::State; use crate::state::state::Transaction; use crate::state::subscriber::ProgressBarSubscriber; +use crate::state::subscriber::TestSubscriber; /// Result data from a non-watch check run, used for telemetry logging. pub struct CheckResult { @@ -262,6 +263,10 @@ struct OutputArgs { /// Format for pysa report output (json or capnp) #[arg(long, value_enum, default_value_t = report::pysa::PysaFormat::Capnp)] report_pysa_format: report::pysa::PysaFormat, + /// Report the cross-module demand tree (aggregated summary of LookupAnswer + /// and LookupExport calls). Useful for analyzing laziness properties. + #[arg(long, value_name = "OUTPUT_FILE")] + report_demand_tree: Option, /// Generate a CinderX-format type report (experimental, internal-only). #[arg(long, value_name = "OUTPUT_DIR", hide = true)] report_cinderx: Option, @@ -949,15 +954,22 @@ impl CheckArgs { } let type_check_start = Instant::now(); - let show_progress_bar = - self.output.summary != Summary::None && !self.output.no_progress_bar; - if show_progress_bar { - transaction.set_subscriber(Some(Box::new(ProgressBarSubscriber::new()))); - } + let demand_tree_subscriber = if self.output.report_demand_tree.is_some() { + transaction + .set_demand_collector(Some(pyrefly_util::demand_tree::DemandCollector::new())); + let sub = TestSubscriber::new(); + transaction.set_subscriber(Some(Box::new(sub.dupe()))); + Some(sub) + } else { + let show_progress_bar = + self.output.summary != Summary::None && !self.output.no_progress_bar; + if show_progress_bar { + transaction.set_subscriber(Some(Box::new(ProgressBarSubscriber::new()))); + } + None + }; transaction.run(handles, require, None); - if show_progress_bar { - transaction.set_subscriber(None); - } + transaction.set_subscriber(None); let loads = if self.behavior.check_all { transaction.get_all_errors() @@ -1201,6 +1213,12 @@ impl CheckArgs { report::dependency_graph::dependency_graph(transaction, handles), )?; } + if let Some(path) = &self.output.report_demand_tree { + let roots = transaction.take_demand_roots(); + let module_steps = demand_tree_subscriber.unwrap().finish_detailed(); + let output = demand_tree_report_json(&roots, &module_steps); + fs_anyhow::write(path, output)?; + } if self.behavior.expectations { loads.check_against_expectations()?; Ok((CommandExitStatus::Success, output_errors)) @@ -1212,6 +1230,50 @@ impl CheckArgs { } } +/// Serialize the demand tree and per-module step info as a pretty-printed +/// JSON document. +fn demand_tree_report_json( + roots: &[pyrefly_util::demand_tree::DemandNode], + module_steps: &SmallMap, +) -> String { + use serde::Serialize; + + #[derive(Serialize)] + struct ModuleStep { + module: String, + last_step: &'static str, + } + + #[derive(Serialize)] + struct Report<'a> { + module_steps: Vec, + demand_tree: &'a [pyrefly_util::demand_tree::DemandNode], + } + + let mut steps: Vec = module_steps + .iter() + .map(|(handle, info)| ModuleStep { + module: handle.module().as_str().to_owned(), + last_step: match info.last_step { + Some(crate::state::steps::Step::Load) => "Load", + Some(crate::state::steps::Step::Ast) => "Ast", + Some(crate::state::steps::Step::Exports) => "Exports", + Some(crate::state::steps::Step::Answers) => "Answers", + Some(crate::state::steps::Step::Solutions) => "Solutions", + None => "Nothing", + }, + }) + .collect(); + // Stable ordering makes diffing reports tractable. + steps.sort_by(|a, b| a.module.cmp(&b.module)); + + let report = Report { + module_steps: steps, + demand_tree: roots, + }; + serde_json::to_string_pretty(&report).expect("demand tree report should always serialize") +} + #[cfg(test)] mod tests { use std::path::PathBuf; diff --git a/pyrefly/lib/state/state.rs b/pyrefly/lib/state/state.rs index ed2a256fc9..eb13a8c3ed 100644 --- a/pyrefly/lib/state/state.rs +++ b/pyrefly/lib/state/state.rs @@ -41,6 +41,7 @@ use pyrefly_python::module_path::ModulePathDetails; use pyrefly_python::sys_info::SysInfo; use pyrefly_types::type_alias::TypeAliasIndex; use pyrefly_util::arc_id::ArcId; +use pyrefly_util::demand_tree::DemandCollector; use pyrefly_util::events::CategorizedEvents; use pyrefly_util::fs_anyhow; use pyrefly_util::lock::Mutex; @@ -598,6 +599,7 @@ impl<'a> TransactionData<'a> { sub_task_telemetry: None, readable, timing: Default::default(), + demand_collector: None, }) } else { Err(state_lock_blocked) @@ -617,6 +619,10 @@ pub struct Transaction<'a> { readable: RwLockReadGuard<'a, StateData>, /// Atomic nanosecond counters accumulated across worker threads during a transaction. timing: TransactionTimingCounters, + /// Optional demand-tree collector for this run. Set via + /// [`Transaction::set_demand_collector`] before `run`; read by + /// `TransactionHandle` event sites. + demand_collector: Option, } impl<'a> Transaction<'a> { @@ -628,6 +634,7 @@ impl<'a> Transaction<'a> { sub_task_telemetry: _, readable, timing, + demand_collector: _, } = self; drop(readable); let mut stats = stats.into_inner(); @@ -645,6 +652,23 @@ impl<'a> Transaction<'a> { self.data.subscriber = subscriber; } + /// Install a demand-tree collector for this transaction. Pass `None` to + /// disable collection. Collected events come from `TransactionHandle` + /// event sites; call [`Transaction::take_demand_roots`] after `run` to + /// harvest the tree. + pub fn set_demand_collector(&mut self, collector: Option) { + self.demand_collector = collector; + } + + /// Take the demand-tree roots collected during this run. Returns an + /// empty vec if no collector was installed. + pub fn take_demand_roots(&self) -> Vec { + self.demand_collector + .as_ref() + .map(|c| c.take_roots()) + .unwrap_or_default() + } + /// Set the pysa reporter for inline extraction during type checking. pub fn set_pysa_reporter(&mut self, reporter: Option>) { self.data.pysa_reporter = reporter; @@ -1307,6 +1331,11 @@ impl<'a> Transaction<'a> { ns_counter.fetch_add(elapsed_ns, Ordering::Relaxed); count_counter.fetch_add(1, Ordering::Relaxed); + // Notify subscriber of step completion. + if let Some(subscriber) = &self.data.subscriber { + subscriber.step_computed(&module_data.handle, todo); + } + let mut load_result = None; // Compute which exports changed for fine-grained invalidation. // All diffing is done at the Solutions step, using old data @@ -2485,8 +2514,12 @@ impl<'a> TransactionHandle<'a> { module: ModuleName, f: impl FnOnce(&Exports, &Self) -> T, dep: ModuleDep, + reason: &str, ) -> Option { let module_data = self.get_module(module, None, dep).finding()?; + if let Some(c) = self.transaction.demand_collector.as_ref() { + c.exports_event(self.module_data.handle.module(), module, reason); + } let exports = self.transaction.lookup_export(module_data); let lookup = TransactionHandle { transaction: self.transaction, @@ -2597,6 +2630,7 @@ impl<'a> LookupExport for TransactionHandle<'a> { module, |exports, lookup| exports.exports(lookup).contains_key(name), dep, + "export_exists", ) .unwrap_or(false) } @@ -2606,12 +2640,16 @@ impl<'a> LookupExport for TransactionHandle<'a> { module, |exports, lookup| exports.wildcard(lookup), ModuleDep::Wildcard, + "get_wildcard", ) } fn module_exists(&self, module: ModuleName) -> FindingOrError<()> { self.get_module(module, None, ModuleDep::Exists) .map(|module_data| { + if let Some(c) = self.transaction.demand_collector.as_ref() { + c.exports_event(self.module_data.handle.module(), module, "module_exists"); + } self.transaction.lookup_export(module_data); }) } @@ -2621,6 +2659,7 @@ impl<'a> LookupExport for TransactionHandle<'a> { module, |exports, _lookup| exports.is_submodule_imported_implicitly(name), ModuleDep::NameMetadata(name.clone()), + "is_submodule_imported_implicitly", ) .unwrap_or(false) } @@ -2636,6 +2675,7 @@ impl<'a> LookupExport for TransactionHandle<'a> { .collect::>() }, ModuleDep::Exists, + "get_every_export_untracked", ) } @@ -2650,6 +2690,7 @@ impl<'a> LookupExport for TransactionHandle<'a> { _ => None, }, ModuleDep::NameMetadata(name.clone()), + "get_deprecated", )? } @@ -2663,6 +2704,7 @@ impl<'a> LookupExport for TransactionHandle<'a> { ) }, ModuleDep::NameMetadata(name.clone()), + "is_reexport", ) .unwrap_or(false) } @@ -2691,6 +2733,7 @@ impl<'a> LookupExport for TransactionHandle<'a> { } }, ModuleDep::NameMetadata(name.clone()), + "is_special_export", )??; match next { @@ -2715,6 +2758,7 @@ impl<'a> LookupExport for TransactionHandle<'a> { _ => None, }, ModuleDep::NameMetadata(name.clone()), + "docstring_range", )? } @@ -2737,6 +2781,7 @@ impl<'a> LookupExport for TransactionHandle<'a> { None => Err(false), }, ModuleDep::NameMetadata(name.clone()), + "is_final", ); match next { @@ -2772,6 +2817,14 @@ impl<'a> LookupAnswer for TransactionHandle<'a> { .get_module(module, path, ModuleDep::Key(k.to_anykey())) .finding() .unwrap(); + // Hold the span guard for the duration of the lookup so that a panic + // inside `lookup_answer` still closes the span (keeping the per-thread + // demand stack balanced on this worker for subsequent demands). + let _demand_span = self + .transaction + .demand_collector + .as_ref() + .map(|c| c.enter(self.module_data.handle.module(), module, k)); let res = self.transaction.lookup_answer(module_data, k, thread_state); if res.is_none() { let msg = format!( @@ -3050,6 +3103,7 @@ impl State { }), sub_task_telemetry: None, timing: Default::default(), + demand_collector: None, data: TransactionData { state: self, stdlib, @@ -3121,6 +3175,7 @@ impl State { stats, sub_task_telemetry: _, timing, + demand_collector: _, data: TransactionData { stdlib, diff --git a/pyrefly/lib/state/subscriber.rs b/pyrefly/lib/state/subscriber.rs index 011815b82c..527395c2e6 100644 --- a/pyrefly/lib/state/subscriber.rs +++ b/pyrefly/lib/state/subscriber.rs @@ -19,6 +19,7 @@ use starlark_map::small_map::SmallMap; use crate::state::load::Load; use crate::state::state::Transaction; +use crate::state::steps::Step; /// Trait to capture which handles are executed by `State`. /// Calls to `start_work` and `finish_work` will be paired. @@ -42,22 +43,38 @@ pub trait Subscriber: Sync { result: &Arc, exports_changed: bool, ); + + /// Called after each step is computed for a module. Default no-op. + fn step_computed(&self, _handle: &Handle, _step: Step) {} +} + +/// Per-module tracking info for TestSubscriber. +#[derive(Debug, Clone)] +pub struct TestModuleInfo { + pub start_count: usize, + pub load: Option>, + /// The highest step computed for this module. + pub last_step: Option, } /// A subscriber that validates all start/finish are paired and returns the final load states. #[derive(Debug, Default, Clone, Dupe)] -pub struct TestSubscriber(Arc>)>>>); +pub struct TestSubscriber(Arc>>); impl Subscriber for TestSubscriber { fn start_work(&self, handle: &Handle) { let mut value = self.0.lock(); match value.entry(handle.dupe()) { Entry::Vacant(e) => { - e.insert((1, None)); + e.insert(TestModuleInfo { + start_count: 1, + load: None, + last_step: None, + }); } Entry::Occupied(mut e) => { - e.get_mut().0 += 1; - e.get_mut().1 = None; + e.get_mut().start_count += 1; + e.get_mut().load = None; } } } @@ -67,11 +84,32 @@ impl Subscriber for TestSubscriber { match value.entry(handle.dupe()) { Entry::Vacant(_) => panic!("Handle finished but never started: {handle:?}"), Entry::Occupied(mut e) => { - assert!(e.get().1.is_none()); - e.get_mut().1 = Some(result.dupe()); + assert!(e.get().load.is_none()); + e.get_mut().load = Some(result.dupe()); } } } + + fn step_computed(&self, handle: &Handle, step: Step) { + let mut value = self.0.lock(); + if let Some(info) = value.get_mut(handle) { + info.last_step = Some(match info.last_step { + Some(prev) if prev > step => prev, + _ => step, + }); + } + // If the module wasn't started yet (e.g., prefetched), add it. + else { + value.insert( + handle.dupe(), + TestModuleInfo { + start_count: 0, + load: None, + last_step: Some(step), + }, + ); + } + } } impl TestSubscriber { @@ -82,6 +120,14 @@ impl TestSubscriber { /// For each handle, return a pair of (the number of times each handle started, the final load state). /// Panics if any handle was started but not finished. pub fn finish(self) -> SmallMap>)> { + mem::take(&mut *self.0.lock()) + .into_iter() + .map(|(h, info)| (h, (info.start_count, info.load))) + .collect() + } + + /// Return per-module info including step tracking. + pub fn finish_detailed(self) -> SmallMap { mem::take(&mut *self.0.lock()) } } diff --git a/pyrefly/test_laziness/OPPORTUNITIES.md b/pyrefly/test_laziness/OPPORTUNITIES.md new file mode 100644 index 0000000000..083d8ae0ba --- /dev/null +++ b/pyrefly/test_laziness/OPPORTUNITIES.md @@ -0,0 +1,230 @@ +# Laziness Opportunities + +Observations from the laziness test demand trees that represent +opportunities to reduce unnecessary work. + +## Impact ranking (from `--report-demand-tree` on 5 real-world files) + +1. **LookupExport during binding** (32-84% of demands) — `is_special_export` + alone is 32-84%. 96.5% of dependency modules exist only for this. +2. **MRO walk without early exit** (10-40% of demands) — drives + `KeyClassSynthesizedFields` (31%), `KeyClassMro` (10%), and part of + `KeyClassMetadata` (19%). Worst for files with deep class hierarchies. +3. **Full function signature on import** — resolves 11 keys per imported + function regardless of usage. +4. **Class metadata for annotation-only usage** — `is_typed_dict()` check + on every class-as-annotation. +5. **Eagerly resolved builtins** — 150 KeyExport + cascading demands per + module. Fixed cost but adds up. +6. **Multiple inheritance check resolves unique fields** — low volume but + real waste for wide hierarchies. + +## Eagerly resolved builtins + +Every module resolves ~150 `KeyExport` entries from `builtins` +(int, str, bool, list, dict, OverflowError, ValueError, ...) even +if the code never references them. This is because `inject_builtins` +creates `Binding::Import` for every name returned by +`get_wildcard(builtins)`, and the solver resolves all bindings. + +**Visible in:** Every test — `(N builtin demands hidden)` line. + +**Ideal behavior:** Only resolve builtin names that are actually +referenced in the module's code. Unreferenced builtins should not +trigger cross-module key solving. + +**Root cause:** `inject_builtins` in `bindings.rs` imports all +builtin names via wildcard. The solver resolves every binding +including unused ones. + +## Full function signature resolved on import + +When `from b import helper` is written, pyrefly resolves the full +function signature even if `helper` is never called. The demand tree +for both `test_import_function_unused` and `test_import_function_called` +shows identical work: `a -> b::KeyExport("helper")` triggers resolving +11 keys in `b` including return annotation, decorated/undecorated +function, and legacy type param checks. + +**Visible in:** `test_import_function_unused.md` and +`test_import_function_called.md` — identical demand trees despite +different usage. + +**Ideal behavior:** `KeyExport(helper)` should return a lightweight +handle (function name + scope ID). The signature should only be +resolved when the function is actually called or its type is inspected. + +**Root cause:** `KeyExport` for a function forwards to the function's +`Key::Definition`, which triggers the full solve chain including +`KeyDecoratedFunction` → `KeyUndecoratedFunction` → return annotation +→ legacy type param checks. + +## Class metadata resolved for annotation-only usage + +When a class is used only as a type annotation (`x: Foo`), pyrefly +resolves `KeyClassMetadata` (1 cross-module demand). This is much +better than instantiation (6 metadata demands) but still unnecessary. + +**Visible in:** `test_import_class_as_annotation.md` — 2 demands: +`KeyExport("Foo")` + `KeyClassMetadata(0)`. + +**Why it happens:** `type_of_instance` in targs.rs:358 calls +`get_metadata_for_class` to check `is_typed_dict()`. This is needed +because `Type::ClassType` and `Type::TypedDict` are different enum +variants — the code must decide which to construct when promoting a +class reference to a type form. The `is_typed_dict` check requires +resolving the class's base classes to see if any ancestor is +`TypedDict`, which can cascade up the MRO. + +**Ideal behavior:** Unify `ClassType` and `TypedDict` into a single +type variant so the TypedDict distinction can be deferred until +TypedDict-specific features are actually used (field lookup, +structural matching, etc.). This would eliminate the metadata demand +for annotation-only class usage. + +## LookupExport during binding forces transitive exports + +Every `LookupExport` method (`module_exists`, `export_exists`, +`get_wildcard`, `is_special_export`, `is_final`, `get_deprecated`) +calls `demand(Step::Exports)` on the target module. These calls happen +during `Bindings::new`, meaning that when module B is being bound, ALL +of B's imports have their exports eagerly computed — even if the caller +(module A) never uses anything from those transitive dependencies. + +**Visible in:** 6 tests all show `c: Exports` with no demand tree +entries pointing to C: +- `test_bare_import_forces_exports` — `import c` triggers `module_exists(c)` +- `test_import_star_forces_exports` — `from c import *` triggers `get_wildcard(c)` +- `test_export_exists_forces_exports` — `from c import Foo` triggers `export_exists(c, "Foo")` +- `test_deprecated_forces_exports` — `from c import old_func` triggers `get_deprecated(c, "old_func")` +- `test_is_final_forces_exports` — `X = 2` (reassigning import) triggers `is_final(c, "X")` +- `test_special_export_forces_exports` — `T = MyTypeVar("T")` triggers `is_special_export(c, "MyTypeVar")` + +Also visible in `test_unused_import_from_same_module` and +`test_transitive_import_annotated` where `c: Exports` appears. + +**Real-world impact:** `--report-demand-tree` across 5 real-world files +shows `Exports(is_special_export)` accounts for 32-84% of ALL cross-module +demands. In one file with ~16,000 dependency modules, a single imported +module is checked 5,334x for special export status during binding. Across +all dependency modules, 96.5% are at Exports only — computed solely +because of LookupExport binding calls. + +**Ideal behavior:** Module C should not be computed at all (`c: Nothing`) +when A doesn't need it. B's bindings should be constructible without +demanding exports from B's own imports. + +**Root cause:** 18 call sites in `bindings.rs`, `stmt.rs`, and `scope.rs` +use `self.lookup.*` which goes through `TransactionHandle::with_exports` +→ `lookup_export` → `demand(Step::Exports)`. + +**Deferral strategies by call site:** +- `module_exists` / `export_exists` — create optimistic `Binding::Import`, + validate at solve time (solver already does cascading check: + export → submodule → `__getattr__` → error) +- `is_final` — move to solve time (emit error when solving the assignment) +- `get_deprecated` — move to solve time (emit warning when solving the import) +- `is_special_export` — hardest to defer; controls which `Binding` variant + is created (e.g., `Binding::TypeVar` vs `Binding::NameAssign`). Could use + syntactic name matching for the common case, with solve-time validation + for re-exports +- `get_wildcard` — unavoidable at bind time (binder needs the set of names + to create binding table entries). But only affects `from X import *` +- `module_exists` for builtins — fixed cost, unavoidable + +## Note on repeated demands to the same key + +Multiple code paths may demand the same key (e.g., `KeyClassMetadata`) +during a single operation like class instantiation. This is NOT a +concern — the Calculation cell caches the result, so repeated lookups +are just hash lookups + Arc clones with negligible cost. The demand +trees in the tests show these repeated lookups but they are not +optimization targets. Only demands for truly UNNECESSARY keys matter. + +## MRO computed even when attribute is on the class itself + +When accessing `c.child_attr` where `child_attr` is defined on `Child` +(not inherited), pyrefly still walks the full MRO, resolving parent +class `Base` from module `c`. + +**Visible in:** `test_attribute_on_class_itself.md` — `c` (Base module) +has 7 solved keys despite the attribute being on `Child`. + +**Why it happens:** `get_class_member_impl` in class_field.rs:3538 +calls `get_field_from_mro` which walks ALL ancestors to collect +candidate attributes, even if the attribute is found on the first +class. The code in attr.rs:651 does `for ancestor in mro.ancestors_no_object()` +without early exit. + +**Real-world impact:** `--report-demand-tree` on a file with deep class +hierarchies (~16 interface classes) shows 90,161 `KeyClassSynthesizedFields` +demands (30.8% of all demands) and 28,497 `KeyClassMro` demands (9.7%) — +both consequences of the full MRO walk. + +**Ideal behavior:** Check the class itself first. Only walk the MRO +if the attribute is not found on the class itself. + +## Abstract class check on every instantiation + +Every `Foo()` call triggers `KeyAbstractClassCheck` which enumerates +ALL abstract methods across ALL parent classes. For a class with no +abstract parents, this is pure overhead. + +**Visible in:** `test_import_class_instantiated.md` — `KeyAbstractClassCheck` +appears. In `test_attribute_inherited.md` it cascades into `b -> c::KeyClassMetadata`. + +**Why it happens:** `construct_class` in call.rs calls +`get_abstract_members_for_class` which recursively checks all parent +classes for unimplemented abstract methods, even when there are none. + +**Ideal behavior:** Defer abstract class checking to error reporting. +Or: cache abstract status on the class metadata so it's a flag check +rather than a full hierarchy walk. If no parent in the MRO extends +`ABC` or has metaclass `ABCMeta`, skip the check entirely. + +## Multiple inheritance check resolves unique parent fields + +`check_consistent_multiple_inheritance` iterates each parent's fields +and calls `get_class_member` to resolve the field type for ALL of them. +But only fields that appear in MULTIPLE parents need type resolution +(line 3382 checks `len() > 1`). Fields unique to a single parent are +resolved then discarded. + +**Visible in:** `test_multiple_inheritance_solves_unique_fields.md` — +`KeyClassField(B1, "p1")` and `KeyClassField(B2, "p2")` are demanded +cross-module but never compared (unique to one parent). + +**Ideal behavior:** Collect field names from all parents first. Only +resolve `KeyClassField` for names that appear in multiple parents. +This is a two-pass approach: pass 1 collects names (cheap metadata), +pass 2 resolves types (expensive) only for shared names. + +## Annotated return types DO break cascades (working correctly) + +`test_annotated_return_breaks_cascade.md` shows that when +`get_config() -> int` has a return annotation, module `c` (containing +`Config` used in the body) has 0 solved keys. The demand tree shows +only `a -> b::KeyExport("get_config")`. + +This demonstrates that pyrefly already implements the "annotation as +cascade breaker" pattern — callers trust the annotation without +inferring the function body. + +## Transitive annotated exports break cascades (partially working) + +`test_transitive_import_annotated.md` shows that when `b` has +`value: int = 42`, module `c` has no keys solved — the annotation +`int` is resolved locally in `b` without cascading to `c`. However, +`c` is still computed to Exports due to LookupExport calls during +`b`'s binding (see "LookupExport during binding forces transitive +exports" above). Ideally `c` would be `Nothing`. + +## Unused imports' transitive deps are not checked (partially working) + +`test_unused_import_from_same_module.md` shows that `c` (Heavy's +module) has 0 solved keys when only `light()` is used from `b`. +The solver only resolves the `light` function and doesn't cascade +into `Heavy`'s module. However, `c` is still computed to Exports +due to LookupExport calls during `b`'s binding (see "LookupExport +during binding forces transitive exports" above). Ideally `c` would +be `Nothing`. diff --git a/pyrefly/test_laziness/build.rs b/pyrefly/test_laziness/build.rs new file mode 100644 index 0000000000..0c813b4a5f --- /dev/null +++ b/pyrefly/test_laziness/build.rs @@ -0,0 +1,70 @@ +/* + * 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. + */ + +//! Build script that generates #[test] functions for each test_*.md file. + +use std::env; +use std::fs; +use std::path::Path; +use std::path::PathBuf; + +fn get_test_dir() -> PathBuf { + match env::var_os("LAZINESS_TEST_PATH") { + Some(root) => PathBuf::from(root), + // Cargo: relative to the crate root (pyrefly/pyrefly/) + None => PathBuf::from("test_laziness"), + } +} + +fn get_output_path() -> PathBuf { + match env::var_os("OUT") { + // Buck genrule: OUT is the output directory + Some(path) => PathBuf::from(path), + // Cargo: OUT_DIR is set by cargo + None => PathBuf::from(env::var("OUT_DIR").expect("OUT_DIR not set")), + } +} + +fn main() { + let test_dir = get_test_dir(); + let output_dir = get_output_path(); + let output_file = output_dir.join("laziness_tests_generated.rs"); + + // Tell cargo to rerun if test files change. + println!("cargo::rerun-if-changed={}", test_dir.display()); + + let mut test_names: Vec = fs::read_dir(&test_dir) + .unwrap_or_else(|e| panic!("Failed to read {}: {e}", test_dir.display())) + .filter_map(|entry| { + let entry = entry.ok()?; + let path = entry.path(); + if path.extension().is_some_and(|ext| ext == "md") + && path + .file_name() + .is_some_and(|n| n.to_string_lossy().starts_with("test_")) + { + let name = path.file_stem()?.to_string_lossy().into_owned(); + Some(name) + } else { + None + } + }) + .collect(); + test_names.sort(); + + let mut code = String::new(); + for name in &test_names { + code.push_str(&format!("laziness_test!({name});\n")); + } + + // Write the file, creating parent dirs if needed. + if let Some(parent) = Path::new(&output_file).parent() { + fs::create_dir_all(parent).ok(); + } + fs::write(&output_file, code) + .unwrap_or_else(|e| panic!("Failed to write {}: {e}", output_file.display())); +} diff --git a/pyrefly/test_laziness/mod.rs b/pyrefly/test_laziness/mod.rs new file mode 100644 index 0000000000..844b5a9498 --- /dev/null +++ b/pyrefly/test_laziness/mod.rs @@ -0,0 +1,483 @@ +/* + * 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. + */ + +//! Markdown-based snapshot tests for laziness properties. +//! +//! Each `.md` file in `test/laziness/` defines a test case with Python files +//! and expected output showing which steps and keys are computed for each module. +//! The test runner checks the actual output against the expected snapshot. +//! If they differ, the `.md` file is updated in-place and the test fails, +//! so the diff shows up in the working directory for review. + +use std::path::Path; +use std::path::PathBuf; +use std::sync::Arc; + +use dupe::Dupe; +use pyrefly::state::load::FileContents; +use pyrefly::state::require::Require; +use pyrefly::state::state::State; +use pyrefly::state::steps::Step; +use pyrefly::state::subscriber::TestSubscriber; +use pyrefly_build::source_db::map_db::MapDatabase; +use pyrefly_config::config::ConfigFile; +use pyrefly_config::finder::ConfigFinder; +use pyrefly_python::module_name::ModuleName; +use pyrefly_python::module_path::ModulePath; +use pyrefly_python::sys_info::SysInfo; +use pyrefly_util::arc_id::ArcId; +use starlark_map::small_map::SmallMap; + +/// Parsed laziness test from a markdown file. +struct LazinessTest { + /// Python files: (module_name, contents) + files: Vec<(String, String)>, + /// Which module(s) to check + check_targets: Vec, +} + +/// Parse a laziness test from markdown content. +fn parse_test(content: &str) -> LazinessTest { + let mut files: Vec<(String, String)> = Vec::new(); + let mut check_targets: Vec = Vec::new(); + + let lines: Vec<&str> = content.lines().collect(); + let mut i = 0; + while i < lines.len() { + let line = lines[i]; + + // Look for file definitions: `module_name.py`: + if line.starts_with('`') && line.ends_with("`:") && line.contains(".py") { + let filename = line.trim_start_matches('`').trim_end_matches("`:").trim(); + let module_name = filename.trim_end_matches(".py").replace('/', "."); + // Next line should be ```python + i += 1; + if i < lines.len() && lines[i].starts_with("```python") { + i += 1; + let mut code = String::new(); + while i < lines.len() && lines[i] != "```" { + if !code.is_empty() { + code.push('\n'); + } + code.push_str(lines[i]); + i += 1; + } + files.push((module_name, code)); + } + } + + // Look for check target: ## Check `module_name.py` + if line.starts_with("## Check ") { + let target = line + .trim_start_matches("## Check ") + .trim_start_matches('`') + .trim_end_matches('`') + .trim_end_matches(".py") + .replace('/', "."); + check_targets.push(target); + } + + i += 1; + } + + LazinessTest { + files, + check_targets, + } +} + +/// Run a laziness test and return the actual output as a string. +fn run_test(test: &LazinessTest) -> String { + let collector = pyrefly_util::demand_tree::DemandCollector::new(); + + let module_names: Vec<&str> = test.files.iter().map(|(n, _)| n.as_str()).collect(); + + let mut config = ConfigFile::default(); + config.python_environment.set_empty_to_default(); + let mut sourcedb = MapDatabase::new(config.get_sys_info()); + for name in &module_names { + sourcedb.insert( + ModuleName::from_str(name), + ModulePath::memory(PathBuf::from(*name)), + ); + } + config.source_db = Some(ArcId::new(Box::new(sourcedb))); + config.configure(); + let config = ArcId::new(config); + + // Single-threaded pyrefly execution for deterministic demand tree ordering. + let thread_count = + pyrefly_util::thread_pool::ThreadCount::NumThreads(std::num::NonZeroUsize::new(1).unwrap()); + let state = State::new(ConfigFinder::new_constant(config), thread_count); + let subscriber = TestSubscriber::new(); + + let mut transaction = + state.new_committable_transaction(Require::Exports, Some(Box::new(subscriber.dupe()))); + transaction + .as_mut() + .set_demand_collector(Some(collector.clone())); + + for (name, contents) in &test.files { + let contents = Arc::new(contents.clone()); + transaction.as_mut().set_memory(vec![( + PathBuf::from(name), + Some(Arc::new(FileContents::Source(contents))), + )]); + } + + let handles: Vec<_> = test + .check_targets + .iter() + .map(|name| { + pyrefly_build::handle::Handle::new( + ModuleName::from_str(name), + ModulePath::memory(PathBuf::from(name.as_str())), + SysInfo::default(), + ) + }) + .collect(); + + state.run_with_committing_transaction(transaction, &handles, Require::Errors, None, None); + + // Collect step info from subscriber + let mut module_steps: SmallMap = SmallMap::new(); + for (handle, info) in subscriber.finish_detailed() { + if let Some(step) = info.last_step { + module_steps.insert(handle.module().as_str().to_owned(), step); + } + } + + // Format the output + let mut output = String::new(); + // Sort modules: check targets first (as Solutions), then others alphabetically + let mut sorted_modules: Vec<&str> = module_names.to_vec(); + sorted_modules.sort_by(|a, b| { + let a_is_target = test.check_targets.iter().any(|t| t == a); + let b_is_target = test.check_targets.iter().any(|t| t == b); + match (a_is_target, b_is_target) { + (true, false) => std::cmp::Ordering::Less, + (false, true) => std::cmp::Ordering::Greater, + _ => a.cmp(b), + } + }); + + for name in &sorted_modules { + let step = module_steps.get(*name); + let step_str = match step { + Some(Step::Load) => "Load", + Some(Step::Ast) => "Ast", + Some(Step::Exports) => "Exports", + Some(Step::Answers) => "Answers", + Some(Step::Solutions) => "Solutions", + None => "Nothing", + }; + + // Skip stdlib modules + if !module_names.contains(name) { + continue; + } + + output.push_str(&format!("{}: {}\n", name, step_str)); + } + + // Append demand tree. + // - Exclude roots where the source module is not a user module (stdlib→stdlib) + // - Count and summarize builtin demands (user→builtins) instead of listing them + // - Deduplicate identical root-level entries (e.g. repeated Exports demands) + // - Filter out stdlib Exports children from answer demand nodes + use pyrefly_util::demand_tree::DemandNode; + let demand_roots = collector.take_roots(); + let user_set: std::collections::HashSet<&str> = module_names.iter().copied().collect(); + let mut builtin_count: usize = 0; + let mut seen_roots: std::collections::HashSet = std::collections::HashSet::new(); + + /// Builtins and typing are pervasive dependencies that would otherwise + /// drown out interesting demands; we count them in aggregate. + fn is_builtin_target(target: &str) -> bool { + target == "builtins" || target == "typing" + } + + /// Filter children of a demand node: remove stdlib Exports demands + /// since they're internal plumbing, not interesting cross-module demands. + fn filter_children(node: &mut DemandNode, builtin_count: &mut usize) { + node.children.retain_mut(|child| { + if is_builtin_target(&child.target) { + *builtin_count += 1; + return false; + } + filter_children(child, builtin_count); + true + }); + } + + let filtered: Vec<_> = demand_roots + .into_iter() + .filter_map(|mut node| { + if !user_set.contains(node.from.as_str()) { + return None; // stdlib→anything: exclude + } + if is_builtin_target(&node.target) { + builtin_count += 1; + return None; // user→builtins/typing: count but exclude + } + // Deduplicate identical leaf roots (e.g. repeated Exports demands). + // Parent roots with children are never deduped — their subtree + // content is significant even when labels match. + if node.children.is_empty() && !seen_roots.insert(render_node_line(&node, 0)) { + return None; + } + filter_children(&mut node, &mut builtin_count); + Some(node) + }) + .collect(); + + if builtin_count > 0 || !filtered.is_empty() { + output.push('\n'); + } + if builtin_count > 0 { + output.push_str(&format!("({builtin_count} builtin demands hidden)\n")); + } + for root in &filtered { + render_tree(root, 0, &mut output); + } + + output +} + +/// Render a single demand node as one indented line for snapshot output. +fn render_node_line(node: &pyrefly_util::demand_tree::DemandNode, depth: usize) -> String { + use pyrefly_util::demand_tree::DemandKind; + let indent = " ".repeat(depth); + match &node.kind { + DemandKind::Exports { reason } => { + format!( + "{indent}{} -> {}::Exports({reason})", + node.from, node.target + ) + } + DemandKind::Answer { key } => { + format!("{indent}{} -> {}::{key}", node.from, node.target) + } + } +} + +/// Recursively render a node and its children into the output buffer. +fn render_tree(node: &pyrefly_util::demand_tree::DemandNode, depth: usize, out: &mut String) { + out.push_str(&render_node_line(node, depth)); + out.push('\n'); + for child in &node.children { + render_tree(child, depth + 1, out); + } +} + +/// Replace the expected block in the markdown content with new content. +fn update_expected(content: &str, new_expected: &str) -> String { + let mut result = String::new(); + let mut in_expected_block = false; + let mut replaced = false; + + for line in content.lines() { + if line.starts_with("```expected") { + result.push_str(line); + result.push('\n'); + result.push_str(new_expected); + in_expected_block = true; + replaced = true; + continue; + } + if in_expected_block { + if line == "```" { + in_expected_block = false; + result.push_str(line); + result.push('\n'); + } + // Skip old content + continue; + } + result.push_str(line); + result.push('\n'); + } + + if !replaced { + // No expected block found — append one + result.push_str("\n```expected\n"); + result.push_str(new_expected); + result.push_str("```\n"); + } + + result +} + +/// Extract the expected output from the markdown content. +fn extract_expected(content: &str) -> String { + let mut in_expected = false; + let mut expected = String::new(); + for line in content.lines() { + if line.starts_with("```expected") { + in_expected = true; + continue; + } + if in_expected { + if line == "```" { + break; + } + expected.push_str(line); + expected.push('\n'); + } + } + expected +} + +/// Map a test file path to the writable source file path. +/// The input `path` may point into a build-tool-specific read-only copy +/// (e.g. a buck sandbox); the source location is always derivable from +/// `file!()` and the test filename. +fn source_path_for(path: &Path) -> PathBuf { + let filename = path.file_name().expect("test path must have a filename"); + // file!() is relative to the build tool's workspace root (repo root under + // buck, crate root under cargo). In both cases, joining its parent with + // the filename yields a path to the source .md file that writes land in. + let source_dir = Path::new(file!()) + .parent() + .expect("file!() must have a parent"); + source_dir.join(filename) +} + +/// Produce a unified diff between two strings. +fn unified_diff(expected: &str, actual: &str) -> String { + let expected_lines: Vec<&str> = expected.lines().collect(); + let actual_lines: Vec<&str> = actual.lines().collect(); + let mut output = String::new(); + output.push_str(" --- expected\n +++ actual\n"); + let max = expected_lines.len().max(actual_lines.len()); + for i in 0..max { + match (expected_lines.get(i), actual_lines.get(i)) { + (Some(e), Some(a)) if e == a => { + output.push_str(&format!(" {e}\n")); + } + (Some(e), Some(a)) => { + output.push_str(&format!("- {e}\n")); + output.push_str(&format!("+ {a}\n")); + } + (Some(e), None) => { + output.push_str(&format!("- {e}\n")); + } + (None, Some(a)) => { + output.push_str(&format!("+ {a}\n")); + } + (None, None) => {} + } + } + output +} + +/// Run a single markdown laziness test file. Returns Ok(()) on match, +/// Ok(message) if the snapshot was updated, or Err(message) on mismatch. +fn run_laziness_test(path: &Path) -> Result<(), String> { + let content = std::fs::read_to_string(path) + .map_err(|e| format!("Failed to read {}: {}", path.display(), e))?; + + let test = parse_test(&content); + if test.files.is_empty() { + return Err(format!("No Python files found in {}", path.display())); + } + if test.check_targets.is_empty() { + return Err(format!("No check target found in {}", path.display())); + } + + let actual = run_test(&test); + let expected = extract_expected(&content); + + if actual == expected { + return Ok(()); + } + + let source = source_path_for(path); + + // If UPDATE_SNAPSHOTS=1, auto-update the source file. + if std::env::var("UPDATE_SNAPSHOTS").is_ok_and(|v| v == "1") && source.exists() { + let source_content = std::fs::read_to_string(&source).unwrap_or_default(); + let updated = update_expected(&source_content, &actual); + if std::fs::write(&source, &updated).is_ok() { + return Err(format!( + "Laziness snapshot updated: {}. Review the diff and commit.", + source.display(), + )); + } + } + + let diff = unified_diff(&expected, &actual); + let test_name = path + .file_stem() + .map(|s| s.to_string_lossy()) + .unwrap_or_default(); + let rerecord_cmd = if std::env::var("LAZINESS_TEST_PATH").is_ok() { + format!( + "buck test pyrefly:pyrefly_laziness_tests -- --env UPDATE_SNAPSHOTS=1 -r {test_name}" + ) + } else { + format!("UPDATE_SNAPSHOTS=1 cargo test {test_name} -- --test-threads=1") + }; + // Print to stderr so buck renders with real newlines. + eprintln!( + "\nLaziness snapshot mismatch: {}\n\n{diff}\nTo re-record:\n {rerecord_cmd}\n", + source.display(), + ); + Err(format!("snapshot mismatch: {}", source.display())) +} + +/// Find the laziness test directory. +/// Tries multiple strategies: LAZINESS_TEST_PATH env var, file!() relative, +/// and cargo's current directory. +fn test_dir() -> PathBuf { + if let Ok(path) = std::env::var("LAZINESS_TEST_PATH") { + let p = PathBuf::from(path); + if p.exists() { + return p; + } + } + // Derive from file!() — works in buck where file paths are relative to workspace + let source_file = Path::new(file!()); + let test_dir = source_file.parent().unwrap(); + if test_dir.exists() { + return test_dir.to_path_buf(); + } + // Cargo: try relative to current dir (cargo runs from crate root) + let cargo_path = PathBuf::from("test_laziness"); + if cargo_path.exists() { + return cargo_path; + } + panic!( + "Laziness test directory not found. Tried:\n - $LAZINESS_TEST_PATH\n - {}\n - lib/test/laziness\n\ + Set LAZINESS_TEST_PATH or run from the pyrefly crate directory.", + test_dir.display(), + ); +} + +/// Run a laziness test by filename. Each test uses its own +/// DemandCollector, so tests can run in parallel. +fn run_laziness_test_by_name(filename: &str) -> Result<(), String> { + let dir = test_dir(); + let path = dir.join(filename); + if !path.exists() { + return Err(format!("Test file not found: {}", path.display())); + } + + run_laziness_test(&path) +} + +macro_rules! laziness_test { + ($name:ident) => { + #[test] + fn $name() -> Result<(), String> { + run_laziness_test_by_name(concat!(stringify!($name), ".md")) + } + }; +} + +// Generated at build time by build.rs — one laziness_test!() per test_*.md file. +include!(concat!(env!("OUT_DIR"), "/laziness_tests_generated.rs")); diff --git a/pyrefly/test_laziness/test_annotated_return_breaks_cascade.md b/pyrefly/test_laziness/test_annotated_return_breaks_cascade.md new file mode 100644 index 0000000000..88ab1ea5fb --- /dev/null +++ b/pyrefly/test_laziness/test_annotated_return_breaks_cascade.md @@ -0,0 +1,57 @@ +# Annotated return breaks cascade + +Calling `get_config()` which has return annotation `-> int`. The +function body uses `Config` from module `c`, but callers should not +need to resolve `c` since the return type is explicitly annotated. + +Only `a -> b::KeyExport("get_config")` appears — this is correct. +Module `c` has 0 solved keys, confirming that the annotation breaks +the cascade. No superfluous demands. + +## Files + +`a.py`: +```python +from b import get_config +x = get_config() +``` + +`b.py`: +```python +from c import Config +def get_config() -> int: + c = Config() + return c.debug +``` + +`c.py`: +```python +class Config: + debug: bool = False +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers +c: Exports + +(160 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("get_config")) + b -> c::Exports(module_exists) + b -> c::Exports(export_exists) + b -> c::Exports(get_deprecated) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) +``` diff --git a/pyrefly/test_laziness/test_attribute_inherited.md b/pyrefly/test_laziness/test_attribute_inherited.md new file mode 100644 index 0000000000..a34402e1f2 --- /dev/null +++ b/pyrefly/test_laziness/test_attribute_inherited.md @@ -0,0 +1,76 @@ +# Attribute inherited from parent + +Accessing `c.base_attr` where `base_attr` is defined on `Base` (in +module `c`), inherited by `Child` (in module `b`). + +All necessary demands: +- `a -> b::KeyExport("Child")` — resolve the import +- `a -> b::KeyClassMetadata(0)` — needed to know Child's bases for MRO +- `b -> c::KeyExport("Base")` and `b -> c::KeyClassMetadata(0)` — + resolving Child's MRO requires knowing Base +- `a -> b::KeyClassMro(0)` — compute MRO to walk ancestors +- `a -> c::KeyClassField(0, "base_attr")` — the actual attribute + resolution; children (`c -> builtins::*`) resolve `int` +- `a -> b::KeyClassSynthesizedFields(0)` and `a -> c::KeyClassSynthesizedFields(0)` + — MRO walk checks synthesized fields on each ancestor + +**Superfluous:** +- `a -> b::KeyAbstractClassCheck(0)` — abstract checking is only + relevant for instantiation, not for attribute access. + +## Files + +`a.py`: +```python +from b import Child +c = Child() +x = c.base_attr +``` + +`b.py`: +```python +from c import Base +class Child(Base): + child_attr: str = 'hello' +``` + +`c.py`: +```python +class Base: + base_attr: int = 1 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers +c: Answers + +(170 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("Child")) + b -> c::Exports(module_exists) + b -> c::Exports(export_exists) + b -> c::Exports(get_deprecated) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) +a -> b::KeyClassMetadata(ClassDefIndex(0)) + b -> c::KeyExport(Name("Base")) + b -> c::KeyClassMetadata(ClassDefIndex(0)) + b -> c::KeyClassMetadata(ClassDefIndex(0)) +a -> b::KeyClassMetadata(ClassDefIndex(0)) +a -> b::KeyAbstractClassCheck(ClassDefIndex(0)) + b -> c::KeyClassMetadata(ClassDefIndex(0)) +a -> b::KeyClassSynthesizedFields(ClassDefIndex(0)) +a -> b::KeyClassMro(ClassDefIndex(0)) + b -> c::KeyClassMetadata(ClassDefIndex(0)) + b -> c::KeyClassBaseType(ClassDefIndex(0)) + b -> c::KeyClassMro(ClassDefIndex(0)) +a -> c::KeyClassSynthesizedFields(ClassDefIndex(0)) +a -> b::KeyClassMro(ClassDefIndex(0)) +a -> c::KeyClassField(ClassDefIndex(0), Name("base_attr")) +``` diff --git a/pyrefly/test_laziness/test_attribute_on_class_itself.md b/pyrefly/test_laziness/test_attribute_on_class_itself.md new file mode 100644 index 0000000000..15508d0601 --- /dev/null +++ b/pyrefly/test_laziness/test_attribute_on_class_itself.md @@ -0,0 +1,78 @@ +# Attribute on class itself + +Accessing `c.child_attr` where `child_attr` is defined on `Child`, not +inherited from `Base`. + +`a -> b::KeyExport("Child")` is necessary. + +The demand tree is nearly identical to `test_attribute_inherited` — all +the same class metadata, MRO, abstract check, and synthesized fields +demands appear. This is because the attribute lookup code in attr.rs +walks the FULL MRO for every attribute access, even when the attribute +is found on the first class checked. + +**Superfluous demands:** +- ALL demands involving `c` (Base's module) are superfluous — `child_attr` + is on `Child`, so `Base` is never needed. +- `a -> b::KeyAbstractClassCheck(0)` is superfluous for attribute access + (only needed for instantiation). +- `a -> b::KeyClassMro(0)` and the cascading `b -> c::*` demands are + superfluous — the MRO iterator trick would avoid materializing the + MRO when the attribute is found on the class itself. + +## Files + +`a.py`: +```python +from b import Child +c = Child() +x = c.child_attr +``` + +`b.py`: +```python +from c import Base +class Child(Base): + child_attr: str = 'hello' +``` + +`c.py`: +```python +class Base: + base_attr: int = 1 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers +c: Answers + +(181 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("Child")) + b -> c::Exports(module_exists) + b -> c::Exports(export_exists) + b -> c::Exports(get_deprecated) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) +a -> b::KeyClassMetadata(ClassDefIndex(0)) + b -> c::KeyExport(Name("Base")) + b -> c::KeyClassMetadata(ClassDefIndex(0)) + b -> c::KeyClassMetadata(ClassDefIndex(0)) +a -> b::KeyClassMetadata(ClassDefIndex(0)) +a -> b::KeyAbstractClassCheck(ClassDefIndex(0)) + b -> c::KeyClassMetadata(ClassDefIndex(0)) +a -> b::KeyClassSynthesizedFields(ClassDefIndex(0)) +a -> b::KeyClassMro(ClassDefIndex(0)) + b -> c::KeyClassMetadata(ClassDefIndex(0)) + b -> c::KeyClassBaseType(ClassDefIndex(0)) + b -> c::KeyClassMro(ClassDefIndex(0)) +a -> c::KeyClassSynthesizedFields(ClassDefIndex(0)) +a -> b::KeyClassMro(ClassDefIndex(0)) +a -> b::KeyClassField(ClassDefIndex(0), Name("child_attr")) +``` diff --git a/pyrefly/test_laziness/test_bare_import_forces_exports.md b/pyrefly/test_laziness/test_bare_import_forces_exports.md new file mode 100644 index 0000000000..80ae505483 --- /dev/null +++ b/pyrefly/test_laziness/test_bare_import_forces_exports.md @@ -0,0 +1,44 @@ +# Bare import forces exports on transitive dep + +`a` imports `light` from `b`. `b` has `import c` (bare import, not +`from c import ...`). `a` only uses `light()` which doesn't involve `c`. + +**Superfluous:** `c` being computed to Exports. `b`'s binding calls +`module_exists(c)` which demands `Step::Exports` on `c`, even though +`b` never actually uses `c` in a way that `a` would need. + +## Files + +`a.py`: +```python +from b import light +x = light() +``` + +`b.py`: +```python +import c +def light() -> int: return 1 +``` + +`c.py`: +```python +class Heavy: + x: int = 1 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers +c: Exports + +(160 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("light")) + b -> c::Exports(module_exists) +``` diff --git a/pyrefly/test_laziness/test_deprecated_forces_exports.md b/pyrefly/test_laziness/test_deprecated_forces_exports.md new file mode 100644 index 0000000000..d250817e4a --- /dev/null +++ b/pyrefly/test_laziness/test_deprecated_forces_exports.md @@ -0,0 +1,50 @@ +# get_deprecated forces exports on transitive dep + +`a` imports `value` from `b`. `b` imports `old_func` from `c` with a +deprecation marker. `a` only uses `value`, not `old_func`. + +**Superfluous:** `c` being computed to Exports. During `b`'s binding, +`from c import old_func` triggers `get_deprecated(c, "old_func")` which +demands `Step::Exports` on `c`, even though `a` never uses `old_func`. + +Note: This test may be hard to trigger since deprecation requires `c` to +actually mark `old_func` as deprecated via `warnings.deprecated` or +similar. The `get_deprecated` call happens regardless of whether the name +is actually deprecated — the check itself forces exports. + +## Files + +`a.py`: +```python +from b import value +x = value +``` + +`b.py`: +```python +from c import old_func +value: int = 42 +``` + +`c.py`: +```python +def old_func() -> None: ... +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers +c: Exports + +(159 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("value")) + b -> c::Exports(module_exists) + b -> c::Exports(export_exists) + b -> c::Exports(get_deprecated) +``` diff --git a/pyrefly/test_laziness/test_export_exists_forces_exports.md b/pyrefly/test_laziness/test_export_exists_forces_exports.md new file mode 100644 index 0000000000..7e05a77915 --- /dev/null +++ b/pyrefly/test_laziness/test_export_exists_forces_exports.md @@ -0,0 +1,52 @@ +# export_exists check forces exports on transitive dep + +`a` imports `value` from `b`. `b` does `from c import Foo` — during +binding, this triggers `export_exists(c, "Foo")` and `module_exists(c)` +to verify the import. `a` only uses `value`, not `Foo`. + +**Superfluous:** `c` being computed to Exports. The `export_exists` and +`module_exists` checks during `b`'s binding demand `Step::Exports` on +`c`, even though `a` never uses `Foo`. Ideally, `from c import Foo` +would create an optimistic `Binding::Import` without demanding `c`'s +exports, deferring validation to solve time. + +This is the same pattern as `test_unused_import_from_same_module` but +simplified: `b` imports from `c` but only exports an unrelated value. + +## Files + +`a.py`: +```python +from b import value +x = value +``` + +`b.py`: +```python +from c import Foo +value: int = 42 +``` + +`c.py`: +```python +class Foo: + x: int = 1 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers +c: Exports + +(159 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("value")) + b -> c::Exports(module_exists) + b -> c::Exports(export_exists) + b -> c::Exports(get_deprecated) +``` diff --git a/pyrefly/test_laziness/test_import_class_as_annotation.md b/pyrefly/test_laziness/test_import_class_as_annotation.md new file mode 100644 index 0000000000..3171978bc1 --- /dev/null +++ b/pyrefly/test_laziness/test_import_class_as_annotation.md @@ -0,0 +1,46 @@ +# Import class as annotation + +Importing a class and using it only as a type annotation (`x: Foo`). + +`a -> b::KeyExport("Foo")` is necessary to resolve the import. + +`a -> b::KeyClassMetadata(0)` is superfluous for annotation-only +usage. It's triggered by `type_of_instance` in targs.rs which calls +`get_metadata_for_class` to check `is_typed_dict()` — because +`Type::ClassType` and `Type::TypedDict` are different type variants, +the code must decide which to create. The `is_typed_dict` check +requires resolving the class's bases to see if any ancestor is +`TypedDict`, which recursively demands metadata up the MRO. + +Fixing this requires unifying `ClassType` and `TypedDict` into a +single type variant, deferring the TypedDict distinction until +TypedDict-specific features are actually used. + +## Files + +`a.py`: +```python +from b import Foo +x: Foo +``` + +`b.py`: +```python +class Foo: + x: int = 1 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers + +(157 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("Foo")) +a -> b::KeyClassMetadata(ClassDefIndex(0)) +``` diff --git a/pyrefly/test_laziness/test_import_class_instantiated.md b/pyrefly/test_laziness/test_import_class_instantiated.md new file mode 100644 index 0000000000..acb7b0872d --- /dev/null +++ b/pyrefly/test_laziness/test_import_class_instantiated.md @@ -0,0 +1,47 @@ +# Import class instantiated + +Importing a class and calling its constructor (`f = Foo()`). + +All demands are necessary for constructor resolution: +- `KeyExport("Foo")` — resolve the import +- `KeyClassMetadata(0)` — needed for metaclass check, `__init__`/`__new__` + lookup, and field synthesis +- `KeyClassSynthesizedFields(0)` — resolve synthesized `__init__` +- `KeyClassMro(0)` — find `__init__`/`__new__` in parent classes + +**Superfluous:** +- `KeyAbstractClassCheck(0)` — checks for unimplemented abstract methods. + For a simple class with no abstract parents, this could be skipped + when metadata shows no ABC lineage. + +## Files + +`a.py`: +```python +from b import Foo +f = Foo() +``` + +`b.py`: +```python +class Foo: + x: int = 1 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers + +(159 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("Foo")) +a -> b::KeyClassMetadata(ClassDefIndex(0)) +a -> b::KeyAbstractClassCheck(ClassDefIndex(0)) +a -> b::KeyClassSynthesizedFields(ClassDefIndex(0)) +a -> b::KeyClassMro(ClassDefIndex(0)) +``` diff --git a/pyrefly/test_laziness/test_import_function_called.md b/pyrefly/test_laziness/test_import_function_called.md new file mode 100644 index 0000000000..d54bd488ea --- /dev/null +++ b/pyrefly/test_laziness/test_import_function_called.md @@ -0,0 +1,35 @@ +# Import function called + +Importing a function and calling it. The demand tree is identical to +the unused case — `a -> b::KeyExport("helper")` — because `KeyExport` +eagerly resolves the full function type regardless of usage. + +All demands are justified here since the function is called and its +return type is needed. + +## Files + +`a.py`: +```python +from b import helper +x = helper() +``` + +`b.py`: +```python +def helper() -> int: return 1 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers + +(160 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("helper")) +``` diff --git a/pyrefly/test_laziness/test_import_function_unused.md b/pyrefly/test_laziness/test_import_function_unused.md new file mode 100644 index 0000000000..f91a76ae60 --- /dev/null +++ b/pyrefly/test_laziness/test_import_function_unused.md @@ -0,0 +1,34 @@ +# Import function unused + +Importing a function without calling it. + +The single demand `a -> b::KeyExport("helper")` is necessary to resolve +the import. However, this triggers resolving the full function type in `b` +(11 solved keys), including the return annotation, decorated/undecorated +function chain, and legacy type param checks. Since `helper` is never +called, all of this work is superfluous — a lightweight handle would suffice. + +## Files + +`a.py`: +```python +from b import helper +``` + +`b.py`: +```python +def helper() -> int: return 1 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers + +(160 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::KeyExport(Name("helper")) +``` diff --git a/pyrefly/test_laziness/test_import_star_forces_exports.md b/pyrefly/test_laziness/test_import_star_forces_exports.md new file mode 100644 index 0000000000..dcc9da4340 --- /dev/null +++ b/pyrefly/test_laziness/test_import_star_forces_exports.md @@ -0,0 +1,51 @@ +# Wildcard import forces exports on transitive dep + +`a` imports `light` from `b`. `b` does `from c import *`, pulling in all +of `c`'s exports — but `a` only uses `light()`, which is defined locally +in `b` and doesn't involve anything from `c`. + +**Superfluous:** `c` being computed to Exports at all. `b`'s binding calls +`get_wildcard(c)` which demands `Step::Exports` on `c`. This is harder to +avoid than other LookupExport calls because the binder needs the set of +wildcard names to create binding entries, but it means every transitive +`import *` forces the target module's exports to be computed. + +## Files + +`a.py`: +```python +from b import light +x = light() +``` + +`b.py`: +```python +from c import * +def light() -> int: return 1 +``` + +`c.py`: +```python +class Heavy: + x: int = 1 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers +c: Exports + +(160 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +b -> c::Exports(get_wildcard) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("light")) + b -> c::Exports(get_wildcard) + b -> c::Exports(module_exists) + b -> c::Exports(get_wildcard) + b -> c::Exports(export_exists) +``` diff --git a/pyrefly/test_laziness/test_is_final_forces_exports.md b/pyrefly/test_laziness/test_is_final_forces_exports.md new file mode 100644 index 0000000000..ea5feec43e --- /dev/null +++ b/pyrefly/test_laziness/test_is_final_forces_exports.md @@ -0,0 +1,48 @@ +# is_final check forces exports on transitive dep + +`a` imports `value` from `b`. `b` imports `X` from `c` and then +reassigns `X = 2`. `a` only uses `value`, not `X`. + +**Superfluous:** `c` being computed to Exports. During `b`'s binding, +the reassignment `X = 2` triggers `is_final(c, "X")` to check if `X` +was declared as `Final` in `c`. This demands `Step::Exports` on `c`, +even though `a` never uses `X`. + +## Files + +`a.py`: +```python +from b import value +x = value +``` + +`b.py`: +```python +from c import X +X = 2 +def value() -> int: return 42 +``` + +`c.py`: +```python +X: int = 1 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers +c: Exports + +(160 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("value")) + b -> c::Exports(module_exists) + b -> c::Exports(export_exists) + b -> c::Exports(get_deprecated) + b -> c::Exports(is_final) +``` diff --git a/pyrefly/test_laziness/test_multiple_inheritance_solves_unique_fields.md b/pyrefly/test_laziness/test_multiple_inheritance_solves_unique_fields.md new file mode 100644 index 0000000000..4ff6673c5c --- /dev/null +++ b/pyrefly/test_laziness/test_multiple_inheritance_solves_unique_fields.md @@ -0,0 +1,73 @@ +# Multiple inheritance check solves unique parent fields + +`a.py` (check target) defines `C(B1, B2)` with no fields of its own. +`B1` (in `b.py`) defines `p1` and `shared`. `B2` (in `c.py`) defines +`p2` and `shared`. Only `shared` appears in both parents and needs a +consistency check. + +`check_consistent_multiple_inheritance` iterates each parent's fields +and calls `get_class_member` on each, resolving `KeyClassField` for +ALL parent fields: `p1`, `shared` (from B1) and `p2`, `shared` (from +B2). But only `shared` appears in multiple bases (line 3382 checks +`len() > 1`). Resolving `p1` and `p2` is wasted work — they're unique +to one parent and can't have consistency issues. + +**Superfluous:** `KeyClassField` demands for `p1` and `p2` — fields +unique to a single parent don't need type resolution for the multiple +inheritance check. The check could collect field names first, then only +resolve fields that appear in multiple bases. + +## Files + +`a.py`: +```python +from b import B1 +from c import B2 +class C(B1, B2): + pass +``` + +`b.py`: +```python +class B1: + p1: int = 1 + shared: int = 10 +``` + +`c.py`: +```python +class B2: + p2: int = 2 + shared: int = 20 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers +c: Answers + +(190 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> c::Exports(module_exists) +a -> c::Exports(export_exists) +a -> c::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> c::Exports(is_special_export) +a -> b::KeyExport(Name("B1")) +a -> c::KeyExport(Name("B2")) +a -> b::KeyClassMetadata(ClassDefIndex(0)) +a -> c::KeyClassMetadata(ClassDefIndex(0)) +a -> b::KeyClassBaseType(ClassDefIndex(0)) +a -> c::KeyClassBaseType(ClassDefIndex(0)) +a -> b::KeyClassMro(ClassDefIndex(0)) +a -> c::KeyClassMro(ClassDefIndex(0)) +a -> b::KeyClassField(ClassDefIndex(0), Name("p1")) +a -> b::KeyClassField(ClassDefIndex(0), Name("shared")) +a -> c::KeyClassField(ClassDefIndex(0), Name("p2")) +a -> c::KeyClassField(ClassDefIndex(0), Name("shared")) +a -> b::KeyClassField(ClassDefIndex(0), Name("shared")) +``` diff --git a/pyrefly/test_laziness/test_special_export_forces_exports.md b/pyrefly/test_laziness/test_special_export_forces_exports.md new file mode 100644 index 0000000000..a61bf51111 --- /dev/null +++ b/pyrefly/test_laziness/test_special_export_forces_exports.md @@ -0,0 +1,51 @@ +# is_special_export forces exports on transitive dep + +`a` imports `value` from `b`. `b` imports `MyTypeVar` from `c` (which +re-exports `TypeVar` from `typing`) and uses it to define a type +variable. `a` only uses `value`, not the type variable. + +**Superfluous:** `c` being computed to Exports. During `b`'s binding, +`x = MyTypeVar("x")` triggers `is_special_export(c, "MyTypeVar")` which +demands `Step::Exports` on `c` to check if it's a re-export of a known +special name (TypeVar, ParamSpec, etc.). This cascades even though `a` +doesn't use the type variable. + +## Files + +`a.py`: +```python +from b import value +x = value +``` + +`b.py`: +```python +from c import MyTypeVar +T = MyTypeVar("T") +value: int = 42 +``` + +`c.py`: +```python +from typing import TypeVar as MyTypeVar +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers +c: Exports + +(159 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("value")) + b -> c::Exports(module_exists) + b -> c::Exports(export_exists) + b -> c::Exports(get_deprecated) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) +``` diff --git a/pyrefly/test_laziness/test_transitive_import_annotated.md b/pyrefly/test_laziness/test_transitive_import_annotated.md new file mode 100644 index 0000000000..6daa358a32 --- /dev/null +++ b/pyrefly/test_laziness/test_transitive_import_annotated.md @@ -0,0 +1,47 @@ +# Transitive import with annotation + +`a` imports `value` from `b`, which has annotation `value: int = 42`. +`b` imports `Inner` from `c`, but `value`'s type is determined by its +annotation, not by inference. + +Only `a -> b::KeyExport("value")` appears. Module `c` has 0 solved +keys — the annotation `int` is resolved locally in `b` without +cascading. No superfluous demands. + +## Files + +`a.py`: +```python +from b import value +x = value + 1 +``` + +`b.py`: +```python +from c import Inner +value: int = 42 +``` + +`c.py`: +```python +class Inner: + x: int = 1 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers +c: Exports + +(164 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("value")) + b -> c::Exports(module_exists) + b -> c::Exports(export_exists) + b -> c::Exports(get_deprecated) +``` diff --git a/pyrefly/test_laziness/test_transitive_import_unannotated.md b/pyrefly/test_laziness/test_transitive_import_unannotated.md new file mode 100644 index 0000000000..12c3c20e80 --- /dev/null +++ b/pyrefly/test_laziness/test_transitive_import_unannotated.md @@ -0,0 +1,58 @@ +# Transitive import without annotation + +`a` imports `value` from `b`, where `value = compute()` has no type +annotation. `compute()` is defined in `c` with `-> int`. + +`b -> c::KeyExport("compute")` resolves the function to determine +`value`'s type. Its children show `c` resolving `int` from builtins. +`a -> b::KeyExport("value")` then gets the resolved type. + +All demands are necessary — without an annotation on `value`, the +type must be inferred by resolving the call chain. No superfluous +demands. + +## Files + +`a.py`: +```python +from b import value +x = value + 1 +``` + +`b.py`: +```python +from c import compute +value = compute() +``` + +`c.py`: +```python +def compute() -> int: return 42 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers +c: Answers + +(168 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("value")) + b -> c::Exports(module_exists) + b -> c::Exports(export_exists) + b -> c::Exports(get_deprecated) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) + b -> c::Exports(is_special_export) + b -> c::KeyExport(Name("compute")) +``` diff --git a/pyrefly/test_laziness/test_unused_import_from_same_module.md b/pyrefly/test_laziness/test_unused_import_from_same_module.md new file mode 100644 index 0000000000..509ca448cb --- /dev/null +++ b/pyrefly/test_laziness/test_unused_import_from_same_module.md @@ -0,0 +1,53 @@ +# Unused import from same module + +`a` imports only `light` from `b`, which also exports `heavy`. +`heavy`'s return type references `Heavy` from `c`. + +Only `a -> b::KeyExport("light")` appears. Module `c` has 0 solved +keys — `heavy`'s signature is not resolved because it's not demanded. + +No superfluous demands in the demand tree. However, `b`'s solved keys +show that `heavy`'s function chain IS partially resolved internally +(KeyDecoratedFunction, KeyUndecoratedFunction for `heavy`) even though +no one imports it. This happens because step_solutions for `b` resolves +all exported keys, not just the demanded one. + +## Files + +`a.py`: +```python +from b import light +x = light() +``` + +`b.py`: +```python +from c import Heavy +def light() -> int: return 1 +def heavy() -> Heavy: ... +``` + +`c.py`: +```python +class Heavy: + x: int = 1 +``` + +## Check `a.py` + +```expected +a: Solutions +b: Answers +c: Exports + +(160 builtin demands hidden) +a -> b::Exports(module_exists) +a -> b::Exports(export_exists) +a -> b::Exports(get_deprecated) +a -> b::Exports(is_special_export) +a -> b::KeyExport(Name("light")) + b -> c::Exports(module_exists) + b -> c::Exports(export_exists) + b -> c::Exports(get_deprecated) + b -> c::Exports(is_special_export) +```