Rustfmt unable to format complex match statement with lots of fully qualified matches

[Manishearth]

Rustfmt is unable to format the following code:

fn foo() {
    match key {
        < :: icu_calendar :: provider :: JapaneseErasV1Marker as ResourceMarker > :: KEY => {
        AnyPayload :: from_static_ref :: << :: icu_calendar :: provider :: JapaneseErasV1Marker as DataMarker > :: Yokeable > (litemap_slice_get (calendar :: japanese_v1 :: DATA , key , req) ?)
        }
    }
}

We're hitting that in unicode-org/icu4x#2098, where we've generated a very large match statement containing a lot of those such arms (but we hit those bugs even for the one-arm match statement above). It woudl be nice if we could have our generated code be readable.

[calebcartwright]

I can't reproduce locally nor with this snippet on the playground.

Would you mind posting any config options you're setting, either via command line or any config files in the hierarchy?

[Manishearth]

I was using the rustfmt CLI on the latest nightly.

Which made me recall that the CLI defaults to edition=2015. We should be running edition=2021 when we're manually invoking it!

Thanks!

[Manishearth]

I'm surprised rustfmt just silently "succeeds" on edition 2015 though, I'd imagine it would report an error?

[Manishearth]

@calebcartwright hmm, even with edition=2021 it still fails on the full testcase: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=17eff8a21464472a1fcfd5f643b8fd44

[robertbastian]

It seems to handle three branches, but no more.

[ytmimi]

I haven't dug too deeply into this, but I'm guessing rustfmt is failing to format the code because it can't satisfy the max_width constraint. Bumping the width up to 152 works well for the snippet you provided on the playground.

You can confirm this by running rustfmt --check --config=max_width=152 issue_5422.rs, where issue_5422.rs contains the content from the playground link.

[ytmimi]

Because of the way we break fully qualified paths there are a a few instances where the match arms aren't that readable, but overall it's an improvement. This might be solved if there was a way to always force match arms to be wrapped in blocks, which is currently being worked on in #4924

+                <::icu_list::provider::AndListV1Marker as ResourceMarker>::KEY => AnyPayload::from_static_ref::<
+                    <::icu_list::provider::AndListV1Marker as DataMarker>::Yokeable
+                >(litemap_slice_get(list::and_v1::DATA, key, req)?),
+                <::icu_list::provider::OrListV1Marker as ResourceMarker>::KEY => AnyPayload::from_static_ref::<
+                    <::icu_list::provider::OrListV1Marker as DataMarker>::Yokeable
+                >(litemap_slice_get(list::or_v1::DATA, key, req)?),

[Manishearth]

Yeah, feels like if rustfmt cannot satisfy max_width it should still attempt to break it up as much as possible

[calebcartwright]

This thread has developed a few additional tentacles since I last checked

I'm surprised rustfmt just silently "succeeds" on edition 2015 though, I'd imagine it would report an error?

rustfmt doesn't use the edition, it just chucks it to rustc_parse. If there's an edition related issue, it would be parsing related and emitted accordingly. I think the edition is a bit of a red herring, particularly given the subsequent messages.

@calebcartwright hmm, even with edition=2021 it still fails on the full testcase:
It seems to handle three branches, but no more.
because it can't satisfy the max_width constraint.

The full test case is illuminating, thank you. @ytmimi is correct, it's not related to the number of arms but because there's too much horizontal content. This is a fairly common occurrence with generated code due to the author being a machine instead of a human. The only practical, readily available solutions are to crank up the max_width value or to use something like David's prettyplease

[calebcartwright]

Yeah, feels like if rustfmt cannot satisfy max_width it should still attempt to break it up as much as possible

I understand this desire but given some of the earliest structural decisions around rustfmt, it's a bit like saying rustc should be able to compile successfully even when there are syntax errors or various files/mods/deps/etc. missing.

Every time you run rustfmt it is explicitly asked to ensure that no code lines run past max_width (whether you use the default max_width value or override it), and in cases like this it's unequivocally impossible for rustfmt to meet that constraint and so defers to the "programmer" to refactor. I assume the motivation for this was largely because a width limit that's ignored when exceeded would have been self-defeating, but that long predates my time with the project.

I appreciate that there are cases when users would prefer to treat the width limits as more of a soft limit or to "proceed anyway", but that starts to get fuzzy quickly and is highly underspecified, as illustrated with cases like #3863.

I'm going to close because this is another case of exceptionally long, generated code and there's no action to be taken, and because broader discussions about width behavior are probably best continued in existing issues.

[Manishearth]

Sounds good, thanks. We can probably run this with a larger max_width (cc @robertbastian ), I was just surprised that it silently gives up

[calebcartwright]

I should've also mentioned but the silent part bugs me too. At the moment both error_on_unformatted and error_on_line_overflow are off by default, but can be turned on to highlight these cases. At some point I'd like to enhance these so that instead of a binary silent/hard-error, we can have a silent/warn-but-don't-fail/hard-error set of variants, as I think that middle warn option would be the best default

[Manishearth]

Makes sense, thanks!

[dtolnay]

This is a fairly common occurrence with generated code due to the author being a machine instead of a human. The only practical, readily available solutions are to crank up the max_width value or to use something like David's prettyplease

Sometimes the best combo is prettyplease followed by rustfmt. In the parts that rustfmt is not able to handle (max_width or any other reason) it will usually just keep prettyplease's formatting, while still formatting the rest of the file in rustfmt's own style.

Example of rustfmt by itself:

impl AnyProvider for BakedDataProvider {
    fn load_any(&self, key: ResourceKey, req: &DataRequest) -> Result<AnyResponse, DataError> {
        Ok (AnyResponse { payload : Some (match key { < :: icu_datetime :: provider :: calendar :: DateSkeletonPatternsV1Marker as ResourceMarker > :: KEY => AnyPayload :: from_rc_payload :: < :: icu_datetime :: provider :: calendar :: DateSkeletonPatternsV1Marker > (alloc :: rc :: Rc :: new (DataPayload :: from_owned (zerofrom :: ZeroFrom :: zero_from (litemap_slice_get (datetime :: skeletons_v1 :: DATA , key , req) ?)))) , }) , metadata : Default :: default () , })
    }
}

Prettyplease by itself:

impl AnyProvider for BakedDataProvider {
    fn load_any(
        &self,
        key: ResourceKey,
        req: &DataRequest,
    ) -> Result<AnyResponse, DataError> {
        Ok(AnyResponse {
            payload: Some(
                match key {
                    <::icu_datetime::provider::calendar::DateSkeletonPatternsV1Marker as ResourceMarker>::KEY => {
                        AnyPayload::from_rc_payload::<
                            ::icu_datetime::provider::calendar::DateSkeletonPatternsV1Marker,
                        >(
                            alloc::rc::Rc::new(
                                DataPayload::from_owned(
                                    zerofrom::ZeroFrom::zero_from(
                                        litemap_slice_get(datetime::skeletons_v1::DATA, key, req)?,
                                    ),
                                ),
                            ),
                        )
                    }
                },
            ),
            metadata: Default::default(),
        })
    }
}

Prettyplease followed by rustfmt:

impl AnyProvider for BakedDataProvider {
    fn load_any(&self, key: ResourceKey, req: &DataRequest) -> Result<AnyResponse, DataError> {
        Ok(AnyResponse {
            payload: Some(
                match key {
                    <::icu_datetime::provider::calendar::DateSkeletonPatternsV1Marker as ResourceMarker>::KEY => {
                        AnyPayload::from_rc_payload::<
                            ::icu_datetime::provider::calendar::DateSkeletonPatternsV1Marker,
                        >(
                            alloc::rc::Rc::new(
                                DataPayload::from_owned(
                                    zerofrom::ZeroFrom::zero_from(
                                        litemap_slice_get(datetime::skeletons_v1::DATA, key, req)?,
                                    ),
                                ),
                            ),
                        )
                    }
                },
            ),
            metadata: Default::default(),
        })
    }
}

[dtolnay]

Example of that in practice: dtolnay-contrib/icu4x@8db0a2a