Produce ArgumentError for signature mismatch

ArgumentError also automatically produces diagnostics

src/errors.rs

@@ -2,6 +2,7 @@
 use crate::prelude::*;
 
 use crate::parser::{Span, Spanned};
+use ansi_term::Color;
 use derive_new::new;
 use language_reporting::{Diagnostic, Label, Severity};
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
@@ -33,6 +34,13 @@ impl Description {
     }
 }
 
+#[derive(Debug, Eq, PartialEq, Clone, Ord, PartialOrd, Serialize, Deserialize)]
+pub enum ArgumentError {
+    MissingMandatoryFlag(String),
+    MissingMandatoryPositional(String),
+    MissingValueForName(String),
+}
+
 #[derive(Debug, Eq, PartialEq, Clone, Ord, PartialOrd, Serialize, Deserialize)]
 pub enum ShellError {
     String(StringError),
@@ -44,6 +52,10 @@ pub enum ShellError {
         subpath: Description,
         expr: Description,
     },
+    ArgumentError {
+        error: ArgumentError,
+        span: Span,
+    },
     Diagnostic(ShellDiagnostic),
     CoerceError {
         left: Spanned<String>,
@@ -107,6 +119,32 @@ impl ShellError {
             ShellError::String(StringError { title, .. }) => {
                 Diagnostic::new(Severity::Error, title)
             }
+            ShellError::ArgumentError { error, span } => match error {
+                ArgumentError::MissingMandatoryFlag(name) => Diagnostic::new(
+                    Severity::Error,
+                    format!(
+                        "Command requires {}{}",
+                        Color::Cyan.paint("--"),
+                        Color::Cyan.paint(name)
+                    ),
+                )
+                .with_label(Label::new_primary(span)),
+                ArgumentError::MissingMandatoryPositional(name) => Diagnostic::new(
+                    Severity::Error,
+                    format!("Command requires {}", Color::Cyan.paint(name)),
+                )
+                .with_label(Label::new_primary(span)),
+
+                ArgumentError::MissingValueForName(name) => Diagnostic::new(
+                    Severity::Error,
+                    format!(
+                        "Missing value for flag {}{}",
+                        Color::Cyan.paint("--"),
+                        Color::Cyan.paint(name)
+                    ),
+                )
+                .with_label(Label::new_primary(span)),
+            },
             ShellError::TypeError {
                 expected,
                 actual:
@@ -271,6 +309,7 @@ impl std::fmt::Display for ShellError {
             ShellError::String(s) => write!(f, "{}", &s.title),
             ShellError::TypeError { .. } => write!(f, "TypeError"),
             ShellError::MissingProperty { .. } => write!(f, "MissingProperty"),
+            ShellError::ArgumentError { .. } => write!(f, "ArgumentError"),
             ShellError::Diagnostic(_) => write!(f, "<diagnostic>"),
             ShellError::CoerceError { .. } => write!(f, "CoerceError"),
         }

src/parser/hir.rs

@@ -45,6 +45,19 @@ pub enum RawExpression {
     Boolean(bool),
 }
 
+impl RawExpression {
+    pub fn type_name(&self) -> &'static str {
+        match self {
+            RawExpression::Literal(literal) => literal.type_name(),
+            RawExpression::Variable(..) => "variable",
+            RawExpression::Binary(..) => "binary",
+            RawExpression::Block(..) => "block",
+            RawExpression::Path(..) => "path",
+            RawExpression::Boolean(..) => "boolean",
+        }
+    }
+}
+
 pub type Expression = Spanned<RawExpression>;
 
 impl Expression {
@@ -99,6 +112,17 @@ pub enum Literal {
     Bare,
 }
 
+impl Literal {
+    fn type_name(&self) -> &'static str {
+        match self {
+            Literal::Integer(_) => "integer",
+            Literal::Size(..) => "size",
+            Literal::String(..) => "string",
+            Literal::Bare => "string",
+        }
+    }
+}
+
 #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
 pub enum Variable {
     It(Span),

src/parser/hir/baseline_parse_tokens.rs

@@ -58,8 +58,10 @@ pub fn baseline_parse_next_expr(
 
     let second = match tokens.next() {
         None => {
-            return Err(ShellError::unimplemented(
+            return Err(ShellError::maybe_labeled_error(
-                "Expected op followed by another expr, found nothing",
+                "Expected something after an operator",
+                "operator",
+                Some(op.span),
             ))
         }
         Some(token) => baseline_parse_semantic_token(token, registry, source)?,
@@ -124,9 +126,11 @@ pub fn baseline_parse_next_expr(
                         item: hir::RawExpression::Variable(..),
                         ..
                     } => first,
-                    _ => {
+                    Spanned { span, item } => {
-                        return Err(ShellError::unimplemented(
+                        return Err(ShellError::labeled_error(
-                            "The first part of a block must be a string",
+                            "The first part of an un-braced block must be a column name",
+                            item.type_name(),
+                            span,
                         ))
                     }
                 };

src/parser/parse_command.rs

@@ -1,6 +1,6 @@
-use crate::errors::ShellError;
+use crate::errors::{ArgumentError, ShellError};
 use crate::parser::registry::{CommandConfig, CommandRegistry, NamedType};
-use crate::parser::{baseline_parse_tokens, CallNode, Spanned};
+use crate::parser::{baseline_parse_tokens, CallNode, Span, Spanned};
 use crate::parser::{
     hir::{self, NamedArguments},
     Flag, RawToken, TokenNode,
@@ -14,13 +14,13 @@ pub fn parse_command(
     call: &Spanned<CallNode>,
     source: &Text,
 ) -> Result<hir::Call, ShellError> {
-    let Spanned { item: call, .. } = call;
+    let Spanned { item: raw_call, .. } = call;
 
     trace!("Processing {:?}", config);
 
     let head = parse_command_head(call.head())?;
 
-    let children: Option<Vec<TokenNode>> = call.children().as_ref().map(|nodes| {
+    let children: Option<Vec<TokenNode>> = raw_call.children().as_ref().map(|nodes| {
         nodes
             .iter()
             .cloned()
@@ -31,7 +31,7 @@ pub fn parse_command(
             .collect()
     });
 
-    match parse_command_tail(&config, registry, children, source)? {
+    match parse_command_tail(&config, registry, children, source, call.span)? {
         None => Ok(hir::Call::new(Box::new(head), None, None)),
         Some((positional, named)) => Ok(hir::Call::new(Box::new(head), positional, named)),
     }
@@ -66,9 +66,10 @@ fn parse_command_tail(
     registry: &dyn CommandRegistry,
     tail: Option<Vec<TokenNode>>,
     source: &Text,
+    command_span: Span,
 ) -> Result<Option<(Option<Vec<hir::Expression>>, Option<NamedArguments>)>, ShellError> {
     let tail = &mut match &tail {
-        None => return Ok(None),
+        None => hir::TokensIterator::new(&[]),
         Some(tail) => hir::TokensIterator::new(tail),
     };
 
@@ -85,10 +86,19 @@ fn parse_command_tail(
 
                 named.insert_switch(name, flag);
             }
-            NamedType::Mandatory(kind) => match extract_mandatory(name, tail, source) {
+            NamedType::Mandatory(kind) => match extract_mandatory(name, tail, source, command_span)
+            {
                 Err(err) => return Err(err), // produce a correct diagnostic
-                Ok((pos, _flag)) => {
+                Ok((pos, flag)) => {
                     tail.move_to(pos);
+
+                    if tail.at_end() {
+                        return Err(ShellError::ArgumentError {
+                            error: ArgumentError::MissingValueForName(name.to_string()),
+                            span: flag.span,
+                        });
+                    }
+
                     let expr = hir::baseline_parse_next_expr(
                         tail,
                         registry,
@@ -102,8 +112,16 @@ fn parse_command_tail(
             },
             NamedType::Optional(kind) => match extract_optional(name, tail, source) {
                 Err(err) => return Err(err), // produce a correct diagnostic
-                Ok(Some((pos, _flag))) => {
+                Ok(Some((pos, flag))) => {
                     tail.move_to(pos);
+
+                    if tail.at_end() {
+                        return Err(ShellError::ArgumentError {
+                            error: ArgumentError::MissingValueForName(name.to_string()),
+                            span: flag.span,
+                        });
+                    }
+
                     let expr = hir::baseline_parse_next_expr(
                         tail,
                         registry,
@@ -132,7 +150,10 @@ fn parse_command_tail(
         trace!("Processing mandatory {:?}", arg);
 
         if tail.len() == 0 {
-            return Err(ShellError::unimplemented("Missing mandatory argument"));
+            return Err(ShellError::ArgumentError {
+                error: ArgumentError::MissingMandatoryPositional(arg.name().to_string()),
+                span: command_span,
+            });
         }
 
         let result = hir::baseline_parse_next_expr(tail, registry, source, arg.to_coerce_hint())?;
@@ -189,23 +210,19 @@ fn extract_mandatory(
     name: &str,
     tokens: &mut hir::TokensIterator<'a>,
     source: &Text,
-) -> Result<(usize, Flag), ShellError> {
+    span: Span,
+) -> Result<(usize, Spanned<Flag>), ShellError> {
     let flag = tokens.extract(|t| t.as_flag(name, source));
 
     match flag {
-        None => Err(ShellError::unimplemented(
+        None => Err(ShellError::ArgumentError {
-            "Better error: mandatory flags must be present",
+            error: ArgumentError::MissingMandatoryFlag(name.to_string()),
-        )),
+            span,
+        }),
+
         Some((pos, flag)) => {
-            if tokens.len() <= pos {
-                return Err(ShellError::unimplemented(
-                    "Better errors: mandatory flags must be followed by values",
-                ));
-            }
-
             tokens.remove(pos);
-
+            Ok((pos, flag))
-            Ok((pos, *flag))
         }
     }
 }
@@ -214,21 +231,14 @@ fn extract_optional(
     name: &str,
     tokens: &mut hir::TokensIterator<'a>,
     source: &Text,
-) -> Result<(Option<(usize, Flag)>), ShellError> {
+) -> Result<(Option<(usize, Spanned<Flag>)>), ShellError> {
     let flag = tokens.extract(|t| t.as_flag(name, source));
 
     match flag {
         None => Ok(None),
         Some((pos, flag)) => {
-            if tokens.len() <= pos {
-                return Err(ShellError::unimplemented(
-                    "Better errors: optional flags must be followed by values",
-                ));
-            }
-
             tokens.remove(pos);
-
+            Ok(Some((pos, flag)))
-            Ok(Some((pos, *flag)))
         }
     }
 }

src/parser/registry.rs

@@ -46,6 +46,13 @@ impl PositionalType {
             PositionalType::Block(_) => Some(ExpressionKindHint::Block),
         }
     }
+
+    crate fn name(&self) -> &str {
+        match self {
+            PositionalType::Value(s) => s,
+            PositionalType::Block(s) => s,
+        }
+    }
 }
 
 #[derive(Debug, Getters)]