Adjustments to Extend<(T, …)> for (ExtendT, …) implementations

[steffahn]

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<B>,
    EA: Extend<A>,
{
    #[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<T>>>, Vec<T>);
impl<T: Clone> Extend<T> 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], [2], [3], [4]]

1.85

[[2], [1], [4], [3]]

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.

[rustbot]

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[workingjubilee]

hm, can this come with a test for the new behavior in terms of ordering?

[steffahn]

It sure can! I’m not sure what’s the best location for the test.

[workingjubilee]

https://github.com/rust-lang/rust/tree/master/library/coretests I believe?

[joboet]

I've spent half an hour staring at the code and my frustration has increased to the point where I want to just rip this whole mess out¹. So even though this is a good improvement and all of these changes are completely sensible, I won't merge this PR. I really don't think maintaining the status quo – a completely unmaintainable macro hack – is a good idea.

So, how do you feel about the following:

1. The 1-tuple case should just be handled as a separate, non-macro implementation. We can move the documentation along with the #[doc(fake_variadic)] to this impl.
2. Since the specialisation trait is only used within extend, it doesn't need to have a separate name for each tuple length – one (macro-internal) name is fine.
3. This will reduce the number of parameters for each case, so we can get rid of the recursion and just list the arguments for each case in every invocation, e.g. (T, t, ExtendT, 0) (U, u, ExtendU, 1) etc.
4. With the recursion removed, there is no need to expand all cases from a single invocation – one invocation per tuple length is less complex.

Footnotes

1. That's definitely not your fault. ↩

[joboet]

I've opened #138799 as an alternative.