Another batch of un-unwrapping (#1148)

Another batch of un-unwrappings

crates/nu-errors/src/lib.rs

@@ -24,6 +24,10 @@ pub enum ParseErrorReason {
         expected: &'static str,
         actual: Spanned<String>,
     },
+
+    /// An unexpected internal error has occurred
+    InternalError { message: Spanned<String> },
+
     /// 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<impl Into<String>>) -> ParseError {
+        ParseError {
+            reason: ParseErrorReason::InternalError {
+                message: message.item.into().spanned(message.span),
+            },
+        }
+    }
+
     /// Construct a [ParseErrorReason::ArgumentError](ParseErrorReason::ArgumentError)
     pub fn argument_error(command: Spanned<impl Into<String>>, kind: ArgumentError) -> ParseError {
         ParseError {
@@ -89,6 +102,11 @@ impl From<ParseError> 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)
             }

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)

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));
         }

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,
+    pub from: usize,
-    to: usize,
+    pub to: usize,
 }
 
 impl<'content, 'me> Peeked<'content, 'me> {

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() {
+fn set_env_from_config() -> Result<(), ShellError> {
-    let config = crate::data::config::read(Tag::unknown(), &None).unwrap();
+    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<String, ReadlineError>, 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();
 

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));

src/commands/from_sqlite.rs

@@ -107,9 +107,9 @@ fn convert_sqlite_value_to_nu_value(value: ValueRef, tag: impl Into<Tag> + 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),

src/commands/from_xlsx.rs

@@ -55,7 +55,10 @@ fn from_xlsx(
             match value.value {
                 UntaggedValue::Primitive(Primitive::Binary(vb)) => {
                     let mut buf: Cursor<Vec<u8>> = 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() {
+                                    row_output.insert_untagged(&format!("Column{}", i), value);
-                            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);
+                                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());

src/commands/from_xml.rs

@@ -60,7 +60,10 @@ fn from_node_to_value<'a, 'd>(n: &roxmltree::Node<'a, 'd>, tag: impl Into<Tag>)
     } else if n.is_pi() {
         UntaggedValue::string("<processing_instruction>").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("<error>").into_value(tag),
+        }
     } else {
         UntaggedValue::string("<unknown>").into_value(tag)
     }
@@ -149,36 +152,40 @@ mod tests {
         UntaggedValue::table(list).into_untagged_value()
     }
 
-    fn parse(xml: &str) -> Value {
+    fn parse(xml: &str) -> Result<Value, roxmltree::Error> {
-        from_xml::from_xml_string_to_value(xml.to_string(), Tag::unknown()).unwrap()
+        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 = "<nu></nu>";
 
         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 = "<nu>La era de los tres caballeros</nu>";
 
         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 = "\
 <nu>
     <dev>Andrés</dev>
@@ -187,7 +194,7 @@ mod tests {
 </nu>";
 
         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(())
     }
 }

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(())
     }
 }

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()
+                                .ok_or_else(|| {
-                                .as_string()
+                                    ShellError::labeled_error(
-                                .unwrap(),
+                                        "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!(
+                        if let Some(rest_positional) = signature.rest_positional {
-                                "  ...args{} {}\n",
+                            long_desc.push_str(&format!("  ...args: {}\n", rest_positional.1));
-                                if signature.rest_positional.is_some() {
-                                    ":"
-                                } else {
-                                    ""
-                                },
-                                signature.rest_positional.unwrap().1
-                            ));
                         }
                     }
                     if !signature.named.is_empty() {

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<String> = group_labels.get(idx).unwrap().clone();
+                        let value: Tagged<String> = 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() {
+                        if let Value { value: UntaggedValue::Primitive(Primitive::Int(ref num)), ref tag } = percentage.clone() {
-                            let string = std::iter::repeat("*").take(num.to_i32().unwrap() as usize).collect::<String>();
+                            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::<String>();
                             fact.insert_untagged(&frequency_column_name, UntaggedValue::string(string));
                         }
 

src/commands/insert.rs

@@ -38,7 +38,7 @@ impl PerItemCommand for Insert {
         value: Value,
     ) -> Result<OutputStream, ShellError> {
         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 {

src/commands/open.rs

@@ -64,7 +64,7 @@ fn run(call_info: &CallInfo, raw_args: &RawCommandArgs) -> Result<OutputStream,
             yield Err(e);
             return;
         }
-        let (file_extension, contents, contents_tag) = result.unwrap();
+        let (file_extension, contents, contents_tag) = result?;
 
         let file_extension = if has_raw {
             None

src/commands/parse.rs

@@ -101,13 +101,21 @@ impl PerItemCommand for Parse {
         value: Value,
     ) -> Result<OutputStream, ShellError> {
         //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![];