Skip to content

Commit cf11f7b

Browse files
nrcnrc
authored
Merge pull request #2203 from topecongiro/issue-2200
Fix budget-related bugs when rewriting chain
2 parents 426ba1c + a6d94b9 commit cf11f7b

File tree

13 files changed

+142
-95
lines changed

13 files changed

+142
-95
lines changed

‎src/bin/rustfmt.rs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,9 +87,10 @@ impl CliOptions {
8787
if let Ok(write_mode) = WriteMode::from_str(write_mode) {
8888
options.write_mode = Some(write_mode);
8989
} else {
90-
return Err(FmtError::from(
91-
format!("Invalid write-mode: {}", write_mode),
92-
));
90+
return Err(FmtError::from(format!(
91+
"Invalid write-mode: {}",
92+
write_mode
93+
)));
9394
}
9495
}
9596

‎src/chains.rs

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -147,9 +147,8 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
147147
let last_subexpr = &subexpr_list[suffix_try_num];
148148
let subexpr_list = &subexpr_list[suffix_try_num..subexpr_num - prefix_try_num];
149149
let iter = subexpr_list.iter().skip(1).rev().zip(child_shape_iter);
150-
let mut rewrites = iter.map(|(e, shape)| {
151-
rewrite_chain_subexpr(e, total_span, context, shape)
152-
}).collect::<Option<Vec<_>>>()?;
150+
let mut rewrites = iter.map(|(e, shape)| rewrite_chain_subexpr(e, total_span, context, shape))
151+
.collect::<Option<Vec<_>>>()?;
153152

154153
// Total of all items excluding the last.
155154
let extend_last_subexpr = last_line_extendable(&parent_rewrite) && rewrites.is_empty();
@@ -166,18 +165,11 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
166165
let all_in_one_line = !parent_rewrite_contains_newline
167166
&& rewrites.iter().all(|s| !s.contains('\n'))
168167
&& almost_total < one_line_budget;
169-
let last_shape = {
170-
let last_shape = if rewrites.len() == 0 {
171-
first_child_shape
172-
} else {
173-
other_child_shape
174-
};
175-
match context.config.indent_style() {
176-
IndentStyle::Visual => last_shape.sub_width(shape.rhs_overhead(context.config))?,
177-
IndentStyle::Block => last_shape,
178-
}
179-
};
180-
let last_shape = last_shape.sub_width(suffix_try_num)?;
168+
let last_shape = if rewrites.len() == 0 {
169+
first_child_shape
170+
} else {
171+
other_child_shape
172+
}.sub_width(shape.rhs_overhead(context.config) + suffix_try_num)?;
181173

182174
// Rewrite the last child. The last child of a chain requires special treatment. We need to
183175
// know whether 'overflowing' the last child make a better formatting:

‎src/comment.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -224,9 +224,7 @@ pub fn rewrite_comment(
224224
// we should stop now.
225225
let num_bare_lines = orig.lines()
226226
.map(|line| line.trim())
227-
.filter(|l| {
228-
!(l.starts_with('*') || l.starts_with("//") || l.starts_with("/*"))
229-
})
227+
.filter(|l| !(l.starts_with('*') || l.starts_with("//") || l.starts_with("/*")))
230228
.count();
231229
if num_bare_lines > 0 && !config.normalize_comments() {
232230
return Some(orig.to_owned());

‎src/expr.rs

Lines changed: 50 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -292,9 +292,7 @@ pub fn format_expr(
292292
};
293293

294294
expr_rw
295-
.and_then(|expr_str| {
296-
recover_comment_removed(expr_str, expr.span, context)
297-
})
295+
.and_then(|expr_str| recover_comment_removed(expr_str, expr.span, context))
298296
.and_then(|expr_str| {
299297
let attrs = outer_attributes(&expr.attrs);
300298
let attrs_str = attrs.rewrite(context, shape)?;
@@ -1814,6 +1812,10 @@ where
18141812
let used_width = extra_offset(callee_str, shape);
18151813
let one_line_width = shape.width.checked_sub(used_width + 2 * paren_overhead)?;
18161814

1815+
// 1 = "(" or ")"
1816+
let one_line_shape = shape
1817+
.offset_left(last_line_width(callee_str) + 1)?
1818+
.sub_width(1)?;
18171819
let nested_shape = shape_from_indent_style(
18181820
context,
18191821
shape,
@@ -1828,6 +1830,7 @@ where
18281830
context,
18291831
args,
18301832
args_span,
1833+
one_line_shape,
18311834
nested_shape,
18321835
one_line_width,
18331836
args_max_width,
@@ -1867,7 +1870,8 @@ fn rewrite_call_args<'a, T>(
18671870
context: &RewriteContext,
18681871
args: &[&T],
18691872
span: Span,
1870-
shape: Shape,
1873+
one_line_shape: Shape,
1874+
nested_shape: Shape,
18711875
one_line_width: usize,
18721876
args_max_width: usize,
18731877
force_trailing_comma: bool,
@@ -1882,7 +1886,7 @@ where
18821886
",",
18831887
|item| item.span().lo(),
18841888
|item| item.span().hi(),
1885-
|item| item.rewrite(context, shape),
1889+
|item| item.rewrite(context, nested_shape),
18861890
span.lo(),
18871891
span.hi(),
18881892
true,
@@ -1896,7 +1900,8 @@ where
18961900
context,
18971901
&mut item_vec,
18981902
&args[..],
1899-
shape,
1903+
one_line_shape,
1904+
nested_shape,
19001905
one_line_width,
19011906
args_max_width,
19021907
);
@@ -1912,22 +1917,21 @@ where
19121917
context.config.trailing_comma()
19131918
},
19141919
separator_place: SeparatorPlace::Back,
1915-
shape: shape,
1920+
shape: nested_shape,
19161921
ends_with_newline: context.use_block_indent() && tactic == DefinitiveListTactic::Vertical,
19171922
preserve_newline: false,
19181923
config: context.config,
19191924
};
19201925

1921-
write_list(&item_vec, &fmt).map(|args_str| {
1922-
(tactic != DefinitiveListTactic::Vertical, args_str)
1923-
})
1926+
write_list(&item_vec, &fmt).map(|args_str| (tactic != DefinitiveListTactic::Vertical, args_str))
19241927
}
19251928

19261929
fn try_overflow_last_arg<'a, T>(
19271930
context: &RewriteContext,
19281931
item_vec: &mut Vec<ListItem>,
19291932
args: &[&T],
1930-
shape: Shape,
1933+
one_line_shape: Shape,
1934+
nested_shape: Shape,
19311935
one_line_width: usize,
19321936
args_max_width: usize,
19331937
) -> DefinitiveListTactic
@@ -1945,7 +1949,7 @@ where
19451949
context.force_one_line_chain = true;
19461950
}
19471951
}
1948-
last_arg_shape(&context, item_vec, shape, args_max_width).and_then(|arg_shape| {
1952+
last_arg_shape(args, item_vec, one_line_shape, args_max_width).and_then(|arg_shape| {
19491953
rewrite_last_arg_with_overflow(&context, args, &mut item_vec[args.len() - 1], arg_shape)
19501954
})
19511955
} else {
@@ -1992,26 +1996,32 @@ where
19921996
tactic
19931997
}
19941998

1995-
fn last_arg_shape(
1996-
context: &RewriteContext,
1999+
/// Returns a shape for the last argument which is going to be overflowed.
2000+
fn last_arg_shape<T>(
2001+
lists: &[&T],
19972002
items: &[ListItem],
19982003
shape: Shape,
19992004
args_max_width: usize,
2000-
) -> Option<Shape> {
2001-
let overhead = items.iter().rev().skip(1).fold(0, |acc, i| {
2002-
acc + i.item.as_ref().map_or(0, |s| first_line_width(s))
2005+
) -> Option<Shape>
2006+
where
2007+
T: Rewrite + Spanned + ToExpr,
2008+
{
2009+
let is_nested_call = lists
2010+
.iter()
2011+
.next()
2012+
.and_then(|item| item.to_expr())
2013+
.map_or(false, is_nested_call);
2014+
if items.len() == 1 && !is_nested_call {
2015+
return Some(shape);
2016+
}
2017+
let offset = items.iter().rev().skip(1).fold(0, |acc, i| {
2018+
// 2 = ", "
2019+
acc + 2 + i.inner_as_ref().len()
20032020
});
2004-
let max_width = min(args_max_width, shape.width);
2005-
let arg_indent = if context.use_block_indent() {
2006-
shape.block().indent.block_unindent(context.config)
2007-
} else {
2008-
shape.block().indent
2009-
};
2010-
Some(Shape {
2011-
width: max_width.checked_sub(overhead)?,
2012-
indent: arg_indent,
2013-
offset: 0,
2014-
})
2021+
Shape {
2022+
width: min(args_max_width, shape.width),
2023+
..shape
2024+
}.offset_left(offset)
20152025
}
20162026

20172027
fn rewrite_last_arg_with_overflow<'a, T>(
@@ -2093,6 +2103,18 @@ pub fn can_be_overflowed_expr(context: &RewriteContext, expr: &ast::Expr, args_l
20932103
}
20942104
}
20952105

2106+
fn is_nested_call(expr: &ast::Expr) -> bool {
2107+
match expr.node {
2108+
ast::ExprKind::Call(..) | ast::ExprKind::Mac(..) => true,
2109+
ast::ExprKind::AddrOf(_, ref expr)
2110+
| ast::ExprKind::Box(ref expr)
2111+
| ast::ExprKind::Try(ref expr)
2112+
| ast::ExprKind::Unary(_, ref expr)
2113+
| ast::ExprKind::Cast(ref expr, _) => is_nested_call(expr),
2114+
_ => false,
2115+
}
2116+
}
2117+
20962118
pub fn wrap_args_with_parens(
20972119
context: &RewriteContext,
20982120
args_str: &str,

‎src/items.rs

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -817,8 +817,7 @@ fn format_impl_ref_and_type(
817817
IndentStyle::Visual => new_line_offset + trait_ref_overhead,
818818
IndentStyle::Block => new_line_offset,
819819
};
820-
result.push_str(&*self_ty
821-
.rewrite(context, Shape::legacy(budget, type_offset))?);
820+
result.push_str(&*self_ty.rewrite(context, Shape::legacy(budget, type_offset))?);
822821
Some(result)
823822
} else {
824823
unreachable!();
@@ -1578,9 +1577,7 @@ fn rewrite_static(
15781577
lhs,
15791578
&**expr,
15801579
Shape::legacy(remaining_width, offset.block_only()),
1581-
).and_then(|res| {
1582-
recover_comment_removed(res, static_parts.span, context)
1583-
})
1580+
).and_then(|res| recover_comment_removed(res, static_parts.span, context))
15841581
.map(|s| if s.ends_with(';') { s } else { s + ";" })
15851582
} else {
15861583
Some(format!("{}{};", prefix, ty_str))
@@ -2096,18 +2093,14 @@ fn rewrite_args(
20962093
generics_str_contains_newline: bool,
20972094
) -> Option<String> {
20982095
let mut arg_item_strs = args.iter()
2099-
.map(|arg| {
2100-
arg.rewrite(context, Shape::legacy(multi_line_budget, arg_indent))
2101-
})
2096+
.map(|arg| arg.rewrite(context, Shape::legacy(multi_line_budget, arg_indent)))
21022097
.collect::<Option<Vec<_>>>()?;
21032098

21042099
// Account for sugary self.
21052100
// FIXME: the comment for the self argument is dropped. This is blocked
21062101
// on rust issue #27522.
21072102
let min_args = explicit_self
2108-
.and_then(|explicit_self| {
2109-
rewrite_explicit_self(explicit_self, args, context)
2110-
})
2103+
.and_then(|explicit_self| rewrite_explicit_self(explicit_self, args, context))
21112104
.map_or(1, |self_str| {
21122105
arg_item_strs[0] = self_str;
21132106
2
@@ -2326,9 +2319,8 @@ fn rewrite_generics(
23262319
) -> Option<String> {
23272320
let g_shape = generics_shape_from_config(context.config, shape, 0)?;
23282321
let one_line_width = shape.width.checked_sub(2).unwrap_or(0);
2329-
rewrite_generics_inner(context, generics, g_shape, one_line_width, span).or_else(|| {
2330-
rewrite_generics_inner(context, generics, g_shape, 0, span)
2331-
})
2322+
rewrite_generics_inner(context, generics, g_shape, one_line_width, span)
2323+
.or_else(|| rewrite_generics_inner(context, generics, g_shape, 0, span))
23322324
}
23332325

23342326
fn rewrite_generics_inner(

‎src/visitor.rs

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,7 @@ impl<'a> FmtVisitor<'a> {
109109
if self.config.remove_blank_lines_at_start_or_end_of_block() {
110110
if let Some(first_stmt) = b.stmts.first() {
111111
let attr_lo = inner_attrs
112-
.and_then(|attrs| {
113-
inner_attributes(attrs).first().map(|attr| attr.span.lo())
114-
})
112+
.and_then(|attrs| inner_attributes(attrs).first().map(|attr| attr.span.lo()))
115113
.or_else(|| {
116114
// Attributes for an item in a statement position
117115
// do not belong to the statement. (rust-lang/rust#34459)
@@ -872,10 +870,7 @@ fn rewrite_first_group_attrs(
872870
for derive in derives {
873871
derive_args.append(&mut get_derive_args(context, derive)?);
874872
}
875-
return Some((
876-
derives.len(),
877-
format_derive(context, &derive_args, shape)?,
878-
));
873+
return Some((derives.len(), format_derive(context, &derive_args, shape)?));
879874
}
880875
}
881876
// Rewrite the first attribute.

‎tests/source/chains.rs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,3 +185,24 @@ fn issue2126() {
185185
}
186186
}
187187
}
188+
189+
// #2200
190+
impl Foo {
191+
pub fn from_ast(diagnostic: &::errors::Handler,
192+
attrs: &[ast::Attribute]) -> Attributes {
193+
let other_attrs = attrs.iter().filter_map(|attr| {
194+
attr.with_desugared_doc(|attr| {
195+
if attr.check_name("doc") {
196+
if let Some(mi) = attr.meta() {
197+
if let Some(value) = mi.value_str() {
198+
doc_strings.push(DocFragment::Include(line,
199+
attr.span,
200+
filename,
201+
contents));
202+
}
203+
}
204+
}
205+
})
206+
}).collect();
207+
}
208+
}

‎tests/target/chains-visual.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,24 +28,24 @@ fn main() {
2828
});
2929

3030
some_fuuuuuuuuunction().method_call_a(aaaaa, bbbbb, |c| {
31-
let x = c;
32-
x
33-
})
31+
let x = c;
32+
x
33+
})
3434
.method_call_b(aaaaa, bbbbb, |c| {
35-
let x = c;
36-
x
37-
});
35+
let x = c;
36+
x
37+
});
3838

3939
fffffffffffffffffffffffffffffffffff(a, {
4040
SCRIPT_TASK_ROOT.with(|root| {
4141
*root.borrow_mut() = Some(&script_task);
4242
});
4343
});
4444

45-
let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum = xxxxxxx.map(|x| x + 5)
46-
.map(|x| x / 2)
47-
.fold(0,
48-
|acc, x| acc + x);
45+
let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum =
46+
xxxxxxx.map(|x| x + 5)
47+
.map(|x| x / 2)
48+
.fold(0, |acc, x| acc + x);
4949

5050
aaaaaaaaaaaaaaaa.map(|x| {
5151
x += 1;

‎tests/target/chains.rs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,3 +213,28 @@ fn issue2126() {
213213
}
214214
}
215215
}
216+
217+
// #2200
218+
impl Foo {
219+
pub fn from_ast(diagnostic: &::errors::Handler, attrs: &[ast::Attribute]) -> Attributes {
220+
let other_attrs = attrs
221+
.iter()
222+
.filter_map(|attr| {
223+
attr.with_desugared_doc(|attr| {
224+
if attr.check_name("doc") {
225+
if let Some(mi) = attr.meta() {
226+
if let Some(value) = mi.value_str() {
227+
doc_strings.push(DocFragment::Include(
228+
line,
229+
attr.span,
230+
filename,
231+
contents,
232+
));
233+
}
234+
}
235+
}
236+
})
237+
})
238+
.collect();
239+
}
240+
}

0 commit comments

Comments
 (0)