match_arm_blocks behavior does not match documentation

[RalfJung]

The docs for match_arm_blocks say

Wrap the body of arms in blocks when it does not fit on the same line with the pattern of arms

However, even with match_arm_blocks = true, I am seeing formatting like this:

            "atomic_cxchg_acq_failrelaxed" => this.atomic_compare_exchange(
                args,
                dest,
                AtomicRwOp::Acquire,
                AtomicReadOp::Relaxed,
            )?,

Clearly this match arm does not fit on the same line as the pattern. Ergo, according to the docs, it should be wrapped in a block. But it is not. So either the docs are wrong or the implementation is buggy -- or am I missing something?

FWIW I'd actually prefer having this wrapped in a block, so my preferred solution here is to make the implementation match the docs. :)

[calebcartwright]

Thanks for the report. Could you share a reproducible snippet that contains the entire match expression with the same levels of indentation where you are seeing this?

Similar to the cascading formatting failures in chains, there's some cases where rustfmt bails on the entire match if it encounters any issues formatting any of the arms or bodies contained within, so it's possible rustfmt isn't formatting any part of the match at all. There could certainly be a bug with the config option as well.

[RalfJung]

Here's an example snippet:

use std::iter;

use log::trace;

use rustc_apfloat::{Float, Round};
use rustc_middle::ty::layout::IntegerExt;
use rustc_middle::{mir, mir::BinOp, ty, ty::FloatTy};
use rustc_target::abi::{Align, Integer, LayoutOf};

use crate::*;
use helpers::check_arg_count;

pub enum AtomicOp {
    MirOp(mir::BinOp, bool),
    Max,
    Min,
}

impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {}
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
    fn call_intrinsic(
        &mut self,
        instance: ty::Instance<'tcx>,
        args: &[OpTy<'tcx, Tag>],
        ret: Option<(&PlaceTy<'tcx, Tag>, mir::BasicBlock)>,
        _unwind: Option<mir::BasicBlock>,
    ) -> InterpResult<'tcx> {
        let this = self.eval_context_mut();

        if this.emulate_intrinsic(instance, args, ret)? {
            return Ok(());
        }

        // All supported intrinsics have a return place.
        let intrinsic_name = &*this.tcx.item_name(instance.def_id()).as_str();
        let (dest, ret) = match ret {
            None => throw_unsup_format!("unimplemented (diverging) intrinsic: {}", intrinsic_name),
            Some(p) => p,
        };

        // Then handle terminating intrinsics.
        match intrinsic_name {
            "atomic_cxchgweak" => this.atomic_compare_exchange_weak(
                args,
                dest,
                AtomicRwOp::SeqCst,
                AtomicReadOp::SeqCst,
            )?,
            "atomic_umax_acq" => this.atomic_op(args, dest, AtomicOp::Max, AtomicRwOp::Acquire)?,
            "atomic_umax_rel" => this.atomic_op(args, dest, AtomicOp::Max, AtomicRwOp::Release)?,
            "atomic_umax_acqrel" => {
                this.atomic_op(args, dest, AtomicOp::Max, AtomicRwOp::AcqRel)?
            }
            "atomic_umax_relaxed" => {
                this.atomic_op(args, dest, AtomicOp::Max, AtomicRwOp::Relaxed)?
            }

            name => throw_unsup_format!("unimplemented intrinsic: {}", name),
        }

        trace!("{:?}", this.dump_place(**dest));
        this.go_to_block(ret);
        Ok(())
    }
}

When I change the formatting, rustfmt resets it to this -- so it doesn't look to me like it is giving up entirely (but I might be misinterpreting what that means).

The rustfmt.toml reads

use_small_heuristics = "Max"
version = "Two"
match_arm_blocks = true

[calebcartwright]

Thanks that's helpful!

When I change the formatting, rustfmt resets it to this -- so it doesn't look to me like it is giving up entirely (but I might be misinterpreting what that means).

Nope you've got it right. There's a few cases, such as comments between arms, within patterns, preceding guards, etc. which makes rustfmt barf on the entire match expression, so it just spits back out whatever was in the corresponding span originally. Unfortunately that happens silently by default, and if the original formatting was close/identical to the way rustfmt would've formatted it successfully it's hard for developers to know that happened.

We've ruled that out though which is good.

My guess is that the check for:

Wrap the body of arms in blocks when it does not fit on the same line with the pattern of arms

is only looking at the length of the first line of the formatted body and not whether the body was multilined itself, but will have to dig into it

[RalfJung]

only-with-option

I am not sure if that's right; match_arm_blocks = true is the default and my snippet shows the same behavior on playground (which I assume means that it behaves like this even without any options).

[calebcartwright]

I am not sure if that's right; match_arm_blocks = true

Yep, you're right. Sorry for the confusion, did some drive by labeling without really digging into it. This actually runs into one of the additional caveats in the Style Guide prescriptions for match expressions and circles back to #4004

However, the point about the documentation being mismatched (or at least incomplete) is fair. It's really more of a true equals whatever the Style Guide said, whereas false permits the skipping of the block in one of the style guide caveats, and the docs should reflect that.

I suspect we will need a new config option to incorporate a Preserve variant as discussed in #3373 though which would include a soft deprecation of match_arm_blocks and automapping any match_arm_blocks usage to the new option. I'm optimistic we'll get to that fairly soon which would make the the match_arm_blocks documentation issue moot, but will keep this open for now and if the new changes to support Preserve drag on for too long then I'll revisit the wording for the match_arm_blocks documentation

[RalfJung]

and the docs should reflect that.

Having an option that truly implements what is currently documented might also be useful though -- i.e., ensuring that multi-line (not "multi-statement" or whatever it currently is) arms always have braces around them (but still keeping single-line arms brace-free). I'd prefer that over the current default any day, though that's probably just my personal taste (see #4004) ;)

Of course I can simulate that with Preserve and a bit of manual work.

[calebcartwright]

Having an option that truly implements what is currently documented might also be useful though

Indeed. Converting an option from a boolean to an enum opens up a lot of formatting opportunities, and I suspect there will be plenty here

[calebcartwright]

Once again optimism about a quick implementation has proven to be unfounded 😄

I've updated the docs for the option to be more explicit about the narrow slice of cases where the option is actually applicable and have opened #4896 to track the implementation of the new, more powerful opt.

Going to go ahead and close as there's not much else to do here, and we can always continue to iterate on the docs of for the current opt if need be.