Third time using && { ...; true } for side effects

#1

This is only vaguely related to language design, and only vaguely related to rust internals (arguably it's more about rust users). However, we find ourselves increasingly often using code of the form:

return something_else.mutate() && {
  something = something_else;
  true
}

In particular:

    /// Advances the instruction address register.
    ///
    /// # Returns
    ///
    /// `true` if successful, `false` otherwise.
    fn next(&mut self) -> bool {
        let new = self.iar.map_or(0, |v| v + 1);
        new < self.ops.len() && {
            self.iar = Some(new);
            true
        }
    }

    /// Rewinds the instruction address register.
    ///
    /// # Returns
    ///
    /// `true` if successful, `false` otherwise.
    fn prev(&mut self) -> bool {
        let new = self.iar.expect("iar").checked_sub(1);
        new.is_some() && {
            self.iar = new;
            true
        }
    }

    /// pattern <- ( subtree / unexpected_token ) ( !. / unexpected_token )
    fn pattern(&mut self, s: &mut &str) -> Result<bool, PatternError> {
        let mut cursor = *s;
        Ok(
            (
                self.subtree(&mut cursor)?
                ||
                self.unexpected_token(&mut cursor)?
            )
            &&
            (
                cursor == ""
                ||
                self.unexpected_token(&mut cursor)?
            )
            && { *s = cursor; true }
        )
    }

(These are incidentally in the order we wrote them.) We find this pattern much nicer than messing with else { false } everywhere. We really do not like else { false } and that's the only reason we use this, even if this is arguably an antipattern. We are wondering how others feel about it, and if maybe there's something that can be done at the language or maybe at the libs to help with this.

#2

Have you considered

let result = something_else.mutate();
if result {
  something = something_else
}
result

?

4 Likes
#3

Interesting. I tend to use patterns like this for similar code:

use anyhow::{Result, ensure};

fn next(&mut self) -> Result<()> {
    let new = self.iar.map_or(0, |v| v + 1);
    ensure!(new < self.ops.len(), "overflow");
    self.iar = Some(new);
    Ok(())
}

fn prev(&mut self) -> Result<()> {
    let new = self.iar.expect("iar").checked_sub(1).ok_or("underflow")?;
    self.iar = Some(new);
    Ok(())
}

(anyhow is used here for illustration, but the code would be similar with any type of Result. With a zero-sized error, the return type would be isomorphic to bool.)

1 Like
#4

Result<(),()> for ergonomics?

Result<Result<(),()>, PatternError> would look a bit more uncomfortable tho... edit: and also wouldn't work.

#5

That's pretty much the pattern I use. Admittedly a better API would be great, but I don't think it's too feasible unfortunately.

#6

Yeah. It's not great when refactoring.

#7

This seems to me like a job for a combinator that accepts a value, runs a closure (supplying a reference to the value) for its side effects, then returns the value. I don't know a good name for it; the only prior art I know of is Lisp's prog1, which is obviously too cryptic to keep. Using side_effect for illustration, suppose there are three new combinators

trait SideEffectCombinator {
    /// Call `f` for its side effects, supplying an immutable reference
    /// to self, then return self.
    fn side_effect<F>(self, f: F) -> Self
        where F: FnOnce(&Self) -> ();

    /// As above but `f` is only called if self is truthy.
    fn and_side_effect<F>(self, f: F) -> Self
        where F: FnOnce(&Self) -> ();

    /// As above but `f` is only called if self is falsey.
    fn or_side_effect<F>(self, f: F) -> Self
        where F: FnOnce(&Self) -> ();
}

then your code would become

    fn next(&mut self) -> bool {
        let new = self.iar.map_or(0, |v| v + 1);
        (new < self.ops.len())
            .and_side_effect(|_| { self.iar = Some(new); })
    }

    fn prev(&mut self) -> bool {
        let new = self.iar.expect("iar").checked_sub(1);
        new.is_some()
            .and_side_effect(|_| { self.iar = new; })
    }

Option<T>::and_side_effect would call f when it's Some, and so on.

This may not need to be in the stdlib, but it occurs to me that if it were, it would be handy for debugging long combinator chains:

    ...
    .do_this()
    .side_effect(|x| { eprintln!("between this and that x={:?}", x); })
    .do_that()
    ...
#8

.run_if(..) ?

#9

Bools will soon have the method then (to be stabilized in the next release) which conditionally runs the closure and returns an Option with the result. With it, you could write something like:

fn next(&mut self) -> bool {
    let new = self.iar.map_or(0, |v| v + 1);
    (new < self.ops.len())
        .then(|| self.iar = Some(new))
        .is_some()
}

However, this doesn't feel that great since it's not obvious that the return value is just the result of the bounds check.

1 Like
#10

As prior art, Rust's Futures has inspect for this.

#11

What would that look like though... like this?

(new < self.ops.len()).inspect(|&result| {
    if result {
        self.iar = Some(new)
    }
})

IMO this is not really better than the straightforward approach

let result = new < self.ops.len();
if result {
    self.iar = Some(new)
}
result

Edit: Perhaps with some nice macros to abbreviate the closure, e.g.

(new < self.ops.len()).inspect(on_match!(true, {
    self.iar = Some(new)
}))

(playground)

#12

Oh sorry, apparently I hi the wrong reply button - inspect was just meant as a naming suggestion instead of side_effect in reply to @zachw's post, so:

trait InspectCombinator {
    /// Call `f` for its side effects, supplying an immutable reference
    /// to self, then return self.
    fn inspect<F>(self, f: F) -> Self
        where F: FnOnce(&Self) -> ();

    /// As above but `f` is only called if self is truthy.
    fn and_inspect<F>(self, f: F) -> Self
        where F: FnOnce(&Self) -> ();

    /// As above but `f` is only called if self is falsey.
    fn or_inspect<F>(self, f: F) -> Self
        where F: FnOnce(&Self) -> ();
}
#13

Oh gosh.. I just noticed (on another read), that you guys are suggesting a new notion of “falsey” and “truthy” for Rust. I don’t think we should introduce something like that besides the Try trait (which is still WIP).

And the signatures as proposed are suboptimal also in that they don’t e.g. allow passing a &T reference to the closure on a Some-valued Option<T>, so you’d encourage unwrapping; and also they don’t allow for having just an inspect method on types that don’t distinguish “truthy” and “falsey” values.

By the way, @illicitonion, you misspelled @zackw ;-)

1 Like
#14

That wasn't my intention; I meant to refer to the existing concept of truthiness shared by Option and Result (e.g. None and Err(x) are falsey for purposes of and_then, or_else, etc.) I'm a little surprised to discover that bool doesn't already implement that family of combinators.

#15

Hopefully this does make it in eventually as

...do_this()
  .dbg!()
  .do_that()

currently it looks like

dbg!(
    ...
    ...
    ...do_this()
  )
  .do_that()
#16

It’s remarkable how close to postfix macros you can get with a function like inspect. Or better even one that passes its argument by-value throught the closure. Look at this code example

fn main() {
    // `ap` stands for “apply”
    // `dbg_!()` is still a macro in order to have it point to the right line number
    [0,1,1,2,3,5,8,13,21].iter().ap(dbg_!()).count();
    Some(42).ap(println_!("The value is {it:?} and one is {}", 1)).is_some();
}

(playground)

#17

Unfortunately we feel like all of these suggestions simply fall short of the simplicity of && { side_effects(); true }...

In the meantime, we are gonna document the && { side_effects(); true } pattern in a HACKING.md file and say that it looks nicer than either introducing a new name or introducing an else { false } somewhere. As confusing as it is when you're not used to it... it's no more confusing than using Result<(), ()> instead of a bool (also considering the fact that it doesn't work for one of the 3 presented use-cases). (Like, thinking about it further, Result<(), ()> is pretty confusing the first time you see it. It wasn't confusing on this thread because the thread does introduce the context for what problem it solves. You don't really get that context on a real codebase.)

Further, as much as it'd be interesting to extend ? to bool... it doesn't work for one of the 3 presented use-cases. (Unless we get labeled ?. Labeled ? on bool would work for us.)

1 Like
#18

If Result is not very semantic and extending ? to bool would be too much of a stretch, would the suggested try_trait_v2 work here?

#19

That's at best subjective. I find that code very confusing and hard to read, and if any colleague were to write such code, I'd immediately reject it in a code review, asking them to rewrite it using a plain if, as suggested by the first post here. Brevity != clarity.

6 Likes
#20

The first comment here suggests introducing a variable, which is unnecessary. And generally if thing { true } else { false } is frowned upon, even if you have side effects in one of the if blocks.

We would happily propose breaking return in if tho, such that if return foo { [...] } becomes sugar for return if foo { [...] true } else { false }. (or just ? on bool)