Improve the low-level API for non-fatal diagnostics

crates/message-format-compiler/src/compile/tests.rs

@@ -166,8 +166,9 @@ fn markup_option_variables_are_canonicalized_like_other_variables() {
         Value::Str("https://example.test".to_string()),
     )];
     let mut sink = MarkupOptionSink::default();
-    let diagnostics = formatter
-        .format_to(message, &args, &mut sink)
+    let mut diagnostics = vec![];
+    formatter
+        .format_to(message, &args, &mut sink, Some(&mut diagnostics))
         .expect("format");
 
     assert!(diagnostics.is_empty());

crates/message-format-compiler/src/test_support.rs

@@ -46,7 +46,7 @@ impl<H: Host> FormatterTestExt<H> for Formatter<'_, H> {
     ) -> Result<String, FormatError> {
         let message = self.resolve(message_id)?;
         let mut sink = OutputStringSink::default();
-        let _diagnostics = self.format_to(message, args, &mut sink)?;
+        self.format_to(message, args, &mut sink, None)?;
         Ok(sink.out)
     }
 }

crates/message-format-conformance/src/runtime_helpers.rs

@@ -15,7 +15,7 @@ pub(crate) fn format<H: Host>(
     args: &dyn Args,
 ) -> Result<String, FormatError> {
     let mut out = String::new();
-    let _diagnostics = formatter.format_to(message, args, &mut out)?;
+    formatter.format_to(message, args, &mut out, None)?;
     Ok(out)
 }
 
@@ -35,7 +35,8 @@ pub(crate) fn format_with_diagnostics_by_id<H: Host>(
 ) -> Result<FormatOutput, FormatError> {
     let message = formatter.resolve(message_id)?;
     let mut out = String::new();
-    let errors = formatter.format_to(message, args, &mut out)?;
+    let mut errors = vec![];
+    formatter.format_to(message, args, &mut out, Some(&mut errors))?;
     Ok(FormatOutput { value: out, errors })
 }
 
@@ -47,5 +48,7 @@ pub(crate) fn format_to_by_id<H: Host>(
     sink: &mut dyn message_format::runtime::FormatSink,
 ) -> Result<Vec<FormatError>, FormatError> {
     let message = formatter.resolve(message_id)?;
-    formatter.format_to(message, args, sink)
+    let mut errors = vec![];
+    formatter.format_to(message, args, sink, Some(&mut errors))?;
+    Ok(errors)
 }

crates/message-format-runtime/src/formatter.rs

@@ -10,7 +10,7 @@ use crate::{
     catalog::Catalog,
     error::{FormatError, Trap},
     value::{Args, MessageArgs, Value},
-    vm::{FormatSink, Host, MessageHandle, VecDiagnostics, run_bytecode},
+    vm::{DiagnosticsSink, FormatSink, Host, MessageHandle, run_bytecode},
 };
 
 #[derive(Default)]
@@ -43,7 +43,7 @@ pub(crate) struct VmState {
 /// }
 /// let mut out = String::new();
 /// let mut sink = StringSink(&mut out);
-/// let _errors = formatter.format_to(message, &[], &mut sink)?;
+/// formatter.format_to(message, &[], &mut sink, None)?;
 /// # Ok(out)
 /// # }
 /// ```
@@ -97,8 +97,8 @@ impl<'a, H: Host> Formatter<'a, H> {
 
     /// Format one message from a previously resolved handle, dispatching events to a [`FormatSink`].
     ///
-    /// Returns recoverable formatting diagnostics collected during fallback
-    /// rendering. Fatal execution failures are returned as `Err`.
+    /// Recoverable formatting diagnostics are collected into [`diagnostics`](DiagnosticsSink)
+    /// during fallback rendering. Fatal execution failures are returned as `Err`.
     ///
     /// This is the runtime API that preserves structured markup. In contrast,
     /// string-oriented convenience helpers flatten only literal/expression text
@@ -108,10 +108,10 @@ impl<'a, H: Host> Formatter<'a, H> {
         message: MessageHandle,
         args: &dyn Args,
         sink: &mut S,
-    ) -> Result<Vec<FormatError>, FormatError> {
+        diagnostics: Option<&mut dyn DiagnosticsSink>,
+    ) -> Result<(), FormatError> {
         #[cfg(feature = "profiling")]
         profiling::function_scope!();
-        let mut diagnostics = VecDiagnostics::default();
         run_bytecode(
             self.catalog,
             &mut self.host,
@@ -121,11 +121,11 @@ impl<'a, H: Host> Formatter<'a, H> {
             self.vm.fuel,
             &mut self.vm.stack,
             sink,
-            Some(&mut diagnostics),
+            diagnostics,
             &mut self.vm.call_args,
             &mut self.vm.call_options,
         )?;
-        Ok(diagnostics.into_inner())
+        Ok(())
     }
 }
 
@@ -251,6 +251,9 @@ impl<'a, H: Host> MultiFormatter<'a, H> {
 
     /// Format one message, dispatching events to a [`FormatSink`].
     ///
+    /// Recoverable formatting diagnostics are collected into [`diagnostics`](DiagnosticsSink)
+    /// during fallback rendering. Fatal execution failures are returned as `Err`.
+    ///
     /// The handle identifies both the catalog and the message entry point.
     ///
     /// # Args safety
@@ -270,14 +273,14 @@ impl<'a, H: Host> MultiFormatter<'a, H> {
         message: MultiMessageHandle,
         args: &dyn Args,
         sink: &mut S,
-    ) -> Result<Vec<FormatError>, FormatError> {
+        diagnostics: Option<&mut dyn DiagnosticsSink>,
+    ) -> Result<(), FormatError> {
         #[cfg(feature = "profiling")]
         profiling::function_scope!();
         let (catalog, index) = self
             .catalogs
             .get(message.catalog_idx as usize)
             .ok_or(FormatError::Trap(Trap::InvalidCatalogIndex))?;
-        let mut diagnostics = VecDiagnostics::default();
         run_bytecode(
             catalog,
             &mut self.host,
@@ -287,11 +290,11 @@ impl<'a, H: Host> MultiFormatter<'a, H> {
             self.vm.fuel,
             &mut self.vm.stack,
             sink,
-            Some(&mut diagnostics),
+            diagnostics,
             &mut self.vm.call_args,
             &mut self.vm.call_options,
         )?;
-        Ok(diagnostics.into_inner())
+        Ok(())
     }
 }
 
@@ -397,7 +400,7 @@ mod tests {
         let mut mf = mf;
         let mut sink = TestStringSink::default();
         assert_eq!(
-            mf.format_to(bad_handle, &vec![] as &Vec<(u32, Value)>, &mut sink)
+            mf.format_to(bad_handle, &vec![] as &Vec<(u32, Value)>, &mut sink, None)
                 .unwrap_err(),
             FormatError::Trap(Trap::InvalidCatalogIndex)
         );
@@ -426,7 +429,9 @@ mod tests {
         args.insert("who", "world").expect("arg interned in cat2");
 
         let mut sink = TestStringSink::default();
-        let diagnostics = mf.format_to(handle, &args, &mut sink).unwrap();
+        let mut diagnostics = vec![];
+        mf.format_to(handle, &args, &mut sink, Some(&mut diagnostics))
+            .unwrap();
         assert!(diagnostics.is_empty());
         assert_eq!(sink.out, "world");
     }

crates/message-format-runtime/src/lib.rs

@@ -64,7 +64,7 @@
 //!     .expect("catalog defines `name`");
 //! let mut out = String::new();
 //! let mut sink = StringSink(&mut out);
-//! let _errors = formatter.format_to(message, &args, &mut sink)?;
+//! formatter.format_to(message, &args, &mut sink, None)?;
 //! # Ok(out)
 //! # }
 //! ```
@@ -149,7 +149,8 @@
 //! args.insert("url", "https://example.com")
 //!     .expect("catalog defines `url`");
 //! let mut sink = CollectingSink::default();
-//! let errors = formatter.format_to(message, &args, &mut sink)?;
+//! let mut errors = vec![];
+//! formatter.format_to(message, &args, &mut sink, Some(&mut errors))?;
 //! assert!(errors.is_empty());
 //! # Ok(sink.events)
 //! # }

crates/message-format-runtime/src/vm.rs

@@ -237,24 +237,18 @@ pub struct FormatOption<'a> {
     pub value: Cow<'a, str>,
 }
 
-pub(crate) trait DiagnosticsSink {
+/// Sink for runtime error diagnostics.
+///
+/// Implement this trait to receive [`FormatError`] events. \
+/// Already implemented for `Vec<FormatError>`.
+pub trait DiagnosticsSink {
+    /// Record an error into the diagnostics sink.
     fn record(&mut self, error: FormatError);
 }
 
-#[derive(Default)]
-pub(crate) struct VecDiagnostics {
-    errors: Vec<FormatError>,
-}
-
-impl VecDiagnostics {
-    pub(crate) fn into_inner(self) -> Vec<FormatError> {
-        self.errors
-    }
-}
-
-impl DiagnosticsSink for VecDiagnostics {
+impl DiagnosticsSink for Vec<FormatError> {
     fn record(&mut self, error: FormatError) {
-        self.errors.push(error);
+        self.push(error);
     }
 }
 
@@ -267,10 +261,21 @@ enum SelectorValue<'a> {
     Owned(Value),
 }
 
-#[derive(Default)]
+enum ExprStatePendingErrors {
+    /// Record the minimum amount of information required
+    Minimal {
+        /// Were there any errors?
+        ///
+        /// This is important for [`ExprState::should_skip_call`].
+        any: bool,
+    },
+    /// Record all errors
+    All(Vec<FormatError>),
+}
+
 struct ExprState {
     fallback_id: Option<u32>,
-    pending_errors: Vec<FormatError>,
+    pending_errors: ExprStatePendingErrors,
 }
 
 impl<'a> SelectorValue<'a> {
@@ -333,16 +338,32 @@ impl<'a> SelectorValue<'a> {
 }
 
 impl ExprState {
+    fn new(diagnostics: &Option<&mut dyn DiagnosticsSink>) -> Self {
+        Self {
+            fallback_id: None,
+            pending_errors: match diagnostics {
+                Some(_) => ExprStatePendingErrors::All(Vec::new()),
+                None => ExprStatePendingErrors::Minimal { any: false },
+            },
+        }
+    }
+
     fn record_error(&mut self, error: FormatError) {
-        self.pending_errors.push(error);
+        match &mut self.pending_errors {
+            ExprStatePendingErrors::Minimal { any } => *any = true,
+            ExprStatePendingErrors::All(errors) => errors.push(error),
+        }
     }
 
     fn set_fallback(&mut self, fallback_id: u32) {
         self.fallback_id = Some(fallback_id);
     }
 
     fn should_skip_call(&self) -> bool {
-        !self.pending_errors.is_empty()
+        match &self.pending_errors {
+            ExprStatePendingErrors::Minimal { any } => *any,
+            ExprStatePendingErrors::All(errors) => !errors.is_empty(),
+        }
     }
 
     fn clear_fallback(&mut self) {
@@ -361,12 +382,23 @@ impl ExprState {
         push_expr_fallback(stack, catalog, self.fallback_id.take())
     }
 
-    fn take_pending_errors(&mut self) -> Vec<FormatError> {
-        core::mem::take(&mut self.pending_errors)
+    fn take_pending_errors(&mut self) -> Option<Vec<FormatError>> {
+        match &mut self.pending_errors {
+            ExprStatePendingErrors::Minimal { any } => {
+                *any = false;
+                None
+            }
+            ExprStatePendingErrors::All(errors) => Some(core::mem::take(errors)),
+        }
     }
 
     fn clear_pending_errors(&mut self) {
-        self.pending_errors.clear();
+        match &mut self.pending_errors {
+            ExprStatePendingErrors::Minimal { any } => {
+                *any = false;
+            }
+            ExprStatePendingErrors::All(errors) => errors.clear(),
+        }
     }
 }
 
@@ -390,7 +422,7 @@ where
     let mut pc = entry_pc;
     stack.clear();
     let mut selector: Option<SelectorValue<'_>> = None;
-    let mut expr_state = ExprState::default();
+    let mut expr_state = ExprState::new(&diagnostics);
     let mut remaining_fuel = fuel;
 
     loop {
@@ -761,21 +793,21 @@ fn handle_call_instruction<H: Host>(
     // skip the call and use the expression fallback (e.g. `{$varname}`) per
     // TR35 §16.
     if expr_state.should_skip_call() {
-        let pending_errors = expr_state.take_pending_errors();
-        if opcode == Opcode::CallSelect {
+        if let Some(pending_errors) = expr_state.take_pending_errors() {
             let mut pending_errors = pending_errors.into_iter();
-            record_bad_selector(diagnostics, pending_errors.next());
+            if opcode == Opcode::CallSelect {
+                record_bad_selector(diagnostics, pending_errors.next());
+            }
             for error in pending_errors {
                 record_diagnostic(diagnostics, error);
             }
+        }
+        if opcode == Opcode::CallSelect {
             expr_state.clear_fallback();
             stack.push(Value::Null);
-            return Ok(());
-        }
-        for error in pending_errors {
-            record_diagnostic(diagnostics, error);
+        } else {
+            expr_state.push_fallback(stack, catalog)?;
         }
-        expr_state.push_fallback(stack, catalog)?;
         return Ok(());
     }
 
@@ -1317,7 +1349,7 @@ mod tests {
         ) -> Result<String, FormatError> {
             let message = self.resolve(message_id)?;
             let mut sink = TestStringSink::default();
-            let _diagnostics = self.format_to(message, args, &mut sink)?;
+            self.format_to(message, args, &mut sink, None)?;
             Ok(sink.out)
         }
 
@@ -1328,7 +1360,9 @@ mod tests {
             sink: &mut dyn FormatSink,
         ) -> Result<Vec<FormatError>, FormatError> {
             let message = self.resolve(message_id)?;
-            self.format_to(message, args, sink)
+            let mut errors = vec![];
+            self.format_to(message, args, sink, Some(&mut errors))?;
+            Ok(errors)
         }
     }
 
@@ -1566,8 +1600,8 @@ mod tests {
             .format_by_id_for_test("main", &args)
             .expect("format");
         let mut sink = TestStringSink::default();
-        let _diagnostics = formatter
-            .format_to(handle, &args, &mut sink)
+        formatter
+            .format_to(handle, &args, &mut sink, None)
             .expect("format");
         let resolved = sink.out;
         assert_eq!(direct, resolved);

crates/message-format/src/formatter.rs

@@ -104,7 +104,7 @@ impl<'a> MessageFormatter<'a> {
         out.clear();
         let catalog = self.inner.catalog_for(message)?;
         let resolved = args.resolve(catalog);
-        let _diagnostics = self.inner.format_to(message, &resolved, out)?;
+        self.inner.format_to(message, &resolved, out, None)?;
         Ok(())
     }