From 0f626dd07647f54a08c312a686bf471da62682c6 Mon Sep 17 00:00:00 2001 From: Jonathan Turner Date: Thu, 2 Jan 2020 17:02:46 +1300 Subject: [PATCH] Another batch of un-unwrapping (#1148) Another batch of un-unwrappings --- crates/nu-errors/src/lib.rs | 18 ++++++++ crates/nu-parser/src/hir/syntax_shape.rs | 12 +++++- .../syntax_shape/expression/variable_path.rs | 14 +++++-- crates/nu-parser/src/hir/tokens_iterator.rs | 4 +- src/cli.rs | 12 ++++-- src/commands/classified/external.rs | 12 +++++- src/commands/from_sqlite.rs | 4 +- src/commands/from_xlsx.rs | 42 +++++++++++-------- src/commands/from_xml.rs | 27 ++++++++---- src/commands/group_by.rs | 23 ++++++---- src/commands/help.rs | 35 +++++++++------- src/commands/histogram.rs | 8 ++-- src/commands/insert.rs | 2 +- src/commands/open.rs | 2 +- src/commands/parse.rs | 14 +++++-- 15 files changed, 156 insertions(+), 73 deletions(-) diff --git a/crates/nu-errors/src/lib.rs b/crates/nu-errors/src/lib.rs index 48aa9b9acd..363f515b65 100644 --- a/crates/nu-errors/src/lib.rs +++ b/crates/nu-errors/src/lib.rs @@ -24,6 +24,10 @@ pub enum ParseErrorReason { expected: &'static str, actual: Spanned, }, + + /// An unexpected internal error has occurred + InternalError { message: Spanned }, + /// The parser tried to parse an argument for a command, but it failed for /// some reason ArgumentError { @@ -69,6 +73,15 @@ impl ParseError { } } + /// Construct a [ParseErrorReason::InternalError](ParseErrorReason::InternalError) + pub fn internal_error(message: Spanned>) -> ParseError { + ParseError { + reason: ParseErrorReason::InternalError { + message: message.item.into().spanned(message.span), + }, + } + } + /// Construct a [ParseErrorReason::ArgumentError](ParseErrorReason::ArgumentError) pub fn argument_error(command: Spanned>, kind: ArgumentError) -> ParseError { ParseError { @@ -89,6 +102,11 @@ impl From for ShellError { ParseErrorReason::Mismatch { actual, expected } => { ShellError::type_error(expected, actual) } + ParseErrorReason::InternalError { message } => ShellError::labeled_error( + format!("Internal error: {}", message.item), + &message.item, + &message.span, + ), ParseErrorReason::ArgumentError { command, error } => { ShellError::argument_error(command, error) } diff --git a/crates/nu-parser/src/hir/syntax_shape.rs b/crates/nu-parser/src/hir/syntax_shape.rs index a42b1a7a78..4d900ec4d9 100644 --- a/crates/nu-parser/src/hir/syntax_shape.rs +++ b/crates/nu-parser/src/hir/syntax_shape.rs @@ -676,7 +676,13 @@ impl FallibleColorSyntax for CommandHeadShape { if context.registry.has(name) { // If the registry has the command, color it as an internal command token_nodes.color_shape(FlatShape::InternalCommand.spanned(text)); - let signature = context.registry.get(name).unwrap(); + let signature = context.registry.get(name).ok_or_else(|| { + ShellError::labeled_error( + "Internal error: could not load signature from registry", + "could not load from registry", + text, + ) + })?; Ok(CommandHeadKind::Internal(signature)) } else { // Otherwise, color it as an external command @@ -716,7 +722,9 @@ impl ExpandSyntax for CommandHeadShape { UnspannedToken::Bare => { let name = token_span.slice(context.source); if context.registry.has(name) { - let signature = context.registry.get(name).unwrap(); + let signature = context.registry.get(name).ok_or_else(|| { + ParseError::internal_error(name.spanned(token_span)) + })?; CommandSignature::Internal(signature.spanned(token_span)) } else { CommandSignature::External(token_span) diff --git a/crates/nu-parser/src/hir/syntax_shape/expression/variable_path.rs b/crates/nu-parser/src/hir/syntax_shape/expression/variable_path.rs index b1c85138d9..605b6b18f6 100644 --- a/crates/nu-parser/src/hir/syntax_shape/expression/variable_path.rs +++ b/crates/nu-parser/src/hir/syntax_shape/expression/variable_path.rs @@ -683,7 +683,11 @@ impl ExpandSyntax for IntMemberShape { UnspannedAtomicToken::Number { number: RawNumber::Int(int), } => Ok(Member::Int( - BigInt::from_str(int.slice(context.source)).unwrap(), + BigInt::from_str(int.slice(context.source)).map_err(|_| { + ParseError::internal_error( + "can't convert from string to big int".spanned(int), + ) + })?, int, )), @@ -732,7 +736,9 @@ impl ExpandSyntax for MemberShape { if let Some(peeked) = number { let node = peeked.not_eof("column")?.commit(); - let (n, span) = node.as_number().unwrap(); + let (n, span) = node.as_number().ok_or_else(|| { + ParseError::internal_error("can't convert node to number".spanned(node.span())) + })?; return Ok(Member::Number(n, span)) }*/ @@ -741,7 +747,9 @@ impl ExpandSyntax for MemberShape { if let Some(peeked) = string { let node = peeked.not_eof("column")?.commit(); - let (outer, inner) = node.as_string().unwrap(); + let (outer, inner) = node.as_string().ok_or_else(|| { + ParseError::internal_error("can't convert node to string".spanned(node.span())) + })?; return Ok(Member::String(outer, inner)); } diff --git a/crates/nu-parser/src/hir/tokens_iterator.rs b/crates/nu-parser/src/hir/tokens_iterator.rs index 792e05d76a..dca99a5913 100644 --- a/crates/nu-parser/src/hir/tokens_iterator.rs +++ b/crates/nu-parser/src/hir/tokens_iterator.rs @@ -67,8 +67,8 @@ impl<'content, 'me> std::ops::Drop for Checkpoint<'content, 'me> { pub struct Peeked<'content, 'me> { pub(crate) node: Option<&'content TokenNode>, iterator: &'me mut TokensIterator<'content>, - from: usize, - to: usize, + pub from: usize, + pub to: usize, } impl<'content, 'me> Peeked<'content, 'me> { diff --git a/src/cli.rs b/src/cli.rs index 874c045f1f..f8250ebea4 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -145,7 +145,7 @@ fn load_plugins(context: &mut Context) -> Result<(), ShellError> { require_literal_leading_dot: false, }; - set_env_from_config(); + set_env_from_config()?; for path in search_paths() { let mut pattern = path.to_path_buf(); @@ -501,8 +501,8 @@ fn chomp_newline(s: &str) -> &str { } } -fn set_env_from_config() { - let config = crate::data::config::read(Tag::unknown(), &None).unwrap(); +fn set_env_from_config() -> Result<(), ShellError> { + let config = crate::data::config::read(Tag::unknown(), &None)?; if config.contains_key("env") { // Clear the existing vars, we're about to replace them @@ -550,6 +550,7 @@ fn set_env_from_config() { } } } + Ok(()) } enum LineResult { @@ -601,7 +602,10 @@ async fn process_line(readline: Result, ctx: &mut Context // Check the config to see if we need to update the path // TODO: make sure config is cached so we don't path this load every call - set_env_from_config(); + // FIXME: we probably want to be a bit more graceful if we can't set the environment + if let Err(err) = set_env_from_config() { + return LineResult::Error(line.to_string(), err); + } let input = ClassifiedInputStream::new(); diff --git a/src/commands/classified/external.rs b/src/commands/classified/external.rs index 13d7207a71..75cde671b3 100644 --- a/src/commands/classified/external.rs +++ b/src/commands/classified/external.rs @@ -181,11 +181,19 @@ pub(crate) async fn run_external_command( Ok(ClassifiedInputStream::new()) } StreamNext::External => { - let stdout = popen.stdout.take().unwrap(); + let stdout = popen.stdout.take().ok_or_else(|| { + ShellError::untagged_runtime_error( + "Can't redirect the stdout for external command", + ) + })?; Ok(ClassifiedInputStream::from_stdout(stdout)) } StreamNext::Internal => { - let stdout = popen.stdout.take().unwrap(); + let stdout = popen.stdout.take().ok_or_else(|| { + ShellError::untagged_runtime_error( + "Can't redirect the stdout for internal command", + ) + })?; let file = futures::io::AllowStdIo::new(stdout); let stream = Framed::new(file, LinesCodec {}); let stream = stream.map(move |line| line.unwrap().into_value(&name_tag)); diff --git a/src/commands/from_sqlite.rs b/src/commands/from_sqlite.rs index afff666f9c..34c2ef182b 100644 --- a/src/commands/from_sqlite.rs +++ b/src/commands/from_sqlite.rs @@ -107,9 +107,9 @@ fn convert_sqlite_value_to_nu_value(value: ValueRef, tag: impl Into + Clone } ValueRef::Integer(i) => UntaggedValue::int(i).into_value(tag), ValueRef::Real(f) => UntaggedValue::decimal(f).into_value(tag), - t @ ValueRef::Text(_) => { + ValueRef::Text(s) => { // this unwrap is safe because we know the ValueRef is Text. - UntaggedValue::Primitive(Primitive::String(t.as_str().unwrap().to_string())) + UntaggedValue::Primitive(Primitive::String(String::from_utf8_lossy(s).to_string())) .into_value(tag) } ValueRef::Blob(u) => UntaggedValue::binary(u.to_owned()).into_value(tag), diff --git a/src/commands/from_xlsx.rs b/src/commands/from_xlsx.rs index 350a37dbc0..3a86a50f1f 100644 --- a/src/commands/from_xlsx.rs +++ b/src/commands/from_xlsx.rs @@ -55,7 +55,10 @@ fn from_xlsx( match value.value { UntaggedValue::Primitive(Primitive::Binary(vb)) => { let mut buf: Cursor> = Cursor::new(vb); - let mut xls = Xlsx::<_>::new(buf).unwrap(); + let mut xls = Xlsx::<_>::new(buf).map_err(|_| ShellError::labeled_error( + "Could not load xlsx file", + "could not load xlsx file", + &tag))?; let mut dict = TaggedDictBuilder::new(&tag); @@ -64,27 +67,32 @@ fn from_xlsx( for sheet_name in &sheet_names { let mut sheet_output = TaggedListBuilder::new(&tag); - let current_sheet = xls.worksheet_range(sheet_name).unwrap().unwrap(); + if let Some(Ok(current_sheet)) = xls.worksheet_range(sheet_name) { + for row in current_sheet.rows() { + let mut row_output = TaggedDictBuilder::new(&tag); + for (i, cell) in row.iter().enumerate() { + let value = match cell { + DataType::Empty => UntaggedValue::nothing(), + DataType::String(s) => UntaggedValue::string(s), + DataType::Float(f) => UntaggedValue::decimal(*f), + DataType::Int(i) => UntaggedValue::int(*i), + DataType::Bool(b) => UntaggedValue::boolean(*b), + _ => UntaggedValue::nothing(), + }; - for row in current_sheet.rows() { - let mut row_output = TaggedDictBuilder::new(&tag); - for (i, cell) in row.iter().enumerate() { - let value = match cell { - DataType::Empty => UntaggedValue::nothing(), - DataType::String(s) => UntaggedValue::string(s), - DataType::Float(f) => UntaggedValue::decimal(*f), - DataType::Int(i) => UntaggedValue::int(*i), - DataType::Bool(b) => UntaggedValue::boolean(*b), - _ => UntaggedValue::nothing(), - }; + row_output.insert_untagged(&format!("Column{}", i), value); + } - row_output.insert_untagged(&format!("Column{}", i), value); + sheet_output.push_untagged(row_output.into_untagged_value()); } - sheet_output.push_untagged(row_output.into_untagged_value()); + dict.insert_untagged(sheet_name, sheet_output.into_untagged_value()); + } else { + yield Err(ShellError::labeled_error( + "Could not load sheet", + "could not load sheet", + &tag)); } - - dict.insert_untagged(sheet_name, sheet_output.into_untagged_value()); } yield ReturnSuccess::value(dict.into_value()); diff --git a/src/commands/from_xml.rs b/src/commands/from_xml.rs index b160c9ab90..166406367c 100644 --- a/src/commands/from_xml.rs +++ b/src/commands/from_xml.rs @@ -60,7 +60,10 @@ fn from_node_to_value<'a, 'd>(n: &roxmltree::Node<'a, 'd>, tag: impl Into) } else if n.is_pi() { UntaggedValue::string("").into_value(tag) } else if n.is_text() { - UntaggedValue::string(n.text().unwrap()).into_value(tag) + match n.text() { + Some(text) => UntaggedValue::string(text).into_value(tag), + None => UntaggedValue::string("").into_value(tag), + } } else { UntaggedValue::string("").into_value(tag) } @@ -149,36 +152,40 @@ mod tests { UntaggedValue::table(list).into_untagged_value() } - fn parse(xml: &str) -> Value { - from_xml::from_xml_string_to_value(xml.to_string(), Tag::unknown()).unwrap() + fn parse(xml: &str) -> Result { + from_xml::from_xml_string_to_value(xml.to_string(), Tag::unknown()) } #[test] - fn parses_empty_element() { + fn parses_empty_element() -> Result<(), roxmltree::Error> { let source = ""; assert_eq!( - parse(source), + parse(source)?, row(indexmap! { "nu".into() => table(&[]) }) ); + + Ok(()) } #[test] - fn parses_element_with_text() { + fn parses_element_with_text() -> Result<(), roxmltree::Error> { let source = "La era de los tres caballeros"; assert_eq!( - parse(source), + parse(source)?, row(indexmap! { "nu".into() => table(&[string("La era de los tres caballeros")]) }) ); + + Ok(()) } #[test] - fn parses_element_with_elements() { + fn parses_element_with_elements() -> Result<(), roxmltree::Error> { let source = "\ Andrés @@ -187,7 +194,7 @@ mod tests { "; assert_eq!( - parse(source), + parse(source)?, row(indexmap! { "nu".into() => table(&[ row(indexmap! {"dev".into() => table(&[string("Andrés")])}), @@ -196,5 +203,7 @@ mod tests { ]) }) ); + + Ok(()) } } diff --git a/src/commands/group_by.rs b/src/commands/group_by.rs index cabfbf6490..f925576009 100644 --- a/src/commands/group_by.rs +++ b/src/commands/group_by.rs @@ -74,7 +74,11 @@ pub fn group( for value in values { let group_key = get_data_by_key(&value, column_name.borrow_spanned()); - if group_key.is_none() { + if let Some(group_key) = group_key { + let group_key = group_key.as_string()?.to_string(); + let group = groups.entry(group_key).or_insert(vec![]); + group.push(value); + } else { let possibilities = value.data_descriptors(); let mut possible_matches: Vec<_> = possibilities @@ -98,10 +102,6 @@ pub fn group( )); } } - - let group_key = group_key.unwrap().as_string()?.to_string(); - let group = groups.entry(group_key).or_insert(vec![]); - group.push(value); } let mut out = TaggedDictBuilder::new(&tag); @@ -117,6 +117,7 @@ pub fn group( mod tests { use crate::commands::group_by::group; use indexmap::IndexMap; + use nu_errors::ShellError; use nu_protocol::{UntaggedValue, Value}; use nu_source::*; @@ -165,11 +166,11 @@ mod tests { } #[test] - fn groups_table_by_date_column() { + fn groups_table_by_date_column() -> Result<(), ShellError> { let for_key = String::from("date").tagged_unknown(); assert_eq!( - group(&for_key, nu_releases_commiters(), Tag::unknown()).unwrap(), + group(&for_key, nu_releases_commiters(), Tag::unknown())?, row(indexmap! { "August 23-2019".into() => table(&[ row(indexmap!{"name".into() => string("AR"), "country".into() => string("EC"), "date".into() => string("August 23-2019")}), @@ -188,14 +189,16 @@ mod tests { ]), }) ); + + Ok(()) } #[test] - fn groups_table_by_country_column() { + fn groups_table_by_country_column() -> Result<(), ShellError> { let for_key = String::from("country").tagged_unknown(); assert_eq!( - group(&for_key, nu_releases_commiters(), Tag::unknown()).unwrap(), + group(&for_key, nu_releases_commiters(), Tag::unknown())?, row(indexmap! { "EC".into() => table(&[ row(indexmap!{"name".into() => string("AR"), "country".into() => string("EC"), "date".into() => string("August 23-2019")}), @@ -214,5 +217,7 @@ mod tests { ]), }) ); + + Ok(()) } } diff --git a/src/commands/help.rs b/src/commands/help.rs index 64f6c86dbb..888f451bf1 100644 --- a/src/commands/help.rs +++ b/src/commands/help.rs @@ -44,15 +44,29 @@ impl PerItemCommand for Help { sorted_names.sort(); for cmd in sorted_names { let mut short_desc = TaggedDictBuilder::new(tag.clone()); - let value = command_dict(registry.get_command(&cmd).unwrap(), tag.clone()); + let value = command_dict( + registry.get_command(&cmd).ok_or_else(|| { + ShellError::labeled_error( + format!("Could not load {}", cmd), + "could not load command", + tag, + ) + })?, + tag.clone(), + ); short_desc.insert_untagged("name", cmd); short_desc.insert_untagged( "description", get_data_by_key(&value, "usage".spanned_unknown()) - .unwrap() - .as_string() - .unwrap(), + .ok_or_else(|| { + ShellError::labeled_error( + "Expected a usage key", + "expected a 'usage' key", + &value.tag, + ) + })? + .as_string()?, ); help.push_back(ReturnSuccess::value(short_desc.into_value())); @@ -102,16 +116,9 @@ impl PerItemCommand for Help { } } } - if signature.rest_positional.is_some() { - long_desc.push_str(&format!( - " ...args{} {}\n", - if signature.rest_positional.is_some() { - ":" - } else { - "" - }, - signature.rest_positional.unwrap().1 - )); + + if let Some(rest_positional) = signature.rest_positional { + long_desc.push_str(&format!(" ...args: {}\n", rest_positional.1)); } } if !signature.named.is_empty() { diff --git a/src/commands/histogram.rs b/src/commands/histogram.rs index 1fcd7f9329..da5ec417cb 100644 --- a/src/commands/histogram.rs +++ b/src/commands/histogram.rs @@ -87,15 +87,15 @@ pub fn histogram( let column = (*column_name).clone(); - if let Value { value: UntaggedValue::Table(start), .. } = datasets.get(0).unwrap() { + if let Value { value: UntaggedValue::Table(start), .. } = datasets.get(0).ok_or_else(|| ShellError::labeled_error("Unable to load dataset", "unabled to load dataset", &name))? { for percentage in start.iter() { let mut fact = TaggedDictBuilder::new(&name); - let value: Tagged = group_labels.get(idx).unwrap().clone(); + let value: Tagged = group_labels.get(idx).ok_or_else(|| ShellError::labeled_error("Unable to load group labels", "unabled to load group labels", &name))?.clone(); fact.insert_value(&column, UntaggedValue::string(value.item).into_value(value.tag)); - if let Value { value: UntaggedValue::Primitive(Primitive::Int(ref num)), .. } = percentage.clone() { - let string = std::iter::repeat("*").take(num.to_i32().unwrap() as usize).collect::(); + if let Value { value: UntaggedValue::Primitive(Primitive::Int(ref num)), ref tag } = percentage.clone() { + let string = std::iter::repeat("*").take(num.to_i32().ok_or_else(|| ShellError::labeled_error("Expected a number", "expected a number", tag))? as usize).collect::(); fact.insert_untagged(&frequency_column_name, UntaggedValue::string(string)); } diff --git a/src/commands/insert.rs b/src/commands/insert.rs index 05dd661947..bb9dc2b44e 100644 --- a/src/commands/insert.rs +++ b/src/commands/insert.rs @@ -38,7 +38,7 @@ impl PerItemCommand for Insert { value: Value, ) -> Result { let value_tag = value.tag(); - let field = call_info.args.expect_nth(0)?.as_column_path().unwrap(); + let field = call_info.args.expect_nth(0)?.as_column_path()?; let replacement = call_info.args.expect_nth(1)?.tagged_unknown(); let stream = match value { diff --git a/src/commands/open.rs b/src/commands/open.rs index 03a4a1abfa..91a5aa031e 100644 --- a/src/commands/open.rs +++ b/src/commands/open.rs @@ -64,7 +64,7 @@ fn run(call_info: &CallInfo, raw_args: &RawCommandArgs) -> Result Result { //let value_tag = value.tag(); - let pattern = call_info.args.expect_nth(0)?.as_string().unwrap(); + let pattern = call_info.args.expect_nth(0)?.as_string()?; - let parse_pattern = parse(&pattern).unwrap(); + let parse_pattern = parse(&pattern).map_err(|_| { + ShellError::labeled_error( + "Could not create parse pattern", + "could not create parse pattern", + &value.tag, + ) + })?; let parse_regex = build_regex(&parse_pattern.1); let column_names = column_names(&parse_pattern.1); - let regex = Regex::new(&parse_regex).unwrap(); + let regex = Regex::new(&parse_regex).map_err(|_| { + ShellError::labeled_error("Could not parse regex", "could not parse regex", &value.tag) + })?; let output = if let Ok(s) = value.as_string() { let mut results = vec![];