From 7957fc502f45920b88972f7761e96a8cba289805 Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Sun, 23 Jun 2019 18:55:31 -0600 Subject: [PATCH] Fix a bunch of bugs --- src/cli.rs | 106 +++++++++++++++--------- src/errors.rs | 78 ++++++++++++++++- src/evaluate/evaluator.rs | 29 ++++--- src/object/base.rs | 52 ++++++------ src/object/desc.rs | 2 +- src/object/dict.rs | 31 +++++-- src/object/types.rs | 2 +- src/parser/hir/baseline_parse_tokens.rs | 2 +- src/parser/parse2/span.rs | 7 +- src/shell/helper.rs | 5 +- 10 files changed, 217 insertions(+), 97 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 52101ff6a3..a4f7c2de9b 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -156,45 +156,60 @@ pub async fn cli() -> Result<(), Box> { LineResult::Error(mut line, err) => { rl.add_history_entry(line.clone()); - match err { - ShellError::Diagnostic(diag) => { - let host = context.host.lock().unwrap(); - let writer = host.err_termcolor(); - line.push_str(" "); - let files = crate::parser::Files::new(line); - language_reporting::emit( - &mut writer.lock(), - &files, - &diag.diagnostic, - &language_reporting::DefaultConfig, - ) - .unwrap(); - } + let diag = err.to_diagnostic(); + let host = context.host.lock().unwrap(); + let writer = host.err_termcolor(); + line.push_str(" "); + let files = crate::parser::Files::new(line); - ShellError::TypeError(desc) => context - .host - .lock() - .unwrap() - .stdout(&format!("TypeError: {}", desc)), + language_reporting::emit( + &mut writer.lock(), + &files, + &diag, + &language_reporting::DefaultConfig, + ) + .unwrap(); - ShellError::MissingProperty { subpath, .. } => context - .host - .lock() - .unwrap() - .stdout(&format!("Missing property {}", subpath)), + // match err { + // ShellError::Diagnostic(diag) => { + // let host = context.host.lock().unwrap(); + // let writer = host.err_termcolor(); + // line.push_str(" "); + // let files = crate::parser::Files::new(line); - ShellError::String(_) => { - context.host.lock().unwrap().stdout(&format!("{}", err)) - } - } + // language_reporting::emit( + // &mut writer.lock(), + // &files, + // &diag.diagnostic, + // &language_reporting::DefaultConfig, + // ) + // .unwrap(); + // } + + // ShellError::TypeError(desc) => context + // .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; - type Error = ShellError; + type Error = (String, ShellError); - fn into_result(self) -> Result, ShellError> { + fn into_result(self) -> Result, (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 { - LineResult::Error(String::new(), v) + fn from_error(v: (String, ShellError)) -> Self { + LineResult::Error(v.0, v.1) } fn from_ok(v: Option) -> Self { @@ -260,7 +275,8 @@ async fn process_line(readline: Result, 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( - "classify_command on command whose head is not bare", + call => Err(ShellError::diagnostic( + language_reporting::Diagnostic::new( + language_reporting::Severity::Error, + "Invalid command", + ) + .with_label(language_reporting::Label::new_primary(call.head().span())), )), } } diff --git a/src/errors.rs b/src/errors.rs index 88d5e78fd3..506aaaa0c6 100644 --- a/src/errors.rs +++ b/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), + Synthetic(String), +} + +impl Description { + pub fn from(item: Spanned>) -> 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, 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), - MissingProperty { subpath: String, expr: String }, + TypeError(Spanned), + MissingProperty { + subpath: Description, + expr: Description, + }, Diagnostic(ShellDiagnostic), + CoerceError { + left: Spanned, + right: Spanned, + }, } 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 { + 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, label: impl Into, @@ -177,6 +246,7 @@ impl std::fmt::Display for ShellError { ShellError::TypeError { .. } => write!(f, "TypeError"), ShellError::MissingProperty { .. } => write!(f, "MissingProperty"), ShellError::Diagnostic(_) => write!(f, ""), + ShellError::CoerceError { .. } => write!(f, "CoerceError"), } } } diff --git a/src/evaluate/evaluator.rs b/src/evaluate/evaluator.rs index 31958af787..16da0ad78c 100644 --- a/src/evaluate/evaluator.rs +++ b/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())), - None => Err(ShellError::unimplemented(&format!( - "Comparison failure {:?}", - binary - ))), + Ok(result) => Ok(Spanned::from_item(Value::boolean(result), *expr.span())), + Err((left_type, right_type)) => Err(ShellError::CoerceError { + left: binary.left().copy_span(left_type), + 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")), - Some(next) => value = next, + None => { + 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!(), } diff --git a/src/object/base.rs b/src/object/base.rs index 81ecf00eae..2a2a5b3c8d 100644 --- a/src/object/base.rs +++ b/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 { + crate fn spanned_type_name(&self) -> Spanned { + 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 { + crate fn compare(&self, operator: &Operator, other: &Value) -> Result { 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 { - 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 { +fn coerce_compare(left: &Value, right: &Value) -> Result { 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 { +fn coerce_compare_primitive( + left: &Primitive, + right: &Primitive, +) -> Result { use Primitive::*; - match (left, right) { - (Int(left), Int(right)) => Some(CompareValues::Ints(*left, *right)), - (Float(left), Int(right)) => Some(CompareValues::Floats(*left, (*right as f64).into())), - (Int(left), Float(right)) => Some(CompareValues::Floats((*left as f64).into(), *right)), - (Int(left), Bytes(right)) => Some(CompareValues::Bytes(*left as i128, *right as i128)), - (Bytes(left), Int(right)) => Some(CompareValues::Bytes(*left as i128, *right as i128)), - (String(left), String(right)) => Some(CompareValues::String(left.clone(), right.clone())), - _ => None, - } + Ok(match (left, right) { + (Int(left), Int(right)) => CompareValues::Ints(*left, *right), + (Float(left), Int(right)) => CompareValues::Floats(*left, (*right as f64).into()), + (Int(left), Float(right)) => CompareValues::Floats((*left as f64).into(), *right), + (Int(left), Bytes(right)) => CompareValues::Bytes(*left as i128, *right as i128), + (Bytes(left), Int(right)) => CompareValues::Bytes(*left as i128, *right as i128), + (String(left), String(right)) => CompareValues::String(left.clone(), right.clone()), + _ => return Err((left.type_name(), right.type_name())), + }) } diff --git a/src/object/desc.rs b/src/object/desc.rs index 7a7e24c765..a3d7784a63 100644 --- a/src/object/desc.rs +++ b/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, diff --git a/src/object/dict.rs b/src/object/dict.rs index 3cd13261b4..146967c81c 100644 --- a/src/object/dict.rs +++ b/src/object/dict.rs @@ -15,9 +15,18 @@ pub struct Dictionary { } impl PartialOrd for Dictionary { - // TODO: FIXME - fn partial_cmp(&self, _other: &Dictionary) -> Option { - Some(Ordering::Less) + fn partial_cmp(&self, other: &Dictionary) -> Option { + let this: Vec<&DataDescriptor> = self.entries.keys().collect(); + 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> for Dictionary { } impl Ord for Dictionary { - // TODO: FIXME - fn cmp(&self, _other: &Dictionary) -> Ordering { - Ordering::Less + fn cmp(&self, other: &Dictionary) -> Ordering { + let this: Vec<&DataDescriptor> = self.entries.keys().collect(); + 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 for Dictionary { } impl PartialEq for Dictionary { - // TODO: FIXME fn eq(&self, other: &Value) -> bool { match other { Value::Object(d) => self == d, diff --git a/src/object/types.rs b/src/object/types.rs index 2f1bc2e4e6..34055e0b0d 100644 --- a/src/object/types.rs +++ b/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, } diff --git a/src/parser/hir/baseline_parse_tokens.rs b/src/parser/hir/baseline_parse_tokens.rs index 81222f66f9..e9c094388b 100644 --- a/src/parser/hir/baseline_parse_tokens.rs +++ b/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)?, }; diff --git a/src/parser/parse2/span.rs b/src/parser/parse2/span.rs index e5905ea4df..ca279eeaa2 100644 --- a/src/parser/parse2/span.rs +++ b/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 { crate span: Span, @@ -46,7 +49,7 @@ impl Spanned { } } -#[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, diff --git a/src/shell/helper.rs b/src/shell/helper.rs index 114fd3a1f5..101b45a971 100644 --- a/src/shell/helper.rs +++ b/src/shell/helper.rs @@ -61,10 +61,7 @@ impl Highlighter for Helper { let tokens = crate::parser::pipeline(nom_input(line)); match tokens { - Err(e) => { - println!("error: {:?}", e); - Cow::Borrowed(line) - } + Err(_) => Cow::Borrowed(line), Ok((_rest, v)) => { let mut out = String::new(); let pipeline = match v.as_pipeline() {