Issue 1570 order key option (#1616)

* Allow $nil for Callable options

Related #1570

* Add a `&key` option to the `order` command.

Resolves #1570

Co-authored-by: Qi Xiao <xiaq@users.noreply.github.com>

0.19.0-release-notes.md

@@ -60,4 +60,7 @@ and compiled, even if it is not executed:
 -   A new `compact` command that replaces consecutive runs of equal values with
     a single copy, similar to the Unix `uniq` command.
 
+-   The `order` command has a new `&key` option
+    ([#1570](https://b.elv.sh/1570)).
+
 -   A new `benchmark` command has been added ([#1586](https://b.elv.sh/1586)).

pkg/eval/builtin_fn_stream.go

@@ -1,6 +1,7 @@
 package eval
 
 import (
+	"errors"
 	"fmt"
 	"sort"
 
@@ -25,6 +26,8 @@ func init() {
 	})
 }
 
+var ErrKeyAndLessThanOpts = errors.New("options &key and &less-than are mutually exclusive")
+
 //elvdoc:fn all
 //
 // ```elvish
@@ -343,23 +346,34 @@ func count(fm *Frame, args ...any) (int, error) {
 //elvdoc:fn order
 //
 // ```elvish
-// order &reverse=$false $less-than=$nil $inputs?
+// order &reverse=$false &key=$nil &less-than=$nil $inputs?
 // ```
 //
-// Outputs the [value inputs](#value-inputs) sorted
-// in ascending order. The sorting process is guaranteed to be
-// [stable](https://en.wikipedia.org/wiki/Sorting_algorithm#Stability).
+// Outputs the [value inputs](#value-inputs) sorted in ascending order (ignoring
+// the behavior of any `&less-than` callable). The sorting process is guaranteed
+// to be [stable](https://en.wikipedia.org/wiki/Sorting_algorithm#Stability).
+//
+// The `&reverse` option, if true, reverses the order of output; e.g.,
+// descending rather than ascending.
 //
-// The `&reverse` option, if true, reverses the order of output.
+// The `&key` option, if not `$nil` (the default value), is a function that
+// accepts an item to be sorted (string, list, map, etc.) and outputs a single
+// value (which could be a list or map) that is passed to the default
+// comparison function. If the key function throws an exception, `order`
+// rethrows the exception.
 //
-// The `&less-than` option, if given, establishes the ordering of the elements.
-// Its value should be a function that takes two arguments and outputs a single
-// boolean indicating whether the first argument is less than the second
+// The `&less-than` option, if not `$nil` (the default value), establishes the
+// ordering of the items. Its value should be a function that takes two
+// arguments and outputs `$true` if the first argument is less than the second
 // argument. If the function throws an exception, `order` rethrows the exception
-// without outputting any value.
+// without outputting any value. If `&less-than` has value `$nil` (the default
+// if not set), it is equivalent to `{|a b| == -1 (compare $a $b) }`.
 //
-// If `&less-than` has value `$nil` (the default if not set), it is equivalent
-// to `{|a b| eq -1 (compare $a $b) }`.
+// You can use `&key` or `&less-than` but not both at the same time. A `&key`
+// function is usually preferable to a `&less-than` function because it is more
+// efficient extract the key for each item just once rather than on each
+// comparison. If both options are `$nil` then the entirety of each item is
+// compared using the equivalent of `{|a b| == -1 (compare $a $b) }`.
 //
 // Examples:
 //
@@ -410,20 +424,103 @@ func count(fm *Frame, args ...any) (int, error) {
 
 type orderOptions struct {
 	Reverse  bool
+	Key      Callable
 	LessThan Callable
 }
 
 func (opt *orderOptions) SetDefaultOptions() {}
 
 func order(fm *Frame, opts orderOptions, inputs Inputs) error {
+	if opts.Key != nil && opts.LessThan != nil {
+		return ErrKeyAndLessThanOpts
+	}
+
+	values, err := collectValues(fm, opts, inputs)
+	if err != nil {
+		return err
+	}
+
+	var sortErr error
+	cmpFn := getCmpFunction(fm, opts, values, &sortErr)
+	sort.SliceStable(values, cmpFn)
+	if sortErr != nil {
+		return sortErr
+	}
+
+	out := fm.ValueOutput()
+	for _, v := range values {
+		var err error
+		if opts.Key == nil {
+			// No `&key` option was used so simply output the original value.
+			err = out.Put(v)
+		} else {
+			// Ignore the key generated by the `&key` function to output the
+			// original value.
+			err = out.Put(v.([2]any)[1])
+		}
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func collectValues(fm *Frame, opts orderOptions, inputs Inputs) ([]any, error) {
 	var values []any
-	inputs(func(v any) { values = append(values, v) })
+	if opts.Key == nil {
+		inputs(func(v any) { values = append(values, v) })
+	} else {
+		var keyErr error
+		inputs(func(v any) {
+			if keyErr != nil {
+				return
+			}
+			outputs, err := fm.CaptureOutput(func(fm *Frame) error {
+				return opts.Key.Call(fm, []any{v}, NoOpts)
+			})
+			if err != nil {
+				keyErr = err
+			} else if len(outputs) != 1 {
+				keyErr = errors.New("&key function output more than one value")
+			} else {
+				t := [2]any{outputs[0], v}
+				values = append(values, t)
+			}
+		})
+		if keyErr != nil {
+			return values, keyErr
+		}
+	}
+	return values, nil
+}
+
+func getCmpFunction(fm *Frame, opts orderOptions, values []any, sortErr *error) func(i, j int) bool {
+	if opts.Key != nil {
+		// Use the default comparison but restricted to the key precomputed by
+		// the `&key` function.
+		return func(i, j int) bool {
+			if *sortErr != nil {
+				return true
+			}
+			ki := values[i].([2]any)[0]
+			kj := values[j].([2]any)[0]
+			o := cmp(ki, kj)
+			if o == uncomparable {
+				*sortErr = ErrUncomparable
+				return true
+			}
+			if opts.Reverse {
+				return o == more
+			}
+			return o == less
+		}
+	}
 
-	var errSort error
-	var lessFn func(i, j int) bool
 	if opts.LessThan != nil {
-		lessFn = func(i, j int) bool {
-			if errSort != nil {
+		// Use the custom function provided by the user to compare the value of
+		// each item.
+		return func(i, j int) bool {
+			if *sortErr != nil {
 				return true
 			}
 			var args []any
@@ -436,11 +533,11 @@ func order(fm *Frame, opts orderOptions, inputs Inputs) error {
 				return opts.LessThan.Call(fm, args, NoOpts)
 			})
 			if err != nil {
-				errSort = err
+				*sortErr = err
 				return true
 			}
 			if len(outputs) != 1 {
-				errSort = errs.BadValue{
+				*sortErr = errs.BadValue{
 					What:   "output of the &less-than callback",
 					Valid:  "a single boolean",
 					Actual: fmt.Sprintf("%d values", len(outputs))}
@@ -449,40 +546,27 @@ func order(fm *Frame, opts orderOptions, inputs Inputs) error {
 			if b, ok := outputs[0].(bool); ok {
 				return b
 			}
-			errSort = errs.BadValue{
+			*sortErr = errs.BadValue{
 				What:  "output of the &less-than callback",
 				Valid: "boolean", Actual: vals.Kind(outputs[0])}
 			return true
 		}
-	} else {
-		// Use default comparison implemented by cmp.
-		lessFn = func(i, j int) bool {
-			if errSort != nil {
-				return true
-			}
-			o := cmp(values[i], values[j])
-			if o == uncomparable {
-				errSort = ErrUncomparable
-				return true
-			}
-			if opts.Reverse {
-				return o == more
-			}
-			return o == less
-		}
 	}
 
-	sort.SliceStable(values, lessFn)
-
-	if errSort != nil {
-		return errSort
-	}
-	out := fm.ValueOutput()
-	for _, v := range values {
-		err := out.Put(v)
-		if err != nil {
-			return err
+	// Use the default comparison of each item. This is the common case when
+	// there is no `&key` or `&less-than` option.
+	return func(i, j int) bool {
+		if *sortErr != nil {
+			return true
 		}
+		o := cmp(values[i], values[j])
+		if o == uncomparable {
+			*sortErr = ErrUncomparable
+			return true
+		}
+		if opts.Reverse {
+			return o == more
+		}
+		return o == less
 	}
-	return nil
 }

pkg/eval/builtin_fn_stream_test.go

@@ -68,6 +68,10 @@ func TestCount(t *testing.T) {
 
 func TestOrder(t *testing.T) {
 	Test(t,
+		// Incompatible order options throws an exception.
+		That("order &key={|v| } &less-than={|a b| }").
+			Throws(ErrKeyAndLessThanOpts),
+
 		// Ordering strings
 		That("put foo bar ipsum | order").Puts("bar", "foo", "ipsum"),
 		That("put foo bar bar | order").Puts("bar", "bar", "foo"),
@@ -124,9 +128,21 @@ func TestOrder(t *testing.T) {
 		// &reverse
 		That("put foo bar ipsum | order &reverse").Puts("ipsum", "foo", "bar"),
 
+		// &key
+		That("put 10 1 5 2 | order &key={|v| num $v }").
+			Puts("1", "2", "5", "10"),
+		That("put 10 1 5 2 | order &reverse &key={|v| num $v }").
+			Puts("10", "5", "2", "1"),
+		That("put 10 1 5 2 | order &key={|v| put 1 }").
+			Puts("10", "1", "5", "2"),
+
 		// &less-than
 		That("put 1 10 2 5 | order &less-than={|a b| < $a $b }").
 			Puts("1", "2", "5", "10"),
+		That("put 10 1 5 2 | order &less-than=$nil").
+			Puts("1", "10", "2", "5"),
+		That("put 10 1 5 2 | order").
+			Puts("1", "10", "2", "5"),
 
 		// &less-than writing more than one value
 		That("put 1 10 2 5 | order &less-than={|a b| put $true $false }").
@@ -155,7 +171,7 @@ func TestOrder(t *testing.T) {
 			Puts("10", "5", "2", "1"),
 
 		// Sort should be stable - test by pretending that all values but one
-		// are equal, an check that the order among them has not changed.
+		// are equal, and check that the order among them has not changed.
 		That("put l x o x r x e x m | order &less-than={|a b| eq $a x }").
 			Puts("x", "x", "x", "x", "l", "o", "r", "e", "m"),
 

pkg/eval/vals/conversion.go

@@ -102,6 +102,11 @@ func ScanToGo(src any, ptr any) error {
 			return fmt.Errorf("internal bug: need pointer to scan to, got %T", ptr)
 		}
 		dstType := ptrType.Elem()
+		if dstType.String() == "eval.Callable" && ValueOf(src) == ValueOf(nil) {
+			// A Callable option is a special-case that allows assignment from $nil.
+			ptr = nil
+			return nil
+		}
 		if !TypeOf(src).AssignableTo(dstType) {
 			var dstKind string
 			if dstType.Kind() == reflect.Interface {