trace errors in tests, add warnings to tbsp

4 files changed, 131 insertions, 44 deletions

diff --git a/src/eval/analysis.rs b/src/eval/analysis.rs
index b3a0083..d5a7165 100644
--- a/src/eval/analysis.rs
+++ b/src/eval/analysis.rs
@@ -6,10 +6,17 @@ pub struct Analysis {
 }
 
 impl Analysis {
-    pub fn print(&self) {
+    pub fn contains_error(&self) -> bool {
+        self.diagnostics.iter().any(|d| d.level == Level::Error)
+    }
+}
+
+impl std::fmt::Display for Analysis {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         for Diagnostic { level, kind } in &self.diagnostics {
-            eprintln!("{level} {kind}");
+            writeln!(f, "{level} {kind}")?;
         }
+        Ok(())
     }
 }
 
@@ -36,16 +43,20 @@ impl std::fmt::Display for Level {
 
 #[derive(Debug, PartialEq, Eq)]
 pub enum Kind {
-    Pattern {
+    InvalidPattern {
         invalid_node_kind: String,
         full_pattern: ast::Pattern,
     },
+    SimplifiablePattern {
+        pattern: ast::TreePattern,
+        simplified: ast::TreePattern,
+    },
 }
 
 impl std::fmt::Display for Kind {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
-            Self::Pattern {
+            Self::InvalidPattern {
                 invalid_node_kind,
                 full_pattern,
             } => {
@@ -54,12 +65,21 @@ impl std::fmt::Display for Kind {
                     "invalid node kind `{invalid_node_kind}` in pattern `{full_pattern}`"
                 )
             }
+            Self::SimplifiablePattern {
+                pattern,
+                simplified,
+            } => {
+                write!(
+                    f,
+                    "the pattern `{pattern}` can be reduced to just `{simplified}`"
+                )
+            }
         }
     }
 }
 
 pub fn run(ctx: &super::Context) -> Analysis {
-    [validate_patterns(ctx)]
+    [validate_patterns(ctx), redundant_list_pattern(ctx)]
         .into_iter()
         .flatten()
         .collect::<Vec<_>>()
@@ -67,11 +87,9 @@ pub fn run(ctx: &super::Context) -> Analysis {
 }
 
 fn validate_patterns(ctx: &super::Context) -> Vec<Diagnostic> {
-    fn validate_pattern(
-        pattern: &ast::TreePattern,
-        language: &tree_sitter::Language,
-    ) -> Option<String> {
+    fn check(pattern: &ast::TreePattern, language: &tree_sitter::Language) -> Option<String> {
         match pattern {
+            // base case
             ast::TreePattern::Atom(a) => {
                 if language.id_for_node_kind(a, true) == 0 {
                     Some(a.to_owned())
@@ -79,9 +97,10 @@ fn validate_patterns(ctx: &super::Context) -> Vec<Diagnostic> {
                     None
                 }
             }
+            // recursive case
             ast::TreePattern::List(items) => {
                 for item in items {
-                    validate_pattern(item, language)?;
+                    check(item, language)?;
                 }
                 None
             }
@@ -94,7 +113,7 @@ fn validate_patterns(ctx: &super::Context) -> Vec<Diagnostic> {
         .flat_map(|s| match &s.pattern {
             ast::Pattern::Begin | ast::Pattern::End => None,
             ast::Pattern::Tree { matcher, .. } => {
-                validate_pattern(matcher, &ctx.language).map(|invalid_node_kind| Kind::Pattern {
+                check(matcher, &ctx.language).map(|invalid_node_kind| Kind::InvalidPattern {
                     invalid_node_kind,
                     full_pattern: s.pattern.clone(),
                 })
@@ -106,3 +125,38 @@ fn validate_patterns(ctx: &super::Context) -> Vec<Diagnostic> {
         })
         .collect()
 }
+
+fn redundant_list_pattern(ctx: &super::Context) -> Vec<Diagnostic> {
+    fn simplify(pattern: &ast::TreePattern) -> ast::TreePattern {
+        match pattern {
+            ast::TreePattern::Atom(a) => ast::TreePattern::Atom(a.to_owned()),
+            ast::TreePattern::List(l) => match l.as_slice() {
+                [a] => simplify(a),
+                items => ast::TreePattern::List(items.iter().map(simplify).collect::<Vec<_>>()),
+            },
+        }
+    }
+
+    ctx.program
+        .stanzas
+        .iter()
+        .flat_map(|s| match &s.pattern {
+            ast::Pattern::Begin | ast::Pattern::End => None,
+            ast::Pattern::Tree { matcher, .. } => {
+                let simplified = simplify(matcher);
+                if &simplified != matcher {
+                    Some(Kind::SimplifiablePattern {
+                        pattern: matcher.clone(),
+                        simplified,
+                    })
+                } else {
+                    None
+                }
+            }
+        })
+        .map(|kind| Diagnostic {
+            level: Level::Warning,
+            kind,
+        })
+        .collect()
+}
diff --git a/src/eval/builtins.rs b/src/eval/builtins.rs
index c91f7ba..e1fa10b 100644
--- a/src/eval/builtins.rs
+++ b/src/eval/builtins.rs
@@ -50,9 +50,7 @@ builtins! {
 fn print(ctx: &mut Context, args: &[ast::Expr]) -> Result {
     for arg in args {
         let val = ctx.eval_expr(arg)?;
-        let mut default_stream = Box::new(std::io::stdout()) as Box<dyn std::io::Write>;
-        let stream = ctx.output_stream.as_mut().unwrap_or(&mut default_stream);
-        write!(stream, "{val}").unwrap();
+        write!(ctx.output_stream, "{val}").unwrap();
     }
     Ok(Value::Unit)
 }
diff --git a/src/eval/mod.rs b/src/eval/mod.rs
index 589be37..9e0d033 100644
--- a/src/eval/mod.rs
+++ b/src/eval/mod.rs
@@ -3,8 +3,8 @@
 use crate::{ast, Wrap};
 use std::{collections::HashMap, fmt, io};
 
-mod builtins;
 mod analysis;
+mod builtins;
 #[cfg(test)]
 mod test;
 
@@ -453,7 +453,8 @@ pub struct Context<'s> {
     cursor: Option<tree_sitter::TreeCursor<'static>>,
     tree: Option<&'static tree_sitter::Tree>,
     cache: HashMap<NodeId, tree_sitter::Node<'static>>,
-    output_stream: Option<Box<dyn io::Write + 's>>,
+    output_stream: Box<dyn io::Write + 's>,
+    error_stream: Box<dyn io::Write + 's>,
 }
 
 impl<'s> fmt::Debug for Context<'s> {
@@ -484,7 +485,8 @@ impl<'s> Context<'s> {
             cursor: None,
             tree: None,
             cache: HashMap::default(),
-            output_stream: Some(Box::new(io::stdout()) as Box<dyn io::Write + 's>),
+            output_stream: Box::new(io::stdout()) as Box<dyn io::Write + 's>,
+            error_stream: Box::new(io::stderr()) as Box<dyn io::Write + 's>,
         }
     }
 
@@ -539,7 +541,12 @@ impl<'s> Context<'s> {
     }
 
     pub fn with_output_stream(mut self, stream: Box<dyn io::Write + 's>) -> Self {
-        self.output_stream = Some(stream);
+        self.output_stream = stream;
+        self
+    }
+
+    pub fn with_error_stream(mut self, stream: Box<dyn io::Write + 's>) -> Self {
+        self.error_stream = stream;
         self
     }
 
@@ -793,13 +800,11 @@ impl<'s> Context<'s> {
             .unwrap_or_default()
     }
 
-    pub fn analyze(&mut self) -> Result {
-        let analysis = analysis::run(&*self);
-        analysis.print();
-        Err(Error::Analysis)
+    pub fn analyze(&mut self) -> analysis::Analysis {
+        analysis::run(&*self)
     }
 
-    pub fn eval(&mut self) -> Result {
+    fn eval_program(&mut self) -> Result {
         let program = std::mem::take(&mut self.program);
         let mut has_next = true;
         let mut postorder = Vec::new();
@@ -847,6 +852,15 @@ impl<'s> Context<'s> {
 
         Ok(Value::Unit)
     }
+
+    pub fn eval(&mut self) -> Result {
+        let analysis = self.analyze();
+        write!(self.error_stream, "{analysis}").unwrap();
+        if analysis.contains_error() {
+            return Err(Error::Analysis);
+        }
+        self.eval_program()
+    }
 }
 
 pub fn evaluate(file: &str, program: &str, language: tree_sitter::Language) -> Result {
@@ -860,8 +874,8 @@ pub fn evaluate(file: &str, program: &str, language: tree_sitter::Language) -> R
         .with_input(file.to_owned())
         .with_tree(tree)
         .with_output_stream(Box::new(io::stdout()))
+        .with_error_stream(Box::new(io::stderr()))
         .with_program(program);
 
-    ctx.analyze()?;
     ctx.eval()
 }
diff --git a/src/eval/test.rs b/src/eval/test.rs
index 83749e0..c9e7630 100644
--- a/src/eval/test.rs
+++ b/src/eval/test.rs
@@ -1,32 +1,27 @@
 use super::*;
 use crate::ast::*;
-use std::io::Write;
 use expect_test::{expect, Expect};
 
-fn gen_test(file: &str, program: &str, expected: Expect) {
+fn gen_test(file: &str, program: &str, stdout: Expect, stderr: Expect) {
     let language = tree_sitter_python::language();
     let mut parser = tree_sitter::Parser::new();
     let _ = parser.set_language(&language);
     let tree = parser.parse(file, None).unwrap();
     let program = ast::Program::new().with_file(program).unwrap();
 
-    let mut output = Vec::new();
-    let result;
+    let mut out = Vec::new();
+    let mut err = Vec::new();
 
-    {
-        let mut ctx = Context::new(language)
-            .with_input(file.to_owned())
-            .with_tree(tree)
-            .with_program(program)
-            .with_output_stream(Box::new(&mut output) as Box<dyn io::Write>);
+    Context::new(language)
+        .with_input(file.to_owned())
+        .with_tree(tree)
+        .with_program(program)
+        .with_output_stream(Box::new(&mut out) as Box<dyn io::Write>)
+        .with_error_stream(Box::new(&mut err) as Box<dyn io::Write>)
+        .eval();
 
-        result = ctx.eval();
-    }
-
-    if let Err(e) = result {
-        writeln!(output, "{e:?}").unwrap();
-    }
-    expected.assert_eq(&String::from_utf8(output).unwrap())
+    stdout.assert_eq(&String::from_utf8(out).unwrap());
+    stderr.assert_eq(&String::from_utf8(err).unwrap());
 }
 
 #[test]
@@ -201,6 +196,7 @@ fn list_1() {
             }
             ",
         expect!["[5]"],
+        expect![],
     );
 }
 
@@ -214,6 +210,7 @@ fn list_2() {
             }
             ",
         expect!["3"],
+        expect![],
     );
 }
 
@@ -229,6 +226,7 @@ fn list_3() {
             }
             "#,
         expect!["true, false"],
+        expect![],
     );
 }
 
@@ -256,6 +254,7 @@ fn list_4() {
                 [5, 4]
                 [5]
             "#]],
+        expect![],
     );
 }
 
@@ -274,6 +273,7 @@ fn list_5() {
                 false
                 true
             "#]],
+        expect![],
     );
 }
 
@@ -291,6 +291,7 @@ fn string_1() {
             FOO
             foo
         "#]],
+        expect![],
     );
 }
 
@@ -320,6 +321,7 @@ fn string_2() {
             FOO is upper? true
             FOO is lower? false
         "#]],
+        expect![],
     );
 }
 
@@ -337,6 +339,7 @@ fn string_3() {
             a[3:5]: ` b`
             a[2:9]: `o bar b`
         "#]],
+        expect![],
     );
 }
 
@@ -349,9 +352,8 @@ fn string_4() {
                 println("a[9:20]: `", substr(a, 9, 20), "`");
             }
             "#,
-        expect![[r#"
-            a[9:20]: `InvalidStringSlice { length: 11, start: 9, end: 20 }
-        "#]],
+        expect!["a[9:20]: `"],
+        expect![],
     );
 }
 
@@ -367,5 +369,24 @@ fn node_1() {
             def foo(a, b): hello()
             foo
         "#]],
+        expect![],
+    );
+}
+
+#[test]
+fn analysis_1() {
+    gen_test(
+        "def foo(a, b): hello()",
+        r#"
+        enter (function) { }
+        enter ((function (name))) { }
+        enter ((function (name args))) { }
+        "#,
+        expect![],
+        expect![[r#"
+            [warning] the pattern `(function)` can be reduced to just `function`
+            [warning] the pattern `((function (name)))` can be reduced to just `(function name)`
+            [warning] the pattern `((function (name args)))` can be reduced to just `(function (name args))`
+        "#]],
     );
 }