Skip to content

Disallow combining a method call with prefix or suffix #2730

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 23, 2018
Merged

Disallow combining a method call with prefix or suffix #2730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d098065
Add tests for #2704.
topecongiro May 22, 2018
fbcc886
Disallow combining a method call with prefix or suffix
topecongiro May 22, 2018
8cb2b8e
Cargo fmt
topecongiro May 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2172,3 +2172,15 @@ impl ToExpr for ast::GenericParam {
false
}
}

pub fn is_method_call(expr: &ast::Expr) -> bool {
match expr.node {
ast::ExprKind::MethodCall(..) => true,
ast::ExprKind::AddrOf(_, ref expr)
| ast::ExprKind::Box(ref expr)
| ast::ExprKind::Cast(ref expr, _)
| ast::ExprKind::Try(ref expr)
| ast::ExprKind::Unary(_, ref expr) => is_method_call(expr),
_ => false,
}
}
6 changes: 3 additions & 3 deletions src/items.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1380,9 +1380,9 @@ fn format_tuple_struct(
// We need to put the where clause on a new line, but we didn't
// know that earlier, so the where clause will not be indented properly.
result.push('\n');
result
.push_str(&(offset.block_only() + (context.config.tab_spaces() - 1))
.to_string(context.config));
result.push_str(
&(offset.block_only() + (context.config.tab_spaces() - 1)).to_string(context.config),
);
}
result.push_str(&where_clause_str);

Expand Down
6 changes: 3 additions & 3 deletions src/overflow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use syntax::parse::token::DelimToken;

use closures;
use codemap::SpanUtils;
use expr::{is_every_expr_simple, is_nested_call, maybe_get_args_offset, ToExpr};
use expr::{is_every_expr_simple, is_method_call, is_nested_call, maybe_get_args_offset, ToExpr};
use lists::{definitive_tactic, itemize_list, write_list, ListFormatting, ListItem, Separator};
use rewrite::{Rewrite, RewriteContext};
use shape::Shape;
Expand Down Expand Up @@ -231,8 +231,8 @@ impl<'a, T: 'a + Rewrite + ToExpr + Spanned> Context<'a, T> {
let placeholder = if overflow_last {
let old_value = *self.context.force_one_line_chain.borrow();
if !combine_arg_with_callee {
if let Some(expr) = self.last_item().and_then(|item| item.to_expr()) {
if let ast::ExprKind::MethodCall(..) = expr.node {
if let Some(ref expr) = self.last_item().and_then(|item| item.to_expr()) {
if is_method_call(expr) {
self.context.force_one_line_chain.replace(true);
}
}
Expand Down
38 changes: 38 additions & 0 deletions tests/source/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,3 +398,41 @@ fn foo() {
let my_var =
Mutex::new(RpcClientType::connect(server_iddd).chain_err(|| "Unable to create RPC client")?);
}

// #2704
// Method call with prefix and suffix.
fn issue2704() {
// We should not combine the callee with a multi-lined method call.
let requires = requires.set(&requires0
.concat(&requires1)
.concat(&requires2)
.distinct_total());
let requires = requires.set(box requires0
.concat(&requires1)
.concat(&requires2)
.distinct_total());
let requires = requires.set(requires0
.concat(&requires1)
.concat(&requires2)
.distinct_total() as u32);
let requires = requires.set(requires0
.concat(&requires1)
.concat(&requires2)
.distinct_total()?);
let requires = requires.set(!requires0
.concat(&requires1)
.concat(&requires2)
.distinct_total());
// We should combine a small callee with an argument.
bar(vec![22]
.into_iter()
.map(|x| x * 2)
.filter(|_| true)
.collect());
// But we should not combine a long callee with an argument.
barrrr(vec![22]
.into_iter()
.map(|x| x * 2)
.filter(|_| true)
.collect());
}
52 changes: 51 additions & 1 deletion tests/target/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,56 @@ fn dots() {
// A function call with a large single argument.
fn foo() {
let my_var = Mutex::new(
RpcClientType::connect(server_iddd).chain_err(|| "Unable to create RPC client")?
RpcClientType::connect(server_iddd).chain_err(|| "Unable to create RPC client")?,
);
}

// #2704
// Method call with prefix and suffix.
fn issue2704() {
// We should not combine the callee with a multi-lined method call.
let requires = requires.set(
&requires0
.concat(&requires1)
.concat(&requires2)
.distinct_total(),
);
let requires = requires.set(
box requires0
.concat(&requires1)
.concat(&requires2)
.distinct_total(),
);
let requires = requires.set(
requires0
.concat(&requires1)
.concat(&requires2)
.distinct_total() as u32,
);
let requires = requires.set(
requires0
.concat(&requires1)
.concat(&requires2)
.distinct_total()?,
);
let requires = requires.set(
!requires0
.concat(&requires1)
.concat(&requires2)
.distinct_total(),
);
// We should combine a small callee with an argument.
bar(vec![22]
.into_iter()
Copy link
Member

Choose a reason for hiding this comment

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

I think we should probably not combine in this case - I feel like the benefit of uniformity outweights the slight ugliness of the floating second line. Also, I think that the floating second line is worse if it beings with punctuation, rather than a name. E.g.,

// pretty bad
x
    .foo
    .bar

// not as bad
x(
    foo.bar
)

Copy link
Member

Choose a reason for hiding this comment

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

And the opening parens make this case look better too - similar to braces in a block (probably more important than the punctuation vs identifier distinction).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I though that we decided to combine a short function call and a single argument in #2178, but no?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, we did, sorry (I'm not 100% sure I agree with my old self, but lets try it out).

.map(|x| x * 2)
.filter(|_| true)
.collect());
// But we should not combine a long callee with an argument.
barrrr(
vec![22]
.into_iter()
.map(|x| x * 2)
.filter(|_| true)
.collect(),
);
}