implemented fix for issue 5249

[konnorandrews]

Similar to PR #5252 but copies the structure of types::rewrite_generic_args.

I'm making this PR since #5252 hasn't had any progress since 2022. The tests are copied from #5252.

[ytmimi]

We need to reconsider the implementaion. First, any change we make will need to be gated so that we don't break stable formatting. Second, Some of the diffs produced by these changes seem to break earlier than expected. We should also reach out to the style-team and see if they can provide some guidance on the formatting, and where to introduce line breaks. The Chains of fields and method calls section of the style guide doesn't provide any details on this.

For reference, here are some examples that I've copied inline:

Diff in /tmp/rust-lang-rust-ThUsciGJ/compiler/rustc_middle/src/ty/codec.rs:286:
 impl<'tcx, D: TyDecoder<I = TyCtxt<'tcx>>> Decodable<D> for CanonicalVarInfos<'tcx> {
     fn decode(decoder: &mut D) -> Self {
         let len = decoder.read_usize();
-        decoder.interner().mk_canonical_var_infos_from_iter(
-            (0..len).map::<CanonicalVarInfo<'tcx>, _>(|_| Decodable::decode(decoder)),
-        )
+        decoder.interner().mk_canonical_var_infos_from_iter((0..len).map::<
+            CanonicalVarInfo<'tcx>,
+            _,
+        >(|_| {
+            Decodable::decode(decoder)
+        }))
     }
 }

Diff in /tmp/cargo-DwkOYvLt/src/bin/cargo/commands/uninstall.rs:5:
 pub fn cli() -> Command {
     subcommand("uninstall")
         .about("Remove a Rust binary")
-        .arg(
-            Arg::new("spec")
-                .value_name("SPEC")
-                .num_args(0..)
-                .add::<clap_complete::ArgValueCandidates>(clap_complete::ArgValueCandidates::new(
-                    || get_installed_crates(),
-                )),
-        )
+        .arg(Arg::new("spec").value_name("SPEC").num_args(0..).add::<
+            clap_complete::ArgValueCandidates,
+        >(
+            clap_complete::ArgValueCandidates::new(|| get_installed_crates()),
+        ))
         .arg(opt("root", "Directory to uninstall packages from").value_name("DIR"))
         .arg_silent_suggestion()
         .arg_package_spec_simple("Package to uninstall")

Diff in /tmp/denoland_deno-M19Gksnz/cli/lsp/client.rs:248:
           )
           .await
       }
-      TestingNotification::DeleteModule(params) => self
-        .0
-        .send_notification::<testing_lsp_custom::TestModuleDeleteNotification>(
-          params,
-        )
-        .await,
+      TestingNotification::DeleteModule(params) => {
+        self
+          .0
+          .send_notification::<
+            testing_lsp_custom::TestModuleDeleteNotification,
+          >(params)
+          .await
+      }
       TestingNotification::Progress(params) => {
         self
           .0
Diff in /tmp/denoland_deno-M19Gksnz/cli/lsp/client.rs:271:
   ) {
     self
       .0
-      .send_notification::<lsp_custom::DidRefreshDenoConfigurationTreeNotification>(
-        params,
-      )
+      .send_notification::<
+        lsp_custom::DidRefreshDenoConfigurationTreeNotification,
+      >(params)
       .await
   }

[konnorandrews]

True this needs an edition gate.

Checking the deno ones those are to be expected because deno has max_width = 80.

The others look to be the interaction of not failing on formatting the chain item. The generics will eagerly wrap, which causes the reflow of callers to not always happen. My question would be, should generics of methods eagerly wrap or attempt to force the code before to wrap. I'm not sure how to implement a lazy approach since it would seem to need a last attempt if the caller isn't able to wrap any code.

[konnorandrews]

Maybe instead of allowing method generics to wrap we can allow the chain to keep going. This would solve the cases in the wild I have hit that causes rustfmt to ignore whole chains of methods because one can't fit in the width.

[konnorandrews]

I have put together a prototype at https://github.com/konnorandrews/rustfmt/tree/issue-5249-prototype that follows existing code more closely. The change is to disable the generic wrapping when checking if the last element of a chain fits on the line. Note the prototype doesn't have the edition gate. If anyone has thoughts on the prototype please let me know.

Along with the prototype satisfying all the tests (except tests/target/issue_3191.rs which needs a minor change), here are the results of running the check diff locally:

running rustfmt (feature) on rust-lang-rust
checking diff
-                .get_result::<(
-                    Uuid,
-                    Option<NaiveDateTime>,
-                    Option<NaiveDateTime>,
-                    Option<NaiveDateTime>,
-                )>(conn)
+                .get_result::<
+                    (
+                        Uuid,
+                        Option<NaiveDateTime>,
+                        Option<NaiveDateTime>,
+                        Option<NaiveDateTime>,
+                    ),
+                >(conn)

running rustfmt (feature) on denoland_deno
checking diff
Diff in /tmp/denoland_deno-aScILM81/cli/lsp/client.rs:248:
           )
           .await
       }
-      TestingNotification::DeleteModule(params) => self
-        .0
-        .send_notification::<testing_lsp_custom::TestModuleDeleteNotification>(
-          params,
-        )
-        .await,
+      TestingNotification::DeleteModule(params) => {
+        self
+          .0
+          .send_notification::<
+            testing_lsp_custom::TestModuleDeleteNotification,
+          >(params)
+          .await
+      }
       TestingNotification::Progress(params) => {
         self
           .0
Diff in /tmp/denoland_deno-aScILM81/cli/lsp/client.rs:271:
   ) {
     self
       .0
-      .send_notification::<lsp_custom::DidRefreshDenoConfigurationTreeNotification>(
-        params,
-      )
+      .send_notification::<
+        lsp_custom::DidRefreshDenoConfigurationTreeNotification,
+      >(params)
       .await
   }