From baff1c42ec91c1d528d56153cb7c3affcfd8294a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jesu=CC=81s=20Pe=CC=81rez?= Date: Tue, 17 Feb 2026 15:49:28 +0000 Subject: [PATCH] refactor(core): eliminate field execution duplication, fix stack-unsafe retry, O(1) context passing Deduplication - Consolidate three copies of match field.field_type into InquireBackend::execute_field_sync as single canonical implementation (backends/cli.rs) - Add previous_results param to execute_field_sync (pub(crate)); propagate options_from filtering previously absent from the cli.rs copy - Move filter_options_from to backends/cli.rs pub(crate); remove duplicate copies from executor.rs and nickel/roundtrip.rs (-346 lines, 0 behavior regressions) - Gate legacy sync path (execute_with_base_dir, execute, load_and_execute_from_file) on #[cfg(feature = "cli")] Retry fix - Replace unbounded recursive retry in execute_field_sync with loop+continue (O(1) stack) RenderContext clone cost - Change RenderContext.results from HashMap to Arc> - Change executor accumulators to Arc in execute_with_backend_complete, execute_with_backend_tw execute_with_backend_tw execute_with_backend_tw execute_with_backend_tw execute_with_bante execute_won: execute_with_backend_tw execute_with_backend_tw execute_with_backend_tw execute_count==1 (guaranteed at each insert site) - typedialog-ai/backend.rs: &context.results -> context.results.iter() for Arc deref Doctests - Fix five broken doctests: FormDefinition fields renamed, confirm() arity, FormBackend methods replaced, cli_loader ignore -> no_run with concr methods replaced, cli_loader ignore -> no_run with concr methods reken methods replaced, cli_loader ignore -> no_run with concr methods replaced, clsync path - docs/architecture.md Known Technical Debt: all items resolved, section closed EOF --- CHANGELOG.md | 16 ++++ crates/typedialog-ai/src/backend.rs | 2 +- crates/typedialog-core/src/backends/mod.rs | 38 ++++++--- crates/typedialog-core/src/backends/tui.rs | 10 +-- .../src/form_parser/executor.rs | 83 ++++++++++--------- docs/architecture.md | 11 +-- 6 files changed, 95 insertions(+), 65 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 799d166..4b90bd2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,22 @@ Five doctests in `typedialog-core` failed to compile against the current API: - `config/cli_loader.rs`: `ignore` annotation with fictional `MyBackendConfig` type; replaced with a concrete inline struct and changed to `no_run` +**Refactored - Eliminate O(n) HashMap clone on every field boundary** + +`RenderContext.results` changed from owned `HashMap` to +`Arc>`. The executor accumulator was also changed to `Arc` +so that `RenderContext` construction costs one atomic ref-count increment instead of +a full HashMap deep clone. After each backend call, `Arc::make_mut` mutates in place +since the ref count drops to 1 (the context is dropped before the insert). The +`execute_form_complete` default impl, all three executor functions +(`execute_with_backend_complete`, `execute_with_backend_two_phase_with_defaults`, +`execute_with_backend_i18n_with_defaults`), and the TUI fragment executor were updated. + +- All backends receive `&context.results` unchanged — `Arc` deref coercion to `&T` + is handled by the compiler with no source changes required in most call sites +- Net: eliminates O(n) allocations per field in the sequential execution hot path +- Resolves last open technical debt item documented in `docs/architecture.md` + **Fixed - Unbounded retry recursion in required-field validation** `InquireBackend::execute_field_sync` retried invalid required-field input by calling itself diff --git a/crates/typedialog-ai/src/backend.rs b/crates/typedialog-ai/src/backend.rs index b7b11b8..e6d1ada 100644 --- a/crates/typedialog-ai/src/backend.rs +++ b/crates/typedialog-ai/src/backend.rs @@ -219,7 +219,7 @@ impl AiBackend { // Previous values context if !context.results.is_empty() { prompt.push_str("## Current Configuration\n"); - for (k, v) in &context.results { + for (k, v) in context.results.iter() { prompt.push_str(&format!("- {}: {}\n", k, v)); } prompt.push('\n'); diff --git a/crates/typedialog-core/src/backends/mod.rs b/crates/typedialog-core/src/backends/mod.rs index 9ef97e7..64d8ef5 100644 --- a/crates/typedialog-core/src/backends/mod.rs +++ b/crates/typedialog-core/src/backends/mod.rs @@ -9,12 +9,17 @@ use async_trait::async_trait; use serde_json::Value; use std::collections::HashMap; use std::path::Path; +use std::sync::Arc; /// Context passed to rendering operations +/// +/// `results` is reference-counted: cloning a `RenderContext` (or creating a new one +/// from an existing accumulator via `Arc::clone`) costs a single atomic increment +/// instead of a full `HashMap` deep clone. #[derive(Debug, Clone)] pub struct RenderContext { - /// Previous field results (for conditional rendering) - pub results: HashMap, + /// Accumulated field results from previous form steps (read-only for backends) + pub results: Arc>, /// Optional locale override pub locale: Option, } @@ -53,24 +58,31 @@ pub trait FormBackend: Send + Sync { fields: Vec, initial_values: Option>, ) -> Result> { - // Default implementation: fall back to field-by-field mode - let mut results = initial_values.unwrap_or_default(); - let mut context = RenderContext { - results: results.clone(), - locale: form.locale.clone(), - }; + // Default implementation: fall back to field-by-field mode. + // The Arc accumulator makes context construction O(1) (atomic ref-count increment) + // instead of O(n) (HashMap clone). Arc::make_mut is O(1) when refcount == 1, + // which is guaranteed because each context is dropped before the next insert. + let mut results: Arc> = Arc::new(initial_values.unwrap_or_default()); + let locale = form.locale.clone(); for item in &items { + let context = RenderContext { + results: Arc::clone(&results), + locale: locale.clone(), + }; self.render_display_item(item, &context).await?; } for field in &fields { - context.results = results.clone(); + let context = RenderContext { + results: Arc::clone(&results), + locale: locale.clone(), + }; let value = self.execute_field(field, &context).await?; - results.insert(field.name.clone(), value); + Arc::make_mut(&mut results).insert(field.name.clone(), value); } - Ok(results) + Ok(Arc::try_unwrap(results).unwrap_or_else(|arc| (*arc).clone())) } /// Cleanup/shutdown the backend @@ -186,10 +198,12 @@ mod tests { #[test] fn test_render_context_clone() { let ctx = RenderContext { - results: HashMap::new(), + results: Arc::new(HashMap::new()), locale: Some("en-US".to_string()), }; let cloned = ctx.clone(); assert_eq!(cloned.locale, ctx.locale); + // Both point to the same allocation + assert!(Arc::ptr_eq(&ctx.results, &cloned.results)); } } diff --git a/crates/typedialog-core/src/backends/tui.rs b/crates/typedialog-core/src/backends/tui.rs index f0441da..733150d 100644 --- a/crates/typedialog-core/src/backends/tui.rs +++ b/crates/typedialog-core/src/backends/tui.rs @@ -199,7 +199,7 @@ impl FormBackend for RatatuiBackend { self.render_display_item( item, &super::RenderContext { - results: results.clone(), + results: Arc::new(results.clone()), locale: form.locale.clone(), }, ) @@ -1565,19 +1565,19 @@ impl RatatuiBackend { } // Execute each field in fragment - let mut results = HashMap::new(); + let mut results: Arc> = Arc::new(HashMap::new()); for element in &fragment_form.elements { if let crate::form_parser::FormElement::Field(field_def) = element { let context = RenderContext { - results: results.clone(), + results: Arc::clone(&results), locale: None, }; let value = self.execute_field(field_def, &context).await?; - results.insert(field_def.name.clone(), value); + Arc::make_mut(&mut results).insert(field_def.name.clone(), value); } } - Ok(results) + Ok(Arc::try_unwrap(results).unwrap_or_else(|arc| (*arc).clone())) } /// Summarize an item for display in TUI lists diff --git a/crates/typedialog-core/src/form_parser/executor.rs b/crates/typedialog-core/src/form_parser/executor.rs index f09a615..fe7939c 100644 --- a/crates/typedialog-core/src/form_parser/executor.rs +++ b/crates/typedialog-core/src/form_parser/executor.rs @@ -5,6 +5,7 @@ use crate::error::Result; use std::collections::{BTreeMap, HashMap}; use std::path::Path; +use std::sync::Arc; use super::conditions::evaluate_condition; use super::parser::load_elements_from_file; @@ -353,7 +354,8 @@ pub async fn execute_with_backend_complete( // Store initial values for later merging before unwrap_or_default let initial_backup = initial_values.clone(); - let mut results = initial_values.unwrap_or_default(); + let mut results: Arc> = + Arc::new(initial_values.unwrap_or_default()); // Initialize backend backend.initialize().await?; @@ -365,11 +367,11 @@ pub async fn execute_with_backend_complete( if let Some(field) = form.fields.iter().find(|f| &f.name == field_name) { let translated_field = translate_field_definition(field, i18n_bundle); let context = RenderContext { - results: results.clone(), + results: Arc::clone(&results), locale: None, }; let value = backend.execute_field(&translated_field, &context).await?; - results.insert(field.name.clone(), value); + Arc::make_mut(&mut results).insert(field.name.clone(), value); } } @@ -410,16 +412,12 @@ pub async fn execute_with_backend_complete( .map(|f| translate_field_definition(f, i18n_bundle)) .collect(); - results = backend + let complete_results = backend .execute_form_complete(&form, base_dir, items_owned, fields_owned, initial_backup) .await?; + results = Arc::new(complete_results); } else { // Field-by-field mode: iterate through element list - let mut context = RenderContext { - results: results.clone(), - locale: None, - }; - for (_, element) in element_list.iter() { match element { FormElement::Item(item) => { @@ -428,7 +426,10 @@ pub async fn execute_with_backend_complete( continue; } } - context.results = results.clone(); + let context = RenderContext { + results: Arc::clone(&results), + locale: None, + }; let translated_item = translate_display_item(item, i18n_bundle); backend .render_display_item(&translated_item, &context) @@ -445,10 +446,13 @@ pub async fn execute_with_backend_complete( } } - context.results = results.clone(); + let context = RenderContext { + results: Arc::clone(&results), + locale: None, + }; let translated_field = translate_field_definition(field, i18n_bundle); let value = backend.execute_field(&translated_field, &context).await?; - results.insert(field.name.clone(), value); + Arc::make_mut(&mut results).insert(field.name.clone(), value); } } } @@ -457,7 +461,7 @@ pub async fn execute_with_backend_complete( // Shutdown backend backend.shutdown().await?; - Ok(results) + Ok(Arc::try_unwrap(results).unwrap_or_else(|arc| (*arc).clone())) } /// Two-phase execution: selectors first, then dynamic fragments, then remaining fields @@ -481,7 +485,8 @@ pub async fn execute_with_backend_two_phase_with_defaults( use crate::backends::RenderContext; let initial_backup = initial_values.clone(); - let mut results = initial_values.unwrap_or_default(); + let mut results: Arc> = + Arc::new(initial_values.unwrap_or_default()); // Migrate legacy format (items/fields) to new unified elements format form.migrate_to_elements(); @@ -508,11 +513,11 @@ pub async fn execute_with_backend_two_phase_with_defaults( if let Some(field) = field_option { let translated_field = translate_field_definition(field, i18n_bundle); let context = RenderContext { - results: results.clone(), + results: Arc::clone(&results), locale: None, }; let value = backend.execute_field(&translated_field, &context).await?; - results.insert(field.name.clone(), value); + Arc::make_mut(&mut results).insert(field.name.clone(), value); } } @@ -534,11 +539,6 @@ pub async fn execute_with_backend_two_phase_with_defaults( } // PHASE 3: Execute remaining fields (non-selectors) - let mut context = RenderContext { - results: results.clone(), - locale: None, - }; - for (_, element) in element_list.iter() { match element { FormElement::Item(item) => { @@ -547,7 +547,10 @@ pub async fn execute_with_backend_two_phase_with_defaults( continue; } } - context.results = results.clone(); + let context = RenderContext { + results: Arc::clone(&results), + locale: None, + }; let translated_item = translate_display_item(item, i18n_bundle); backend .render_display_item(&translated_item, &context) @@ -564,10 +567,13 @@ pub async fn execute_with_backend_two_phase_with_defaults( } } - context.results = results.clone(); + let context = RenderContext { + results: Arc::clone(&results), + locale: None, + }; let translated_field = translate_field_definition(field, i18n_bundle); let value = backend.execute_field(&translated_field, &context).await?; - results.insert(field.name.clone(), value); + Arc::make_mut(&mut results).insert(field.name.clone(), value); } } } @@ -576,7 +582,7 @@ pub async fn execute_with_backend_two_phase_with_defaults( if let Some(init_vals) = &initial_backup { for (k, v) in init_vals.iter() { if !results.contains_key(k) { - results.insert(k.clone(), v.clone()); + Arc::make_mut(&mut results).insert(k.clone(), v.clone()); } } } @@ -584,7 +590,7 @@ pub async fn execute_with_backend_two_phase_with_defaults( // Shutdown backend backend.shutdown().await?; - Ok(results) + Ok(Arc::try_unwrap(results).unwrap_or_else(|arc| (*arc).clone())) } /// Execute a form using a specific backend @@ -625,7 +631,8 @@ pub async fn execute_with_backend_i18n_with_defaults( // Store initial values for later merging before unwrap_or_default let initial_backup = initial_values.clone(); - let mut results = initial_values.unwrap_or_default(); + let mut results: Arc> = + Arc::new(initial_values.unwrap_or_default()); // Initialize backend backend.initialize().await?; @@ -679,20 +686,19 @@ pub async fn execute_with_backend_i18n_with_defaults( .map(|f| translate_field_definition(f, i18n_bundle)) .collect(); - results = backend + let complete_results = backend .execute_form_complete(&form, base_dir, items_owned, fields_owned, initial_backup) .await?; + results = Arc::new(complete_results); } else { // Field-by-field mode - let mut context = RenderContext { - results: results.clone(), - locale: None, - }; - for (_, element) in element_list.iter() { match element { FormElement::Item(item) => { - context.results = results.clone(); + let context = RenderContext { + results: Arc::clone(&results), + locale: None, + }; let translated_item = translate_display_item(item, i18n_bundle); backend .render_display_item(&translated_item, &context) @@ -705,10 +711,13 @@ pub async fn execute_with_backend_i18n_with_defaults( } } - context.results = results.clone(); + let context = RenderContext { + results: Arc::clone(&results), + locale: None, + }; let translated_field = translate_field_definition(field, i18n_bundle); let value = backend.execute_field(&translated_field, &context).await?; - results.insert(field.name.clone(), value); + Arc::make_mut(&mut results).insert(field.name.clone(), value); } } } @@ -717,7 +726,7 @@ pub async fn execute_with_backend_i18n_with_defaults( // Shutdown backend backend.shutdown().await?; - Ok(results) + Ok(Arc::try_unwrap(results).unwrap_or_else(|arc| (*arc).clone())) } /// Execute a form using a specific backend (backward compatible wrapper) diff --git a/docs/architecture.md b/docs/architecture.md index 8106524..dd25cff 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -202,13 +202,4 @@ for new code is `execute_with_backend_two_phase_with_defaults`. ## Known Technical Debt -### `RenderContext` Clone Cost - -The executor clones `results` into `context` on every field iteration: - -```rust -context.results = results.clone(); -``` - -For forms with many fields containing large values (editor content, multi-select arrays), each -field execution pays O(n) clone cost over all previous results. +No unresolved items.