Change successor function in basic_block module

[kevaundray]

Problem

Referencing https://github.com/noir-lang/noir/pull/1211/files#r1175710844

Proposed solution

Once #1200 has been merged, modify that function.

Alternatives considered

No response

Additional context

No response

Submission Checklist

• Once I hit submit, I will assign this issue to the Project Board with the appropriate tags.

[joss-aztec]

I'm having doubts about the visitor pattern I introduced in #1200 as it doesn't seem versatile. Forcing the use of lamdas makes result handling inconvenient. For example if I attempt to apply it to display_block_with_successors I end up with something like:

pub(crate) fn display_block_with_successors(
    function: &Function,
    block_id: BasicBlockId,
    f: &mut Formatter,
) -> Result {
    display_block(function, block_id, f)?;

    let basic_block = &function.dfg[block_id];
    if basic_block.maybe_terminator().is_none() {
        // If the terminator is assigned it is not safe to attempt to visit its successors.
        return Ok(());
    }
    let mut ok_or_first_err = Ok(());
    basic_block_visitors::visit_block_succs(basic_block, |succ_id| {
        match display_block(function, succ_id, f) {
            Err(err) => ok_or_first_err = Err(err),
            _ => {}
        }
    });
    ok_or_first_err
}

Or probably better:

    let mut successor_ids = vec![];
    basic_block_visitors::visit_block_succs(basic_block, |succ_id| successor_ids.push(succ_id));
    for succ_id in successor_ids {
        display_block_with_successors(function, succ_id, f)?
    }
    Ok(())

Either is feels bit clumsy. @jfecher would you be in favour of killing visitors? I was merely mimicking cranelift without a great deal of thought.

Tangential point: I've also included handling of the terminator's optionality in the above snippet because I think there may be demons down the line. I'd in favour of having a separate factory struct.

[jfecher]

Either is feels bit clumsy. @jfecher would you be in favour of killing visitors?

I generally do like having functions for traversing the data since it helps separate the traversal from the "business logic" of whatever operation is actually being done. That being said, they're just a tool so if they're hard to use then we should change them. It seems to me that visit_block_succs would benefit from having a version that handles Results internally for usecases like display_block_with_successors.

[joss-aztec]

How about?

for successor in basic_block_views::successors_iter(&function.dfg[block_id]) {
    display_block(function, successor, f)?;
}

[jfecher]

successors_iter would work but you'll have a hard time creating an iterator without collecting each id into a Vec first I think, and at that point you can use the successors function.

[joss-aztec]

Yep, I see what you mean. I've done a PR nonetheless, but feel free to chuck it.
#1231

[joss-aztec]

Duplication removed in 52ce1fd