Poor formatting of some winrt-rs generated code

[kennykerr]

Here is some code that was generated by https://github.com/microsoft/winrt-rs that I have copied into a standalone repro to illustrate how rustfmt fails to format some portions of the code.

https://github.com/kennykerr/rustfmt-repro

It may have something to do with the complexity of the generated code, but at some point the formatting just seems to give up. For example, main.rs lines 94-97 looks like this:

(this . vtable() . operation_completed)
(this, handler . into() . get_abi(), < super :: super
    :: foundation :: EventRegistrationToken as :: winrt
    :: AbiTransferable > :: set_abi(& mut result__)) .

Subsequent lines are formatted correctly, but it seems to give up on this expression and passes right on by. Here's another example from lines 587-589:

< super :: super :: foundation :: collections :: IMap :: <
:: winrt :: HString, :: winrt :: Object > as :: std ::
convert :: From < & Self >> :: from(self) . lookup(key,)

Some like 6194-6212 are really hard on the eyes:

pub struct abi_IDataProviderRequest where
{
    base__ : [usize ; 6], pub format_id : unsafe extern "system"
    fn(:: winrt :: NonNullRawComPtr < IDataProviderRequest >,
        result__ : * mut < :: winrt :: HString as :: winrt ::
        AbiTransferable > :: Abi,) -> :: winrt :: ErrorCode, pub
    deadline : unsafe extern "system"
    fn(:: winrt :: NonNullRawComPtr < IDataProviderRequest >,
        result__ : * mut < super :: super :: foundation :: DateTime
        as :: winrt :: AbiTransferable > :: Abi,) -> :: winrt ::
    ErrorCode, pub get_deferral : unsafe extern "system"
    fn(:: winrt :: NonNullRawComPtr < IDataProviderRequest >,
        result__ : * mut < DataProviderDeferral as :: winrt ::
        AbiTransferable > :: Abi,) -> :: winrt :: ErrorCode, pub
    set_data : unsafe extern "system"
    fn(:: winrt :: NonNullRawComPtr < IDataProviderRequest >,
        value : < :: winrt :: Object as :: winrt :: AbiTransferable
        > :: Abi,) -> :: winrt :: ErrorCode,
}

Meta

• rustfmt version: rustfmt 1.4.17-stable (8a93416 2020-07-20)
• From where did you install rustfmt?: rustup

[calebcartwright]

There's a couple things going on with the first snippet in your issue description, all related to these portions of the generated code exceeding max_width when formatted. Missing from the snippet in the OP (but viewable in the repro repo) is a trailing .and_then(|| result__) which makes makes this a chain with an exceptionally long root/parent. There's a known issue in rustfmt wherein if any single chain item exceeds the value of max_width, then rustfmt has to bail on formatting the entire chain and leaves the original formatting in place (#3863)

Shortened, full repro with relevant indentation.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6220215ef9ed981c658f02bb9a1afd66

pub mod windows {
pub mod application_model {
        pub mod data_transfer {
            #[repr(transparent)]
            #[derive(Default, Clone, PartialEq)]
            pub struct DataPackage {
                ptr: ::winrt::ComPtr<IDataPackage>,
            }
            impl DataPackage {

                pub fn operation_completed<
                    'a,
                    T0__: ::std::convert::Into<
                        ::winrt::Param<
                            'a,
                            super::super::foundation::TypedEventHandler<
                                DataPackage,
                                OperationCompletedEventArgs,
                            >,
                        >,
                    >,
                >(
                    &self,
                    handler: T0__,
                ) -> ::winrt::Result<super::super::foundation::EventRegistrationToken>
                {
                    let this = <Self as ::winrt::AbiTransferable>::get_abi(self)
                        .expect("The `this` pointer was null when calling method");
                    unsafe {
                        let mut result__: super::super::foundation::EventRegistrationToken =
                            ::std::mem::zeroed();
                        (this . vtable() . operation_completed)
                        (this, handler . into() . get_abi(), < super :: super
                         :: foundation :: EventRegistrationToken as :: winrt
                         :: AbiTransferable > :: set_abi(& mut result__)) .
                        and_then(|| result__)
                    }
                }
                }
                }
                }
}

Secondly, the combination of the indentation levels and the very long path expression within the tuple also causes a max_width related formatting failure, that causes rustfmt to bail. Minimal repro (blocks and indentation can be excluded with a lower max_width value):

fn foo() {
    {
        {
            (< super :: super
                :: foundation :: EventRegistrationToken as :: winrt
                :: AbiTransferable > :: set_abi(& mut result))
        }
    }
}

[calebcartwright]

Also slightly related FWIW @kennykerr, I briefly chatted about this with @rylev in a different thread within this repo, but want to reiterate that I'd advise against the current rustfmt usage within in the winrt-rs build macro that users utilize.

In my experience generated code is typically configured to have rustfmt ignore the generated code when users run rustfmt (by adding #![rustfmt::skip] to the top of generated files) vs. trying to continue to format the generated code.

I say that because there will inevitably be users that don't have rustfmt on their system, either because they deliberately don't use/install the rustfmt component or because they weren't aware that rustfmt is not included in the default rustup profile. There's also several scenarios where rustfmt will exit with a non-zero exit code (invalid config file for example), even when just formatting the generated code from the winrt-rs build script.

I believe that the resulting panic and stacktrace users will see in these scenarios when running a cargo build will likely be a source of confusion, especially for users that are new to the Rust ecosystem.

[kennykerr]

Thanks @calebcartwright - that's very helpful to understand.

I agree that it shouldn't fail if rustfmt isn't present. Our needs for formatting are at best lossy. The code shouldn't be committed/checked-in and only exists (1) to provide a reasonable debugging experience and (2) to enable rust-analyzer to work.

So if these are known issues that's great. We may eventually replace rustfmt anyway, as we have serious challenges with the performance of the Rust toolchain when it comes to the generated code and need to save time wherever possible. We may end up updating our code generator to generate pre-formatted code instead of a TokenStream as the latter has a very poor default string representation and running rustfmt is quite time consuming.

Still, really appreciate the time and effort you've put into it. As a long-time C++ developer, it's so refreshing to have such a tool.

[ytmimi]

@calebcartwright can this be closed as a duplicate of #3863?

[rylev]

Yes, I think so. The context in this PR is itself a bit outdated, and I think fixing #3863 will mostly fix the issues we're seeing in windows-rs (modulo some performance concerns).