Fix a bunch of bugs

src/cli.rs

@@ -156,8 +156,8 @@ pub async fn cli() -> Result<(), Box<dyn Error>> {
 
             LineResult::Error(mut line, err) => {
                 rl.add_history_entry(line.clone());
-                match err {
+
-                    ShellError::Diagnostic(diag) => {
+                let diag = err.to_diagnostic();
                 let host = context.host.lock().unwrap();
                 let writer = host.err_termcolor();
                 line.push_str(" ");
@@ -166,35 +166,50 @@ pub async fn cli() -> Result<(), Box<dyn Error>> {
                 language_reporting::emit(
                     &mut writer.lock(),
                     &files,
-                            &diag.diagnostic,
+                    &diag,
                     &language_reporting::DefaultConfig,
                 )
                 .unwrap();
-                    }
 
-                    ShellError::TypeError(desc) => context
+                // match err {
-                        .host
+                //     ShellError::Diagnostic(diag) => {
-                        .lock()
+                //         let host = context.host.lock().unwrap();
-                        .unwrap()
+                //         let writer = host.err_termcolor();
-                        .stdout(&format!("TypeError: {}", desc)),
+                //         line.push_str(" ");
+                //         let files = crate::parser::Files::new(line);
 
-                    ShellError::MissingProperty { subpath, .. } => context
+                //         language_reporting::emit(
-                        .host
+                //             &mut writer.lock(),
-                        .lock()
+                //             &files,
-                        .unwrap()
+                //             &diag.diagnostic,
-                        .stdout(&format!("Missing property {}", subpath)),
+                //             &language_reporting::DefaultConfig,
+                //         )
+                //         .unwrap();
+                //     }
 
-                    ShellError::String(_) => {
+                //     ShellError::TypeError(desc) => context
-                        context.host.lock().unwrap().stdout(&format!("{}", err))
+                //         .host
-                    }
+                //         .lock()
-                }
+                //         .unwrap()
+                //         .stdout(&format!("TypeError: {}", desc)),
+
+                //     ShellError::MissingProperty { subpath, .. } => context
+                //         .host
+                //         .lock()
+                //         .unwrap()
+                //         .stdout(&format!("Missing property {}", subpath)),
+
+                //     ShellError::String(_) => {
+                //         context.host.lock().unwrap().stdout(&format!("{}", err))
+                //     }
+                // }
             }
 
             LineResult::Break => {
                 break;
             }
 
-            LineResult::FatalError(err) => {
+            LineResult::FatalError(_, err) => {
                 context
                     .host
                     .lock()
@@ -216,24 +231,24 @@ enum LineResult {
     Break,
 
     #[allow(unused)]
-    FatalError(ShellError),
+    FatalError(String, ShellError),
 }
 
 impl std::ops::Try for LineResult {
     type Ok = Option<String>;
-    type Error = ShellError;
+    type Error = (String, ShellError);
 
-    fn into_result(self) -> Result<Option<String>, ShellError> {
+    fn into_result(self) -> Result<Option<String>, (String, ShellError)> {
         match self {
             LineResult::Success(s) => Ok(Some(s)),
-            LineResult::Error(_, s) => Err(s),
+            LineResult::Error(string, err) => Err((string, err)),
             LineResult::Break => Ok(None),
             LineResult::CtrlC => Ok(None),
-            LineResult::FatalError(err) => Err(err),
+            LineResult::FatalError(string, err) => Err((string, err)),
         }
     }
-    fn from_error(v: ShellError) -> Self {
+    fn from_error(v: (String, ShellError)) -> Self {
-        LineResult::Error(String::new(), v)
+        LineResult::Error(v.0, v.1)
     }
 
     fn from_ok(v: Option<String>) -> Self {
@@ -260,7 +275,8 @@ async fn process_line(readline: Result<String, ReadlineError>, ctx: &mut Context
             debug!("=== Parsed ===");
             debug!("{:#?}", result);
 
-            let mut pipeline = classify_pipeline(&result, ctx, &Text::from(line))?;
+            let mut pipeline = classify_pipeline(&result, ctx, &Text::from(line))
+                .map_err(|err| (line.clone(), err))?;
 
             match pipeline.commands.last() {
                 Some(ClassifiedCommand::Sink(_)) => {}
@@ -438,7 +454,13 @@ fn classify_command(
                             //Some(args) => args.iter().map(|i| i.as_external_arg(source)).collect(),
                             Some(args) => args
                                 .iter()
-                                .map(|i| Spanned::from_item(i.as_external_arg(source), i.span()))
+                                .filter_map(|i| match i {
+                                    TokenNode::Whitespace(_) => None,
+                                    other => Some(Spanned::from_item(
+                                        other.as_external_arg(source),
+                                        other.span(),
+                                    )),
+                                })
                                 .collect(),
                             None => vec![],
                         };
@@ -453,8 +475,12 @@ fn classify_command(
             }
         }
 
-        _ => Err(ShellError::unimplemented(
+        call => Err(ShellError::diagnostic(
-            "classify_command on command whose head is not bare",
+            language_reporting::Diagnostic::new(
+                language_reporting::Severity::Error,
+                "Invalid command",
+            )
+            .with_label(language_reporting::Label::new_primary(call.head().span())),
         )),
     }
 }

src/errors.rs

@@ -1,17 +1,51 @@
 #[allow(unused)]
 use crate::prelude::*;
 
-use crate::parser::Span;
+use crate::parser::{Span, Spanned};
 use derive_new::new;
 use language_reporting::{Diagnostic, Label, Severity};
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
 
+#[derive(Debug, Eq, PartialEq, Clone, Ord, PartialOrd, Serialize, Deserialize)]
+pub enum Description {
+    Source(Spanned<String>),
+    Synthetic(String),
+}
+
+impl Description {
+    pub fn from(item: Spanned<impl Into<String>>) -> Description {
+        match item {
+            Spanned {
+                span: Span { start: 0, end: 0 },
+                item,
+            } => Description::Synthetic(item.into()),
+            Spanned { span, item } => Description::Source(Spanned::from_item(item.into(), span)),
+        }
+    }
+}
+
+impl Description {
+    fn into_label(self) -> Result<Label<Span>, String> {
+        match self {
+            Description::Source(s) => Ok(Label::new_primary(s.span).with_message(s.item)),
+            Description::Synthetic(s) => Err(s),
+        }
+    }
+}
+
 #[derive(Debug, Eq, PartialEq, Clone, Ord, PartialOrd, Serialize, Deserialize)]
 pub enum ShellError {
     String(StringError),
-    TypeError(String),
+    TypeError(Spanned<String>),
-    MissingProperty { subpath: String, expr: String },
+    MissingProperty {
+        subpath: Description,
+        expr: Description,
+    },
     Diagnostic(ShellDiagnostic),
+    CoerceError {
+        left: Spanned<String>,
+        right: Spanned<String>,
+    },
 }
 
 impl ShellError {
@@ -24,7 +58,7 @@ impl ShellError {
             nom::Err::Incomplete(_) => unreachable!(),
             nom::Err::Failure(span) | nom::Err::Error(span) => {
                 let diagnostic =
-                    Diagnostic::new(Severity::Error, format!("{:?}", span))
+                    Diagnostic::new(Severity::Error, format!("Parse Error"))
                         .with_label(Label::new_primary(Span::from(span.0)));
 
                 ShellError::diagnostic(diagnostic)
@@ -57,6 +91,41 @@ impl ShellError {
         ShellError::Diagnostic(ShellDiagnostic { diagnostic })
     }
 
+    crate fn to_diagnostic(self) -> Diagnostic<Span> {
+        match self {
+            ShellError::String(StringError { title, .. }) => {
+                Diagnostic::new(Severity::Error, title)
+            }
+            ShellError::TypeError(s) => Diagnostic::new(Severity::Error, "Type Error")
+                .with_label(Label::new_primary(s.span).with_message(s.item)),
+
+            ShellError::MissingProperty { subpath, expr } => {
+                let subpath = subpath.into_label();
+                let expr = expr.into_label();
+
+                let mut diag = Diagnostic::new(Severity::Error, "Missing property");
+
+                match subpath {
+                    Ok(label) => diag = diag.with_label(label),
+                    Err(ty) => diag.message = format!("Missing property (for {})", ty),
+                }
+
+                if let Ok(label) = expr {
+                    diag = diag.with_label(label);
+                }
+
+                diag
+            }
+
+            ShellError::Diagnostic(diag) => diag.diagnostic,
+            ShellError::CoerceError { left, right } => {
+                Diagnostic::new(Severity::Error, "Coercion error")
+                    .with_label(Label::new_primary(left.span).with_message(left.item))
+                    .with_label(Label::new_secondary(right.span).with_message(right.item))
+            }
+        }
+    }
+
     crate fn labeled_error(
         msg: impl Into<String>,
         label: impl Into<String>,
@@ -177,6 +246,7 @@ impl std::fmt::Display for ShellError {
             ShellError::TypeError { .. } => write!(f, "TypeError"),
             ShellError::MissingProperty { .. } => write!(f, "MissingProperty"),
             ShellError::Diagnostic(_) => write!(f, "<diagnostic>"),
+            ShellError::CoerceError { .. } => write!(f, "CoerceError"),
         }
     }
 }

src/evaluate/evaluator.rs

@@ -1,3 +1,4 @@
+use crate::errors::Description;
 use crate::object::base::Block;
 use crate::parser::{
     hir::{self, Expression, RawExpression},
@@ -37,11 +38,11 @@ crate fn evaluate_baseline_expr(
             let right = evaluate_baseline_expr(binary.right(), registry, scope, source)?;
 
             match left.compare(binary.op(), &*right) {
-                Some(result) => Ok(Spanned::from_item(Value::boolean(result), *expr.span())),
+                Ok(result) => Ok(Spanned::from_item(Value::boolean(result), *expr.span())),
-                None => Err(ShellError::unimplemented(&format!(
+                Err((left_type, right_type)) => Err(ShellError::CoerceError {
-                    "Comparison failure {:?}",
+                    left: binary.left().copy_span(left_type),
-                    binary
+                    right: binary.right().copy_span(right_type),
-                ))),
+                }),
             }
         }
         RawExpression::Block(block) => Ok(Spanned::from_item(
@@ -50,18 +51,26 @@ crate fn evaluate_baseline_expr(
         )),
         RawExpression::Path(path) => {
             let value = evaluate_baseline_expr(path.head(), registry, scope, source)?;
-            let mut value = value.item();
+            let mut item = value;
 
             for name in path.tail() {
-                let next = value.get_data_by_key(name);
+                let next = item.get_data_by_key(name);
 
                 match next {
-                    None => return Err(ShellError::unimplemented("Invalid property from path")),
+                    None => {
-                    Some(next) => value = next,
+                        return Err(ShellError::MissingProperty {
+                            subpath: Description::from(item.spanned_type_name()),
+                            expr: Description::from(name.clone()),
+                        })
+                    }
+                    Some(next) => {
+                        item =
+                            Spanned::from_item(next.clone(), (expr.span().start, name.span().end))
+                    }
                 };
             }
 
-            Ok(Spanned::from_item(value.clone(), expr.span()))
+            Ok(Spanned::from_item(item.item().clone(), expr.span()))
         }
         RawExpression::Boolean(_boolean) => unimplemented!(),
     }

src/object/base.rs

@@ -165,7 +165,7 @@ impl Block {
     }
 }
 
-#[derive(Debug, Ord, PartialOrd, Eq, PartialEq, Clone)]
+#[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Clone)]
 pub enum Value {
     Primitive(Primitive),
     Object(crate::object::Dictionary),
@@ -211,6 +211,13 @@ impl fmt::Debug for ValueDebug<'a> {
     }
 }
 
+impl Spanned<Value> {
+    crate fn spanned_type_name(&self) -> Spanned<String> {
+        let name = self.type_name();
+        Spanned::from_item(name, self.span)
+    }
+}
+
 impl Value {
     crate fn type_name(&self) -> String {
         match self {
@@ -301,7 +308,7 @@ impl Value {
     }
 
     #[allow(unused)]
-    crate fn compare(&self, operator: &Operator, other: &Value) -> Option<bool> {
+    crate fn compare(&self, operator: &Operator, other: &Value) -> Result<bool, (String, String)> {
         match operator {
             _ => {
                 let coerced = coerce_compare(self, other)?;
@@ -322,7 +329,7 @@ impl Value {
                     _ => false,
                 };
 
-                Some(result)
+                Ok(result)
             }
         }
     }
@@ -383,18 +390,6 @@ impl Value {
         }
     }
 
-    #[allow(unused)]
-    crate fn as_bool(&self) -> Result<bool, ShellError> {
-        match self {
-            Value::Primitive(Primitive::Boolean(b)) => Ok(*b),
-            // TODO: this should definitely be more general with better errors
-            other => Err(ShellError::string(format!(
-                "Expected integer, got {:?}",
-                other
-            ))),
-        }
-    }
-
     crate fn is_true(&self) -> bool {
         match self {
             Value::Primitive(Primitive::Boolean(true)) => true,
@@ -591,24 +586,27 @@ impl CompareValues {
     }
 }
 
-fn coerce_compare(left: &Value, right: &Value) -> Option<CompareValues> {
+fn coerce_compare(left: &Value, right: &Value) -> Result<CompareValues, (String, String)> {
     match (left, right) {
         (Value::Primitive(left), Value::Primitive(right)) => coerce_compare_primitive(left, right),
 
-        _ => None,
+        _ => Err((left.type_name(), right.type_name())),
     }
 }
 
-fn coerce_compare_primitive(left: &Primitive, right: &Primitive) -> Option<CompareValues> {
+fn coerce_compare_primitive(
+    left: &Primitive,
+    right: &Primitive,
+) -> Result<CompareValues, (String, String)> {
     use Primitive::*;
 
-    match (left, right) {
+    Ok(match (left, right) {
-        (Int(left), Int(right)) => Some(CompareValues::Ints(*left, *right)),
+        (Int(left), Int(right)) => CompareValues::Ints(*left, *right),
-        (Float(left), Int(right)) => Some(CompareValues::Floats(*left, (*right as f64).into())),
+        (Float(left), Int(right)) => CompareValues::Floats(*left, (*right as f64).into()),
-        (Int(left), Float(right)) => Some(CompareValues::Floats((*left as f64).into(), *right)),
+        (Int(left), Float(right)) => CompareValues::Floats((*left as f64).into(), *right),
-        (Int(left), Bytes(right)) => Some(CompareValues::Bytes(*left as i128, *right as i128)),
+        (Int(left), Bytes(right)) => CompareValues::Bytes(*left as i128, *right as i128),
-        (Bytes(left), Int(right)) => Some(CompareValues::Bytes(*left as i128, *right as i128)),
+        (Bytes(left), Int(right)) => CompareValues::Bytes(*left as i128, *right as i128),
-        (String(left), String(right)) => Some(CompareValues::String(left.clone(), right.clone())),
+        (String(left), String(right)) => CompareValues::String(left.clone(), right.clone()),
-        _ => None,
+        _ => return Err((left.type_name(), right.type_name())),
-    }
+    })
 }

src/object/desc.rs

@@ -39,7 +39,7 @@ impl DescriptorName {
     }
 }
 
-#[derive(Debug, Deserialize, Clone, Eq, PartialEq, Hash, new)]
+#[derive(Debug, Deserialize, Clone, Eq, PartialEq, Hash, Ord, PartialOrd, new)]
 pub struct DataDescriptor {
     crate name: DescriptorName,
     crate readonly: bool,

src/object/dict.rs

@@ -15,9 +15,18 @@ pub struct Dictionary {
 }
 
 impl PartialOrd for Dictionary {
-    // TODO: FIXME
+    fn partial_cmp(&self, other: &Dictionary) -> Option<Ordering> {
-    fn partial_cmp(&self, _other: &Dictionary) -> Option<Ordering> {
+        let this: Vec<&DataDescriptor> = self.entries.keys().collect();
-        Some(Ordering::Less)
+        let that: Vec<&DataDescriptor> = other.entries.keys().collect();
+
+        if this != that {
+            return this.partial_cmp(&that);
+        }
+
+        let this: Vec<&Value> = self.entries.values().collect();
+        let that: Vec<&Value> = self.entries.values().collect();
+
+        this.partial_cmp(&that)
     }
 }
 
@@ -56,9 +65,18 @@ impl From<IndexMap<String, Value>> for Dictionary {
 }
 
 impl Ord for Dictionary {
-    // TODO: FIXME
+    fn cmp(&self, other: &Dictionary) -> Ordering {
-    fn cmp(&self, _other: &Dictionary) -> Ordering {
+        let this: Vec<&DataDescriptor> = self.entries.keys().collect();
-        Ordering::Less
+        let that: Vec<&DataDescriptor> = other.entries.keys().collect();
+
+        if this != that {
+            return this.cmp(&that);
+        }
+
+        let this: Vec<&Value> = self.entries.values().collect();
+        let that: Vec<&Value> = self.entries.values().collect();
+
+        this.cmp(&that)
     }
 }
 
@@ -69,7 +87,6 @@ impl PartialOrd<Value> for Dictionary {
 }
 
 impl PartialEq<Value> for Dictionary {
-    // TODO: FIXME
     fn eq(&self, other: &Value) -> bool {
         match other {
             Value::Object(d) => self == d,

src/object/types.rs

@@ -1,7 +1,7 @@
 use derive_new::new;
 use serde_derive::{Deserialize, Serialize};
 
-#[derive(Debug, Serialize, Deserialize, Clone, Eq, PartialEq, Hash, new)]
+#[derive(Debug, Serialize, Deserialize, Clone, Eq, PartialEq, Hash, Ord, PartialOrd, new)]
 pub enum Type {
     Any,
 }

src/parser/hir/baseline_parse_tokens.rs

@@ -45,7 +45,7 @@ pub fn baseline_parse_next_expr(
     let first = next_token(&mut tokens);
 
     let first = match first {
-        None => return Err(ShellError::unimplemented("Expected token, found none")),
+        None => return Err(ShellError::string("Expected token, found none")),
         Some(token) => baseline_parse_semantic_token(token, source)?,
     };
 

src/parser/parse2/span.rs

@@ -1,8 +1,11 @@
 use crate::Text;
 use derive_new::new;
 use getset::Getters;
+use serde_derive::{Deserialize, Serialize};
 
-#[derive(new, Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Hash, Getters)]
+#[derive(
+    new, Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Serialize, Deserialize, Hash, Getters,
+)]
 #[get = "crate"]
 pub struct Spanned<T> {
     crate span: Span,
@@ -46,7 +49,7 @@ impl<T> Spanned<T> {
     }
 }
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd, Hash)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd, Serialize, Deserialize, Hash)]
 pub struct Span {
     crate start: usize,
     crate end: usize,

src/shell/helper.rs

@@ -61,10 +61,7 @@ impl Highlighter for Helper {
         let tokens = crate::parser::pipeline(nom_input(line));
 
         match tokens {
-            Err(e) => {
+            Err(_) => Cow::Borrowed(line),
-                println!("error: {:?}", e);
-                Cow::Borrowed(line)
-            }
             Ok((_rest, v)) => {
                 let mut out = String::new();
                 let pipeline = match v.as_pipeline() {