From 76aa02c61e2466728d53988043dba41ea0364da4 Mon Sep 17 00:00:00 2001 From: Malo <57839069+MDLC01@users.noreply.github.com> Date: Fri, 28 Feb 2025 23:17:27 +0100 Subject: [PATCH] Let `by` return a boolean instead of an integer --- crates/typst-library/src/foundations/array.rs | 90 +++++++++++++------ tests/suite/foundations/array.typ | 8 +- 2 files changed, 65 insertions(+), 33 deletions(-) diff --git a/crates/typst-library/src/foundations/array.rs b/crates/typst-library/src/foundations/array.rs index 18020e4cb..2a6bf8a2f 100644 --- a/crates/typst-library/src/foundations/array.rs +++ b/crates/typst-library/src/foundations/array.rs @@ -822,10 +822,14 @@ impl Array { key: Option, /// If given, uses this function to compare elements in the array. /// - /// This function should return an integer, whose sign is used to - /// determine the relative order of two given elements: Negative if the - /// first element is smaller, positive if the second element is smaller. - /// If `{0}` is returned, the order of the elements is not modified. + /// This function should return a boolean: `{true}` indicates that the + /// elements are in order, while `{false}` indicates that they should be + /// swapped. To keep the sort stable, if the two elements are equal, the + /// function should return `{true}`. + /// + /// If this function does not order the elements properly (e.g., by + /// returning `{false}` for both `{(x, y)}` and `{(y, x)}`), the + /// resulting array will be unspecified order. /// /// When used together with `key`, `by` will be passed the keys instead /// of the elements. @@ -838,45 +842,73 @@ impl Array { /// "length", /// ).sorted( /// key: s => s.len(), - /// by: (x, y) => y - x, + /// by: (l, r) => l >= r, /// ) /// ``` #[named] by: Option, ) -> SourceResult { - let mut result = Ok(()); - let mut vec = self.0; - let mut compare = |x: Value, y: Value| { - let mut key_of = |x: Value| match &key { - // NOTE: We are relying on `comemo`'s memoization of function - // evaluation to not excessively reevaluate the `key`. - Some(f) => f.call(engine, context, [x]), - None => Ok(x), - }; - let x = key_of(x)?; - let y = key_of(y)?; + let mut are_in_order = |mut x: Value, mut y: Value| { + if let Some(f) = &key { + x = f.call(engine, context, [x])?; + y = f.call(engine, context, [y])?; + } match &by { - Some(f) => Ok(match f.call(engine, context, [x, y])? { - Value::Int(x) => x.cmp(&0), + Some(f) => match f.call(engine, context, [x, y])? { + Value::Bool(b) => Ok(b), x => { - bail!(span, "expected integer from `by` function, got {}", x.ty()) + bail!(span, "expected boolean from `by` function, got {}", x.ty()) } - }), - None => ops::compare(&x, &y).at(span), + }, + None => { + // `x` and `y` are in order iff `y` is not strictly greater + // than `y`. + ops::compare(&x, &y).at(span).map(|o| o != Ordering::Greater) + } } }; + let mut vec = self.0.into_iter().enumerate().collect::>(); + let mut result = Ok(()); // We use `glidesort` instead of the standard library sorting algorithm // to prevent panics (see https://github.com/typst/typst/pull/5627). - glidesort::sort_by(vec.make_mut(), |a, b| { - // Until we get `try` blocks :) - compare(a.clone(), b.clone()).unwrap_or_else(|err| { - if result.is_ok() { - result = Err(err); + glidesort::sort_by(&mut vec, |(i, x), (j, y)| { + // Because we use booleans for the comparison function, in order to + // keep the sort stable, we need to compare in the right order. + if i < j { + // If `x` and `y` appear in this order in the original array, + // then we should change their order (i.e., return + // `Ordering::Greater`) iff `y` is strictly less than `x` (i.e., + // `compare(x, y)` returns `false`). Otherwise, we should keep + // them in the same order (i.e., return `Ordering::Less`). + match are_in_order(x.clone(), y.clone()) { + Ok(false) => Ordering::Greater, + Ok(true) => Ordering::Less, + Err(err) => { + if result.is_ok() { + result = Err(err); + } + Ordering::Equal + } } - Ordering::Equal - }) + } else { + // If `x` and `y` appear in the opposite order in the original + // array, then we should change their order (i.e., return + // `Ordering::Less`) iff `x` is strictly less than `y` (i.e., + // `compare(y, x)` returns `false`). Otherwise, we should keep + // them in the same order (i.e., return `Ordering::Less`). + match are_in_order(y.clone(), x.clone()) { + Ok(false) => Ordering::Less, + Ok(true) => Ordering::Greater, + Err(err) => { + if result.is_ok() { + result = Err(err); + } + Ordering::Equal + } + } + } }); - result.map(|_| vec.into()) + result.map(|_| vec.into_iter().map(|(_, x)| x).collect()) } /// Deduplicates all items in the array. diff --git a/tests/suite/foundations/array.typ b/tests/suite/foundations/array.typ index edcbe7e13..9c1c28636 100644 --- a/tests/suite/foundations/array.typ +++ b/tests/suite/foundations/array.typ @@ -355,11 +355,11 @@ #test((2, 1, 3, 10, 5, 8, 6, -7, 2).sorted(), (-7, 1, 2, 2, 3, 5, 6, 8, 10)) #test((2, 1, 3, -10, -5, 8, 6, -7, 2).sorted(key: x => x), (-10, -7, -5, 1, 2, 2, 3, 6, 8)) #test((2, 1, 3, -10, -5, 8, 6, -7, 2).sorted(key: x => x * x), (1, 2, 2, 3, -5, 6, -7, 8, -10)) -#test(("I", "the", "hi", "text").sorted(by: (x, y) => x.len() - y.len()), ("I", "hi", "the", "text")) -#test(("I", "the", "hi", "text").sorted(key: x => x.len(), by: (x, y) => y - x), ("text", "the", "hi", "I")) +#test(("I", "the", "hi", "text").sorted(by: (x, y) => x.len() < y.len()), ("I", "hi", "the", "text")) +#test(("I", "the", "hi", "text").sorted(key: x => x.len(), by: (x, y) => y < x), ("text", "the", "hi", "I")) --- array-sorted-invalid-by-function --- -// Error: 2-39 expected integer from `by` function, got string +// Error: 2-39 expected boolean from `by` function, got string #(1, 2, 3).sorted(by: (_, _) => "hmm") --- array-sorted-key-function-positional-1 --- @@ -451,7 +451,7 @@ #([Hi], [There]).sorted() --- array-sorted-uncomparable-lengths --- -// Error: 2-26 cannot compare 3em with 2pt +// Error: 2-26 cannot compare 2pt with 3em #(1pt, 2pt, 3em).sorted() --- array-sorted-key-function-positional-2 ---