Mutable reference (within a struct) doesn't mutate (within a nested function call)

[iAmMichaelConnor]

Apologies for not making this one potato-themed.

I encountered a bug when migrating all of our Noir Contract examples to use mutable references.
All of the contracts compile (hooray!), but the typescript tests were throwing lots of errors when simulating.

I've tried to provide a minimal example. It's a bit messy, but it's neater than the 6 files that the original error came from!

The problem highlighted in this issue is that if you pass a value to a function as a mutable reference*, the value does get mutated.

*It's a bit more complicated than that... it's actually if you pass a value (which is a member of some struct) to a function by mutable reference, and then to another function by mutable reference, and then try to mutate the value, the value does not get mutated.

use dep::std;

// ---

struct BoundedVec<T, MaxLen> {
    storage: [T; MaxLen],
    len: comptime Field,
}

impl<T, MaxLen> BoundedVec<T, MaxLen> {
    fn new(initial_value: T) -> Self {
        BoundedVec { storage: [initial_value; MaxLen], len: 0 }
    }

    fn push(&mut self, elem: T) {
        assert(self.len as u64 < MaxLen as u64);

        std::println(2);
        std::println(self.len);
        std::println(self.storage);

        self.storage[self.len] = elem;
        self.len += 1;

        std::println(3);
        std::println(self.len);
        std::println(self.storage); // These logs are as expected
    }
}

// ---

fn indirect(vec_wrapper: &mut VecWrapper, value: Field) {
    (*vec_wrapper).vec.push(value);
    // I also tried with:
    // vec_wrapper.vec.push(value);
}

struct VecWrapper {
    vec: BoundedVec<Field, 4>,
}

impl VecWrapper {
    fn new(initial_value: Field) -> VecWrapper {
        VecWrapper {
            vec: BoundedVec::new(initial_value),
        }
    }
}

// ---

fn main(value: Field, initial_value: Field) -> pub [Field; 4] {

    let mut vec_wrapper = VecWrapper::new(initial_value);

    std::println(1);
    std::println(vec_wrapper.vec.len);
    std::println(vec_wrapper.vec.storage);

    indirect(&mut vec_wrapper, value);

    std::println(4);
    std::println(vec_wrapper.vec.len);
    std::println(vec_wrapper.vec.storage); // These logs don't reflect the 'push' that just happened!!!
                                // The vec is unchanged!

    vec_wrapper.vec.storage
}

// ---

#[test]
fn test_main() {
    let storage = main(5, 0);
    assert(storage == [5, 0, 0, 0]); // FAILS. Actually returns [0, 0, 0, 0]
}

Compiling from source. Branch master.

• As an aside: I'm surprised both attempts (*vec_wrapper).vec.push(value); and vec_wrapper.vec.push(value); are both accepted by the compiler. Isn't the first one the correct one? And the second one should fail because we haven't dereferenced vec_wrapper? Would it be correct to say vec_wrapper has no member vec; and only *vec_wrapper has a member vec? Or is that wrong?

[iAmMichaelConnor]

If you modify the test assertion to pass (assert(storage == [0, 0, 0, 0]);, you get the following println statements, which help illustrate what's happening:

Testing test_main...
0x01
0x
[0x, 0x, 0x, 0x]

0x02
0x
[0x, 0x, 0x, 0x]

0x03
0x01
[0x05, 0x, 0x, 0x]

0x04
0x
[0x, 0x, 0x, 0x] // Uh oh! It hasn't mutated the member of the original struct!

[jfecher]

(*vec_wrapper).vec.push(value);

The intended behavior of this is actually to not mutate the original. By dereferencing here you're copying the entire vec_wrapper into a new one.

The intended solution is the commented out line // vec_wrapper.vec.push(value); to mutate the actual reference passed in as a parameter. If this doesn't mutate it as you say then it is indeed a bug.

As an aside: I'm surprised both attempts (*vec_wrapper).vec.push(value); and vec_wrapper.vec.push(value); are both accepted by the compiler. Isn't the first one the correct one? And the second one should fail because we haven't dereferenced vec_wrapper? Would it be correct to say vec_wrapper has no member vec; and only *vec_wrapper has a member vec? Or is that wrong?

It is wrong - following rust's semantics you don't need a separate pointer-dereference operator like -> in C or C++. When you access a member with . it automatically dereferences if the lhs is a reference. Also like Rust, references cannot be null in Noir so there's no danger of dereferencing an invalid reference. For method calls requiring a mutable object instead of dereferencing it should perform essentially a field offset instead. This isn't quite how it works in Noir since Noir doesn't have reference offsets, but there must be something similar going wrong here.

[iAmMichaelConnor]

The intended solution is the commented out line // vec_wrapper.vec.push(value); to mutate the actual reference passed in as a parameter. If this doesn't mutate it as you say then it is indeed a bug.

I just double-checked and using vec_wrapper.vec.push(value); results in the same unexpected output, so it's not mutating.

Thanks for all of the explanation, that's really helpful.

[jfecher]

@iAmMichaelConnor @kevaundray looks like this is a repeat of an older issue: #1887. I'm closing this one since the example is larger.

[jfecher]

Updating here for tracking purposes: #1887 (and thus this issue as well) has been fixed by #2087