Base.Ordering: do not ever discard order parameter (#34719)

Prior to this commit, the `order` parameter was discarded if
either `by` or `order` was passed. However, there is a sane
interpretation for most of these combinations, and we should
use that interpretation for least surprise.

The one case that doesn't have an obvious interpretation is when a
non-trivial `order` is passed together with an `lt` function; in that
case it is better to error out rather than let it pass silently.

Author: tkluck

NEWS.md

@@ -66,6 +66,11 @@ Standard library changes
 ------------------------
 * The `@timed` macro now returns a `NamedTuple` ([#34149])
 * New `supertypes(T)` function returns a tuple of all supertypes of `T` ([#34419]).
+* Sorting-related functions such as `sort` that take the keyword arguments `lt`, `rev`, `order`
+  and `by` now do not discard `order` if `by` or `lt` are passed. In the former case, the
+  order from `order` is used to compare the values of `by(element)`. In the latter case,
+  any order different from `Forward` or `Reverse` will raise an error about the
+  ambiguity.
 
 #### LinearAlgebra
 * The BLAS submodule now supports the level-2 BLAS subroutine `hpmv!` ([#34211]).

base/compiler/ssair/ir.jl

@@ -608,7 +608,7 @@ function count_added_node!(compact::IncrementalCompact, @nospecialize(v))
 end
 
 function resort_pending!(compact)
-    sort!(compact.pending_perm, DEFAULT_STABLE, Order.By(x->compact.pending_nodes[x].pos))
+    sort!(compact.pending_perm, DEFAULT_STABLE, Order.By(x->compact.pending_nodes[x].pos, Order.Forward))
 end
 
 function insert_node!(compact::IncrementalCompact, before, @nospecialize(typ), @nospecialize(val), attach_after::Bool=false)

base/ordering.jl

@@ -34,10 +34,14 @@ const DirectOrdering = Union{ForwardOrdering,ReverseOrdering{ForwardOrdering}}
 const Forward = ForwardOrdering()
 const Reverse = ReverseOrdering()
 
-struct By{T} <: Ordering
+struct By{T, O} <: Ordering
     by::T
+    order::O
 end
 
+# backwards compatibility with VERSION < v"1.5-"
+By(by) = By(by, Forward)
+
 struct Lt{T} <: Ordering
     lt::T
 end
@@ -47,9 +51,12 @@ struct Perm{O<:Ordering,V<:AbstractVector} <: Ordering
     data::V
 end
 
+ReverseOrdering(by::By) = By(by.by, ReverseOrdering(by.order))
+ReverseOrdering(perm::Perm) = Perm(ReverseOrdering(perm.order), perm.data)
+
 lt(o::ForwardOrdering,       a, b) = isless(a,b)
 lt(o::ReverseOrdering,       a, b) = lt(o.fwd,b,a)
-lt(o::By,                    a, b) = isless(o.by(a),o.by(b))
+lt(o::By,                    a, b) = lt(o.order,o.by(a),o.by(b))
 lt(o::Lt,                    a, b) = o.lt(a,b)
 
 @propagate_inbounds function lt(p::Perm, a::Integer, b::Integer)
@@ -58,16 +65,18 @@ lt(o::Lt,                    a, b) = o.lt(a,b)
     lt(p.order, da, db) | (!lt(p.order, db, da) & (a < b))
 end
 
-ordtype(o::ReverseOrdering, vs::AbstractArray) = ordtype(o.fwd, vs)
-ordtype(o::Perm,            vs::AbstractArray) = ordtype(o.order, o.data)
-# TODO: here, we really want the return type of o.by, without calling it
-ordtype(o::By,              vs::AbstractArray) = try typeof(o.by(vs[1])) catch; Any end
-ordtype(o::Ordering,        vs::AbstractArray) = eltype(vs)
-
 _ord(lt::typeof(isless), by::typeof(identity), order::Ordering) = order
-_ord(lt::typeof(isless), by,                   order::Ordering) = By(by)
-_ord(lt,                 by::typeof(identity), order::Ordering) = Lt(lt)
-_ord(lt,                 by,                   order::Ordering) = Lt((x,y)->lt(by(x),by(y)))
+_ord(lt::typeof(isless), by,                   order::Ordering) = By(by, order)
+
+function _ord(lt, by, order::Ordering)
+    if order === Forward
+        return Lt((x, y) -> lt(by(x), by(y)))
+    elseif order === Reverse
+        return Lt((x, y) -> lt(by(y), by(x)))
+    else
+        error("Passing both lt= and order= arguments is ambiguous; please pass order=Forward or order=Reverse (or leave default)")
+    end
+end
 
 ord(lt, by, rev::Nothing, order::Ordering=Forward) = _ord(lt, by, order)
 
@@ -76,4 +85,19 @@ function ord(lt, by, rev::Bool, order::Ordering=Forward)
     return rev ? ReverseOrdering(o) : o
 end
 
+
+# This function is not in use anywhere in Base but we observed
+# use in sorting-related packages (#34719). It's probably best to move
+# this functionality to those packages in the future; let's remind/force
+# ourselves to deprecate this in v2.0.
+# The following clause means `if VERSION < v"2.0-"` but it also works during
+# bootstrap. For the same reason, we need to write `Int32` instead of `Cint`.
+if ccall(:jl_ver_major, Int32, ()) < 2
+    ordtype(o::ReverseOrdering, vs::AbstractArray) = ordtype(o.fwd, vs)
+    ordtype(o::Perm,            vs::AbstractArray) = ordtype(o.order, o.data)
+    # TODO: here, we really want the return type of o.by, without calling it
+    ordtype(o::By,              vs::AbstractArray) = try typeof(o.by(vs[1])) catch; Any end
+    ordtype(o::Ordering,        vs::AbstractArray) = eltype(vs)
+end
+
 end

test/choosetests.jl

@@ -40,7 +40,7 @@ function choosetests(choices = [])
         "arrayops", "tuple", "reduce", "reducedim", "abstractarray",
         "intfuncs", "simdloop", "vecelement", "rational",
         "bitarray", "copy", "math", "fastmath", "functional", "iterators",
-        "operators", "path", "ccall", "parse", "loading", "gmp",
+        "operators", "ordering", "path", "ccall", "parse", "loading", "gmp",
         "sorting", "spawn", "backtrace", "exceptions",
         "file", "read", "version", "namedtuple",
         "mpfr", "broadcast", "complex",

test/ordering.jl

@@ -0,0 +1,38 @@
+using Test
+
+import Base.Order: Forward, Reverse
+
+# every argument can flip the integer order by passing the right value. Here,
+# we enumerate a few of these combinations and check that all these flips
+# compound so that in total we either have an increasing or decreasing sort.
+for (s1, rev) in enumerate([true, false])
+    for (s2, lt) in enumerate([>, <, (a, b) -> a - b > 0, (a, b) -> a - b < 0])
+        for (s3, by) in enumerate([-, +])
+            for (s4, order) in enumerate([Reverse, Forward])
+                if iseven(s1 + s2 + s3 + s4)
+                    target = [1, 2, 3]
+                else
+                    target = [3, 2, 1]
+                end
+                @test target == sort([2, 3, 1], rev=rev, lt=lt, by=by, order=order)
+            end
+        end
+    end
+end
+
+@test [1 => 3, 2 => 5, 3 => 1] ==
+            sort([1 => 3, 2 => 5, 3 => 1]) ==
+            sort([1 => 3, 2 => 5, 3 => 1], by=first) ==
+            sort([1 => 3, 2 => 5, 3 => 1], rev=true, order=Reverse) ==
+            sort([1 => 3, 2 => 5, 3 => 1], lt= >, order=Reverse)
+
+@test [3 => 1, 1 => 3, 2 => 5] ==
+            sort([1 => 3, 2 => 5, 3 => 1], by=last) ==
+            sort([1 => 3, 2 => 5, 3 => 1], by=last, rev=true, order=Reverse) ==
+            sort([1 => 3, 2 => 5, 3 => 1], by=last, lt= >, order=Reverse)
+
+
+struct SomeOtherOrder <: Base.Order.Ordering end
+
+@test_throws ErrorException sort([1, 2, 3], lt=(a, b) -> a - b < 0, order=SomeOtherOrder())
+