Add more functions for adding nodes to MastForest

[sergerad]

Add methods to MastForest to allow adding nodes with fewer LOC.

Follow on from #1404 which added similar methods to MastForestBuilder.

As per comment.

[sergerad]

@bobbinth - following on from previous PR.

[bobbinth]

Looks great! Thank you! I left one comment inline about how we can make validation even more robust.

[bobbinth]

One potentially nice thing to validate here would be that left_child and right_child have smaller values than the newly created node ID. We could create a new error variant in MastForestError for this.

This is also relevant for add_split(), add_loop(), add_call(), and add_syscall() methods below.

[sergerad]

Updated.

LMK if you would like the error string to be more descriptive and contain references to the type of node and other IDs.

[bobbinth]

All looks good! Thank you!

[sergerad]

@bobbinth I added tests in this commit.

Just want to double check you weren't expecting that logic to be captured by my changes - because what happens instead is that the overflow causes a panic. Understood you may not have intended my logic to avoid this overflow behaviour, just wanted to check.

thread 'mast::serialization::tests::mast_forest_invalid_node_id' panicked at core/src/mast/node/join_node.rs:34:46:
index out of bounds: the len is 2 but the index is 3

I have put the PR into draft to avoid merging the currently failing test.

[bobbinth]

Just want to double check you weren't expecting that logic to be captured by my changes - because what happens instead is that the overflow causes a panic. Understood you may not have intended my logic to avoid this overflow behaviour, just wanted to check.

thread 'mast::serialization::tests::mast_forest_invalid_node_id' panicked at core/src/mast/node/join_node.rs:34:46:
index out of bounds: the len is 2 but the index is 3

I have put the PR into draft to avoid merging the currently failing test.

Ah - good catch!

I think we should fix this and there are two was to do it:

1. Change node constructors to return a Result (e.g., JoinNode::new() would return an error if the children are not in the forest).
2. Make a preemptive checks in MastForest::add_join() etc. methods. For example, it could look something like this:

/// Adds a join node to the forest, and returns the [`MastNodeId`] associated with it.
pub fn add_join(
    &mut self,
    left_child: MastNodeId,
    right_child: MastNodeId,
) -> Result<MastNodeId, MastForestError> {
    if left_child >= self.nodes.len() {
        Err(MastForestError::InvalidNodeId(left_child))
    } else if right_child >= self.nodes.len() {
        Err(MastForestError::InvalidNodeId(right_child))
    } else {
        self.add_node(MastNode::new_join(left_child, right_child, self))
    }
}

The first option may be a more intrusive change, but the second option would require duplication of logic (i.e., we'd need something similar in MastForestBuilder::esnure_join()). So, I'm a bit on the fence here, but leaning a bit toward the first option.

[sergerad]

@bobbinth I have implemented option 1 as you suggested. LMK if anything needs to be changed.

I preferred option 1 on the basis that it felt more in line with the "parse, don't validate" idiom by enforcing the invariant in the node constructors.

[bobbinth]

Looks good! Thank you! I left just one non-blocking comment inline.

[bobbinth]

All looks good! Thank you again!