Forbid sorting lists and record values of different types

crates/nu-command/src/filters/sort.rs

@@ -162,8 +162,7 @@ impl Command for Sort {
             }
             Value::List { vals, .. } => {
                 let mut vec = vals.to_owned();
-
+                crate::sort(&mut vec, call.head, insensitive, natural)?;
-                crate::sort(&mut vec, insensitive, natural)?;
 
                 if reverse {
                     vec.reverse()

crates/nu-command/src/sort_utils.rs

@@ -1,4 +1,5 @@
 use alphanumeric_sort::compare_str;
+use itertools::Itertools;
 use nu_engine::ClosureEval;
 use nu_protocol::{
     ast::CellPath,
@@ -6,7 +7,7 @@ use nu_protocol::{
     PipelineData, Record, ShellError, Span, Value,
 };
 use nu_utils::IgnoreCaseExt;
-use std::cmp::Ordering;
+use std::{borrow::Borrow, cmp::Ordering, mem::Discriminant};
 
 // This module includes sorting functionality that is useful in sort-by and elsewhere.
 // Eventually it would be nice to find a better home for it; sorting logic is only coupled
@@ -17,23 +18,83 @@ pub enum Comparator {
     CellPath(CellPath),
 }
 
-pub fn sort(vec: &mut [Value], insensitive: bool, natural: bool) -> Result<(), ShellError> {
+// TODO(rose) remove span, or explicitly ask for call.head span (depending on error impl)
-    // to apply insensitive or natural sorting, all values must be strings
+fn typecheck(
-    let string_sort: bool = vec
+    vals: &[Value],
+    span: Span,
+    insensitive: bool,
+    natural: bool,
+) -> Result<(), ShellError> {
+    let variants: Vec<_> = vals
         .iter()
-        .all(|value| matches!(value, &Value::String { .. }));
+        .filter(|val| !val.is_nothing())
+        .map(|val| (val, std::mem::discriminant(val)))
+        .collect();
+
+    match variants.iter().map(|(_, disc)| disc).all_equal_value() {
+        Ok(_) | Err(None) => (),
+        Err(Some((first, second))) => {
+            let first_val = variants
+                .iter()
+                .filter(|(_, disc)| std::ptr::eq(disc, first))
+                .at_most_one()
+                .unwrap() // TODO(rose)
+                .unwrap();
+
+            let second_val = variants
+                .iter()
+                .filter(|(_, disc)| std::ptr::eq(disc, second))
+                .at_most_one()
+                .unwrap()
+                .unwrap();
+
+            // TODO(rose) replace with bespoke error
+            return Err(ShellError::OperatorMismatch {
+                op_span: span,
+                lhs_ty: first_val.0.get_type().to_string(),
+                lhs_span: first_val.0.span(),
+                rhs_ty: second_val.0.get_type().to_string(),
+                rhs_span: second_val.0.span(),
+            });
+        }
+    }
+
+    if insensitive || natural {
+        // does not seem like it is possible to get the discriminant without constructing a value :(
+        let string_disc: Discriminant<Value> = std::mem::discriminant(&Value::String {
+            val: String::new(),
+            internal_span: Span::unknown(),
+        });
+        if let Some((val, _)) = variants.iter().find(|(_, disc)| disc != &string_disc) {
+            return Err(ShellError::IncompatibleParametersSingle {
+                msg: "unable to use case insensitive or natural sorting with non-string values"
+                    .to_string(),
+                span: val.span(),
+            });
+        }
+    }
+
+    Ok(())
+}
+
+pub fn sort(
+    vals: &mut [Value],
+    span: Span,
+    insensitive: bool,
+    natural: bool,
+) -> Result<(), ShellError> {
+    typecheck(vals, span, insensitive, natural)?;
 
     // allow the comparator function to indicate error
     // by mutating this option captured by the closure,
     // since sort_by closure must be infallible
     let mut compare_err: Option<ShellError> = None;
 
-    vec.sort_by(|a, b| {
+    vals.sort_by(|a, b| {
-        crate::compare_values(a, b, insensitive && string_sort, natural && string_sort)
+        crate::compare_values(a, b, insensitive, natural).unwrap_or_else(|err| {
-            .unwrap_or_else(|err| {
+            compare_err.get_or_insert(err);
-                compare_err.get_or_insert(err);
+            Ordering::Equal
-                Ordering::Equal
+        })
-            })
     });
 
     if let Some(err) = compare_err {
@@ -61,17 +122,23 @@ pub fn sort_by(
         });
     }
 
-    // to apply insensitive or natural sorting, all values must be strings
+    for cmp in comparators.iter() {
-    let string_sort: bool = comparators.iter().all(|cmp| {
+        // closures shouldn't affect whether cell paths are sorted naturally/insensitively
         let Comparator::CellPath(cell_path) = cmp else {
-            // closures shouldn't affect whether cell paths are sorted naturally/insensitively
+            continue;
-            return true;
         };
-        vec.iter().all(|value| {
+
-            let inner = value.clone().follow_cell_path(&cell_path.members, false);
+        let follow: Vec<_> = vec
-            matches!(inner, Ok(Value::String { .. }))
+            .iter()
-        })
+            .filter_map(|value| {
-    });
+                value
+                    .clone()
+                    .follow_cell_path(&cell_path.members, false)
+                    .ok() // we can ignore for now, we'll notice later if there's an error
+            })
+            .collect();
+        typecheck(&follow, span, insensitive, natural)?;
+    }
 
     // allow the comparator function to indicate error
     // by mutating this option captured by the closure,
@@ -84,8 +151,8 @@ pub fn sort_by(
             b,
             &comparators,
             span,
-            insensitive && string_sort,
+            insensitive,
-            natural && string_sort,
+            natural,
             &mut compare_err,
         )
     });
@@ -106,6 +173,17 @@ pub fn sort_record(
 ) -> Result<Record, ShellError> {
     let mut input_pairs: Vec<(String, Value)> = record.into_iter().collect();
 
+    if sort_by_value {
+        // TODO(rose): don't clone here?
+        let vals: Vec<_> = input_pairs.iter().map(|(_, val)| val.clone()).collect();
+        typecheck(
+            &vals,
+            Span::unknown(), // TODO(rose)
+            insensitive,
+            natural,
+        )?;
+    }
+
     // allow the comparator function to indicate error
     // by mutating this option captured by the closure,
     // since sort_by closure must be infallible