Adjustments to Extend<(T, …)> for (ExtendT, …) implementations by steffahn · Pull Request #137400 · rust-lang/rust (original) (raw)

This PR is still containing too many intermediate commits, but I thought keeping them makes it easier to review for now.

I can certainly squash everything – I can also try to rebase into a cleaner multiple-commits result (which might especially be useful to keep e.g. the renaming steps separate, and so on).

Before putting in such works, I’d like to first hear feedback that this change is generally desired/reasonable; and also I’m waiting for feedback on whether or squashing everything to a simple single commit is alright, too.

I know that reviewing the macro madness here in-depth will be hard, so here’s a short explanation of what changed:

minor:

• some naming changes internally and documented, e.g.
  • the whole ty_name / trait_name / TraitA style made the code hard to read
  • the names A and EA in the docs felt weird; this is switching to T and ExtendT, inspired on one hand by the (A, B) (ExtendA, ExtendB) naming before the generalization, and the T_n-style names for other fake_variadics
  • the param of fn extend is called iter now, following the parameter naming of the method in the trait itself
• the two #[allow(non_snake_case)] are instead changed from a style of
  move |(), (t, u)| {
  a.extend_one(t);
  b.extend_one(u);
  }
  to
  move |(), item| {
  a.extend_one(item.0);
  b.extend_one(item.1);
  }
  to avoid the need for a second set of variables. (same for the extend_one_unchecked-using variant)

relevant behavior changes

the generalized implementation had changed the behavior of e.g. iter::unzip and the impls for (A, B) pairs

this is perhaps most visible by just macro-expanding a section of the current master macro expansion

#[doc(hidden)] #[stable(feature = "extend_for_tuple", since = "1.56.0")] impl<B, A, EB, EA> Extend<(B, A)> for (EB, EA) where EB: Extend, EA: Extend, { #[doc = "…"] fn extend<T: IntoIterator<Item = (B, A)>>(&mut self, into_iter: T) { let (b, a) = self; let iter = into_iter.into_iter(); TraitB::extend(iter, b, a); } fn extend_one(&mut self, item: (B, A)) { self.1.extend_one(item.1); self.0.extend_one(item.0); } fn extend_reserve(&mut self, additional: usize) { self.1.extend_reserve(additional); self.0.extend_reserve(additional); } unsafe fn extend_one_unchecked(&mut self, item: (B, A)) { unsafe { self.1.extend_one_unchecked(item.1); self.0.extend_one_unchecked(item.0); } } }

note how self.1 comes before self.0

Here’s a test case showing this change between 1.84 and 1.85

use std::iter::zip; use std::cell::RefCell;

fn main() { let record = RefCell::new(vec![]); let l = TrackingExtender(&record, vec![]); let r = TrackingExtender(&record, vec![]); let mut p = ((l, r), ()); p.extend(zip([(1, 2), (3, 4)], [(), ()])); let record = record.into_inner(); println!("{record:?}"); }

struct TrackingExtender<'a, T>(&'a RefCell<Vec<Vec>>, Vec); impl<T: Clone> Extend for TrackingExtender<'_, T> { fn extend<I: IntoIterator<Item = T>>(&mut self, i: I) { let items = Vec::from_iter(i); self.0.borrow_mut().push(items.clone()); self.1.extend(items); } }

(compiler explorer)

1.84

1.85

I’m not sure if this is much of an actual issue, because it might not break any guaranteed behavior/properties of the relevant traits and types. So I’m not sure if this deserves to become a test-case in the library and/or ui test suite…

But it still seems an unnecessary change, and the less natural order of side-effects.

This PR fixes that, and returns to the 1.84 behavior.

The change happened in the first place, because it’s harder to generate macro invocations of the form

m!(A)
m!(A B)
m!(A B C)
m!(A B C D)

than of the form

m!(A)
m!(B A)
m!(C B A)
m!(D C B A)

This PR restructures the macro to a partially tt-munching style that manages to un-reverse the list partial macro inputs. I.e. instead of invocations on trailing sublists of a reversed (… D C B A)-style macro input, it instead now generates invocations on leading sublists of a non-reversed (A B C D …) input.

the new implementations for (T,) used a loop of extend_one unnecessarily

I would assume that a simple approach of

fn extend<I: IntoIterator<Item = (T,)>>(self: &mut (ExtendT,), iter: I) { self.0.extend(iter.into_iter().map(|(a,)| a)); }

is more performant, as it can make use of the normal SpecExtend specialization chain, incorporating e.g. preservation of capacity in Vec, and such things.

But I haven’t tested (yet) if this has any performance downsides.

Not to say that (T,) is a particularly common use-case; it isn’t; though I do suppose there could be future users anyway that end up using this Extend impl by virtue of being macro-users or users of code-generators themselves.