Prefer to break arguments over putting output type on the next line (r…

…ust-lang#3190)
rust-lang-ci · Nov 18, 2018 · 1dd54e6 · 1dd54e6
1 parent ef4176a
commit 1dd54e6
Show file tree

Hide file tree

Showing 5 changed files with 74 additions and 49 deletions.
diff --git a/src/types.rs b/src/types.rs
@@ -289,6 +289,19 @@ where
 {
     debug!("format_function_type {:#?}", shape);
 
+    let ty_shape = match context.config.indent_style() {
+        // 4 = " -> "
+        IndentStyle::Block => shape.offset_left(4)?,
+        IndentStyle::Visual => shape.block_left(4)?,
+    };
+    let output = match *output {
+        FunctionRetTy::Ty(ref ty) => {
+            let type_str = ty.rewrite(context, ty_shape)?;
+            format!(" -> {}", type_str)
+        }
+        FunctionRetTy::Default(..) => String::new(),
+    };
+
     // Code for handling variadics is somewhat duplicated for items, but they
     // are different enough to need some serious refactoring to share code.
     enum ArgumentKind<T>
@@ -307,19 +320,18 @@ where
         None
     };
 
-    // 2 for ()
-    let budget = shape.width.checked_sub(2)?;
-    // 1 for (
-    let offset = match context.config.indent_style() {
-        IndentStyle::Block => {
-            shape
-                .block()
-                .block_indent(context.config.tab_spaces())
-                .indent
-        }
-        IndentStyle::Visual => shape.indent + 1,
+    let list_shape = if context.use_block_indent() {
+        Shape::indented(
+            shape.block().indent.block_indent(context.config),
+            context.config,
+        )
+    } else {
+        // 2 for ()
+        let budget = shape.width.checked_sub(2)?;
+        // 1 for (
+        let offset = shape.indent + 1;
+        Shape::legacy(budget, offset)
     };
-    let list_shape = Shape::legacy(budget, offset);
     let list_lo = context.snippet_provider.span_after(span, "(");
     let items = itemize_list(
         context.snippet_provider,
@@ -345,12 +357,18 @@ where
 
     let item_vec: Vec<_> = items.collect();
 
-    let tactic = definitive_tactic(
-        &*item_vec,
-        ListTactic::HorizontalVertical,
-        Separator::Comma,
-        budget,
-    );
+    // If the return type is multi-lined, then force to use multiple lines for
+    // arguments as well.
+    let tactic = if output.contains('\n') {
+        DefinitiveListTactic::Vertical
+    } else {
+        definitive_tactic(
+            &*item_vec,
+            ListTactic::HorizontalVertical,
+            Separator::Comma,
+            shape.width.saturating_sub(2 + output.len()),
+        )
+    };
     let trailing_separator = if !context.use_block_indent() || variadic {
         SeparatorTactic::Never
     } else {
@@ -364,27 +382,12 @@ where
         .preserve_newline(true);
     let list_str = write_list(&item_vec, &fmt)?;
 
-    let ty_shape = match context.config.indent_style() {
-        // 4 = " -> "
-        IndentStyle::Block => shape.offset_left(4)?,
-        IndentStyle::Visual => shape.block_left(4)?,
-    };
-    let output = match *output {
-        FunctionRetTy::Ty(ref ty) => {
-            let type_str = ty.rewrite(context, ty_shape)?;
-            format!(" -> {}", type_str)
-        }
-        FunctionRetTy::Default(..) => String::new(),
-    };
-
-    let args = if (!list_str.contains('\n') || list_str.is_empty()) && !output.contains('\n')
-        || !context.use_block_indent()
-    {
+    let args = if tactic == DefinitiveListTactic::Horizontal || !context.use_block_indent() {
         format!("({})", list_str)
     } else {
         format!(
             "({}{}{})",
-            offset.to_string_with_newline(context.config),
+            list_shape.indent.to_string_with_newline(context.config),
             list_str,
             shape.block().indent.to_string_with_newline(context.config),
         )
@@ -395,7 +398,7 @@ where
         Some(format!(
             "{}\n{}{}",
             args,
-            offset.to_string(context.config),
+            list_shape.indent.to_string(context.config),
             output.trim_left()
         ))
     }

diff --git a/tests/source/type.rs b/tests/source/type.rs
@@ -129,3 +129,9 @@ fn issue3139() {
         json!(  { "test": None  ::  <i32> }  )
     );
 }
+
+// #3180
+fn foo(a: SomeLongComplexType, b: SomeOtherLongComplexType) -> Box<Future<Item = AnotherLongType, Error = ALongErrorType>> {
+}
+
+type MyFn = fn(a: SomeLongComplexType, b: SomeOtherLongComplexType,) -> Box<Future<Item = AnotherLongType, Error = ALongErrorType>>;
diff --git a/tests/target/fn-simple.rs b/tests/target/fn-simple.rs
@@ -9,8 +9,9 @@ fn simple(
         x: Typ,
         key: &[u8],
         upd: Box<
-            Fn(Option<&memcache::Item>)
-                -> (memcache::Status, Result<memcache::Item, Option<String>>),
+            Fn(
+                Option<&memcache::Item>,
+            ) -> (memcache::Status, Result<memcache::Item, Option<String>>),
         >,
     ) -> MapResult {
     }

diff --git a/tests/target/issue-2164.rs b/tests/target/issue-2164.rs
@@ -56,8 +56,10 @@ pub struct emacs_env_25 {
         ) -> emacs_value,
     >,
     pub intern: ::std::option::Option<
-        unsafe extern "C" fn(env: *mut emacs_env, symbol_name: *const ::libc::c_char)
-            -> emacs_value,
+        unsafe extern "C" fn(
+            env: *mut emacs_env,
+            symbol_name: *const ::libc::c_char,
+        ) -> emacs_value,
     >,
     pub type_of: ::std::option::Option<
         unsafe extern "C" fn(env: *mut emacs_env, value: emacs_value) -> emacs_value,
@@ -87,15 +89,16 @@ pub struct emacs_env_25 {
         ) -> bool,
     >,
     pub make_string: ::std::option::Option<
-        unsafe extern "C" fn(env: *mut emacs_env, contents: *const ::libc::c_char, length: isize)
-            -> emacs_value,
+        unsafe extern "C" fn(
+            env: *mut emacs_env,
+            contents: *const ::libc::c_char,
+            length: isize,
+        ) -> emacs_value,
     >,
     pub make_user_ptr: ::std::option::Option<
         unsafe extern "C" fn(
             env: *mut emacs_env,
-            fin: ::std::option::Option<
-                unsafe extern "C" fn(arg1: *mut ::libc::c_void),
-            >,
+            fin: ::std::option::Option<unsafe extern "C" fn(arg1: *mut ::libc::c_void)>,
             ptr: *mut ::libc::c_void,
         ) -> emacs_value,
     >,
@@ -107,7 +110,9 @@ pub struct emacs_env_25 {
     >,
     pub get_user_finalizer: ::std::option::Option<
         unsafe extern "C" fn(
-            arg1: *mut ::libc::c_void, env: *mut emacs_env, uptr: emacs_value
+            arg1: *mut ::libc::c_void,
+            env: *mut emacs_env,
+            uptr: emacs_value,
         ) -> ::std::option::Option<
             unsafe extern "C" fn(arg1: *mut ::libc::c_void, env: *mut emacs_env, uptr: emacs_value),
         >,
@@ -116,9 +121,7 @@ pub struct emacs_env_25 {
         unsafe extern "C" fn(
             env: *mut emacs_env,
             uptr: emacs_value,
-            fin: ::std::option::Option<
-                unsafe extern "C" fn(arg1: *mut ::libc::c_void),
-            >,
+            fin: ::std::option::Option<unsafe extern "C" fn(arg1: *mut ::libc::c_void)>,
         ),
     >,
     pub vec_get: ::std::option::Option<

diff --git a/tests/target/type.rs b/tests/target/type.rs
@@ -128,3 +128,15 @@ fn issue3139() {
         json!({ "test": None::<i32> })
     );
 }
+
+// #3180
+fn foo(
+    a: SomeLongComplexType,
+    b: SomeOtherLongComplexType,
+) -> Box<Future<Item = AnotherLongType, Error = ALongErrorType>> {
+}
+
+type MyFn = fn(
+    a: SomeLongComplexType,
+    b: SomeOtherLongComplexType,
+) -> Box<Future<Item = AnotherLongType, Error = ALongErrorType>>;