fix: fix broken tests and make test parse a hard error

Also several cleanups to the parsing/errors to properly produce spans/context.

I keep checking in broken test files and not noticing because the test harness considers it ok
for a test to fail to parse/type (emitting a warning that gets buried by test output immediately).
there's literally no reason to be permissive in this case, as the tests loading/parsing isn't
platform specific, and there's no reason to toss non-tests in these dirs.

include/tests/normal/simple.kdl

@@ -13,7 +13,7 @@ fn "print" {
 
 fn "scale" {
     inputs { _ "Point3"; factor "f32"; }
-    outputs { "Point3"; }
+    outputs { _ "Point3"; }
 }
 
 fn "add" {

include/tests/procgen/struct/TwoAlignedU32s.procgen.kdl

@@ -3,7 +3,7 @@
 // https://github.com/rust-lang/rust/issues/80127
 
 @align 16
-struct "TwoU32s" {
+struct "TwoAlignedU32s" {
     a "u32"
     b "u32"
 }

include/tests/procgen/struct/TwoAlignedU64s.procgen.kdl

@@ -3,7 +3,7 @@
 // https://github.com/rust-lang/rust/issues/122767
 
 @align 16
-struct "TwoU64s" {
+struct "TwoAlignedU64s" {
     a "u64"
     b "u64"
 }

kdl-script/src/lib.rs

@@ -115,22 +115,6 @@ impl Compiler {
         }
         Ok(None)
     }
-
-    /*
-    fn diagnose(&mut self, err: KdlScriptError) {
-        use ErrorMode::*;
-        use ErrorStyle::*;
-        match &mut self.error_handler {
-            ErrorHandler {
-                error_mode: Scream,
-                error_style: Human,
-            } => {
-                eprintln!("{:?}", miette::Report::new(err));
-            }
-            _ => todo!(),
-        }
-    }
-     */
 }
 
 impl Default for Compiler {

kdl-script/src/parse.rs

@@ -190,7 +190,7 @@ pub enum Attr {
 
 /// An attribute declaring this type should be packed (remove padding/align).
 ///
-/// TODO: add support for an integer argument for the max align of a
+/// FIXME? add support for an integer argument for the max align of a
 /// field? Without one the default is 1. I never see packed(2) or whatever
 /// so I've never seriously thought through the implications...
 ///
@@ -348,8 +348,6 @@ pub enum PunSelector {
 #[derive(Debug, Clone)]
 pub struct PunEnv {
     /// The target language
-    ///
-    /// TODO: make this an enum?
     pub lang: String,
     // compiler: String,
     // os: String,
@@ -1013,7 +1011,6 @@ impl Parser<'_> {
                         help: Some("remove this?".to_owned()),
                     })?;
                 }
-                // TODO: deny any other members of `entries`
                 self.no_children(var)?;
                 Ok(EnumVariant { name, val })
             })
@@ -1041,8 +1038,6 @@ impl Parser<'_> {
     }
 
     /// Parse this node's name as a possibly-positional variable name
-    ///
-    /// TODO: probably want this `Option<Ident>` to be its own special enum?
     fn var_name_decl(&mut self, var: &KdlNode) -> Result<Option<Ident>> {
         let name = var.name();
         let name = if name.value() == "_" {
@@ -1056,14 +1051,17 @@ impl Parser<'_> {
 
     /// Parse a [`Tydent`][] from this String.
     fn tydent(&mut self, input: &Spanned<String>) -> Result<Spanned<Tydent>> {
-        let (_, ty_ref) = all_consuming(context("a type", tydent))(input)
+        let (_, mut ty_ref) = all_consuming(context("a type", tydent))(input)
             .finish()
             .map_err(|_e| KdlScriptParseError {
                 message: String::from("couldn't parse type"),
                 src: self.src.clone(),
                 span: Spanned::span(input),
                 help: None,
             })?;
+
+        // Inherit spans
+        inherit_spans(&mut ty_ref, input);
         Ok(ty_ref)
     }
 
@@ -1113,6 +1111,24 @@ impl Parser<'_> {
     }
 }
 
+fn inherit_spans(tydent: &mut Spanned<Tydent>, input: &Spanned<String>) {
+    Spanned::clone_span_from(tydent, input);
+    match &mut **tydent {
+        Tydent::Name(ident) => {
+            Spanned::clone_span_from(&mut ident.val, input);
+        }
+        Tydent::Array(elem_tydent, _) => {
+            inherit_spans(elem_tydent, input);
+        }
+        Tydent::Ref(pointee_tydent) => {
+            inherit_spans(pointee_tydent, input);
+        }
+        Tydent::Empty => {
+            // noop
+        }
+    }
+}
+
 // A fuckton of nom parsing for sub-syntax like Tydents.
 
 type NomResult<I, O> = IResult<I, O, VerboseError<I>>;
@@ -1128,7 +1144,6 @@ fn tydent_ref(input: &str) -> NomResult<&str, Spanned<Tydent>> {
         tag("&"),
         context("pointee type", cut(preceded(many0(unicode_space), tydent))),
     )(input)?;
-    // TODO: properly setup this span!
     Ok((input, Spanned::from(Tydent::Ref(Box::new(pointee_ty)))))
 }
 
@@ -1149,8 +1164,6 @@ fn tydent_array(input: &str) -> NomResult<&str, Spanned<Tydent>> {
         )),
         tag("]"),
     )(input)?;
-
-    // TODO: properly setup these spans!
     Ok((
         input,
         Spanned::from(Tydent::Array(Box::new(elem_ty), array_len)),
@@ -1165,7 +1178,6 @@ fn array_len(input: &str) -> NomResult<&str, u64> {
 /// Matches the empty tuple
 fn tydent_empty_tuple(input: &str) -> NomResult<&str, Spanned<Tydent>> {
     let (input, _tup) = tag("()")(input)?;
-    // TODO: properly setup this span!
     Ok((input, Spanned::from(Tydent::Empty)))
 }
 
@@ -1186,7 +1198,6 @@ fn tydent_named(input: &str) -> NomResult<&str, Spanned<Tydent>> {
     if let Some(_generics) = generics {
         panic!("generics aren't yet implemented!");
     }
-    // TODO: properly setup this span!
     Ok((
         input,
         Spanned::from(Tydent::Name(Ident {
@@ -1231,7 +1242,7 @@ fn unicode_space(input: &str) -> NomResult<&str, &str> {
 
 /// An integer expression (literal)
 ///
-/// TODO: this should actually defer deserializing into an integer
+/// FIXME: this should actually defer deserializing into an integer
 /// so that it can be some huge type like `u256`. Not sure who/where
 /// would be responsible for validating that the value fits in the
 /// expected range for where it's placed!

kdl-script/src/spanned.rs

@@ -25,6 +25,7 @@ impl<T> Spanned<T> {
             end: span.offset() + span.len(),
         }
     }
+
     /// Access the start of the span of the contained value.
     pub fn start(this: &Self) -> usize {
         this.start
@@ -35,6 +36,12 @@ impl<T> Spanned<T> {
         this.end
     }
 
+    // Acquire the span of the input
+    pub fn clone_span_from<U>(this: &mut Self, other: &Spanned<U>) {
+        this.start = other.start;
+        this.end = other.end;
+    }
+
     /// Update the span
     pub fn update_span(this: &mut Self, start: usize, end: usize) {
         this.start = start;

kdl-script/src/tests/type_pass.rs

@@ -72,7 +72,7 @@ fn c_enum_literals2() -> Result<(), miette::Report> {
 
 #[test]
 fn c_enum_literals3() -> Result<(), miette::Report> {
-    // TODO: this one might be dubious? Check what C/C++ allow with puns here
+    // FIXME: this one might be dubious? Check what C/C++ allow with puns here
     let program = r##"
         enum "Cases" {
             A 8

kdl-script/src/types.rs

@@ -64,7 +64,7 @@ pub struct KdlScriptTypeError {
     pub src: Arc<NamedSource>,
     #[label]
     pub span: SourceSpan,
-    #[help]
+    #[diagnostic(help)]
     pub help: Option<String>,
 }
 
@@ -724,7 +724,7 @@ impl TyCtx {
                     ty_idx
                 } else {
                     return Err(KdlScriptTypeError {
-                        message: "use of undefined type name".to_string(),
+                        message: format!("use of undefined type name: {name}"),
                         src: self.src.clone(),
                         span: Spanned::span(name),
                         help: None,

src/abis/c/init.rs

@@ -110,7 +110,6 @@ impl CcAbiImpl {
                 // The value is a mutable reference to a temporary
                 write!(f, "&{ref_temp_name}")?;
 
-                // TODO: should this be a recursive call to create_var (need create_var_inner?)
                 // Now do the rest of the recursion on constructing the temporary
                 let mut ref_temp = String::new();
                 let mut ref_temp_f = Fivemat::new(&mut ref_temp, INDENT);

src/abis/rust/init.rs

@@ -84,8 +84,6 @@ impl RustcAbiImpl {
             Ty::Ref(RefTy { pointee_ty }) => {
                 // The value is a mutable reference to a temporary
                 write!(f, "&mut {ref_temp_name}")?;
-
-                // TODO: should this be a recursive call to create_var (need create_var_inner?)
                 // Now do the rest of the recursion on constructing the temporary
                 let mut ref_temp = String::new();
                 let mut ref_temp_f = Fivemat::new(&mut ref_temp, INDENT);

src/error.rs

@@ -1,5 +1,7 @@
 use miette::Diagnostic;
 
+use crate::TestId;
+
 #[derive(Debug, thiserror::Error, Diagnostic)]
 pub enum CliParseError {
     #[error("{0}")]
@@ -39,6 +41,19 @@ pub enum GenerateError {
         block2: String,
         block2_val_count: usize,
     },
+    #[error("failed to read and parse test {test}")]
+    ReadTest {
+        test: TestId,
+        #[source]
+        #[diagnostic_source]
+        details: Box<GenerateError>,
+    },
+}
+
+impl std::borrow::Borrow<dyn Diagnostic> for Box<GenerateError> {
+    fn borrow(&self) -> &(dyn Diagnostic + 'static) {
+        &**self
+    }
 }
 
 #[derive(Debug, thiserror::Error, Diagnostic)]

src/harness/read.rs

@@ -100,13 +100,12 @@ pub fn spawn_read_test(
 
 /// Read a test .kdl file
 async fn read_test(test: TestId, test_file: TestFile) -> Result<Arc<Test>, GenerateError> {
-    read_test_inner(&test, test_file).await.map_err(|e| {
-        warn!(
-            "failed to read and parse test {test}, skipping\n{:?}",
-            miette::Report::new(e)
-        );
-        GenerateError::Skipped
-    })
+    read_test_inner(&test, test_file)
+        .await
+        .map_err(|e| GenerateError::ReadTest {
+            test,
+            details: Box::new(e),
+        })
 }
 
 async fn read_test_inner(test: &TestId, test_file: TestFile) -> Result<Arc<Test>, GenerateError> {

src/main.rs

@@ -19,7 +19,7 @@ use std::error::Error;
 use std::process::Command;
 use std::sync::Arc;
 use tokio::sync::OnceCell;
-use tracing::{debug, info};
+use tracing::{debug, error, info};
 use vals::ValueGeneratorKind;
 
 pub type SortedMap<K, V> = std::collections::BTreeMap<K, V>;
@@ -96,10 +96,25 @@ fn main() -> Result<(), Box<dyn Error>> {
         .map(|(test, test_file)| harness::spawn_read_test(&rt, test, test_file));
 
     // We could async pipeline this harder but it's nice to know all the tests upfront
-    let tests = read_tasks
-        .filter_map(|task| rt.block_on(task).expect("failed to join on task").ok())
-        .map(|test| (test.name.clone(), test))
-        .collect();
+    // Also we want it to be a hard error for any test to fail to load, as this indicates
+    // an abi-cafe developer error
+    let mut tests = SortedMap::new();
+    let mut test_read_fails = false;
+    for task in read_tasks {
+        let res = rt.block_on(task).expect("failed to join on task");
+        match res {
+            Ok(test) => {
+                tests.insert(test.name.clone(), test);
+            }
+            Err(e) => {
+                test_read_fails = true;
+                error!("{:?}", miette::Report::new(e));
+            }
+        }
+    }
+    if test_read_fails {
+        Err(TestsFailed {})?;
+    }
     let mut harness = TestHarness::new(tests, cfg.paths.clone());
 
     harness.add_abi_impl(

src/report.rs

@@ -440,7 +440,7 @@ impl FullReport {
             let (status, status_message) = match test.conclusion {
                 TestConclusion::Skipped => continue,
                 TestConclusion::Passed => ("ok", None),
-                TestConclusion::Failed => ("failed", Some("todo fill this message in")),
+                TestConclusion::Failed => ("failed", Some("FIXME fill this message in")),
                 TestConclusion::Busted => ("ok", None),
             };
             let test_name = harness.full_test_name(&test.key);