Enabling struct_field_align_threshold=20 broke struct

[jnqnfe]

With the following config, having just enabled the struct_field_align_threshold = 20 line, and running cargo +nightly fmt, rustfmt removed the terminating commas from some structs like the following, breaking them.

brace_style = "SameLineWhere"
comment_width = 100
edition = "2018"
fn_args_layout = "Compressed"
hard_tabs = false
match_block_trailing_comma = true
max_width = 100
merge_derives = false
newline_style = "Unix"
normalize_doc_attributes = true
overflow_delimited_expr = true
reorder_imports = false
reorder_modules = true
struct_field_align_threshold = 20
tab_spaces = 4
trailing_comma = "Never"
use_small_heuristics = "Max"
use_try_shorthand = true
wrap_comments = true

/// Playback and record buffer metrics.
#[repr(C)]
#[derive(Debug, Default, Copy, Clone, PartialEq, Eq)]
pub struct BufferAttr {
    /* NOTE: This struct must be directly usable by the C API, thus same attributes/layout/etc */
    /// Maximum length of the buffer in bytes.
    ///
    /// Setting this to [`std::u32::MAX`] will initialize this to the maximum value supported by
    /// the server, which is recommended. In strict low-latency playback scenarios you might
    /// want to set this to a lower value, likely together with the
    /// [`stream::FlagSet::ADJUST_LATENCY`] flag. If you do so, you ensure that the latency
    /// doesn’t grow beyond what is acceptable for the use case, at the cost of getting more
    /// underruns if the latency is lower than what the server can reliably handle.
    ///
    /// [`stream::FlagSet::ADJUST_LATENCY`]: crate::stream::FlagSet::ADJUST_LATENCY
    pub maxlength: u32

    /// Target length of the buffer (playback only). The server tries to assure that at least
    /// `tlength` bytes are always available in the per-stream server-side playback buffer. The
    /// server will only send requests for more data as long as the buffer has less than this
    /// number of bytes of data.
    ///
    /// It is recommended to set this to [`std::u32::MAX`], which will initialize this to a value
    /// that is deemed sensible by the server. However, this value will default to something like
    /// 2s; for applications that have specific latency requirements this value should be set to
    /// the maximum latency that the application can deal with.
    ///
    /// When [`stream::FlagSet::ADJUST_LATENCY`] is not set this value will influence only the
    /// per-stream playback buffer size. When [`stream::FlagSet::ADJUST_LATENCY`] is set, the
    /// overall latency of the sink plus the playback buffer size is configured to this value. Set
    /// [`stream::FlagSet::ADJUST_LATENCY`] if you are interested in adjusting the overall latency.
    /// Don’t set it if you are interested in configuring the server-side per-stream playback
    /// buffer size.
    ///
    /// [`stream::FlagSet::ADJUST_LATENCY`]: crate::stream::FlagSet::ADJUST_LATENCY
    pub tlength: u32

    /// Pre-buffering (playback only). The server does not start with playback before at least
    /// `prebuf` bytes are available in the buffer. It is recommended to set this to
    /// [`std::u32::MAX`], which will initialize this to the same value as `tlength`, whatever that
    /// may be.
    ///
    /// Initialize to `0` to enable manual start/stop control of the stream. This means that
    /// playback will not stop on underrun and playback will not start automatically, instead
    /// [`Stream::cork()`] needs to be called explicitly. If you set this value to `0` you should
    /// also set [`stream::FlagSet::START_CORKED`]. Should underrun occur, the read index of the
    /// output buffer overtakes the write index, and hence the fill level of the buffer is
    /// negative.
    ///
    /// Start of playback can be forced using [`Stream::trigger()`] even though the prebuffer size
    /// hasn’t been reached. If a buffer underrun occurs, this prebuffering will be again enabled.
    ///
    /// [`Stream::cork()`]: crate::stream::Stream::cork
    /// [`Stream::trigger()`]: crate::stream::Stream::trigger
    /// [`stream::FlagSet::START_CORKED`]: crate::stream::FlagSet::START_CORKED
    pub prebuf: u32

    /// Minimum request (playback only). The server does not request less than `minreq` bytes from
    /// the client, instead it waits until the buffer is free enough to request more bytes at once.
    ///
    /// It is recommended to set this to [`std::u32::MAX`], which will initialize this to a value
    /// that is deemed sensible by the server. This should be set to a value that gives PulseAudio
    /// enough time to move the data from the per-stream playback buffer into the hardware playback
    /// buffer.
    pub minreq: u32

    /// Fragment size (recording only). The server sends data in blocks of `fragsize` bytes size.
    ///
    /// Large values diminish interactivity with other operations on the connection context but
    /// decrease control overhead. It is recommended to set this to `std::u32::MAX`, which will
    /// initialize this to a value that is deemed sensible by the server. However, this value will
    /// default to something like 2s; For applications that have specific latency requirements this
    /// value should be set to the maximum latency that the application can deal with.
    ///
    /// If [`stream::FlagSet::ADJUST_LATENCY`] is set the overall source latency will be adjusted
    /// according to this value. If it is not set the source latency is left unmodified.
    ///
    /// [`stream::FlagSet::ADJUST_LATENCY`]: crate::stream::FlagSet::ADJUST_LATENCY
    pub fragsize: u32
}

Trying to compile or re-run rustfmt afterwards obviously causes compile errors.

[calebcartwright]

Could you please provide your original snippet that can be used for reproduction, as well as the version of rustfmt you are using?

We have issue templates to help structure the input but looks like we accidentally dropped those in some branch shuffling

[jnqnfe]

Could you please provide your original snippet that can be used for reproduction, as well as the version of rustfmt you are using?

We have issue templates to help structure the input but looks like we accidentally dropped those in some branch shuffling

In terms of version, just before posting these issues I'd switched from my distro's rustc to using rustup, so I had the latest stable and latest nightly. I was using the +nightly switch here, so the applicable version would have been the nightly available that day.

The project I was playing with applying rustfmt to is https://github.com/jnqnfe/pulse-binding-rust

The specific snippet in it's original form is:

/// Playback and record buffer metrics.
#[repr(C)]
#[derive(Debug, Default, Copy, Clone, PartialEq, Eq)]
pub struct BufferAttr {
    /* NOTE: This struct must be directly usable by the C API, thus same attributes/layout/etc */

    /// Maximum length of the buffer in bytes.
    ///
    /// Setting this to [`std::u32::MAX`] will initialize this to the maximum value supported by the
    /// server, which is recommended. In strict low-latency playback scenarios you might want to set
    /// this to a lower value, likely together with the [`stream::FlagSet::ADJUST_LATENCY`] flag. If
    /// you do so, you ensure that the latency doesn’t grow beyond what is acceptable for the use
    /// case, at the cost of getting more underruns if the latency is lower than what the server can
    /// reliably handle.
    ///
    /// [`stream::FlagSet::ADJUST_LATENCY`]: crate::stream::FlagSet::ADJUST_LATENCY
    pub maxlength: u32,

    /// Target length of the buffer (playback only). The server tries to assure that at least
    /// `tlength` bytes are always available in the per-stream server-side playback buffer. The
    /// server will only send requests for more data as long as the buffer has less than this number
    /// of bytes of data.
    ///
    /// It is recommended to set this to [`std::u32::MAX`], which will initialize this to a value
    /// that is deemed sensible by the server. However, this value will default to something like
    /// 2s; for applications that have specific latency requirements this value should be set to
    /// the maximum latency that the application can deal with.
    ///
    /// When [`stream::FlagSet::ADJUST_LATENCY`] is not set this value will influence only the
    /// per-stream playback buffer size. When [`stream::FlagSet::ADJUST_LATENCY`] is set, the
    /// overall latency of the sink plus the playback buffer size is configured to this value. Set
    /// [`stream::FlagSet::ADJUST_LATENCY`] if you are interested in adjusting the overall latency.
    /// Don’t set it if you are interested in configuring the server-side per-stream playback buffer
    /// size.
    ///
    /// [`stream::FlagSet::ADJUST_LATENCY`]: crate::stream::FlagSet::ADJUST_LATENCY
    pub tlength: u32,

    /// Pre-buffering (playback only). The server does not start with playback before at least
    /// `prebuf` bytes are available in the buffer. It is recommended to set this to
    /// [`std::u32::MAX`], which will initialize this to the same value as `tlength`, whatever that
    /// may be.
    ///
    /// Initialize to `0` to enable manual start/stop control of the stream. This means that
    /// playback will not stop on underrun and playback will not start automatically, instead
    /// [`Stream::cork()`] needs to be called explicitly. If you set this value to `0` you should
    /// also set [`stream::FlagSet::START_CORKED`]. Should underrun occur, the read index of the
    /// output buffer overtakes the write index, and hence the fill level of the buffer is negative.
    ///
    /// Start of playback can be forced using [`Stream::trigger()`] even though the prebuffer size
    /// hasn’t been reached. If a buffer underrun occurs, this prebuffering will be again enabled.
    ///
    /// [`Stream::cork()`]: crate::stream::Stream::cork
    /// [`Stream::trigger()`]: crate::stream::Stream::trigger
    /// [`stream::FlagSet::START_CORKED`]: crate::stream::FlagSet::START_CORKED
    pub prebuf: u32,

    /// Minimum request (playback only). The server does not request less than `minreq` bytes from
    /// the client, instead it waits until the buffer is free enough to request more bytes at once.
    ///
    /// It is recommended to set this to [`std::u32::MAX`], which will initialize this to a value
    /// that is deemed sensible by the server. This should be set to a value that gives PulseAudio
    /// enough time to move the data from the per-stream playback buffer into the hardware playback
    /// buffer.
    pub minreq: u32,

    /// Fragment size (recording only). The server sends data in blocks of `fragsize` bytes size.
    ///
    /// Large values diminish interactivity with other operations on the connection context but
    /// decrease control overhead. It is recommended to set this to `std::u32::MAX`, which will
    /// initialize this to a value that is deemed sensible by the server. However, this value will
    /// default to something like 2s; For applications that have specific latency requirements this
    /// value should be set to the maximum latency that the application can deal with.
    ///
    /// If [`stream::FlagSet::ADJUST_LATENCY`] is set the overall source latency will be adjusted
    /// according to this value. If it is not set the source latency is left unmodified.
    ///
    /// [`stream::FlagSet::ADJUST_LATENCY`]: crate::stream::FlagSet::ADJUST_LATENCY
    pub fragsize: u32,
}

which can be found in https://github.com/jnqnfe/pulse-binding-rust/blob/master/pulse-binding/src/def.rs

[calebcartwright]

In terms of version, just before posting these issues I'd switched from my distro's rustc to using rustup, so I had the latest stable and latest nightly. I was using the +nightly switch here, so the applicable version would have been the nightly available that day.

Please run rustfmt --version and share the output. It's a lot easier to have the information, including the commit hash, readily available inline so that folks don't have to take several extra steps just to figure out the version associated with the reported issue.

[jnqnfe]

Please run rustfmt --version and share the output. It's a lot easier to have the information, including the commit hash, readily available inline so that folks don't have to take several extra steps just to figure out the version associated with the reported issue.

I understand and I appreciate that it would have been most helpful to have specified which nightly version I was actually using in the original reports. However I've updated several times since then so giving you the current version string is problematic unless I go through every report to confirm that the problems all still occur with with this version. Sigh.

So, starting with this report, the version string is rustfmt 1.4.37-stable (a178d03 2021-07-26) and yes it still occurs against my master branch of the linked project (if you add the quoted config).

[calebcartwright]

However I've updated several times since then so giving you the current version string is problematic unless I go through every report to confirm that the problems all still occur with with this version. Sigh.

No worries! You mentioned running cargo +nightly fmt in the issue description, and IIRC struct_field_align_threshold is still an unstable/nightly only option so I gather your system and/or directory is defaulted to a stable toolchain and you're overriding on every invocation?

If so could you actually share rustfmt +nightly --version or cargo +nightly fmt --version?

The stable version you shared is precisely the type of thing I was looking for, but just want to make sure we've got the same toolchain you're actually using in the project. For future reference, there's no need to vet against every version or anything like that, we just need a version where it can be reproduced to have that marker on the issue.

[cassaundra]

This seems to be related to #4791, and may be solved by #5159.

[ytmimi]

@calebcartwright can this be closed? I believe this was fixed by #5159.

[calebcartwright]

@calebcartwright can this be closed? I believe this was fixed by #5159.

Indeed. Do you have access to close?

[ytmimi]

Yeah, I do. I just wanted to double check that it's okay that I go ahead and close it.

[ytmimi]

closing as this has been resolved by #5159.