From facfe8fb2a39a1c01d72dc5da193e4ffceb60694 Mon Sep 17 00:00:00 2001 From: branchseer Date: Sun, 15 Feb 2026 23:55:16 +0800 Subject: [PATCH] refactor: implement Index for ExecutionGraph Add Index<&LeafExecutionPath> for ExecutionGraph with Output = LeafExecutionKind, providing direct indexing into nested execution graphs to retrieve leaf execution kinds. - Extract shared graph traversal into LeafExecutionPath::resolve_item() - Remove Option from display fields in labeled reporter (was unreachable dead code) - Add explicit Index> on AcyclicGraph to prevent shadowing through Deref --- .../vite_task/src/session/reporter/labeled.rs | 61 +++++++------------ crates/vite_task/src/session/reporter/mod.rs | 50 +++++++++------ crates/vite_task_plan/src/execution_graph.rs | 15 ++++- 3 files changed, 64 insertions(+), 62 deletions(-) diff --git a/crates/vite_task/src/session/reporter/labeled.rs b/crates/vite_task/src/session/reporter/labeled.rs index 8f3ef1d9..ba8ee098 100644 --- a/crates/vite_task/src/session/reporter/labeled.rs +++ b/crates/vite_task/src/session/reporter/labeled.rs @@ -17,8 +17,8 @@ use vite_task_plan::{ExecutionGraph, ExecutionItemDisplay, ExecutionItemKind, Le use super::{ CACHE_MISS_STYLE, COMMAND_STYLE, ColorizeExt, ExitStatus, GraphExecutionReporter, GraphExecutionReporterBuilder, LeafExecutionPath, LeafExecutionReporter, StdioConfig, - StdioSuggestion, format_cache_hit_message, format_command_display, - format_command_with_cache_status, format_error_message, + StdioSuggestion, format_command_display, format_command_with_cache_status, + format_error_message, }; use crate::session::{ cache::format_cache_status_summary, @@ -28,9 +28,7 @@ use crate::session::{ /// Information tracked for each leaf execution, used in the final summary. #[derive(Debug)] struct ExecutionInfo { - /// Display info for this execution. `None` for displayless executions - /// (e.g., synthetics reached via nested expansion). - display: Option, + display: ExecutionItemDisplay, /// Cache status, determined at `start()`. cache_status: CacheStatus, /// Exit status from the process. `None` means no process was spawned (cache hit or in-process). @@ -146,8 +144,7 @@ pub struct LabeledGraphReporter { )] impl GraphExecutionReporter for LabeledGraphReporter { fn new_leaf_execution(&mut self, path: &LeafExecutionPath) -> Box { - // Look up display info from the graph using the path - let display = path.resolve_display(&self.graph).cloned(); + let display = path.resolve_item(&self.graph).execution_item_display.clone(); Box::new(LabeledLeafReporter { shared: Rc::clone(&self.shared), writer: Rc::clone(&self.writer), @@ -164,16 +161,8 @@ impl GraphExecutionReporter for LabeledGraphReporter { let (summary_buf, result) = { let shared = self.shared.borrow(); - // Print summary. - // Special case: single execution without display info (e.g., synthetic via - // nested expansion) → skip summary since there's nothing meaningful to show. - let is_single_displayless = - shared.executions.len() == 1 && shared.executions[0].display.is_none(); - let summary_buf = if is_single_displayless { - None - } else { - Some(format_summary(&shared.executions, &shared.stats, &self.workspace_path)) - }; + let summary_buf = + Some(format_summary(&shared.executions, &shared.stats, &self.workspace_path)); // Determine exit code based on failed tasks and infrastructure errors: // - Infrastructure errors (cache lookup, spawn failure) have error_message set @@ -231,7 +220,7 @@ struct LabeledLeafReporter { shared: Rc>, writer: Rc>>, /// Display info for this execution, looked up from the graph via the path. - display: Option, + display: ExecutionItemDisplay, workspace_path: Arc, /// Whether `start()` has been called. Used to determine if stats should be updated /// in `finish()` and whether to push an `ExecutionInfo` entry. @@ -281,16 +270,14 @@ impl LeafExecutionReporter for LabeledLeafReporter { // Format command line with cache status (sync), then write asynchronously. // The shared borrow to read cache_status is brief and dropped before the await. - if let Some(ref display) = self.display { - let line = { - let shared = self.shared.borrow(); - let cache_status = &shared.executions.last().unwrap().cache_status; - format_command_with_cache_status(display, &self.workspace_path, cache_status) - }; - let mut writer = self.writer.borrow_mut(); - let _ = writer.write_all(line.as_bytes()).await; - let _ = writer.flush().await; - } + let line = { + let shared = self.shared.borrow(); + let cache_status = &shared.executions.last().unwrap().cache_status; + format_command_with_cache_status(&self.display, &self.workspace_path, cache_status) + }; + let mut writer = self.writer.borrow_mut(); + let _ = writer.write_all(line.as_bytes()).await; + let _ = writer.flush().await; StdioConfig { suggestion, @@ -352,12 +339,6 @@ impl LeafExecutionReporter for LabeledLeafReporter { buf.extend_from_slice(format_error_message(message).as_bytes()); } - // For executions without display info (synthetics via nested expansion) that are - // cache hits, print the cache hit message - if self.started && self.display.is_none() && self.is_cache_hit { - buf.extend_from_slice(format_cache_hit_message().as_bytes()); - } - // Add a trailing newline after each task's output for readability. // Skip if start() was never called (e.g. cache lookup failure) — there's // no task output to separate. @@ -516,10 +497,7 @@ fn format_summary( ); for (idx, exec) in executions.iter().enumerate() { - // Skip executions without display info (they have nothing to show in the summary) - let Some(ref display) = exec.display else { - continue; - }; + let display = &exec.display; let task_display = &display.task_display; @@ -677,13 +655,16 @@ mod tests { /// Build a `LabeledGraphReporter` for the given graph and return a leaf reporter /// for the first node's first item. fn build_labeled_leaf(graph: ExecutionGraph) -> Box { + use vite_task_plan::execution_graph::ExecutionNodeIndex; + let graph_arc = Arc::new(graph); let builder = Box::new(LabeledReporterBuilder::new(test_path(), Box::new(tokio::io::sink()))); let mut reporter = builder.build(&graph_arc); - // Create a leaf reporter for the first node - let path = LeafExecutionPath::default(); + // Create a leaf reporter for the first node's first item + let mut path = LeafExecutionPath::default(); + path.push(ExecutionNodeIndex::new(0), 0); reporter.new_leaf_execution(&path) } diff --git a/crates/vite_task/src/session/reporter/mod.rs b/crates/vite_task/src/session/reporter/mod.rs index 6a52552e..f66f1c04 100644 --- a/crates/vite_task/src/session/reporter/mod.rs +++ b/crates/vite_task/src/session/reporter/mod.rs @@ -37,7 +37,9 @@ use smallvec::SmallVec; use tokio::io::AsyncWrite; use vite_path::AbsolutePath; use vite_str::Str; -use vite_task_plan::{ExecutionGraph, ExecutionItem, ExecutionItemDisplay, ExecutionItemKind}; +use vite_task_plan::{ + ExecutionGraph, ExecutionItem, ExecutionItemDisplay, ExecutionItemKind, LeafExecutionKind, +}; use super::{ cache::format_cache_status_inline, @@ -144,44 +146,52 @@ impl LeafExecutionPath { self.0.push(ExecutionPathItem { graph_node_ix, task_execution_item_index }); } - /// Look up the [`ExecutionItemDisplay`] for the leaf identified by this path, - /// traversing through nested `Expanded` graphs as needed. + /// Resolve this path against a root execution graph, returning the final + /// [`ExecutionItem`] the path points to. /// - /// Returns `None` if the path is empty. + /// This is the shared traversal logic that walks through nested `Expanded` + /// graphs. Used by: + /// - `Index<&LeafExecutionPath> for ExecutionGraph` — extracts `&LeafExecutionKind` + /// - `new_leaf_execution` in `labeled.rs` — extracts `ExecutionItemDisplay` /// /// # Panics /// - /// Panics if an intermediate path element does not point to an `Expanded` item, - /// which indicates a bug in path construction. - fn resolve_display<'a>( - &self, - root_graph: &'a ExecutionGraph, - ) -> Option<&'a ExecutionItemDisplay> { + /// - If the path is empty (indicates a bug in path construction). + /// - If an intermediate path element points to a `Leaf` item instead of + /// `Expanded` (only `Expanded` items contain nested graphs to descend into). + fn resolve_item<'a>(&self, root_graph: &'a ExecutionGraph) -> &'a ExecutionItem { let mut current_graph = root_graph; + let last_depth = self.0.len() - 1; for (depth, path_item) in self.0.iter().enumerate() { let item = path_item.resolve(current_graph); - let is_last = depth == self.0.len() - 1; - if is_last { - // Last element — return the display info regardless of Leaf/Expanded - return Some(&item.execution_item_display); + if depth == last_depth { + return item; } - // Intermediate element — must be Expanded so we can descend into it match &item.kind { ExecutionItemKind::Expanded(nested_graph) => { current_graph = nested_graph; } ExecutionItemKind::Leaf(_) => { - // A Leaf in the middle of the path means the execution engine - // pushed a Leaf node as an intermediate step, which is a bug — - // only Expanded items can contain nested graphs to descend into. unreachable!( "LeafExecutionPath: intermediate element at depth {depth} is a Leaf, expected Expanded" ) } } } - // Empty path - None + unreachable!("LeafExecutionPath: empty path") + } +} + +impl std::ops::Index<&LeafExecutionPath> for ExecutionGraph { + type Output = LeafExecutionKind; + + fn index(&self, path: &LeafExecutionPath) -> &Self::Output { + match &path.resolve_item(self).kind { + ExecutionItemKind::Leaf(kind) => kind, + ExecutionItemKind::Expanded(_) => { + unreachable!("LeafExecutionPath: final element is Expanded, expected Leaf") + } + } } } diff --git a/crates/vite_task_plan/src/execution_graph.rs b/crates/vite_task_plan/src/execution_graph.rs index e1010397..5a884296 100644 --- a/crates/vite_task_plan/src/execution_graph.rs +++ b/crates/vite_task_plan/src/execution_graph.rs @@ -1,4 +1,4 @@ -use std::ops::Deref; +use std::ops::{Deref, Index}; use petgraph::{ graph::{DefaultIx, DiGraph, EdgeIndex, IndexType, NodeIndex}, @@ -134,7 +134,7 @@ impl Default for AcyclicGraph { } /// Deref to the inner `DiGraph` so that read-only graph operations -/// (`node_count()`, `node_weights()`, `node_indices()`, indexing by `NodeIndex`, etc.) +/// (`node_count()`, `node_weights()`, `node_indices()`, etc.) /// work transparently on `AcyclicGraph`. impl Deref for AcyclicGraph { type Target = DiGraph; @@ -144,6 +144,17 @@ impl Deref for AcyclicGraph { } } +/// Explicit `NodeIndex` indexing so that `graph[node_ix]` continues to work +/// even when downstream crates add their own `Index` impls on `AcyclicGraph` +/// (a direct `Index` impl shadows any `Index` that would be found through `Deref`). +impl Index> for AcyclicGraph { + type Output = N; + + fn index(&self, index: NodeIndex) -> &Self::Output { + &self.graph[index] + } +} + /// The execution graph type alias, specialized for task execution. pub type ExecutionGraph = AcyclicGraph;