feat: Add Option<T> to noir stdlib

[jfecher]

Description

Problem*

Relates to #988, although this PR does not add enums in general, nor does it add match to Noir. Only the Option type is provided along with some helper methods.

Summary*

Documentation

• This PR requires documentation updates when merged.

  • I will submit a noir-lang/docs PR.
  • I will request for and support Dev Rel's help in documenting this PR.
    • We'll need to document the Option type along with each method on it. Each method I've added should match exactly the method of the same name on https://doc.rust-lang.org/std/option/enum.Option.html.
    • Option's fields (value and _is_some) do not need to be documented, they can be considered private.

Additional Context

PR Checklist*

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt on default settings.

[jfecher]

I added a test for option but unfortunately it is causing a panic related to DecisionTree handling in the current SSA IR.

It works when running with --experimental-ssa so I think this PR is blocked until the ssa refactor is finished, assuming the decision tree bug is not fixed in the meantime.

Edit: For context the panic message is:

The application panicked (crashed).
Message:  No element at index
Location: crates/noirc_evaluator/src/ssa/conditional.rs:446

This is a bug. We may have already fixed this in newer versions of Nargo so try searching for similar issues at https://github.com/noir-lang/noir/issues/.
If there isn't an open issue for this bug, consider opening one at https://github.com/noir-lang/noir/issues/new?labels=bug&template=bug_report.yml

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
                                ⋮ 13 frames hidden ⋮                              
  14: core::panicking::panic_display::h4aab9670ede8cc38
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/core/src/panicking.rs:139

The application panicked (crashed).
Message:  No element at index
Location: crates/noirc_evaluator/src/ssa/conditional.rs:446

This is a bug. We may have already fixed this in newer versions of Nargo so try searching for similar issues at https://github.com/noir-lang/noir/issues/.
If there isn't an open issue for this bug, consider opening one at https://github.com/noir-lang/noir/issues/new?labels=bug&template=bug_report.yml

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
                                ⋮ 13 frames hidden ⋮                              
  14: core::panicking::panic_display::h4aab9670ede8cc38
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/core/src/panicking.rs:139
  15: core::panicking::panic_str::h8692426b1216e163
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/core/src/panicking.rs:123
  16: core::option::expect_failed::he7581704548985ec
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/core/src/option.rs:1879
  17: core::option::Option<T>::expect::hc63ebdead25dadab
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/core/src/option.rs:741
  18: <generational_arena::Arena<T> as core::ops::index::Index<generational_arena::Index>>::index::h44d422805b567d89
      at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/generational-arena-0.2.9/src/lib.rs:1359
  19: <noirc_evaluator::ssa::conditional::DecisionTree as core::ops::index::Index<noirc_evaluator::ssa::conditional::AssumptionId>>::index::h494c50175a3e139e
      at noir/crates/noirc_evaluator/src/ssa/conditional.rs:77
  20: noirc_evaluator::ssa::conditional::DecisionTree::apply_condition_to_instructions::hf6b0345a1d00c8a3
      at noir/crates/noirc_evaluator/src/ssa/conditional.rs:446
  21: noirc_evaluator::ssa::inline::inline_in_block::h0dfcba6feb75fbde
      at noir/crates/noirc_evaluator/src/ssa/inline.rs:389
  ...

[jfecher]

Ok, after some testing I've confirmed the panic happens when any of the methods accepting other functions as arguments are used. That is: map, map_or, map_or_else, unwrap_or_else, and_then, and or_else

[iAmMichaelConnor]

so I think this PR is blocked until the ssa refactor is finished

Is this PR still blocked?
We were about to write a version of Option in the Noir Contracts library of Aztec3, then stumbled upon this PR.

[jfecher]

@iAmMichaelConnor it is still blocked since the --experimental-ssa flag is still not enabled by default. Feel free to copy paste this into your own library in the meantime. Just be aware that it will likely crash if you're using it with the old ssa.

[jfecher]

This PR is no longer blocked since the old SSA code was removed. It is ready for review/merge now

[iAmMichaelConnor]

FYI I ended up taking a copy of this (thank you) and putting it in aztec-packages (temporarily). I had to make some edits to make it work over user-defined structs T - edits which make the interface deviate slightly from this one (mainly because Option::none() had to take a parameter for what "none" means for type T). I also added some comments, mostly copy-pasting the rust docs, because I wasn't sure what some of these methods were for (because I'm not familiar enough with Rust). AztecProtocol/aztec-packages#1272

[jfecher]

@iAmMichaelConnor what were the issues that arose from using std::unsafe::zeroed() for the None case?

[vezenovm]

This PR looks to me for the most part. I mainly request that we add some comments on what each of the functions are supposed to do (I find that and on Options can be confusing if you are not familiar with Rust).

[vezenovm]

I had to make some edits to make it work over user-defined structs T

Seeing Mike's comments now it would be good to check why std::unsafe::zeroed() may be causing problems

[iAmMichaelConnor]

@iAmMichaelConnor what were the issues that arose from using std::unsafe::zeroed() for the None case?

I think I just got scared about using std::unsafe::zeroed() for struct types, because the docs (https://noir-lang.org/standard_library/zeroed/) warn against doing so (and because it's called "unsafe"). So I opted to encourage the user to pass as an argument an instance of what "none" means for their struct.

[jfecher]

@iAmMichaelConnor good to know. zeroed() should be safe for this case then because Option guarantees the internal value won't be used if it is set to None. At least if you access it through its methods rather than its raw fields.

The docs mention:

It can however, be useful in cases when the value is guaranteed not to be used such as in a BoundedVec library implementing a growable vector, up to a certain length, backed by an array. The array can be initialized with zeroed values which are guaranteed to be inaccessible until the vector is pushed to. Similarly, enumerations in noir can be implemented using this method by providing zeroed values for the unused variants.

And this falls under a similar case where the value is guaranteed not to be used. The last line of the docs also mentions implementing enums like Option using it. Do you think we should re-word the docs to prevent confusion in the future? I think the main misunderstanding is coming from a very strict definition of "unsafe" the docs are using. There the function is "unsafe" because it can be used to generate values that break a struct's internal assumptions. E.g. if we had a struct:

// Invariant: `self.first ^ self.second` must hold
struct EitherOr {
    first: bool,
    second: bool,
}

impl EitherOr {
    fn new(first: bool, second: bool) -> Self {
        assert(first ^ second);
        Self { first, second }
    }
}

we could use let x: EitherOr = std::unsafe::zeroed() to create a value of it that breaks the internal assumptions of the struct. This is also why if the value is never actually used - like in Option or BoundedVec - it should be fine to create these values even if they break invariants.

[iAmMichaelConnor]

Ah, ok, I misunderstood. In that case, I'll start using this stdlib Option type, instead of the modified version.

Re improving the docs wording. I'd maybe break the two examples (bounded vec and enumerations) into separate paragraphs, and perhaps add some code snippets to illustrate the points more clearly - i.e. to show how zeroed is being used and why it won't ever be accessed. Basically, I need my hand to be held a bit more, because these sentences assume some knowledge and experience that my brain doesn't have.

I'd also add this Option type as a 3rd example.

I know this PR has gone through, but might I suggest the value property of the Option struct is renamed to _value, if it should only ever be 'got' via an unwrap method? I actually encountered confusion when I was debugging the wrapping of some types in Option last week, because one of my types T also had a value method which I was trying to access.

Perhaps also could we add an unwrap_unsafe() which doesn't assert(self._is_some())? I found myself following a pattern of checking

if my_option.is_some() {
    let my_thing = my_option.unwrap_unsafe();
    // do some other stuff
}

[jfecher]

@iAmMichaelConnor thanks for the feedback, I'll make a follow-up PR to adjust Option a bit.

Another option you could try over the if opt.is_some() { ... let x = opt.unwrap_unchecked(); ... } pattern is using opt.map(|x| { ... }); instead of the explicit if.