From d5e6b6f8cc404ee5d3c1a08c6d2ef63fa96862fa Mon Sep 17 00:00:00 2001 From: joboet Date: Fri, 21 Mar 2025 19:42:17 +0100 Subject: [PATCH 1/2] core: simplify `Extend` for tuples This is an alternative to #137400. The current macro is incredibly complicated and introduces subtle bugs like calling the `extend_one` of the individual collections in backwards order. This PR drastically simplifies the macro by removing recursion and moving the specialization out of the macro. It also fixes the ordering issue described above (I've stolen the test of the new behaviour from #137400). Additionally, the 1-tuple is now special-cased to allow taking advantage of the well-optimized `Extend` implementations of the individual collection. --- library/core/src/iter/traits/collect.rs | 432 +++++++++++++----------- 1 file changed, 236 insertions(+), 196 deletions(-) diff --git a/library/core/src/iter/traits/collect.rs b/library/core/src/iter/traits/collect.rs index 97bb21c8a36e8..b4ab81d15a330 100644 --- a/library/core/src/iter/traits/collect.rs +++ b/library/core/src/iter/traits/collect.rs @@ -459,234 +459,274 @@ impl Extend<()> for () { fn extend_one(&mut self, _item: ()) {} } -macro_rules! spec_tuple_impl { - ( - ( - $ty_name:ident, $var_name:ident, $extend_ty_name: ident, - $trait_name:ident, $default_fn_name:ident, $cnt:tt - ), - ) => { - spec_tuple_impl!( - $trait_name, - $default_fn_name, - #[doc(fake_variadic)] - #[doc = "This trait is implemented for tuples up to twelve items long. The `impl`s for \ - 1- and 3- through 12-ary tuples were stabilized after 2-tuples, in \ - 1.85.0."] - => ($ty_name, $var_name, $extend_ty_name, $cnt), - ); - }; - ( - ( - $ty_name:ident, $var_name:ident, $extend_ty_name: ident, - $trait_name:ident, $default_fn_name:ident, $cnt:tt - ), - $( - ( - $ty_names:ident, $var_names:ident, $extend_ty_names:ident, - $trait_names:ident, $default_fn_names:ident, $cnts:tt - ), - )* - ) => { - spec_tuple_impl!( - $( - ( - $ty_names, $var_names, $extend_ty_names, - $trait_names, $default_fn_names, $cnts - ), - )* - ); - spec_tuple_impl!( - $trait_name, - $default_fn_name, - #[doc(hidden)] - => ( - $ty_name, $var_name, $extend_ty_name, $cnt - ), - $( - ( - $ty_names, $var_names, $extend_ty_names, $cnts - ), - )* - ); - }; - ( - $trait_name:ident, $default_fn_name:ident, #[$meta:meta] - $(#[$doctext:meta])? => $( - ( - $ty_names:ident, $var_names:ident, $extend_ty_names:ident, $cnts:tt - ), - )* - ) => { - #[$meta] - $(#[$doctext])? - #[stable(feature = "extend_for_tuple", since = "1.56.0")] - impl<$($ty_names,)* $($extend_ty_names,)*> Extend<($($ty_names,)*)> for ($($extend_ty_names,)*) - where - $($extend_ty_names: Extend<$ty_names>,)* - { - /// Allows to `extend` a tuple of collections that also implement `Extend`. - /// - /// See also: [`Iterator::unzip`] - /// - /// # Examples - /// ``` - /// // Example given for a 2-tuple, but 1- through 12-tuples are supported - /// let mut tuple = (vec![0], vec![1]); - /// tuple.extend([(2, 3), (4, 5), (6, 7)]); - /// assert_eq!(tuple.0, [0, 2, 4, 6]); - /// assert_eq!(tuple.1, [1, 3, 5, 7]); - /// - /// // also allows for arbitrarily nested tuples as elements - /// let mut nested_tuple = (vec![1], (vec![2], vec![3])); - /// nested_tuple.extend([(4, (5, 6)), (7, (8, 9))]); - /// - /// let (a, (b, c)) = nested_tuple; - /// assert_eq!(a, [1, 4, 7]); - /// assert_eq!(b, [2, 5, 8]); - /// assert_eq!(c, [3, 6, 9]); - /// ``` - fn extend>(&mut self, into_iter: T) { - let ($($var_names,)*) = self; - let iter = into_iter.into_iter(); - $trait_name::extend(iter, $($var_names,)*); - } +/// This trait is implemented for tuples up to twelve items long. The `impl`s for +/// 1- and 3- through 12-ary tuples were stabilized after 2-tuples, in 1.85.0. +#[doc(fake_variadic)] // the other implementations are below. +#[stable(feature = "extend_for_tuple", since = "1.56.0")] +impl Extend<(T,)> for (ExtendT,) +where + ExtendT: Extend, +{ + /// Allows to `extend` a tuple of collections that also implement `Extend`. + /// + /// See also: [`Iterator::unzip`] + /// + /// # Examples + /// ``` + /// // Example given for a 2-tuple, but 1- through 12-tuples are supported + /// let mut tuple = (vec![0], vec![1]); + /// tuple.extend([(2, 3), (4, 5), (6, 7)]); + /// assert_eq!(tuple.0, [0, 2, 4, 6]); + /// assert_eq!(tuple.1, [1, 3, 5, 7]); + /// + /// // also allows for arbitrarily nested tuples as elements + /// let mut nested_tuple = (vec![1], (vec![2], vec![3])); + /// nested_tuple.extend([(4, (5, 6)), (7, (8, 9))]); + /// + /// let (a, (b, c)) = nested_tuple; + /// assert_eq!(a, [1, 4, 7]); + /// assert_eq!(b, [2, 5, 8]); + /// assert_eq!(c, [3, 6, 9]); + /// ``` + fn extend>(&mut self, iter: I) { + self.0.extend(iter.into_iter().map(|t| t.0)); + } - fn extend_one(&mut self, item: ($($ty_names,)*)) { - $(self.$cnts.extend_one(item.$cnts);)* - } + fn extend_one(&mut self, item: (T,)) { + self.0.extend_one(item.0) + } - fn extend_reserve(&mut self, additional: usize) { - $(self.$cnts.extend_reserve(additional);)* - } + fn extend_reserve(&mut self, additional: usize) { + self.0.extend_reserve(additional) + } - unsafe fn extend_one_unchecked(&mut self, item: ($($ty_names,)*)) { - // SAFETY: Those are our safety preconditions, and we correctly forward `extend_reserve`. - unsafe { - $(self.$cnts.extend_one_unchecked(item.$cnts);)* - } - } - } + unsafe fn extend_one_unchecked(&mut self, item: (T,)) { + // SAFETY: the caller guarantees all preconditions. + unsafe { self.0.extend_one_unchecked(item.0) } + } +} - trait $trait_name<$($ty_names),*> { - fn extend(self, $($var_names: &mut $ty_names,)*); - } +/// This implementation turns an iterator of tuples into a tuple of types which implement +/// [`Default`] and [`Extend`]. +/// +/// This is similar to [`Iterator::unzip`], but is also composable with other [`FromIterator`] +/// implementations: +/// +/// ```rust +/// # fn main() -> Result<(), core::num::ParseIntError> { +/// let string = "1,2,123,4"; +/// +/// // Example given for a 2-tuple, but 1- through 12-tuples are supported +/// let (numbers, lengths): (Vec<_>, Vec<_>) = string +/// .split(',') +/// .map(|s| s.parse().map(|n: u32| (n, s.len()))) +/// .collect::>()?; +/// +/// assert_eq!(numbers, [1, 2, 123, 4]); +/// assert_eq!(lengths, [1, 1, 3, 1]); +/// # Ok(()) } +/// ``` +#[doc(fake_variadic)] // the other implementations are below. +#[stable(feature = "from_iterator_for_tuple", since = "1.79.0")] +impl FromIterator<(T,)> for (ExtendT,) +where + ExtendT: Default + Extend, +{ + fn from_iter>(iter: Iter) -> Self { + let mut res = ExtendT::default(); + res.extend(iter.into_iter().map(|t| t.0)); + (res,) + } +} - fn $default_fn_name<$($ty_names,)* $($extend_ty_names,)*>( - iter: impl Iterator, - $($var_names: &mut $extend_ty_names,)* - ) where - $($extend_ty_names: Extend<$ty_names>,)* - { - fn extend<'a, $($ty_names,)*>( - $($var_names: &'a mut impl Extend<$ty_names>,)* - ) -> impl FnMut((), ($($ty_names,)*)) + 'a { - #[allow(non_snake_case)] - move |(), ($($extend_ty_names,)*)| { - $($var_names.extend_one($extend_ty_names);)* - } - } +/// An implementation of [`extend`](Extend::extend) that calls `extend_one` or +/// `extend_one_unchecked` for each element of the iterator. +fn default_extend(collection: &mut ExtendT, iter: I) +where + ExtendT: Extend, + I: IntoIterator, +{ + // Specialize on `TrustedLen` and call `extend_one_unchecked` where + // applicable. + trait SpecExtend { + fn extend(&mut self, iter: I); + } + // Extracting these to separate functions avoid monomorphising the closures + // for every iterator type. + fn extender(collection: &mut ExtendT) -> impl FnMut(T) + use<'_, ExtendT, T> + where + ExtendT: Extend, + { + move |item| collection.extend_one(item) + } + + unsafe fn unchecked_extender( + collection: &mut ExtendT, + ) -> impl FnMut(T) + use<'_, ExtendT, T> + where + ExtendT: Extend, + { + // SAFETY: we make sure that there is enough space at the callsite of + // this function. + move |item| unsafe { collection.extend_one_unchecked(item) } + } + + impl SpecExtend for ExtendT + where + ExtendT: Extend, + I: Iterator, + { + default fn extend(&mut self, iter: I) { let (lower_bound, _) = iter.size_hint(); if lower_bound > 0 { - $($var_names.extend_reserve(lower_bound);)* + self.extend_reserve(lower_bound); } - iter.fold((), extend($($var_names,)*)); + iter.for_each(extender(self)) } + } - impl<$($ty_names,)* $($extend_ty_names,)* Iter> $trait_name<$($extend_ty_names),*> for Iter - where - $($extend_ty_names: Extend<$ty_names>,)* - Iter: Iterator, - { - default fn extend(self, $($var_names: &mut $extend_ty_names),*) { - $default_fn_name(self, $($var_names),*); + impl SpecExtend for ExtendT + where + ExtendT: Extend, + I: TrustedLen, + { + fn extend(&mut self, iter: I) { + let (lower_bound, upper_bound) = iter.size_hint(); + if lower_bound > 0 { + self.extend_reserve(lower_bound); + } + + if upper_bound.is_none() { + // We cannot reserve more than `usize::MAX` items, and this is likely to go out of memory anyway. + iter.for_each(extender(self)) + } else { + // SAFETY: We reserve enough space for the `size_hint`, and the iterator is + // `TrustedLen` so its `size_hint` is exact. + iter.for_each(unsafe { unchecked_extender(self) }) } } + } - impl<$($ty_names,)* $($extend_ty_names,)* Iter> $trait_name<$($extend_ty_names),*> for Iter + SpecExtend::extend(collection, iter.into_iter()); +} + +// Implements `Extend` and `FromIterator` for tuples with length larger than one. +macro_rules! impl_extend_tuple { + ($(($ty:tt, $extend_ty:tt, $index:tt)),+) => { + #[doc(hidden)] + #[stable(feature = "extend_for_tuple", since = "1.56.0")] + impl<$($ty,)+ $($extend_ty,)+> Extend<($($ty,)+)> for ($($extend_ty,)+) where - $($extend_ty_names: Extend<$ty_names>,)* - Iter: TrustedLen, + $($extend_ty: Extend<$ty>,)+ { - fn extend(self, $($var_names: &mut $extend_ty_names,)*) { - fn extend<'a, $($ty_names,)*>( - $($var_names: &'a mut impl Extend<$ty_names>,)* - ) -> impl FnMut((), ($($ty_names,)*)) + 'a { - #[allow(non_snake_case)] - // SAFETY: We reserve enough space for the `size_hint`, and the iterator is - // `TrustedLen` so its `size_hint` is exact. - move |(), ($($extend_ty_names,)*)| unsafe { - $($var_names.extend_one_unchecked($extend_ty_names);)* - } - } + fn extend>(&mut self, iter: T) { + default_extend(self, iter) + } - let (lower_bound, upper_bound) = self.size_hint(); + fn extend_one(&mut self, item: ($($ty,)+)) { + $(self.$index.extend_one(item.$index);)+ + } - if upper_bound.is_none() { - // We cannot reserve more than `usize::MAX` items, and this is likely to go out of memory anyway. - $default_fn_name(self, $($var_names,)*); - return; - } + fn extend_reserve(&mut self, additional: usize) { + $(self.$index.extend_reserve(additional);)+ + } - if lower_bound > 0 { - $($var_names.extend_reserve(lower_bound);)* + unsafe fn extend_one_unchecked(&mut self, item: ($($ty,)+)) { + // SAFETY: Those are our safety preconditions, and we correctly forward `extend_reserve`. + unsafe { + $(self.$index.extend_one_unchecked(item.$index);)+ } - - self.fold((), extend($($var_names,)*)); } } - /// This implementation turns an iterator of tuples into a tuple of types which implement - /// [`Default`] and [`Extend`]. - /// - /// This is similar to [`Iterator::unzip`], but is also composable with other [`FromIterator`] - /// implementations: - /// - /// ```rust - /// # fn main() -> Result<(), core::num::ParseIntError> { - /// let string = "1,2,123,4"; - /// - /// // Example given for a 2-tuple, but 1- through 12-tuples are supported - /// let (numbers, lengths): (Vec<_>, Vec<_>) = string - /// .split(',') - /// .map(|s| s.parse().map(|n: u32| (n, s.len()))) - /// .collect::>()?; - /// - /// assert_eq!(numbers, [1, 2, 123, 4]); - /// assert_eq!(lengths, [1, 1, 3, 1]); - /// # Ok(()) } - /// ``` - #[$meta] - $(#[$doctext])? + #[doc(hidden)] #[stable(feature = "from_iterator_for_tuple", since = "1.79.0")] - impl<$($ty_names,)* $($extend_ty_names,)*> FromIterator<($($extend_ty_names,)*)> for ($($ty_names,)*) + impl<$($ty,)+ $($extend_ty,)+> FromIterator<($($ty,)+)> for ($($extend_ty,)+) where - $($ty_names: Default + Extend<$extend_ty_names>,)* + $($extend_ty: Default + Extend<$ty>,)+ { - fn from_iter>(iter: Iter) -> Self { - let mut res = <($($ty_names,)*)>::default(); + fn from_iter>(iter: Iter) -> Self { + let mut res = Self::default(); res.extend(iter); - res } } - }; } -spec_tuple_impl!( - (L, l, EL, TraitL, default_extend_tuple_l, 11), - (K, k, EK, TraitK, default_extend_tuple_k, 10), - (J, j, EJ, TraitJ, default_extend_tuple_j, 9), - (I, i, EI, TraitI, default_extend_tuple_i, 8), - (H, h, EH, TraitH, default_extend_tuple_h, 7), - (G, g, EG, TraitG, default_extend_tuple_g, 6), - (F, f, EF, TraitF, default_extend_tuple_f, 5), - (E, e, EE, TraitE, default_extend_tuple_e, 4), - (D, d, ED, TraitD, default_extend_tuple_d, 3), - (C, c, EC, TraitC, default_extend_tuple_c, 2), - (B, b, EB, TraitB, default_extend_tuple_b, 1), - (A, a, EA, TraitA, default_extend_tuple_a, 0), +impl_extend_tuple!((A, ExA, 0), (B, ExB, 1)); +impl_extend_tuple!((A, ExA, 0), (B, ExB, 1), (C, ExC, 2)); +impl_extend_tuple!((A, ExA, 0), (B, ExB, 1), (C, ExC, 2), (D, ExD, 3)); +impl_extend_tuple!((A, ExA, 0), (B, ExB, 1), (C, ExC, 2), (D, ExD, 3), (E, ExE, 4)); +impl_extend_tuple!((A, ExA, 0), (B, ExB, 1), (C, ExC, 2), (D, ExD, 3), (E, ExE, 4), (F, ExF, 5)); +impl_extend_tuple!( + (A, ExA, 0), + (B, ExB, 1), + (C, ExC, 2), + (D, ExD, 3), + (E, ExE, 4), + (F, ExF, 5), + (G, ExG, 6) +); +impl_extend_tuple!( + (A, ExA, 0), + (B, ExB, 1), + (C, ExC, 2), + (D, ExD, 3), + (E, ExE, 4), + (F, ExF, 5), + (G, ExG, 6), + (H, ExH, 7) +); +impl_extend_tuple!( + (A, ExA, 0), + (B, ExB, 1), + (C, ExC, 2), + (D, ExD, 3), + (E, ExE, 4), + (F, ExF, 5), + (G, ExG, 6), + (H, ExH, 7), + (I, ExI, 8) +); +impl_extend_tuple!( + (A, ExA, 0), + (B, ExB, 1), + (C, ExC, 2), + (D, ExD, 3), + (E, ExE, 4), + (F, ExF, 5), + (G, ExG, 6), + (H, ExH, 7), + (I, ExI, 8), + (J, ExJ, 9) +); +impl_extend_tuple!( + (A, ExA, 0), + (B, ExB, 1), + (C, ExC, 2), + (D, ExD, 3), + (E, ExE, 4), + (F, ExF, 5), + (G, ExG, 6), + (H, ExH, 7), + (I, ExI, 8), + (J, ExJ, 9), + (K, ExK, 10) +); +impl_extend_tuple!( + (A, ExA, 0), + (B, ExB, 1), + (C, ExC, 2), + (D, ExD, 3), + (E, ExE, 4), + (F, ExF, 5), + (G, ExG, 6), + (H, ExH, 7), + (I, ExI, 8), + (J, ExJ, 9), + (K, ExK, 10), + (L, ExL, 11) ); From af66ece8221eb0a20fa17407182f6d01739bf8c9 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Sat, 22 Feb 2025 01:09:51 +0100 Subject: [PATCH 2/2] Add tests for `Extend<(T, U)> for (ExtendT, ExtendU)` ordering of side-effects to `coretest`. --- .../coretests/tests/iter/traits/iterator.rs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/library/coretests/tests/iter/traits/iterator.rs b/library/coretests/tests/iter/traits/iterator.rs index e31d2e15b6d7e..5ef1f797ae55d 100644 --- a/library/coretests/tests/iter/traits/iterator.rs +++ b/library/coretests/tests/iter/traits/iterator.rs @@ -1,3 +1,5 @@ +use core::cell::RefCell; +use core::iter::zip; use core::num::NonZero; /// A wrapper struct that implements `Eq` and `Ord` based on the wrapped @@ -642,6 +644,26 @@ fn test_collect_for_tuples() { assert!(e.2 == d); } +#[test] +fn test_extend_for_tuple_side_effects_order() { + struct TrackingExtender<'a, T>(&'static str, &'a RefCell)>>, Vec); + impl Extend for TrackingExtender<'_, T> { + fn extend>(&mut self, i: I) { + let items = Vec::from_iter(i); + self.1.borrow_mut().push((self.0, items.clone())); + self.2.extend(items); + } + } + + let effects = RefCell::new(vec![]); + let l = TrackingExtender("l", &effects, vec![]); + let r = TrackingExtender("r", &effects, vec![]); + let mut p = ((l, r), ()); + p.extend(zip([(1, 2), (3, 4)], [(), ()])); + let effects = effects.into_inner(); + assert_eq!(effects, [("l", vec![1]), ("r", vec![2]), ("l", vec![3]), ("r", vec![4])]); +} + // just tests by whether or not this compiles fn _empty_impl_all_auto_traits() { use std::panic::{RefUnwindSafe, UnwindSafe};