Skip to content

Commit f15dd87

Browse files
nrcnrc
authored
Merge pull request #1877 from topecongiro/overflowing-closure-with-loop
Force to use block for closure body with a single control flow expression
2 parents 175c0c6 + bfaeac2 commit f15dd87

12 files changed

+163
-71
lines changed

‎src/bin/cargo-fmt.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,10 @@ fn format_crate(
133133
let files: Vec<_> = targets
134134
.into_iter()
135135
.filter(|t| t.kind.should_format())
136-
.inspect(|t| if verbosity == Verbosity::Verbose {
137-
println!("[{:?}] {:?}", t.kind, t.path)
136+
.inspect(|t| {
137+
if verbosity == Verbosity::Verbose {
138+
println!("[{:?}] {:?}", t.kind, t.path)
139+
}
138140
})
139141
.map(|t| t.path)
140142
.collect();

‎src/chains.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -297,10 +297,12 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
297297

298298
// True if the chain is only `?`s.
299299
fn chain_only_try(exprs: &[ast::Expr]) -> bool {
300-
exprs.iter().all(|e| if let ast::ExprKind::Try(_) = e.node {
301-
true
302-
} else {
303-
false
300+
exprs.iter().all(|e| {
301+
if let ast::ExprKind::Try(_) = e.node {
302+
true
303+
} else {
304+
false
305+
}
304306
})
305307
}
306308

‎src/comment.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -311,10 +311,12 @@ fn rewrite_comment_inner(
311311
line
312312
})
313313
.map(|s| left_trim_comment_line(s, &style))
314-
.map(|line| if orig.starts_with("/*") && line_breaks == 0 {
315-
line.trim_left()
316-
} else {
317-
line
314+
.map(|line| {
315+
if orig.starts_with("/*") && line_breaks == 0 {
316+
line.trim_left()
317+
} else {
318+
line
319+
}
318320
});
319321

320322
let mut result = opener.to_owned();

‎src/expr.rs

Lines changed: 84 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -630,8 +630,7 @@ fn rewrite_closure(
630630
}
631631

632632
// Figure out if the block is necessary.
633-
let needs_block = block.rules != ast::BlockCheckMode::Default || block.stmts.len() > 1
634-
|| context.inside_macro
633+
let needs_block = is_unsafe_block(block) || block.stmts.len() > 1 || context.inside_macro
635634
|| block_contains_comment(block, context.codemap)
636635
|| prefix.contains('\n');
637636

@@ -642,8 +641,12 @@ fn rewrite_closure(
642641
};
643642
if no_return_type && !needs_block {
644643
// block.stmts.len() == 1
645-
if let Some(expr) = stmt_expr(&block.stmts[0]) {
646-
if let Some(rw) = rewrite_closure_expr(expr, &prefix, context, body_shape) {
644+
if let Some(ref expr) = stmt_expr(&block.stmts[0]) {
645+
if let Some(rw) = if is_block_closure_forced(expr) {
646+
rewrite_closure_with_block(context, shape, &prefix, expr)
647+
} else {
648+
rewrite_closure_expr(expr, &prefix, context, body_shape)
649+
} {
647650
return Some(rw);
648651
}
649652
}
@@ -1195,12 +1198,13 @@ impl<'a> ControlFlow<'a> {
11951198
context
11961199
.codemap
11971200
.span_after(mk_sp(lo, self.span.hi()), self.keyword.trim()),
1198-
self.pat
1199-
.map_or(cond_span.lo(), |p| if self.matcher.is_empty() {
1201+
self.pat.map_or(cond_span.lo(), |p| {
1202+
if self.matcher.is_empty() {
12001203
p.span.lo()
12011204
} else {
12021205
context.codemap.span_before(self.span, self.matcher.trim())
1203-
}),
1206+
}
1207+
}),
12041208
);
12051209

12061210
let between_kwd_cond_comment = extract_comment(between_kwd_cond, context, shape);
@@ -2248,7 +2252,34 @@ fn rewrite_last_closure(
22482252
if prefix.contains('\n') {
22492253
return None;
22502254
}
2255+
// If we are inside macro, we do not want to add or remove block from closure body.
2256+
if context.inside_macro {
2257+
return expr.rewrite(context, shape);
2258+
}
2259+
22512260
let body_shape = shape.offset_left(extra_offset)?;
2261+
2262+
// We force to use block for the body of the closure for certain kinds of expressions.
2263+
if is_block_closure_forced(body) {
2264+
return rewrite_closure_with_block(context, body_shape, &prefix, body).and_then(
2265+
|body_str| {
2266+
// If the expression can fit in a single line, we need not force block closure.
2267+
if body_str.lines().count() <= 7 {
2268+
match rewrite_closure_expr(body, &prefix, context, shape) {
2269+
Some(ref single_line_body_str)
2270+
if !single_line_body_str.contains('\n') =>
2271+
{
2272+
Some(single_line_body_str.clone())
2273+
}
2274+
_ => Some(body_str),
2275+
}
2276+
} else {
2277+
Some(body_str)
2278+
}
2279+
},
2280+
);
2281+
}
2282+
22522283
// When overflowing the closure which consists of a single control flow expression,
22532284
// force to use block if its condition uses multi line.
22542285
let is_multi_lined_cond = rewrite_cond(context, body, body_shape)
@@ -2264,6 +2295,23 @@ fn rewrite_last_closure(
22642295
None
22652296
}
22662297

2298+
fn is_block_closure_forced(expr: &ast::Expr) -> bool {
2299+
match expr.node {
2300+
ast::ExprKind::If(..) |
2301+
ast::ExprKind::IfLet(..) |
2302+
ast::ExprKind::Loop(..) |
2303+
ast::ExprKind::While(..) |
2304+
ast::ExprKind::WhileLet(..) |
2305+
ast::ExprKind::ForLoop(..) => true,
2306+
ast::ExprKind::AddrOf(_, ref expr) |
2307+
ast::ExprKind::Box(ref expr) |
2308+
ast::ExprKind::Try(ref expr) |
2309+
ast::ExprKind::Unary(_, ref expr) |
2310+
ast::ExprKind::Cast(ref expr, _) => is_block_closure_forced(expr),
2311+
_ => false,
2312+
}
2313+
}
2314+
22672315
fn rewrite_last_arg_with_overflow<'a, T>(
22682316
context: &RewriteContext,
22692317
args: &[&T],
@@ -2281,15 +2329,7 @@ where
22812329
ast::ExprKind::Closure(..) => {
22822330
// If the argument consists of multiple closures, we do not overflow
22832331
// the last closure.
2284-
if args.len() > 1
2285-
&& args.iter()
2286-
.rev()
2287-
.skip(1)
2288-
.filter_map(|arg| arg.to_expr())
2289-
.any(|expr| match expr.node {
2290-
ast::ExprKind::Closure(..) => true,
2291-
_ => false,
2292-
}) {
2332+
if args_have_many_closure(args) {
22932333
None
22942334
} else {
22952335
rewrite_last_closure(context, expr, shape)
@@ -2310,6 +2350,23 @@ where
23102350
}
23112351
}
23122352

2353+
/// Returns true if the given vector of arguments has more than one `ast::ExprKind::Closure`.
2354+
fn args_have_many_closure<T>(args: &[&T]) -> bool
2355+
where
2356+
T: ToExpr,
2357+
{
2358+
args.iter()
2359+
.filter(|arg| {
2360+
arg.to_expr()
2361+
.map(|e| match e.node {
2362+
ast::ExprKind::Closure(..) => true,
2363+
_ => false,
2364+
})
2365+
.unwrap_or(false)
2366+
})
2367+
.count() > 1
2368+
}
2369+
23132370
fn can_be_overflowed<'a, T>(context: &RewriteContext, args: &[&T]) -> bool
23142371
where
23152372
T: Rewrite + Spanned + ToExpr + 'a,
@@ -2698,13 +2755,17 @@ where
26982755
if items.len() == 1 {
26992756
// 3 = "(" + ",)"
27002757
let nested_shape = shape.sub_width(3)?.visual_indent(1);
2701-
return items.next().unwrap().rewrite(context, nested_shape).map(
2702-
|s| if context.config.spaces_within_parens() {
2703-
format!("( {}, )", s)
2704-
} else {
2705-
format!("({},)", s)
2706-
},
2707-
);
2758+
return items
2759+
.next()
2760+
.unwrap()
2761+
.rewrite(context, nested_shape)
2762+
.map(|s| {
2763+
if context.config.spaces_within_parens() {
2764+
format!("( {}, )", s)
2765+
} else {
2766+
format!("({},)", s)
2767+
}
2768+
});
27082769
}
27092770

27102771
let list_lo = context.codemap.span_after(span, "(");

‎src/types.rs

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -670,10 +670,12 @@ impl Rewrite for ast::Ty {
670670
ast::TyKind::Paren(ref ty) => {
671671
let budget = shape.width.checked_sub(2)?;
672672
ty.rewrite(context, Shape::legacy(budget, shape.indent + 1))
673-
.map(|ty_str| if context.config.spaces_within_parens() {
674-
format!("( {} )", ty_str)
675-
} else {
676-
format!("({})", ty_str)
673+
.map(|ty_str| {
674+
if context.config.spaces_within_parens() {
675+
format!("( {} )", ty_str)
676+
} else {
677+
format!("({})", ty_str)
678+
}
677679
})
678680
}
679681
ast::TyKind::Slice(ref ty) => {
@@ -683,10 +685,12 @@ impl Rewrite for ast::Ty {
683685
shape.width.checked_sub(2)?
684686
};
685687
ty.rewrite(context, Shape::legacy(budget, shape.indent + 1))
686-
.map(|ty_str| if context.config.spaces_within_square_brackets() {
687-
format!("[ {} ]", ty_str)
688-
} else {
689-
format!("[{}]", ty_str)
688+
.map(|ty_str| {
689+
if context.config.spaces_within_square_brackets() {
690+
format!("[ {} ]", ty_str)
691+
} else {
692+
format!("[{}]", ty_str)
693+
}
690694
})
691695
}
692696
ast::TyKind::Tup(ref items) => rewrite_tuple(

‎src/vertical.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -191,13 +191,13 @@ fn struct_field_prefix_max_min_width<T: AlignedItem>(
191191
fields
192192
.iter()
193193
.map(|field| {
194-
field
195-
.rewrite_prefix(context, shape)
196-
.and_then(|field_str| if field_str.contains('\n') {
194+
field.rewrite_prefix(context, shape).and_then(|field_str| {
195+
if field_str.contains('\n') {
197196
None
198197
} else {
199198
Some(field_str.len())
200-
})
199+
}
200+
})
201201
})
202202
.fold(Some((0, ::std::usize::MAX)), |acc, len| match (acc, len) {
203203
(Some((max_len, min_len)), Some(len)) => {

‎src/visitor.rs

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -136,13 +136,15 @@ impl<'a> FmtVisitor<'a> {
136136
self.last_pos,
137137
attr_lo.unwrap_or(first_stmt.span.lo()),
138138
));
139-
let len = CommentCodeSlices::new(&snippet).nth(0).and_then(
140-
|(kind, _, s)| if kind == CodeCharKind::Normal {
141-
s.rfind('\n')
142-
} else {
143-
None
144-
},
145-
);
139+
let len = CommentCodeSlices::new(&snippet)
140+
.nth(0)
141+
.and_then(|(kind, _, s)| {
142+
if kind == CodeCharKind::Normal {
143+
s.rfind('\n')
144+
} else {
145+
None
146+
}
147+
});
146148
if let Some(len) = len {
147149
self.last_pos = self.last_pos + BytePos::from_usize(len);
148150
}

‎tests/source/closure-block-inside-macro.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
// rustfmt-fn_call_style: Block
2+
3+
// #1547
4+
fuzz_target!(|data: &[u8]| if let Some(first) = data.first() {
5+
let index = *first as usize;
6+
if index >= ENCODINGS.len() {
7+
return;
8+
}
9+
let encoding = ENCODINGS[index];
10+
dispatch_test(encoding, &data[1..]);
11+
});

‎tests/system.rs

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -446,15 +446,17 @@ struct CharsIgnoreNewlineRepr<'a>(Peekable<Chars<'a>>);
446446
impl<'a> Iterator for CharsIgnoreNewlineRepr<'a> {
447447
type Item = char;
448448
fn next(&mut self) -> Option<char> {
449-
self.0.next().map(|c| if c == '\r' {
450-
if *self.0.peek().unwrap_or(&'\0') == '\n' {
451-
self.0.next();
452-
'\n'
449+
self.0.next().map(|c| {
450+
if c == '\r' {
451+
if *self.0.peek().unwrap_or(&'\0') == '\n' {
452+
self.0.next();
453+
'\n'
454+
} else {
455+
'\r'
456+
}
453457
} else {
454-
'\r'
458+
c
455459
}
456-
} else {
457-
c
458460
})
459461
}
460462
}

‎tests/target/chains-visual.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,12 @@ fn main() {
2121
false => (),
2222
});
2323

24-
loong_func().quux(move || if true {
25-
1
26-
} else {
27-
2
24+
loong_func().quux(move || {
25+
if true {
26+
1
27+
} else {
28+
2
29+
}
2830
});
2931

3032
some_fuuuuuuuuunction().method_call_a(aaaaa, bbbbb, |c| {

‎tests/target/chains.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,12 @@ fn main() {
2323
false => (),
2424
});
2525

26-
loong_func().quux(move || if true {
27-
1
28-
} else {
29-
2
26+
loong_func().quux(move || {
27+
if true {
28+
1
29+
} else {
30+
2
31+
}
3032
});
3133

3234
some_fuuuuuuuuunction().method_call_a(aaaaa, bbbbb, |c| {

‎tests/target/hard-tabs.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,12 @@ fn main() {
7474
arg(a, b, c, d, e)
7575
}
7676

77-
loong_func().quux(move || if true {
78-
1
79-
} else {
80-
2
77+
loong_func().quux(move || {
78+
if true {
79+
1
80+
} else {
81+
2
82+
}
8183
});
8284

8385
fffffffffffffffffffffffffffffffffff(a, {

0 commit comments

Comments
 (0)