ControlFlow tweaks - Add #[must_use] and methods to convert to Result

[dhedey]

Proposal

Problem statement

The ControlFlow docs recommend that ControlFlow can be used in Visitor patterns, but there are a couple of rough edges I noted when implementing my own visitor:

1. It is easy to miss a ? after a method call, and fail to propagate a ControlFlow::Break, resulting in bugs.
2. ControlFlow (with variants Continue and Break) is an abstraction with semantics related to execution of an algorithm, and not greatly suited to an external interface. In one interface, I wanted to map it to a more semantic type: Result. But I noticed that whilst ControlFlow has utility methods break_value() and continue_value() to map to an Option, it does not have any for Result.

Motivating examples or use cases

My personal experience with this was implementing a visitor pattern for a validator (more specifically, it was an intepreter / validator / visitor-runner combination).

The main issue stemmed from the fact ControlFlow wasn't #[must_use]. The interpreter logic covered about 500 lines, and I missed a couple of ? after some method calls, which resulted in bugs. Thankfully I caught these in tests and made sure to plaster a #[must_use] on every method which returned a ControlFlow. But ideally the compiler would have helped me out.

Trimmed down example code demonstrating the problems follows. It could be argued that just using Result here might have been easier, but I think ControlFlow does represent the semantics better - particularly if you think about this as an Interpreter/Visitor rather than as a Validator.

The example also includes a cheeky note on do yeet:

pub trait Visitor {
    type BreakValue: From<InterpreterError>;

    #[must_use]
    fn on_start_instruction<'a>(
        &mut self,
        details: OnStartInstruction<'a>,
    ) -> ControlFlow<Self::BreakValue> {
        ControlFlow::Continue(())
    }
}

impl Visitor for () {
    type BreakValue = InterpreterError;
}

impl Interpreter {
//...
    pub fn validate(self) -> Result<(), InterpreterError> {
        // PROBLEM 2 - Manually written match statement due to missing utility method.
        // I think I'm spoilt by rust and calling it a "PROBLEM" is rather overstating things.
        match self.validate_and_visit(&mut ()) {
            ControlFlow::Continue(()) => Ok(()),
            ControlFlow::Break(err) => Err(err),
        }
    }

    #[must_use] 
    fn validate_and_visit<V: Visitor>(mut self, visitor: &mut V) -> ControlFlow<V::BreakValue> {
        for instruction in self.instructions.into_iter() {
            visitor.on_start_instruction(/*...*/)?;
            // PROBLEM 1 - You can miss `?` unless #[must_use] is added manually to all methods
            self.handle_instruction(visitor, instruction)?;
        }
        ControlFlow::Continue(())
    }

    #[must_use]
    fn handle_instruction<V: Visitor>(&mut self, visitor: &mut V, instruction: &Instruction) -> ControlFlow<V::BreakValue> {
        if !self.is_supported(instruction) {
            // BETTER YET: do yeet InterpreterError::InstructionNotSupported;
            // ... which is particularly nice as `?` on ControlFlow doesn't automatically call `into()`
            return ControlFlow::Break(InterpreterError::InstructionNotSupported.into());
        }
        ControlFlow::Continue(())
    }
//...
}

Solution sketch

1. I suggest adding #[must_use] to ControlFlow to avoid these problems.
2. New methods, continue_ok(self) -> Result<C, B> and break_ok(self) -> Result<B, C> (name suggestions courtesy of @NoraCodes in this comment)

Alternatives

1. Alternatives to adding #[must_use]
   • We could advise users to add #[must_use] to all their methods/functions... but like with Result, it feels that the 99% use case is to expect that ControlFlow should always be handled, to avoid missing a Break. So I think it makes sense to put it on the type, and avoid this class of error.
2. Alternatives to continue_ok and break_ok:
   • We could consider doing nothing and having users write these matches manually. Although I think this is inconsistent with the existing utility conversion methods in Rust's core library.
   • We could consider different names for these methods (I'm not tied to particular names).

Links and related work

• Tracking issue for ControlFlow enum, for use with try_fold and in Try rust#75744
• Tracking Issue for experimental yeet expressions (feature(yeet_expr)) rust#96373

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

• We think this problem seems worth solving, and the standard library might be the right place to solve it.
• We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

• We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
• We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

[dtolnay]

+1. I would be happy to accept a PR for these changes.