Combining openings and closings

[joshtriplett]

(Suggestions welcome for a better title.)

This arose from a discussion in a style-team meeting, as well as discussions in #35.

A generalized rule that may apply to structs, function calls, closures, match arms, and similar: If you have an outer construct that takes a single expression parameter (a struct with a single field, a function with a single parameter...), and that single expression in turn consists of a construct containing a list that needs breaking across multiple lines, you can break the inner list across multiple lines without first breaking the outer thing across multiple lines.

Concretely:

foo(bar(
    param1,
    param2,
    param3,
));

foo(Bar {
    x: value,
    y: value2,
});

let opt = Some(Struct(
    long_argument_one,
    long_argument_two,
    long_argument_three,
));

do_thing(|param| {
    action();
    foo(param)
});

// at the end of some block
Ok(some_function(
    long_argument_one,
    long_argument_two,
    long_argument_three,
))

I've seen this pattern extensively in Rust code.

This may nest further, as long as all but the innermost construct have only a single argument:

Ok(bar(baz(
    param,
    param2,
)));

This should never apply if the outer construct contains more than just the single expression; for instance:

// don't do this
let thing = Struct(first, OtherStruct(
    second,
    third,
));

// don't do this either
foo(thing, bar(
    param1,
    param2,
    param3,
));

[joshtriplett]

(Posting this as a separate comment so people can evaluate it and react to it independently.)

As a possible exception to the requirement that the outer construct must have only one argument, you can also apply this to an invocation of print!, println!, or format!, where the first argument consists of a format string and the second argument consists of a single expression:

println!("Thing: {}", generate_thing(
    long_argument_one,
    long_argument_two,
    long_argument_three,
));

This form similarly nests:

println!("Thing: {}", transform_into_thing(SomeStruct {
    f1: long_argument_one,
    f2: long_argument_two,
    f3: long_argument_three,
}));

process_string(format!("some prefix: {}", func(
    long_argument_one,
    long_argument_two,
    long_argument_three,
)));

[joshtriplett]

(Posting this as a separate comment so people can evaluate it and react to it independently.)

This can also apply to match arms:

match opt {
    Some(x) => somefunc(anotherfunc(
        long_argument_one,
        long_argument_two,
        long_argument_three,
    )),
    None => Ok(SomeStruct {
        f1: long_argument_one,
        f2: long_argument_two,
        f3: long_argument_three,
    }),
}

[gsingh93]

It seems like this goes against some principles that were involved with other style decisions, such as preserving blame. If any one or a combination of the opening identifiers is renamed such that the line grows too long, the block will be moved to the next line and all the block contents will be indented, breaking blame.

I'm not strongly against this, but I wanted to bring that up.

[est31]

I'm strongly in favour, but I'm running a highly customized config file already, so would be happy too if it weren't the default but there was a setting. Also, would like a setting where you could optimize the second/third/etc. argument in a similar fashion as well, not just the first (contrary to this proposal).

[nrc]

Related, rust-lang/rustfmt#1494, is about combining closures and function calls (specifically where the function just has a single arg).

[nikomatsakis]

This should never apply if the outer construct contains more than just the single expression; for instance:

I'm curious what you think about closures as the last argument. For example:

foo.map_or(x, || {
    foo;
    bar;
    baz;
});

It seems like this would be formatted as:

foo.map_or(
    x,
    || {
        foo;
        bar;
        baz;
    });

[nrc]

Moving to FCP, I would like to propose we accept the original proposal plus the extension to match arms, and closures, but not format strings. I think we may consider format strings later, but there are some other special cases there that I'd like to consider at the same time, so I'd rather punt for now. E.g.,

println!(
    "a {} format {} string {:?}",
    var1, var2, var3,
);

Re closures, I would like to restrict this to block closures, and in this case we allow any number of parameters as long as everything up to the { of the closure fits on one line (including inside the 'short' heuristic for function args), e.g.,:

foo(1 + 2, x, some_variable.bar(), |x| {
    closure_body();
    ret_value
})

[joshtriplett]

On May 8, 2017 9:04:33 PM PDT, Nick Cameron ***@***.***> wrote: Moving to FCP, I would like to propose we accept the original proposal plus the extension to match arms, and closures, but not format strings. I think we may consider format strings later, but there are some other special cases there that I'd like to consider at the same time, so I'd rather punt for now. E.g., ``` println!( "a {} format {} string {:?}", var1, var2, var3, ); ``` Re closures, I would like to restrict this to block closures, and in this case we allow any number of parameters as long as everything up to the `{` of the closure fits on one line (including inside the 'short' heuristic for function args), e.g.,: ``` foo(1 + 2, x, some_variable.bar(), |x| { closure_body(); ret_value }) ```

Sounds good to me. +1

[topecongiro]

Should we extend the proposal to other expressions which end with closing parens, braces or brackets?
Like if, for, while, loop and others.

// if
foo(bar(if x {
    foo()
} else {
    bar()
}))

// loop
foo!(loop {
    bar();
});

[est31]

@topecongiro yes! and if not per default, then as option.

[nrc]

For comparison, if we don't follow @topecongiro's suggestion, then that code would look like:

// if
foo(bar(
    if x {
        foo()
    } else {
        bar()
    },
))

// loop
foo!(
    loop {
        bar();
    },
);

I think I'm in favour of extending to control flow, but not strongly so. Perhaps we should also consider how many levels deep we go here, for example, would we allow:

foo(bar(baz(qux(foo(if x {
    foo()
} else {
    bar()
})))))

[topecongiro]

The (non-exhaustive) list of expressions which could possibly be considered as combinable.
I am not saying that we should combine all these by default, though not against it.

fn main() {
    // Call
    foo(bar(
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
        bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
    ));

    // Mac
    foo(foo!(
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
        bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
    ));

    // MethodCall
    foo(x.foo::<Bar, Baz>(
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
        bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
    ));

    // Block
    foo!({
        foo();
        bar();
    });

    // Closure
    foo(|x| {
        let y = x + 1;
        y
    });

    // Match
    foo(match opt {
        Some(x) => x,
        None => y,
    });

    // Struct
    foo(Bar {
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
        bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
    });

    // If
    foo!(if x {
        foo();
    } else {
        bar();
    });

    // IfLet
    foo!(if let Some(..) = x {
        foo();
    } else {
        bar();
    });

    // While
    foo!(while x {
        foo();
        bar();
    });

    // WhileLet
    foo!(while let Some(..) = x {
        foo();
        bar();
    });

    // ForLoop
    foo!(for x in y {
        foo();
        bar();
    });

    // Loop
    foo!(loop {
        foo();
        bar();
    });

    // Tuple
    foo((
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
        bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
    ));

    // AddrOf
    foo(&bar(
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
        bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
    ));

    // Box
    foo(box Bar {
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
        bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
    });

    // Unary
    foo(!bar(
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
        bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
    ));

    // Try
    foo(bar(
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
        bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
    )?);

    // Cast
    foo(Bar {
        xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx,
        xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx,
    } as i64);
}

[nrc]

@joshtriplett thoughts on extending to other expressions?

[joshtriplett]

Looking over the list of possibilities, I'm not a big fan of extending this to control flow, especially control flow that contains arguments (for, while, while let, if, if let, match). loop seems harmless enough, but the rest I'd hesitate to support, precisely because they add more on the same line rather than on subsequent lines.

That said, I don't have that strong an objection as long as we never combine two or more control flow expressions (e.g. loop { loop { or loop { if x {).

[topecongiro]

Perhaps we should also consider how many levels deep we go here

I would like to propose using width to restrict how many expressioins can be combined, rather than counting the level of nesting.
We could use fn_call_width or introduce another config option.

[daboross]

Why not do both a width and logical complexity max?

Question in general on this: when we don't support direct nesting an expression, would it be reasonable to add a {} scope rather than just leave it indented only? Bringing this up because implementing this would also mean support for just plain extra scopes being at the right level.

What about using this:

    foo(bar({
        match opt {
            Some(x) => x,
            None => 0,
        }
    }));

Rather than this:

    foo(bar(
        match opt {
            Some(x) => x,
            None => 0,
        }
    ));

when the inner code doesn't itself collapse.

[joshtriplett]

@daboross I don't think we need the extra scope there; I'd prefer to always render that code using the second form, without the extra pair of braces. That looks reasonable to me. (Though I'd probably pull the match out to a let, in practice.)

[emilio]

On 05/31/2017 07:04 AM, Seiichi Uchida wrote:> Perhaps we should also consider how many levels deep we go here

I would like to propose using width to restrict how many expressioins
can be combined, rather than counting the level of nesting.
We could use |fn_call_width| or introduce another config option.

I think that's not particularly great. In particular:

self.my_very_long_callsite(|x| {
    // ...
})

self.foo(bar(baz(match a {
    // ...
}))));

I think the the first snippet's closure being block-formatted, while nothing in the second one would be a really weird behavior.

[nrc]

As an exception to the closures exception, we should probably not apply that rule when there are multiple closures in the arg list, see rust-lang/rustfmt#1713

[kennytm]

rustfmt-nightly 0.1.7 currently emits:

fn main() {
    foo(
        &[
            "--emit=asm",
            "-C",
            "opt-level=3",
            "-o",
            "abcdef",
            "-Z",
            "sanitizer=asan",
            "-A",
            "warnings",
        ],
    );
}

Is the double-indent intended here? This issue did not mention arrays at all.

[joshtriplett]

@kennytm That does seem like something we should allow. We discussed this in one of our recent meetings, and came to the conclusion (in general terms) that we wanted to allow simple unary prefix operators like & for precisely that kind of situation. Can you confirm that if you remove the & that expression formats as you'd expect?

[kennytm]

@joshtriplett It is the same with or without the &. I guess this means it is a just rustfmt bug?

fn main() {
    foo(
        [
            "--emit=asm",
            "-C",
            "opt-level=3",
            "-o",
            "abcdef",
            "-Z",
            "sanitizer=asan",
            "-A",
            "warnings",
        ],
    );
}

[nrc]

Having seen this in action, I'm having second thoughts about some of the match arm combinings - I think control flow looks good there, but struct lits and (especially) function calls are hard to read. See rust-lang/rustfmt@e3310a6 for lots of examples.

[joshtriplett]

@nrc Reading through all of those examples, I agree. For the same reason that we shouldn't fold control flow opens/closes into function/struct/etc opens/closes, we shouldn't do the reverse either. In addition to the function calls and struct literals, I also think we shouldn't do things like this:

-        ast::ExprKind::WhileLet(..) => {
-            to_control_flow(expr, expr_type)
-                .and_then(|control_flow| control_flow.rewrite(context, shape))
-        }
+        ast::ExprKind::WhileLet(..) => to_control_flow(expr, expr_type)
+            .and_then(|control_flow| control_flow.rewrite(context, shape)),

There's also one other match case I don't think we should do: I don't think we should put if on the same line as match. It's too easy to mistake the following two things, which have subtly different meanings (specifically, whether the match checks the next pattern or not):

big(complicated, pattern) if condition => { code }
big(complicated, pattern) => if condition { code }

[shepmaster]

Sorry for jumping in late here, but I still haven't been able to tell if my case is undecided, decided but not implemented in rustfmt, or decided and implemented.

rustfmt-nightly (0.2.2) formats as:

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

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.

I'd advocate for forcing the body to be placed in the braces and nested:

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

[topecongiro]

@shepmaster It is implemented on rust-lang/rustfmt#1877 but pending.
For now, with multiline_closure_forces_block = true rustfmt always use block for the closure body when it is multi-lined (rust-lang/rustfmt#1898).

[shepmaster]

@topecongiro thank you! I see that issue refers to #35, which is presumably where I should also chime in.

[djc]

Similar to what Josh mentioned about println!(), I think it would be good to consider extending this to not just allowing single-argument function calls, but also just if the last argument can be broken up and the preceding arguments don't exceed some particular limited length.

[pitaj]

Apologies for necroing this issue, but I'm trying to dig up why the rustfmt overflow_delimited_expr shouldn't be enabled by default. And I found it derives from the following statement in the guide

Only where the multi-line sub-expression is a closure with an explicit block, this combining behaviour may be used where there are other arguments, as long as all the arguments and the first line of the closure fit on the first line, the closure is the last argument, and there is only one closure argument

History

I see in the issue comment @joshtriplett says

This should never apply if the outer construct contains more than just the single expression

// don't do this
let thing = Struct(first, OtherStruct(
    second,
    third,
));

// don't do this either
foo(thing, bar(
    param1,
    param2,
    param3,
));

In the next comment they suggest a possible exception for format macros

As a possible exception to the requirement that the outer construct must have only one argument, you can also apply this to an invocation of print!, println!, or format!, where the first argument consists of a format string and the second argument consists of a single expression:

println!("Thing: {}", generate_thing(
    long_argument_one,
    long_argument_two,
    long_argument_three,
));

AFAIK this did not make it into the guide

Then @nrc suggests an exception for closures as the last argument in this comment and @joshtriplett agrees

Re closures, I would like to restrict this to block closures, and in this case we allow any number of parameters as long as everything up to the { of the closure fits on one line (including inside the 'short' heuristic for function args), e.g.,:

foo(1 + 2, x, some_variable.bar(), |x| {
    closure_body();
    ret_value
})

This did make into the guide

Then there's a lot of discussion about what control flow expressions should be allowed.

Then there's discussion about arrays where @kennytm brings up arrays and @joshtriplett agrees that it should be treated similarly.

However, there was never any discussion about allowing arrays, match, and other delimited expressions as the last argument.

Getting to the point

I don't see any reason why other multi-line delimited expressions shouldn't be allowed by default in the same context as closures. For instance, right now, rustfmt will format a match inside a closure and a match without a closure differently:

    // with a closure
    let f = fn_call(123, |cond| match cond {
        A => 1,
        B => 2,
    });
    // without a closure, not consistent
    let f = fn_call(
        123,
        match cond {
            A => 1,
            B => 2,
        },
    );
    // works fine with normal blocks though
    let f = fn_call(123, {
        let a = 1;
        let b = 2;
        a + b
    });
    // or without any other arguments
    let f = fn_call(match cond {
        A => 1,
        B => 2,
    });

That doesn't make any sense to me, and looks super ugly. Also, consider an append macro, used to push a list of elements to a Vec without copying or cloning:

     append!(v, [
        352732685652347985,
        348974392769832796759287,
        4738973482759382759832,
        178957349857932759834729857,
    ]);

Awesome for cleaning up dozens of lines of v.push(...) calls into an easily readable format. But rustfmt just kills me here (without enabling the overflow_delimited_expr option):

     append!(
        v,
        [
            352732685652347985,
            348974392769832796759287,
            4738973482759382759832,
            178957349857932759834729857,
        ]
    );

So my question is, why does this "last argument exception" not apply to match, arrays, vec![], etc? Seems inconsistent, yet alone ugly.

[Ekleog]

@pitaj Just coming across this -- I think you may have to open a new issue to get discussion on this topic, few people usually read the closed issues.

[pitaj]

@Ekleog I will do so