Better error handling for AssignedCell value queries

[CPerezz]

I've been working with/reviewing the AssignedCell API. And there's one thing that it's a bit annoying.
When you need the value of the Cell as V, you can call AssignedCell::value, and that's great.

The problem is that it returns an Option<V> which then requires to do something like .ok_or_else(ERR_HANDLING)?;

After discussing with @therealyingtong different aproaches, like MaybeUninit etc.. I suggested one that seemed to be Ok:

Maybe value returning an Error where internally you handle the ok_or_else and so you can unify the error type returned.
Something like Error::CellNotAssigned.
So that when calling value you either get this or the value directly and match over the err as a user.

After that, the suggested err variant was Error::Synthesis.

I think that would help in cases where we need to do some extra stuff with the value and we don't want to end up with a bunch of ok_or_else or unwraps/expects in the code.

[CPerezz]

There's only one part which I'm unsure which is the desired behavior.

This change means that APIs like Var which currently also work with Option<T> would benefit also from switching into Result<T,Error>. See: https://github.com/zcash/halo2/blob/main/halo2_gadgets/src/utilities.rs#L16-L22

But I'm not sure if this is the desired behavior for you.

[CPerezz]

Something that would help a lot would be to be able to make usage of copied/cloned_result. See: rust-lang/rust#92483.

With this the API for Var for example would be as simple as:

impl<F: FieldExt> Var<F> for AssignedCell<F, F> {
    fn cell(&self) -> Cell {
        self.cell()
    }

    fn value(&self) -> Result<F, Error> {
        self.value().cloned()
    }
}

While now needs quite some work as:

impl<F: FieldExt> Var<F> for AssignedCell<F, F> {
    fn cell(&self) -> Cell {
        self.cell()
    }

    fn value(&self) -> Result<F, Error> {
        self.value().and_then(|val| Ok(val.to_owned()))
    }
}

Was stabilized not so long ago and marked for release 1.59.
Just pointing this out in case you think is better to wait until this hits stable (and only in case you're ok with updating the MSRV).

[str4d]

Returning Result<V, Error> instead of Option<V> makes things more complicated, because then when synthesizing witnesses you end up with much more verbose code, because you can't just leverage Option::zip to merge separate witness values together. Instead of:

let a = foo.value();
let b = bar.value();
let c = a.zip(b).map(|(a, b)| a + b);

you'd need

let a = foo.value();
let b = bar.value();
let c = a.and_then(|a| b.map(|b| a + b));

which is both more complex (nested closures, which doesn't look too bad in this contrived example but would quickly get out of hand), and forces the user to make a judgement call about which value's error is more important.

It is also a non-starter because this is a type error: during witness synthesis we don't want to expose an Error, because we aren't necessarily in an error state yet. That is, having AssignedCell::value return Result makes it far more likely that someone writes something like:

let a = foo.value()?;
let b = bar.value()?;
let c = a + b;

and breaks their code, because we can't raise those errors except inside the value closure of a Region::assign_*.

I think what we instead need to do (I've described this to people at various times, but appear to have not written it down yet) is introduce a custom Value type that handles all this for you. The API I want is:

let a = foo.value();
let b = bar.value();
let c = a + b;

i.e. it should Just Work for common operations, while providing a trapdoor that exposes the underlying Option<V> nature (probably by just providing equivalent methods like Option::zip).

[str4d]

I implemented my proposed Value type in #598.

[CPerezz]

Sorry @str4d . I've been extremely bussy lately. Having a kid soon, so can't really invest the time I wanted on this.

Will take a look to the PR! Thanks a lot for working on this! 😄