Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add overflow_closures and overflow_match_arms opts #1898

Merged
merged 1 commit into from
Aug 24, 2017

Conversation

spinda
Copy link
Contributor

@spinda spinda commented Aug 19, 2017

cc #1791

overflow_closures

Allow overflow on closures

  • Default value: true
  • Possible values: true, false

true:

result.and_then(|maybe_value| match maybe_value {
    None => ...,
    Some(value) => ...,
})

false:

result.and_then(|maybe_value| {
    match maybe_value {
        None => ...,
        Some(value) => ...,
    }
})

overflow_match_arms

Allow overflow on match arms

  • Default value: true
  • Possible values: true, false

true:

match lorem {
    None => if ipsum {
        println!("Hello World");
    },
    Some(dolor) => ...,
}

false:

match lorem {
    None => {
        if ipsum {
            println!("Hello World");
        }
    }
    Some(dolor) => ...,
}

@topecongiro
Copy link
Contributor

Thank you for you PR! Could you please add tests for overflow_closures as well?

@spinda
Copy link
Contributor Author

spinda commented Aug 19, 2017

Oops, I thought I'd done that. Will add.

src/expr.rs Outdated
@@ -688,6 +688,13 @@ fn rewrite_closure_expr(
if classify::expr_requires_semi_to_be_stmt(left_most_sub_expr(expr)) {
rewrite = and_one_line(rewrite);
}
rewrite = rewrite.and_then(|rw| {
if !context.config.overflow_closures() && rw.contains('\n') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Totally not a rustfmt expert, so could totally be wrong, but won't this undo any other formatting on everything else inside the expression?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at how rewrite_closure_expr is used:

https://github.com/rust-lang-nursery/rustfmt/blob/6fb17c41301e5a98748882cc6ad6a627def8eb80/src/expr.rs#L629-L648

https://github.com/rust-lang-nursery/rustfmt/blob/6fb17c41301e5a98748882cc6ad6a627def8eb80/src/expr.rs#L650-L653

It's called to attempt to format the closure body expression without wrapping it in a block. If it fails and returns None, rustfmt will fall back to rewrite_closure_block and format the expression with a block surrounding it. rewrite_closure_block will perform any formatting that's still appropriate for the expression in that context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, ok, fair enough. That happens when drive-by reviewing, thanks for explaining :)

@spinda spinda force-pushed the overflow-controls branch 2 times, most recently from 8ccdf28 to 48b0c35 Compare August 19, 2017 22:08
@nrc
Copy link
Member

nrc commented Aug 20, 2017

Bikeshedding, I would prefer closures_force_block rather than overflow_closures (and likewise for match arms). "Overflow" suggests something being too full, which is not the case here, we're just using a block (you'd need to flip the true/false too).

@spinda
Copy link
Contributor Author

spinda commented Aug 21, 2017

@nrc That would seem to imply always forcing a block for closures and match arms, whereas this option only forces a block when the respective constructs "overflow" a single line.

@nrc
Copy link
Member

nrc commented Aug 21, 2017

Perhaps multline_closure_forces_block or something similar? The issues with the current names are that they don't indicate what happens on overflow, and "overflow" is not a term we use in Rustfmt at the moment.

@spinda
Copy link
Contributor Author

spinda commented Aug 21, 2017

I like multiline_closure_forces_block. My original choice of overflow did in fact come from the rustfmt code, though; see https://github.com/rust-lang-nursery/rustfmt/pull/1898/files#diff-180bb3ad456cc10517d878fb7795341cR1708 for example (can_be_overflowed).

multiline_closure_forces_block = false (default):
    result.and_then(|maybe_value| match maybe_value {
        None => ...,
        Some(value) => ...,
    })

multiline_closure_forces_block = true:
    result.and_then(|maybe_value| {
        match maybe_value {
            None => ...,
            Some(value) => ...,
        }
    })

multiline_match_arm_forces_block = false (default):
    match lorem {
        None => if ipsum {
            println!("Hello World");
        },
        Some(dolor) => ...,
    }

multiline_match_arm_forces_block = true:
    match lorem {
        None => {
            if ipsum {
                println!("Hello World");
            }
        }
        Some(dolor) => ...,
    }
@nrc nrc merged commit ba6121c into rust-lang:master Aug 24, 2017
@nrc
Copy link
Member

nrc commented Aug 24, 2017

Thanks for making the changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants