Yet more ununwraps (#1150)

crates/nu-parser/src/hir/baseline_parse/tests.rs

@@ -103,11 +103,11 @@ impl TestRegistry {
 }
 
 impl SignatureRegistry for TestRegistry {
-    fn has(&self, name: &str) -> bool {
-        self.signatures.contains_key(name)
+    fn has(&self, name: &str) -> Result<bool, ShellError> {
+        Ok(self.signatures.contains_key(name))
     }
-    fn get(&self, name: &str) -> Option<Signature> {
-        self.signatures.get(name).cloned()
+    fn get(&self, name: &str) -> Result<Option<Signature>, ShellError> {
+        Ok(self.signatures.get(name).cloned())
     }
 }
 

crates/nu-parser/src/hir/syntax_shape.rs

@@ -127,8 +127,8 @@ impl ExpandExpression for SyntaxShape {
 }
 
 pub trait SignatureRegistry {
-    fn has(&self, name: &str) -> bool;
-    fn get(&self, name: &str) -> Option<Signature>;
+    fn has(&self, name: &str) -> Result<bool, ShellError>;
+    fn get(&self, name: &str) -> Result<Option<Signature>, ShellError>;
 }
 
 #[derive(Getters, new)]
@@ -673,10 +673,20 @@ impl FallibleColorSyntax for CommandHeadShape {
                 UnspannedAtomicToken::Word { text } => {
                     let name = text.slice(context.source);
 
-                    if context.registry.has(name) {
+                    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).ok_or_else(|| {
+                        let signature = context
+                            .registry
+                            .get(name)
+                            .map_err(|_| {
+                                ShellError::labeled_error(
+                                    "Internal error: could not load signature from registry",
+                                    "could not load from registry",
+                                    text,
+                                )
+                            })?
+                            .ok_or_else(|| {
                                 ShellError::labeled_error(
                                     "Internal error: could not load signature from registry",
                                     "could not load from registry",
@@ -721,8 +731,12 @@ impl ExpandSyntax for CommandHeadShape {
                     },
                     UnspannedToken::Bare => {
                         let name = token_span.slice(context.source);
-                        if context.registry.has(name) {
-                            let signature = context.registry.get(name).ok_or_else(|| {
+                        if context.registry.has(name)? {
+                            let signature = context
+                                .registry
+                                .get(name)
+                                .map_err(|_| ParseError::internal_error(name.spanned(token_span)))?
+                                .ok_or_else(|| {
                                     ParseError::internal_error(name.spanned(token_span))
                                 })?;
                             CommandSignature::Internal(signature.spanned(token_span))

src/commands/classified/internal.rs

@@ -26,7 +26,7 @@ pub(crate) async fn run_internal_command(
 
     let result = {
         context.run_command(
-            internal_command,
+            internal_command?,
             command.name_tag.clone(),
             command.args.clone(),
             &source,

src/commands/split_by.rs

@@ -156,6 +156,7 @@ mod tests {
     use crate::commands::group_by::group;
     use crate::commands::split_by::split;
     use indexmap::IndexMap;
+    use nu_errors::ShellError;
     use nu_protocol::{UntaggedValue, Value};
     use nu_source::*;
 
@@ -171,9 +172,9 @@ mod tests {
         UntaggedValue::table(list).into_untagged_value()
     }
 
-    fn nu_releases_grouped_by_date() -> Value {
+    fn nu_releases_grouped_by_date() -> Result<Value, ShellError> {
         let key = String::from("date").tagged_unknown();
-        group(&key, nu_releases_commiters(), Tag::unknown()).unwrap()
+        group(&key, nu_releases_commiters(), Tag::unknown())
     }
 
     fn nu_releases_commiters() -> Vec<Value> {
@@ -209,11 +210,11 @@ mod tests {
     }
 
     #[test]
-    fn splits_inner_tables_by_key() {
+    fn splits_inner_tables_by_key() -> Result<(), ShellError> {
         let for_key = String::from("country").tagged_unknown();
 
         assert_eq!(
-            split(&for_key, &nu_releases_grouped_by_date(), Tag::unknown()).unwrap(),
+            split(&for_key, &nu_releases_grouped_by_date()?, Tag::unknown())?,
             UntaggedValue::row(indexmap! {
                 "EC".into() => row(indexmap! {
                     "August 23-2019".into() => table(&[
@@ -250,6 +251,8 @@ mod tests {
                 })
             }).into_untagged_value()
         );
+
+        Ok(())
     }
 
     #[test]

src/commands/to_json.rs

@@ -39,12 +39,28 @@ pub fn value_to_json_value(v: &Value) -> Result<serde_json::Value, ShellError> {
         UntaggedValue::Primitive(Primitive::Date(d)) => serde_json::Value::String(d.to_string()),
         UntaggedValue::Primitive(Primitive::EndOfStream) => serde_json::Value::Null,
         UntaggedValue::Primitive(Primitive::BeginningOfStream) => serde_json::Value::Null,
-        UntaggedValue::Primitive(Primitive::Decimal(f)) => serde_json::Value::Number(
-            serde_json::Number::from_f64(
+        UntaggedValue::Primitive(Primitive::Decimal(f)) => {
+            if let Some(f) = f.to_f64() {
+                if let Some(num) = serde_json::Number::from_f64(
                     f.to_f64().expect("TODO: What about really big decimals?"),
-            )
-            .unwrap(),
-        ),
+                ) {
+                    serde_json::Value::Number(num)
+                } else {
+                    return Err(ShellError::labeled_error(
+                        "Could not convert value to decimal number",
+                        "could not convert to decimal",
+                        &v.tag,
+                    ));
+                }
+            } else {
+                return Err(ShellError::labeled_error(
+                    "Could not convert value to decimal number",
+                    "could not convert to decimal",
+                    &v.tag,
+                ));
+            }
+        }
+
         UntaggedValue::Primitive(Primitive::Int(i)) => {
             serde_json::Value::Number(serde_json::Number::from(CoerceInto::<i64>::coerce_into(
                 i.tagged(&v.tag),

src/context.rs

@@ -17,13 +17,25 @@ pub struct CommandRegistry {
 }
 
 impl SignatureRegistry for CommandRegistry {
-    fn has(&self, name: &str) -> bool {
-        let registry = self.registry.lock().unwrap();
-        registry.contains_key(name)
+    fn has(&self, name: &str) -> Result<bool, ShellError> {
+        if let Ok(registry) = self.registry.lock() {
+            Ok(registry.contains_key(name))
+        } else {
+            Err(ShellError::untagged_runtime_error(format!(
+                "Could not load from registry: {}",
+                name
+            )))
+        }
+    }
+    fn get(&self, name: &str) -> Result<Option<Signature>, ShellError> {
+        if let Ok(registry) = self.registry.lock() {
+            Ok(registry.get(name).map(|command| command.signature()))
+        } else {
+            Err(ShellError::untagged_runtime_error(format!(
+                "Could not get from registry: {}",
+                name
+            )))
         }
-    fn get(&self, name: &str) -> Option<Signature> {
-        let registry = self.registry.lock().unwrap();
-        registry.get(name).map(|command| command.signature())
     }
 }
 
@@ -48,8 +60,10 @@ impl CommandRegistry {
         registry.get(name).cloned()
     }
 
-    pub(crate) fn expect_command(&self, name: &str) -> Arc<Command> {
-        self.get_command(name).unwrap()
+    pub(crate) fn expect_command(&self, name: &str) -> Result<Arc<Command>, ShellError> {
+        self.get_command(name).ok_or_else(|| {
+            ShellError::untagged_runtime_error(format!("Could not load command: {}", name))
+        })
     }
 
     pub(crate) fn has(&self, name: &str) -> bool {
@@ -171,7 +185,7 @@ impl Context {
         self.registry.get_command(name)
     }
 
-    pub(crate) fn expect_command(&self, name: &str) -> Arc<Command> {
+    pub(crate) fn expect_command(&self, name: &str) -> Result<Arc<Command>, ShellError> {
         self.registry.expect_command(name)
     }
 

src/data/base.rs

@@ -224,25 +224,28 @@ mod tests {
         move |(_obj_source, _column_path_tried, _err)| ShellError::unimplemented(reason)
     }
 
-    fn column_path(paths: &[Value]) -> Tagged<ColumnPathValue> {
-        as_column_path(&table(paths)).unwrap()
+    fn column_path(paths: &[Value]) -> Result<Tagged<ColumnPathValue>, ShellError> {
+        as_column_path(&table(paths))
     }
 
     #[test]
-    fn gets_matching_field_from_a_row() {
+    fn gets_matching_field_from_a_row() -> Result<(), ShellError> {
         let row = UntaggedValue::row(indexmap! {
             "amigos".into() => table(&[string("andres"),string("jonathan"),string("yehuda")])
         })
         .into_untagged_value();
 
         assert_eq!(
-            row.get_data_by_key("amigos".spanned_unknown()).unwrap(),
+            row.get_data_by_key("amigos".spanned_unknown())
+                .ok_or_else(|| ShellError::unexpected("Failure during testing"))?,
             table(&[string("andres"), string("jonathan"), string("yehuda")])
         );
+
+        Ok(())
     }
 
     #[test]
-    fn gets_matching_field_from_nested_rows_inside_a_row() {
+    fn gets_matching_field_from_nested_rows_inside_a_row() -> Result<(), ShellError> {
         let field_path = column_path(&[string("package"), string("version")]);
 
         let (version, tag) = string("0.4.0").into_parts();
@@ -256,16 +259,19 @@ mod tests {
         });
 
         assert_eq!(
-            *value
-                .into_value(tag)
-                .get_data_by_column_path(&field_path, Box::new(error_callback("package.version")))
-                .unwrap(),
+            *value.into_value(tag).get_data_by_column_path(
+                &field_path?.item,
+                Box::new(error_callback("package.version"))
+            )?,
             version
-        )
+        );
+
+        Ok(())
     }
 
     #[test]
-    fn gets_first_matching_field_from_rows_with_same_field_inside_a_table() {
+    fn gets_first_matching_field_from_rows_with_same_field_inside_a_table() -> Result<(), ShellError>
+    {
         let field_path = column_path(&[string("package"), string("authors"), string("name")]);
 
         let (_, tag) = string("Andrés N. Robalino").into_parts();
@@ -283,23 +289,22 @@ mod tests {
         });
 
         assert_eq!(
-            value
-                .into_value(tag)
-                .get_data_by_column_path(
-                    &field_path,
+            value.into_value(tag).get_data_by_column_path(
+                &field_path?.item,
                 Box::new(error_callback("package.authors.name"))
-                )
-                .unwrap(),
+            )?,
             table(&[
                 string("Andrés N. Robalino"),
                 string("Jonathan Turner"),
                 string("Yehuda Katz")
             ])
-        )
+        );
+
+        Ok(())
     }
 
     #[test]
-    fn column_path_that_contains_just_a_number_gets_a_row_from_a_table() {
+    fn column_path_that_contains_just_a_number_gets_a_row_from_a_table() -> Result<(), ShellError> {
         let field_path = column_path(&[string("package"), string("authors"), int(0)]);
 
         let (_, tag) = string("Andrés N. Robalino").into_parts();
@@ -317,18 +322,20 @@ mod tests {
         });
 
         assert_eq!(
-            *value
-                .into_value(tag)
-                .get_data_by_column_path(&field_path, Box::new(error_callback("package.authors.0")))
-                .unwrap(),
+            *value.into_value(tag).get_data_by_column_path(
+                &field_path?.item,
+                Box::new(error_callback("package.authors.0"))
+            )?,
             UntaggedValue::row(indexmap! {
                 "name".into() => string("Andrés N. Robalino")
             })
         );
+
+        Ok(())
     }
 
     #[test]
-    fn column_path_that_contains_just_a_number_gets_a_row_from_a_row() {
+    fn column_path_that_contains_just_a_number_gets_a_row_from_a_row() -> Result<(), ShellError> {
         let field_path = column_path(&[string("package"), string("authors"), string("0")]);
 
         let (_, tag) = string("Andrés N. Robalino").into_parts();
@@ -346,21 +353,20 @@ mod tests {
         });
 
         assert_eq!(
-            *value
-                .into_value(tag)
-                .get_data_by_column_path(
-                    &field_path,
+            *value.into_value(tag).get_data_by_column_path(
+                &field_path?.item,
                 Box::new(error_callback("package.authors.\"0\""))
-                )
-                .unwrap(),
+            )?,
             UntaggedValue::row(indexmap! {
                 "name".into() => string("Andrés N. Robalino")
             })
         );
+
+        Ok(())
     }
 
     #[test]
-    fn replaces_matching_field_from_a_row() {
+    fn replaces_matching_field_from_a_row() -> Result<(), ShellError> {
         let field_path = column_path(&[string("amigos")]);
 
         let sample = UntaggedValue::row(indexmap! {
@@ -375,14 +381,16 @@ mod tests {
 
         let actual = sample
             .into_untagged_value()
-            .replace_data_at_column_path(&field_path, replacement)
+            .replace_data_at_column_path(&field_path?.item, replacement)
             .unwrap();
 
         assert_eq!(actual, row(indexmap! {"amigos".into() => string("jonas")}));
+
+        Ok(())
     }
 
     #[test]
-    fn replaces_matching_field_from_nested_rows_inside_a_row() {
+    fn replaces_matching_field_from_nested_rows_inside_a_row() -> Result<(), ShellError> {
         let field_path = column_path(&[
             string("package"),
             string("authors"),
@@ -404,7 +412,7 @@ mod tests {
 
         let actual = sample
             .into_value(tag.clone())
-            .replace_data_at_column_path(&field_path, replacement.clone())
+            .replace_data_at_column_path(&field_path?.item, replacement.clone())
             .unwrap();
 
         assert_eq!(
@@ -417,9 +425,11 @@ mod tests {
                     "los.3.caballeros".into()  => replacement})})})
             .into_value(tag)
         );
+
+        Ok(())
     }
     #[test]
-    fn replaces_matching_field_from_rows_inside_a_table() {
+    fn replaces_matching_field_from_rows_inside_a_table() -> Result<(), ShellError> {
         let field_path = column_path(&[
             string("shell_policy"),
             string("releases"),
@@ -456,7 +466,7 @@ mod tests {
 
         let actual = sample
             .into_value(tag.clone())
-            .replace_data_at_column_path(&field_path, replacement.clone())
+            .replace_data_at_column_path(&field_path?.item, replacement.clone())
             .unwrap();
 
         assert_eq!(
@@ -481,5 +491,7 @@ mod tests {
                 })
             }).into_value(&tag)
         );
+
+        Ok(())
     }
 }