closures

[nrc]

What should they look like? I'd like to define the un-nested cases first and deal with nesting at the same time as nested blocks once we have the basics for a few other expression classes done.

[steveklabnik]

Some random initial thoughts from me:

Always leave off the {}s if it's a single expression:

|x| x + 1 // yes
|x| { x + 1 } // no

single-expression closures can stay on one line, multi-statement ones should be spread out like this:

|x| {
    let mut y = x + 5;
    y = y * 2;
    y
}

That is, following the usual function formatting.

... I thought I had more, but I'm not sure yet.

[KalitaAlexey]

@steveklabnik I think you meant "one-line expression" instead of "single expression".
For example:

|| Foo {
  a: 5,
  b: 10,
}

I like to write the code above like this:

|| {
  Foo {
    a: 5,
    b: 10,
}

Of course instead of a, b, 5, 10 is something bigger.

[chriskrycho]

For what it's worth, the style @steveklabnik suggested is exactly the one we use for JavaScript internally, and it's extremely reliable and maintainable. (The difference there is the requirement of return in the block, but the effect is the same.)

[nrc]

Question, if the closure has a single expression, but it spans multiple lines, should we use braces? Possible answers are (at least): always, never, if the expression is not block-like (for some definition thereof).

E.g.s,

|x| a_long_function_call_which_needs_many_lines(
    a_long_argument,
    another_long_argument,
    x
)

// vs

|x| {
    a_long_function_call_which_needs_many_lines(
        a_long_argument,
        another_long_argument,
        x
    )
}

// and block case

|x| if a_long_function_call(x) {
    foo(x)
} else {
    bar()
}

// vs

|x| {
    if a_long_function_call(x) {
        foo(x)
    } else {
        bar()
    }
}

I believe Rustfmt always uses braces if the expression is multi-line.

Related is the match arm case.

[solson]

For multi-line non-block-like expressions, I would definitely use braces in both closures and match arms.

As for block-like expressions... I used to do things like this pretty often, to avoid a level of indentation:

match e {
    pat => for i in blah {
        // ...
    },
}

But gradually I have found this style more readable in real code, despite the extra indentation:

match e {
    pat => {
        for i in blah {
            // ...
        }
    }
}

Hence, I would choose the option to simply always use braces for multi-line expressions. To be honest, I don't run into this exact case much with closures, but this is what I would do if it came up.

[joshtriplett]

@nrc @solson Personally, I prefer to always use braces for closures that span multiple lines, as well as closures containing statements (as opposed to expressions). I'd only omit braces for single-line closures containing expressions.

[solson]

By statements, do you generally mean expressions with the value ()? A literal statement like let would have to be inside braces, but I imagine you mean stuff like |x| var = x or |x| println!("x: {}", x)?

[joshtriplett]

@solson I don't know if "has type ()" precisely captures the distinction for me, and in any case rustfmt can't easily do type-based formatting.

I don't think I'd classify a simple function call without a semicolon as a statement, even if it returned (). Add a semicolon and it becomes a statement.

Some examples:

takes_closure1(|x| x + 1);

takes_closure2(|x| f(x + 1));

takes_closure3(|x| {
    f(x + 1);
});

takes_closure4(|x| if cond { x } else { 42 });

takes_closure5(|x| {
    if cond {
        f(x);
    } else {
        g(42);
    }
});

Note that #21 and #34 have similar discussions regarding expressions versus statements; for instance, in #21 I argued that if cond { a } else { b } can appear on one line with expressions a and b, but not with statements a; and b;.

[solson]

I see. What do you think of single-line takes_closure3(|x| { f(x + 1); }, but only when there's just one statement? I do that sort of thing in match arms.

[joshtriplett]

@solson Not a fan, but in some circumstances you could write that as takes_closure3(|x| f(x + 1));, which looks fine to me.

[nrc]

@solson I feel like we should apply roughly the same rules for closures as other blocks (but where a single-line block would not have braces for a closure), and I think we would not permit that as a one-line block.

@joshtriplett there is a class of expressions which can be statements without semi-colons (basically anything which ends with a block), I think you want to force braces for these expressions and 'proper' statements. However, you seem to want to allow eliding braces for single line if, so I think perhaps all the others are subsumed by 'multi-line expression'.

[nrc]

Some more points: we should recommend what to do with types, but I think it is pretty obvious how to format them (just like other functions).

The consensus here seems to be that we should force braces for multi-line single expressions. This is interesting to me because it is what Rustfmt currently does, but I got the sense from issues filed there and comments that sentiment was moving to allow eliding braces in that case (similarly for match arms).

So, I'd like to explicitly ask the question (beyond those who have already comment in the thread), do you prefer:

let f = |x| Foo {
    f: x,
    g: "foo",
};

match x {
    foo => {},
    bar => Foo {
        f: x,
        g: "foo",        
    },
}

or

let f = |x| {
    Foo {
        f: x,
        g: "foo",
    }
};

match x {
    foo => {},
    bar => {
        Foo {
            f: x,
            g: "foo",        
        }
    }
}

Another question (which I think is orthogonal to the above) is should we have an extra indent for closure bodies?

let f = |x| {
    Foo {
        f: x,
        g: "foo",
    }
};

or

let f = |x| {
        Foo {
            f: x,
            g: "foo",
        }
    };

I think I prefer the first. Rustfmt does the second (at least sometimes), but I believe that is a consequence of nesting and visual indent, and with our preference for block indent, it will not be an issue.

[solson]

Question 1: I used to prefer the former (no block) but after using both for a while, I ended up preferring the latter (with surrounding block). I think the benefit of the block is apparent when long patterns push the Foo far to the right, away from its fields:

match x {
    SuperLongPattern(_, SoMuchPattern(..), PatternPatternPattern(a, b, c)) => Foo {
        f: x,
        g: "foo",
    },
}

match x {
    SuperLongPattern(_, SoMuchPattern(..), PatternPatternPattern(a, b, c)) => {
        Foo {
            f: x,
            g: "foo",
        }
    }
}

Question 2: I strongly prefer no extra indentation for closure bodies.

[joshtriplett]

@nrc "multi-line expression" accurately captures the distinction for me, yes: if you could otherwise write the expression on one line if not inside a closure, you can do so in a closure and omit the block. Together with the existing rule about when to write if on one line, I think that works.

For question 1: in the examples you posted (and very specifically for a struct constructor), I prefer no extra block. A similar case arises for function parameters, and I think we should make a consistent decision there; do we allow the following, if the line lengths fit?

some_function_call(Foo {
    f: x,
    g: "foo",
});

For question 2: I also strongly prefer no extra indentation level for closure bodies (one level of indentation, not two).

[nrc]

@joshtriplett as opposed to forcing block indent?

some_function_call(
    Foo {
        f: x,
        g: "foo",
    },
);

Or something else?

[joshtriplett]

@nrc Right. I don't know if we should have that special case, but I've seen it in a lot of code, and it seems quite readable to me. Note that by design you can't nest it more than one level deep on the same line, since the next opportunity to insert a constructor would occur inside the struct initializer on a separate line; that pattern can only appear as the last thing on a line.

But I think we should apply the same rule to both function calls and closure bodies here: we should allow either both or neither of the following, for consistency:

some_call(Foo {
    f: x,
    g: "foo",
})

some_call(|x| Foo {
    f: x,
    g: "foo",
})

[nrc]

@joshtriplett

"multi-line expression" accurately captures the distinction for me, yes: if you could otherwise write the expression on one line if not inside a closure, you can do so in a closure and omit the block. Together with the existing rule about when to write if on one line, I think that works.

I'm not clear about what you mean here, actually. The examples above are all multi-line expressions (i.e., you couldn't write them on one line if not inside a closure) but don't have blocks.

My interpretation is that if the body of the closure is a single expression (with no comments outside) then it can be written without a block in a closure (or match arm), no matter how many lines it spans. Is that correct?

@solson the argument about pushing the struct name away from its fields seems to me like a general criticism of block indent and occurs in many places independent of this block issue. E.g.,

a.long.method.chain.last.function.here(
    arg1,
    arg2,
);

I don't think I'm persuaded that it is worth the extra vertical space and syntactic noise of forcing a block here, but not in other cases.

[joshtriplett]

@nrc

My interpretation is that if the body of the closure is a single expression (with no comments outside) then it can be written without a block in a closure (or match arm), no matter how many lines it spans. Is that correct?

Yes, that works, assuming that "single expression" has the obvious intuitive meaning.

[nrc]

To summarise the answers (that I prefer and seem to be close to consensus in this thread) to questions I asked earlier:

• we should not do any extra indenting for closures, i.e., a block gets indented the same whether there is | ... | before it or not.
• we should allow multi-line closures which are single expressions without requiring a block. That is (I think) only use a block if you must use a block. I would however like to make an exception for a single expression with comments which should get a block.
• contradicting an earlier statement of mine, I think the above rule should apply for all expressions, even those which terminate with a block.

[joshtriplett]

@nrc Based on your referencing of it when discussing function call formatting, do you agree with applying the rules in #61 to closures?

[nrc]

do you agree with applying the rules in #61 to closures?

yes

[nagisa]

I myself omit the block whenever possible (including multiline expressions) in most cases. That’s what I prefer.

That being said, I also use sanity factor, which is kinda hard to encode into a formatter :)

[jeanm]

I haven't been following the style RFCs, but it's worth mentioning that there are some issues raised in the rustfmt repo regarding closure syntax: rust-lang/rustfmt#1791

[nrc]

From, #1791, there is some discussion about whether we should be allowing multi-line single expressions which are block-terminated (i.e., mostly control flow) as the body of a closure without a block (which we has agreed on further up this thread).

@joshtriplett asserts that "This definitely shouldn't happen with for". It is not clear to me whether that is 'should' as in we had discussed this and decided not to, or we had discussed control flow in general but for is different. (I can't find any discussion explicitly about for). Personally, I don't see any material difference between for and if here.

There is an argument that not introducing a block makes it easy to make mistakes, e.g.:

crossbeam::scope(|s| for i in 0..555 {
    s.spawn(move || loop {
        if let Ok(x) = rx.select() {
            assert_ne!(i, x);
            break;
        }
        if let Ok(()) = tx.select(i) {
            break;
        }
    });
    tx.send(!0).unwrap(); // I added this line, expecting it to be run once after the loop ends.
});

I'm not sure how sympathetic I am to that argument. It seems reasonable at first blush, but I feel there are other places where you need to be careful about matching braces or you get errors too, and this doesn't seem to be the worst.

So, I feel like this is a topic we've moved around a lot on and wherever we land we get some unhappy people. So I think we need to acknowledge that we're not going to be perfect here.

So I see two options:

• we distinguish some block expressions and treat for and if differently and work out what to do with the others
• we always wrap block expressions in a block (perhaps only if they are multiline themselves).

The first option seems bad to me because of predictability.

The second has some unfortunate effects, e.g.,

|x| {
    if x {
        do_something();
    }
}

I guess we might also have some heuristic based on the number of lines in the block, but I feel these have been more loss than win in the past.

[emilio]

(FWIW, I don't see the last one as an unfortunate effect, I'd be fine with such formatting)

[ghost]

My preferred rules would be:

• If a multi-line single-block expression is a control-flow expression, it needs a dedicated block.
• Otherwise, do whatever makes the code look nice and reasonably compact.

I'm okay with compacting expressions that don't affect control-flow, e.g.:

|x| unsafe {
    // ...
}

Those are block-expressions that are simply marked or labeled in some way.

But if the block-expression affects the direction of the program flow (if, for, while, loop), then I want a dedicated block to stand on its own.

I also find collapsed control-flow blocks rather annoying to edit, for example, to get from this:

match *self {
    State::Promise { closed_count } => if closed_count < len {
        actor::current_wait_until(deadline);
    } else {
        actor::current_select(CaseId::abort());
    },
    // ...
}

To this:

match *self {
    State::Promise { closed_count } => {
        if closed_count < len {
            actor::current_wait_until(deadline);
        } else {
            actor::current_select(CaseId::abort());
        }
        *self = State::Revoke { case_id: actor::current_selected() }; // The new line.
    }
    // ...
}

I do the following steps:

1. Manually insert a new { after => and move the if-expression onto a new line.
2. Indent the block.
3. Remove that comma.
4. Add a closing }.
5. Finally, insert my new line of code at the end of the block.

Having to do the 5th step only would make my life a bit easier. :)

Honestly, when rustfmt first started putting if-expressions right after => in match statements, I thought that looked great. Even || for x in y { and || if x { seemed nice and compact. But after actively using this formatting for a while, now I'm pretty strongly against it.

[shepmaster]

Count me in the category of "if the closure has multiple lines (regardless of # of expressions), it should use braces"

Dislike 🤢

fn main() {
    thread::spawn(|| {
        thread::spawn(|| if true {
            100000000000000000000
        } else {
            200000000000000000000
        })
    });
}

Like 😋

fn main() {
    thread::spawn(|| {
        thread::spawn(|| {
            if true {
                100000000000000000000
            } else {
                200000000000000000000
            }
        })
    });
}

Rationale 🤔

I don't quite know how to explain it, but having the else align with the thread:spawn is just... wrong. At first read, it looks like there's an else associated with the thread:spawn, which doesn't make sense to me.

---

I also think this applies beyond closures. It appears as if my example above does what I want with a match statement in rustfmt-nightly 0.2.2:

Like 😋

fn main() {
    match a {
        _ => if true { 100000000000 } else { 2000000000000 },
    }
}

Like 😋

fn main() {
    match a {
        _ => {
            if true {
                10000000000000
            } else {
                200000000000000
            }
        }
    }
}

[ghost]

Just to throw in yet another example that has been bugging me for a while...

I have the following piece of code:

*self = Machine::Counting {
    len: len + 1,
    first_id: if first_id == CaseId::none() {
        case_id
    } else {
        first_id
    },
    deadline,
    seen_blocked,
};

What throws me off here is that else is aligned with len, first_id, deadline, and seen_blocked, as if it was just another field.

But the else is only relevant to first_id, and some indentation would easily remove any potential confusion:

*self = Machine::Counting {
    len: len + 1,
    first_id: {
        if first_id == CaseId::none() {
            case_id
        } else {
            first_id
        }
    },
    deadline,
    seen_blocked,
};

If you're not convinced, here's a more difficult example. Try skimming over the code and figuring out what belongs where. It's kinda tricky. :) Then try comparing with a version with more indentation.