From 711ed05b43ced24366ac9e96f57abd0d1fb2e503 Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Sat, 29 Jun 2019 23:14:40 -0700 Subject: [PATCH] Produce ArgumentError for signature mismatch ArgumentError also automatically produces diagnostics --- src/errors.rs | 39 ++++++++++++++ src/parser/hir.rs | 24 +++++++++ src/parser/hir/baseline_parse_tokens.rs | 14 +++-- src/parser/parse_command.rs | 72 ++++++++++++++----------- src/parser/registry.rs | 7 +++ 5 files changed, 120 insertions(+), 36 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index 72630b9f53..0279068ac4 100644 --- a/src/errors.rs +++ b/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, @@ -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, ""), ShellError::CoerceError { .. } => write!(f, "CoerceError"), } diff --git a/src/parser/hir.rs b/src/parser/hir.rs index 16b79cf117..888a8a732d 100644 --- a/src/parser/hir.rs +++ b/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; 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), diff --git a/src/parser/hir/baseline_parse_tokens.rs b/src/parser/hir/baseline_parse_tokens.rs index 95e4beaaea..0a6068656b 100644 --- a/src/parser/hir/baseline_parse_tokens.rs +++ b/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( - "Expected op followed by another expr, found nothing", + return Err(ShellError::maybe_labeled_error( + "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, - _ => { - return Err(ShellError::unimplemented( - "The first part of a block must be a string", + Spanned { span, item } => { + return Err(ShellError::labeled_error( + "The first part of an un-braced block must be a column name", + item.type_name(), + span, )) } }; diff --git a/src/parser/parse_command.rs b/src/parser/parse_command.rs index 8e785e628e..a216de2928 100644 --- a/src/parser/parse_command.rs +++ b/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, source: &Text, ) -> Result { - let Spanned { item: call, .. } = call; + let Spanned { item: raw_call, .. } = call; trace!("Processing {:?}", config); let head = parse_command_head(call.head())?; - let children: Option> = call.children().as_ref().map(|nodes| { + let children: Option> = 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>, source: &Text, + command_span: Span, ) -> Result>, Option)>, 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), ShellError> { let flag = tokens.extract(|t| t.as_flag(name, source)); match flag { - None => Err(ShellError::unimplemented( - "Better error: mandatory flags must be present", - )), + None => Err(ShellError::ArgumentError { + 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)>), 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))) } } } diff --git a/src/parser/registry.rs b/src/parser/registry.rs index 6f786616bf..c3ee5ebf0f 100644 --- a/src/parser/registry.rs +++ b/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)]