Confusion with Xorcism exercise

[snoyberg]

A coworker pointed me to the Xorcism exercise, and pointed out some difficulty with it. We both ultimately worked through it, and the exercise overall was a great stress test of the interaction of lifetimes and impl Trait. However, IMO, in its current incarnation it deserves to be bumped from Medium to Hard. And even so, I would recommend providing some additional guidance in the instructions, and perhaps some simplifications.

Some parts of the exercise, in particular the ExactSizeIterator, seem to be unnecessary. My solution ultimately did not leverage that trait, and it's unclear why it would be needed from the test suite.

use std::borrow::Borrow;

#[derive(Clone)]
pub struct Xorcism<'a> {
    key: &'a [u8],
    index: usize,
}

fn next_key(key: &[u8], index: &mut usize) -> u8 {
    let b = key[*index];
    *index += 1;
    if *index >= key.len() {
        *index = 0;
    }
    b
}

impl<'a> Xorcism<'a> {
    pub fn new<Key: AsRef<[u8]> + ?Sized>(key: &'a Key) -> Xorcism<'a> {
        Xorcism {
            key: key.as_ref(),
            index: 0,
        }
    }

    pub fn munge_in_place(&mut self, data: &mut [u8]) {
        for b in data {
            *b ^= next_key(self.key, &mut self.index);
        }
    }

    pub fn munge<'b, Data>(&'b mut self, data: Data) -> impl Iterator<Item=u8> + 'b
    where
        Data: IntoIterator,
        Data::Item: Borrow<u8>,
        Data::IntoIter: 'b,
    {
        let key = &self.key;
        let index = &mut self.index;
        data.into_iter().map(move |b| *b.borrow() ^ next_key(key, index))
    }
}

I would also recommend introducing a few simple test cases of munge_in_place before munge, so that someone implementing it can get a feel for how munging is supposed to work before diving into the more complicated issues with streaming and lifetimes.

Finally, I would recommend making the impl Trait approach a bonus point, and instead recommend starting with a concrete struct. I looked at a number of the existing solutions on Exercism, and it seems like that is the most intuitive place to start. It also avoids the major complexity around lifetimes and impl Trait.

I'd be happy to open a PR to make some of these modifications, but wanted to start with a discussion issue first.

Note that I've also opened rust-lang/rust#80518 for some surprising compiler behavior around this exercise.

[coriolinus]

Great feedback! Xorcism is the newest exercise in the Rust track, and I'm very happy to have experience reports like this to improve it.

I'm going to restate your points, with my responses appended. This is to ensure I haven't misunderstood or overlooked anything. If I have, then it is unintentional; please feel free to update me.

• Should bump difficulty to Hard. Sure, that's reasonable. IIRC it's a 7 (of 10) right now in the track's config.json file; we have a track convention that we only use the difficulties 1, 4, 7, and 10, so that should be a simple fix to change it to 10.

• Could use additional guidance in the instructions. No argument! Would want to see specific suggestions.

• ExactSizeIterator is unnecessary. That's an artifact of how I implemented the example (example.rs). I'd be very glad if it could be removed from both the example and the stub.

• Some test cases of munge_in_place would be helpful. I'm always glad to see new tests added to these exercises!

• fn munge should return a concrete struct which implements Iterator. I'm not sure this is helpful. It's already possible for students to return a concrete struct implementing Iterator, without even changing the function's signature. Even if they do change the signature to return that concrete type, all tests should still pass.

  However, requiring some concrete struct would prevent a more iterator-centric approach which involves unnameable types. For example, from the example:

  rust/exercises/xorcism/example.rs

  Lines 102 to 112 in 3543b7e

  	pub fn munge<'b, Data, B>(&mut self, data: Data) -> impl 'b + MungeOutput
  	where
  	'a: 'b,
  	Data: IntoIterator<Item = B>,
  	<Data as IntoIterator>::IntoIter: 'b + ExactSizeIterator,
  	B: Borrow<u8>,
  	{
  	let data = data.into_iter();
  	let pos = self.incr_pos(data.len());
  	data.zip(self.key(pos)).map(|(d, k)| d.borrow() ^ k)
  	}

  I think there's value in permitting that approach.

Overall, I'd be positively inclined toward a PR implementing your suggestions with the exception of defaulting munge to return a concrete struct instead of an impl Iterator. I have no objection to adding a hint in the README that, if students are having trouble implementing munge, they can return some concrete struct Munged where Munged: Iterator.

[snoyberg]

You expressed each of my points perfectly, thank you! I also agree with your final point: I think a comment in the README.md about the possibility of using a concrete type should be sufficient.

I'll put together a PR based on this discussion, thank you for the quick response!