From 8eb0f9fefb7064be5c14dd4d6a9e01bba8ffeabc Mon Sep 17 00:00:00 2001 From: Timo Kluck Date: Sat, 15 Feb 2020 13:41:44 +0100 Subject: [PATCH] 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. --- NEWS.md | 5 +++++ base/compiler/ssair/ir.jl | 2 +- base/ordering.jl | 46 +++++++++++++++++++++++++++++---------- test/choosetests.jl | 2 +- test/ordering.jl | 38 ++++++++++++++++++++++++++++++++ 5 files changed, 80 insertions(+), 13 deletions(-) create mode 100644 test/ordering.jl diff --git a/NEWS.md b/NEWS.md index b1876a6c5f649..d097474a91629 100644 --- a/NEWS.md +++ b/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]). diff --git a/base/compiler/ssair/ir.jl b/base/compiler/ssair/ir.jl index 45184203a7fdb..1fa189d1fb509 100644 --- a/base/compiler/ssair/ir.jl +++ b/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) diff --git a/base/ordering.jl b/base/ordering.jl index ea1887c6ae471..dc2f2c8595be2 100644 --- a/base/ordering.jl +++ b/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 diff --git a/test/choosetests.jl b/test/choosetests.jl index 9f1469ad523d1..bc9ec0192e054 100644 --- a/test/choosetests.jl +++ b/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", diff --git a/test/ordering.jl b/test/ordering.jl new file mode 100644 index 0000000000000..0fb663e0ef3ed --- /dev/null +++ b/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()) +