`AllocVar` implementation for `Vec<T>` does not respect the setup mode

[weikengchen]

In alloc.rs, we have the following implementation for Vec<T>:

/// This blanket implementation just allocates variables in `Self`
/// element by element.
impl<I, F: Field, A: AllocVar<I, F>> AllocVar<[I], F> for Vec<A> {
    fn new_variable<T: Borrow<[I]>>(
        cs: impl Into<Namespace<F>>,
        f: impl FnOnce() -> Result<T, SynthesisError>,
        mode: AllocationMode,
    ) -> Result<Self, SynthesisError> {
        let ns = cs.into();
        let cs = ns.cs();
        let mut vec = Vec::new();
        for value in f()?.borrow().iter() {
            vec.push(A::new_variable(cs.clone(), || Ok(value), mode)?);
        }
        Ok(vec)
    }
}

However, in the setup mode, assignments are missing, and we expect all executions of f to be wrapped, layer by layer, inside the closures of new_variable. But the current implementation does not respect the setup mode. Instead, it runs f and unwraps its result in the setup mode.

There seems no easy solution. One may want to unwrap f() in each A::new_variable, but f is FnOnce, so calling f()? inside the closure of each value in the Vec does not work.

Now let me describe some thinking on how to solve it.

First, we can let new_variable break the "fourth wall" and check whether the ConstraintSystem being referenced is in the setup mode. This would allow it to treat the case in the setup mode differently.

However, this is not enough, because it also needs to learn the size of the Vec. If f() cannot be run and unwrapped, there is no way for it to learn the size.

So, this becomes a tangled situation. Going further, did we use this Vec<T> blanket implementation anywhere?

[Pratyush]

Hmm it seems that this needs to be fixed by also requiring the length to be passed in somehow. One way is to change [I] to be [I; n] for some common values of n. Another way is to have a wrapper struct

pub struct SliceWrapper<'a, I> { size: usize, slice: Option<&'a [I]> }

impl<...> SliceWrapper<...> {
	fn empty(size: usize) -> Self {
		Self {
			size,
			slice: None,
		}
	}

	fn new(slice: &[I]) -> Self {
		Self {
			size: slice.len(),
			slice: Some(slice),
		}
	}
}

This latter always has has a defined length, and so we should always be able to unwrap it inside AllocVar methods

[weikengchen]

For the solution [I; n], it requires a Rust feature that would be stable after Feb 11 next year.

The feature is min_const_generics.
rust-lang/rust#79135

Should we do the second solution or wait for min_const_generics?

The second one would be more convenient because you can still use Vec. The first solution, [I; n], may create an additional barrier for one who has a Vec on hand.

[Pratyush]

We can do both solutions, and for the array based one we can just implement it for common sizes (say, up to 32)

[Pratyush]

I think we should take the approach of the wrapper struct for now, and remove the impl for Vec<T>, because that just encourages errors.

[weikengchen]

Yes. Especially because the first one, [I; n], would make using Vec difficult.