Merge #378: Add multi-error support to is_consistent and print colored errors in main.rs

38a4531 feat: add errors with color and multierrors to is_consistent funcs (LesterEvSe)

Pull request description:

  - Output errors to `stderr` with color support in `main.rs`.
  - Add multi-error support to `Arguments` and `Witness`.

ACKs for top commit:
  KyrylR:
    ACK 38a4531; successfully ran local tests

Tree-SHA512: a88de0c45fd79760cde9a5cfef55f72eca696b229687721d9de90e568bcfcf595116ffdb3f0e90219ec46d2a347a7104854d8f6adc710cdccd5bf18550101385

Author: LesterEvSe

src/error.rs

@@ -1,7 +1,8 @@
 use std::collections::hash_map::Entry;
 use std::collections::HashMap;
+use std::ffi::OsStr;
 use std::fmt;
-use std::io::{self, Write};
+use std::io::{self, IsTerminal, Write};
 use std::ops::Range;
 use std::path::PathBuf;
 use std::sync::Arc;
@@ -380,6 +381,8 @@ where
     }
 }
 
+/// Collects diagnostics emitted during a single compilation and renders
+/// them against the source files they refer to.
 #[derive(Debug, Clone, Default)]
 pub struct DiagnosticManager {
     diags: Vec<Diagnostic>,
@@ -520,6 +523,36 @@ impl<'a> Cache<usize> for RenderCache<'a> {
     }
 }
 
+/// Pure color-decision logic.
+fn decide_color(clicolor_force: Option<&OsStr>, no_color: Option<&OsStr>, is_tty: bool) -> bool {
+    let zero = OsStr::new("0");
+
+    if let Some(v) = clicolor_force {
+        if !v.is_empty() && v != zero {
+            return true;
+        }
+    }
+    if let Some(v) = no_color {
+        if !v.is_empty() {
+            return false;
+        }
+    }
+    is_tty
+}
+
+/// Whether the given stream should be rendered with ANSI color.
+///
+/// Precedence: `CLICOLOR_FORCE` (non-empty, non-`"0"`) then force on;
+/// `NO_COLOR` (non-empty, per <https://no-color.org>) then force off;
+/// otherwise follow the stream's TTY status.
+pub fn should_color<S: IsTerminal>(stream: &S) -> bool {
+    decide_color(
+        std::env::var_os("CLICOLOR_FORCE").as_deref(),
+        std::env::var_os("NO_COLOR").as_deref(),
+        stream.is_terminal(),
+    )
+}
+
 fn render_one(
     diag: &Diagnostic,
     cache: &mut RenderCache,
@@ -824,6 +857,9 @@ pub enum Error {
     WitnessReused {
         name: WitnessName,
     },
+    WitnessMissing {
+        name: WitnessName,
+    },
     WitnessTypeMismatch {
         name: WitnessName,
         declared: ResolvedType,
@@ -1075,6 +1111,10 @@ impl fmt::Display for Error {
                 f,
                 "Witness `{name}` has been used before somewhere in the program"
             ),
+            Error::WitnessMissing { name } => write!(
+                f,
+                "Missing witness for `{name}`"
+            ),
             Error::WitnessTypeMismatch { name, declared, assigned } => write!(
                 f,
                 "Witness `{name}` was declared with type `{declared}` but its assigned value is of type `{assigned}`"
@@ -1189,6 +1229,7 @@ mod render_tests {
     use crate::source::CanonSourceFile;
     use crate::test_utils::TempWorkspace;
 
+    use std::ffi::OsStr;
     use std::sync::Arc;
 
     const CONTENT: &str = "let a1: List<u32, 5> = None;\nlet x: u32 = Left(\n    Right(0)\n);";
@@ -1247,6 +1288,16 @@ mod render_tests {
         Span::new(MAIN_MODULE, range)
     }
 
+    #[test]
+    fn clicolor_force_zero_does_not_force() {
+        // Regression case: CLICOLOR_FORCE=0 with non-TTY stderr must not emit escapes.
+        assert!(!decide_color(Some(OsStr::new("0")), None, false));
+        assert!(decide_color(Some(OsStr::new("1")), None, false));
+        assert!(!decide_color(None, Some(OsStr::new("1")), true));
+        assert!(decide_color(None, None, true));
+        assert!(!decide_color(None, None, false));
+    }
+
     #[test]
     fn golden_full_diagnostic() {
         let mut fixture = Fixture::new(CONTENT);

src/lib.rs

@@ -202,9 +202,14 @@ impl TemplateProgram {
         arguments: Arguments,
         include_debug_symbols: bool,
     ) -> Result<CompiledProgram, String> {
-        arguments
-            .is_consistent(self.simfony.parameters())
-            .map_err(|error| error.to_string())?;
+        // This function returns Result<_, String> and its neighbors do not carry a
+        // DiagnosticManager, so we mint a local one to collect all witness mismatches,
+        // then render it to a message on failure.
+        let mut diagnostics = DiagnosticManager::new();
+        arguments.is_consistent(self.simfony.parameters(), &mut diagnostics);
+        if diagnostics.has_errors() {
+            return Err(diagnostics.to_string());
+        }
 
         let commit = self.simfony.compile(
             arguments,
@@ -352,9 +357,14 @@ impl CompiledProgram {
         witness_values: WitnessValues,
         env: Option<&ElementsEnv<Arc<elements::Transaction>>>,
     ) -> Result<SatisfiedProgram, String> {
-        witness_values
-            .is_consistent(&self.witness_types)
-            .map_err(|e| e.to_string())?;
+        // This function returns Result<_, String> and its neighbors do not carry a
+        // DiagnosticManager, so we mint a local one to collect all witness mismatches,
+        // then render it to a message on failure.
+        let mut diagnostics = DiagnosticManager::new();
+        witness_values.is_consistent(&self.witness_types, &mut diagnostics);
+        if diagnostics.has_errors() {
+            return Err(diagnostics.to_string());
+        }
 
         let mut simplicity_redeem = named::populate_witnesses(&self.simplicity, witness_values)?;
         if let Some(env) = env {

src/main.rs

@@ -3,14 +3,15 @@ use base64::engine::general_purpose::STANDARD;
 use clap::{Arg, ArgAction, Command};
 
 use simplicityhl::ast::ElementsJetHinter;
+use simplicityhl::error::should_color;
 use simplicityhl::version::SimcDirective;
 use simplicityhl::{
     resolution::DependencyMapBuilder, source::CanonPath, source::CanonSourceFile, AbiMeta,
     TemplateProgram,
 };
 use simplicityhl::{UnstableFeature, UnstableFeatures};
 use std::path::Path;
-use std::{env, fmt};
+use std::{env, fmt, io};
 
 #[cfg_attr(feature = "serde", derive(serde::Serialize))]
 /// The compilation output.
@@ -203,10 +204,14 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
         Box::new(ElementsJetHinter::new()),
     ) {
         Ok(program) => program,
-        Err(e) => {
-            // `Display` is message-only; render for source snippets with
-            // file and line pointers, as the single-file path does.
-            eprintln!("{}", e.render_to_string());
+        Err(diags) => {
+            let stderr = io::stderr();
+            let with_color = should_color(&stderr);
+            let mut lock = stderr.lock();
+
+            diags
+                .render(with_color, &mut lock)
+                .expect("writing to stderr");
             std::process::exit(1);
         }
     };
@@ -222,7 +227,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
     let compiled = match template.instantiate(args_opt, include_debug_symbols) {
         Ok(program) => program,
         Err(e) => {
-            eprintln!("{}", e);
+            eprintln!("{e}");
             std::process::exit(1);
         }
     };
@@ -250,7 +255,13 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
 
     let (program_bytes, witness_bytes) = match witness_opt {
         Some(witness) => {
-            let satisfied = compiled.satisfy(witness)?;
+            let satisfied = match compiled.satisfy(witness) {
+                Ok(s) => s,
+                Err(e) => {
+                    eprintln!("{e}");
+                    std::process::exit(1);
+                }
+            };
             let (program_bytes, witness_bytes) = satisfied.redeem().to_vec_with_witness();
             (program_bytes, Some(witness_bytes))
         }

src/named.rs

@@ -210,7 +210,12 @@ pub fn populate_witnesses(
         ) -> Result<simplicity::Value, Self::Error> {
             match self.values.get(witness) {
                 Some(val) => Ok(simplicity::Value::from(StructuralValue::from(val))),
-                None => Err(format!("missing witness for {witness}")),
+                // Presence is validated by `WitnessValues::is_consistent` before this
+                // runs; reaching this arm means the caller skipped validation.
+                None => Err(format!(
+                    "internal error: witness `{witness}` missing; \
+                     call WitnessValues::is_consistent before populate_witnesses"
+                )),
             }
         }
 

src/witness.rs

@@ -2,7 +2,7 @@ use std::collections::HashMap;
 use std::fmt;
 use std::sync::Arc;
 
-use crate::error::{Diagnostic, Error, WithSpan};
+use crate::error::{Diagnostic, DiagnosticManager, Error, WithSpan};
 use crate::parse::ParseFromStr;
 use crate::str::WitnessName;
 use crate::types::{AliasedType, ResolvedType};
@@ -110,23 +110,25 @@ impl WitnessValues {
     /// There may be witnesses that are referenced in the program that are not assigned a value
     /// in the witness map. These witnesses may lie on pruned branches that will not be part of the
     /// finalized Simplicity program. However, before the finalization, we cannot know which
-    /// witnesses will be pruned and which won't be pruned. This check skips unassigned witnesses.
-    pub fn is_consistent(&self, witness_types: &WitnessTypes) -> Result<(), Error> {
-        for name in self.0.keys() {
-            let Some(declared_ty) = witness_types.get(name) else {
+    /// witnesses will be pruned and which won't be pruned.
+    pub fn is_consistent(&self, witness_types: &WitnessTypes, diagnostics: &mut DiagnosticManager) {
+        for (name, declared_ty) in witness_types.iter() {
+            let Some(value) = self.get(name) else {
+                diagnostics.push(Diagnostic::global(Error::WitnessMissing {
+                    name: name.shallow_clone(),
+                }));
                 continue;
             };
-            let assigned_ty = self.0[name].ty();
+
+            let assigned_ty = value.ty();
             if assigned_ty != declared_ty {
-                return Err(Error::WitnessTypeMismatch {
+                diagnostics.push(Diagnostic::global(Error::WitnessTypeMismatch {
                     name: name.clone(),
                     declared: declared_ty.clone(),
                     assigned: assigned_ty.clone(),
-                });
+                }));
             }
         }
-
-        Ok(())
     }
 }
 
@@ -234,21 +236,23 @@ impl Arguments {
     /// 2. The type of each parameter must match the type of its argument.
     ///
     /// Arguments without a corresponding parameter are ignored.
-    pub fn is_consistent(&self, parameters: &Parameters) -> Result<(), Error> {
+    pub fn is_consistent(&self, parameters: &Parameters, diagnostics: &mut DiagnosticManager) {
         for (name, parameter_ty) in parameters.iter() {
-            let argument = self.get(name).ok_or_else(|| Error::ArgumentMissing {
-                name: name.shallow_clone(),
-            })?;
+            let Some(argument) = self.get(name) else {
+                diagnostics.push(Diagnostic::global(Error::ArgumentMissing {
+                    name: name.shallow_clone(),
+                }));
+                continue;
+            };
+
             if !argument.is_of_type(parameter_ty) {
-                return Err(Error::ArgumentTypeMismatch {
+                diagnostics.push(Diagnostic::global(Error::ArgumentTypeMismatch {
                     name: name.clone(),
                     declared: parameter_ty.clone(),
                     assigned: argument.ty().clone(),
-                });
+                }));
             }
         }
-
-        Ok(())
     }
 }
 
@@ -313,7 +317,7 @@ mod tests {
         ) {
             Ok(_) => panic!("Ill-typed witness assignment was falsely accepted"),
             Err(error) => assert_eq!(
-                "Witness `A` was declared with type `u32` but its assigned value is of type `u16`",
+                "Witness `A` was declared with type `u32` but its assigned value is of type `u16`\n",
                 error
             ),
         }