Poor formatting of long method call chain with use_small_heuristics="Max"

[camelid]

The full input is quite long so I posted it at the bottom of this issue (it's extracted from rust-lang/rust#81546). The key part is:

Input

                            Arc::get_mut(&mut runtest).unwrap().get_mut().unwrap().take().unwrap()();

Output

                            Arc::get_mut(&mut runtest).unwrap().get_mut().unwrap().take().unwrap()(
                            );

The output is poorly formatted. The issue occurs with the use_small_heuristics = "Max" config option (because the code is from rust-lang/rust). The first line of the output is 99 characters, so there's no more space for the ) and a newline is added. In this case, I think rustfmt should forget about fitting it on one line and just fall back to the default behavior, which is:

                            Arc::get_mut(&mut runtest)
                                .unwrap()
                                .get_mut()
                                .unwrap()
                                .take()
                                .unwrap()();

Another approach could work as well, I'm just suggesting that one since it's the behavior without this config option set.

Full Input

pub fn run_test(
    opts: &TestOpts,
    force_ignore: bool,
    test: TestDescAndFn,
    strategy: RunStrategy,
    monitor_ch: Sender<CompletedTest>,
    concurrency: Concurrent,
) -> Option<thread::JoinHandle<()>> {
    let TestDescAndFn { desc, testfn } = test;

    // Emscripten can catch panics but other wasm targets cannot
    let ignore_because_no_process_support = desc.should_panic != ShouldPanic::No
        && cfg!(target_arch = "wasm32")
        && !cfg!(target_os = "emscripten");

    if force_ignore || desc.ignore || ignore_because_no_process_support {
        let message = CompletedTest::new(desc, TrIgnored, None, Vec::new());
        monitor_ch.send(message).unwrap();
        return None;
    }

    struct TestRunOpts {
        pub strategy: RunStrategy,
        pub nocapture: bool,
        pub concurrency: Concurrent,
        pub time: Option<time::TestTimeOptions>,
    }

    fn run_test_inner(
        desc: TestDesc,
        monitor_ch: Sender<CompletedTest>,
        testfn: Box<dyn FnOnce() + Send>,
        opts: TestRunOpts,
    ) -> Option<thread::JoinHandle<()>> {
        let concurrency = opts.concurrency;
        let name = desc.name.clone();

        let runtest = move || match opts.strategy {
            RunStrategy::InProcess => run_test_in_process(
                desc,
                opts.nocapture,
                opts.time.is_some(),
                testfn,
                monitor_ch,
                opts.time,
            ),
            RunStrategy::SpawnPrimary => spawn_test_subprocess(
                desc,
                opts.nocapture,
                opts.time.is_some(),
                monitor_ch,
                opts.time,
            ),
        };

        // If the platform is single-threaded we're just going to run
        // the test synchronously, regardless of the concurrency
        // level.
        let supports_threads = !cfg!(target_os = "emscripten") && !cfg!(target_arch = "wasm32");
        if concurrency == Concurrent::Yes && supports_threads {
            let cfg = thread::Builder::new().name(name.as_slice().to_owned());
            let mut runtest = Arc::new(Mutex::new(Some(runtest)));
            let runtest2 = runtest.clone();
            cfg.spawn(move || runtest2.lock().unwrap().take().unwrap()())
                .map_or_else(
                    |e| {
                        if e.kind() == io::ErrorKind::WouldBlock {
                            // `ErrorKind::WouldBlock` means hitting the thread limit on some
                            // platforms, so run the test synchronously here instead.
                            Arc::get_mut(&mut runtest).unwrap().get_mut().unwrap().take().unwrap()();
                            Ok(None)
                        } else {
                            Err(e)
                        }
                    },
                    |h| Ok(Some(h)),
                )
                .unwrap()
        } else {
            runtest();
            None
        }
    }

    let test_run_opts =
        TestRunOpts { strategy, nocapture: opts.nocapture, concurrency, time: opts.time_options };

    match testfn {
        DynBenchFn(bencher) => {
            // Benchmarks aren't expected to panic, so we run them all in-process.
            crate::bench::benchmark(desc, monitor_ch, opts.nocapture, |harness| {
                bencher.run(harness)
            });
            None
        }
        StaticBenchFn(benchfn) => {
            // Benchmarks aren't expected to panic, so we run them all in-process.
            crate::bench::benchmark(desc, monitor_ch, opts.nocapture, benchfn);
            None
        }
        DynTestFn(f) => {
            match strategy {
                RunStrategy::InProcess => (),
                _ => panic!("Cannot run dynamic test fn out-of-process"),
            };
            run_test_inner(
                desc,
                monitor_ch,
                Box::new(move || __rust_begin_short_backtrace(f)),
                test_run_opts,
            )
        }
        StaticTestFn(f) => run_test_inner(
            desc,
            monitor_ch,
            Box::new(move || __rust_begin_short_backtrace(f)),
            test_run_opts,
        ),
    }
}

Meta

• rustfmt version: rustfmt 1.4.32-nightly (216a6430 2021-01-16)
• From where did you install rustfmt?: rustup

[calebcartwright]

I agree with you in the abstract, but practically speaking I'm not sure there's much we can really do.

Even as minor/preferential as such a change may seem to some in this particular case, it would still be an explicit violation of our formatting stability guarantee which has always been a non-starter. Additionally, the wrapping of the closing paren is part of a last ditch effort to still be able to format while staying within the max_width constraints, and there's plenty of non-chain specific cases where this happens (such as heavy indentation, really long idents, etc.)

By overriding the small heuristics option to Max you're basically opting out of letting rustfmt do the chain wrapping here and asking it to stretch the one-line chain til the very end. While I suppose we could do something like a fall back reformatting the entire chain, I suspect it would be rather fiddly (chains are probably the most complicated to format).

The formatting of that particular chain wouldn't be my preference either, but the result is at least consistent and predictable, especially given the Max heuristics value. Maybe some minor refactoring could help reduce the indention levels or single line chain run?

[ghost]

Maybe some minor refactoring could help reduce the indention levels or single line chain run?

Indeed... rust-lang/rust@cec83b5 (rust-lang/rust#81546 (commits))

[ytmimi]

@calebcartwright can this be closed?

[camelid]

Seems like while the status quo isn't great, there's not really a better option, so I'll close this. Thanks for the detailed explanation!