Skip to content

Commit 725d887

Browse files
moulinstorokati44
authored andcommitted
avm2: replace Rust's sort by avmplus' for Array sorts; unblocks #17812
Replace the use of Rust's standard sort in `Array.sort` and `sortOn` by a port of avmplus' QuickSort algorithm. This avoid panics on Rust >=1.81 when `Array.sort` is called with a non-Ord comparison function, and will always produce the same result as Flash Player.
1 parent b32bb16 commit 725d887

File tree

2 files changed

+102
-16
lines changed

2 files changed

+102
-16
lines changed

core/src/avm2/globals/array.rs

+102-15
Original file line numberDiff line numberDiff line change
@@ -866,38 +866,125 @@ where
866866
C: FnMut(&mut Activation<'a, 'gc>, Value<'gc>, Value<'gc>) -> Result<Ordering, Error<'gc>>,
867867
{
868868
let mut unique_sort_satisfied = true;
869-
let mut error_signal = Ok(());
870869

871-
values.sort_unstable_by(|(_a_index, a), (_b_index, b)| {
870+
qsort(values, &mut |(_, a), (_, b)| {
872871
let unresolved_a = *a;
873872
let unresolved_b = *b;
874873

875874
if matches!(unresolved_a, Value::Undefined) && matches!(unresolved_b, Value::Undefined) {
876875
unique_sort_satisfied = false;
877-
return Ordering::Equal;
876+
return Ok(Ordering::Equal);
878877
} else if matches!(unresolved_a, Value::Undefined) {
879-
return Ordering::Greater;
878+
return Ok(Ordering::Greater);
880879
} else if matches!(unresolved_b, Value::Undefined) {
881-
return Ordering::Less;
880+
return Ok(Ordering::Less);
882881
}
883882

884-
match sort_func(activation, *a, *b) {
885-
Ok(Ordering::Equal) => {
883+
sort_func(activation, *a, *b).map(|cmp| {
884+
if cmp == Ordering::Equal {
886885
unique_sort_satisfied = false;
887886
Ordering::Equal
887+
} else if options.contains(SortOptions::DESCENDING) {
888+
cmp.reverse()
889+
} else {
890+
cmp
891+
}
892+
})
893+
})?;
894+
895+
Ok(!options.contains(SortOptions::UNIQUE_SORT) || unique_sort_satisfied)
896+
}
897+
898+
/// A port of the avmplus QuickSort implementation.
899+
///
900+
/// This differs from Rust's `slice::sort` in the following way:
901+
/// - the comparison function is faillible and can return an error, short-circuiting the sort;
902+
/// - the comparison function isn't required to define a valid total order: in such cases, the sort
903+
/// will permute the slice arbitrarily, but won't return an error.
904+
///
905+
/// Original code: https://github.com/adobe/avmplus/blob/master/core/ArrayClass.cpp#L637
906+
fn qsort<T, E>(
907+
slice: &mut [T],
908+
cmp: &mut impl FnMut(&T, &T) -> Result<Ordering, E>,
909+
) -> Result<(), E> {
910+
match slice {
911+
// Empty and one-element slices are trivially sorted.
912+
[] | [_] => return Ok(()),
913+
// Fast-path for two elements.
914+
[a, b] => {
915+
if cmp(a, b)?.is_gt() {
916+
swap(a, b);
917+
}
918+
return Ok(());
919+
}
920+
// Fast-path for three elements.
921+
[a, b, c] => {
922+
if cmp(a, b)?.is_gt() {
923+
swap(a, b);
924+
}
925+
if cmp(b, c)?.is_gt() {
926+
swap(b, c);
927+
if cmp(a, b)?.is_gt() {
928+
swap(a, b);
929+
}
930+
}
931+
return Ok(());
932+
}
933+
_ => (),
934+
}
935+
936+
// Select the middle element of the slice as the pivot, and put it at the beginning.
937+
slice.swap(0, slice.len() / 2);
938+
939+
// Order the elements (excluding the pivot) such that all elements lower
940+
// than the pivot come before all elements greater than the pivot.
941+
//
942+
// This is done by iterating from both ends, swapping greater elements with
943+
// lower ones along the way.
944+
let mut left = 0;
945+
let mut right = slice.len();
946+
loop {
947+
// Find an element greater than the pivot from the left.
948+
loop {
949+
left += 1;
950+
if left >= slice.len() || cmp(&slice[left], &slice[0])?.is_gt() {
951+
break;
888952
}
889-
Ok(v) if options.contains(SortOptions::DESCENDING) => v.reverse(),
890-
Ok(v) => v,
891-
Err(e) => {
892-
error_signal = Err(e);
893-
Ordering::Less
953+
}
954+
955+
// Find an element lower than the pivot from the right.
956+
loop {
957+
right -= 1;
958+
if right == 0 || cmp(&slice[right], &slice[0])?.is_lt() {
959+
break;
894960
}
895961
}
896-
});
897962

898-
error_signal?;
963+
// Nothing left to swap, we are done.
964+
if right < left {
965+
break;
966+
}
899967

900-
Ok(!options.contains(SortOptions::UNIQUE_SORT) || unique_sort_satisfied)
968+
// Otherwise, swap left and right, and keep going.
969+
slice.swap(left, right);
970+
}
971+
972+
// Put the pivot in its final position.
973+
slice.swap(0, right);
974+
975+
// The elements are now ordered as follows:
976+
// [..right]: lower partition
977+
// [right..left]: middle partition (equal to pivot)
978+
// [left..]: higher partition
979+
980+
// Recurse into both higher and lower partitions, with the smallest first.
981+
let (mut fst, mut snd) = slice.split_at_mut(left);
982+
fst = &mut fst[..right];
983+
if fst.len() >= snd.len() {
984+
swap(&mut fst, &mut snd);
985+
}
986+
qsort(fst, cmp)?;
987+
qsort(snd, cmp)
901988
}
902989

903990
pub fn compare_string_case_sensitive<'gc>(
Original file line numberDiff line numberDiff line change
@@ -1,2 +1 @@
11
num_frames = 1
2-
known_failure = true

0 commit comments

Comments
 (0)