Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should not visual-align same-line comments within function arguments #4108

Open
joshtriplett opened this issue Apr 6, 2020 · 2 comments
Open

Comments

@joshtriplett
Copy link
Member

joshtriplett commented Apr 6, 2020

Sample code:

fn main() {
    w(
        thing,
        0, // thing2
        thing3,
        long_thing4, // acomment
        long_thing4, // bcomment
        long_thing4, // ccomment
        0, // another
        0, // comment
        thing5,
        long_thing6,
        thing7,
        0, // yet another comment
    );
}

rustfmt turns this into:

fn main() {
    w(
        thing,
        0, // thing2
        thing3,
        long_thing4, // acomment
        long_thing4, // bcomment
        long_thing4, // ccomment
        0,           // another
        0,           // comment
        thing5,
        long_thing6,
        thing7,
        0, // yet another comment
    );
}

Notice that // another and // comment got indented to visually align with the previous three comments.

This has multiple issues:

  1. rustfmt should not, in general, be visually aligning anything; its defaults normally block-align everything, per Alignment versus non-aligning indentation style-team#8 .
  2. In this specific case, rustfmt has made the code less readable by doing so, moving comments away from the code they comment (0,) to align it with an unrelated comment, while leaving other comments in the same function call untouched. The alignment misleadingly suggests some relationship between the comments or the arguments, which doesn't exist.

I would expect rustfmt to always leave same-line comments one space away from the code they comment.

@RalfJung
Copy link
Member

Here's another example of rustfmt weirdly realigning comments:

trait EvalContextExtPrivate<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
    fn macos_stat_write_buf(
        &mut self,
        metadata: FileMetadata,
        buf_op: &OpTy<'tcx, Tag>,
    ) -> InterpResult<'tcx, i32> {
        let imms = [
            immty_from_uint_checked(0u128, dev_t_layout)?, // st_dev
            immty_from_uint_checked(mode, mode_t_layout)?, // st_mode
            immty_from_uint_checked(0u128, nlink_t_layout)?, // st_nlink
            immty_from_uint_checked(0u128, ino_t_layout)?, // st_ino
            immty_from_uint_checked(0u128, uid_t_layout)?, // st_uid
            immty_from_uint_checked(0u128, gid_t_layout)?, // st_gid
            immty_from_uint_checked(0u128, dev_t_layout)?, // st_rdev
            immty_from_uint_checked(0u128, uint32_t_layout)?, // padding
            immty_from_uint_checked(access_sec, time_t_layout)?, // st_atime
            immty_from_uint_checked(access_nsec, long_layout)?, // st_atime_nsec
            immty_from_uint_checked(modified_sec, time_t_layout)?, // st_mtime
            immty_from_uint_checked(modified_nsec, long_layout)?, // st_mtime_nsec
            immty_from_uint_checked(0u128, time_t_layout)?, // st_ctime
            immty_from_uint_checked(0u128, long_layout)?, // st_ctime_nsec
            immty_from_uint_checked(created_sec, time_t_layout)?, // st_birthtime
            immty_from_uint_checked(created_nsec, long_layout)?, // st_birthtime_nsec
            immty_from_uint_checked(metadata.size, off_t_layout)?, // st_size
            immty_from_uint_checked(0u128, blkcnt_t_layout)?, // st_blocks
            immty_from_uint_checked(0u128, blksize_t_layout)?, // st_blksize
            immty_from_uint_checked(0u128, uint32_t_layout)?, // st_flags
            immty_from_uint_checked(0u128, uint32_t_layout)?, // st_gen
        ];

        Ok(0)
    }
}

When formatting this, rustfmt adds a space before the comment in the st_ctime_nsec line. That doesn't even align it with anything, no idea what it is doing.

This slight variant:

trait EvalContextExtPrivate<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
    fn macos_stat_write_buf(
        &mut self,
        metadata: FileMetadata,
        buf_op: &OpTy<'tcx, Tag>,
    ) -> InterpResult<'tcx, i32> {
        let imms = [
            immty_from_uint_checked(access_nsec, long_layout)?, // st_atime_nsec
            immty_from_uint_checked(modified_sec, time_t_layout)?, // st_mtime
            immty_from_uint_checked(modified_nsec, long_layout)?, // st_mtime_nsec
            immty_from_uint_checked(0u128, time_t_layout)?, // st_ctime
            immty_from_uint_checked(0u128, long_layout)?, // st_ctime_nsec
            immty_from_uint_checked(created_sec, time_t_layout)?, // st_birthtime
            immty_from_uint_checked(created_nsec, long_layout)?, // st_birthtime_nsec
            immty_from_uint_checked(metadata.size, off_t_layout)?, // st_size
            immty_from_uint_checked(0u128, blkcnt_t_layout)?, // st_blocks
        ];

        Ok(0)
    }
}

changes as follows:

trait EvalContextExtPrivate<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
    fn macos_stat_write_buf(
        &mut self,
        metadata: FileMetadata,
        buf_op: &OpTy<'tcx, Tag>,
    ) -> InterpResult<'tcx, i32> {
        let imms = [
            immty_from_uint_checked(access_nsec, long_layout)?, // st_atime_nsec
            immty_from_uint_checked(modified_sec, time_t_layout)?, // st_mtime
            immty_from_uint_checked(modified_nsec, long_layout)?, // st_mtime_nsec
            immty_from_uint_checked(0u128, time_t_layout)?,     // st_ctime
            immty_from_uint_checked(0u128, long_layout)?,       // st_ctime_nsec
            immty_from_uint_checked(created_sec, time_t_layout)?, // st_birthtime
            immty_from_uint_checked(created_nsec, long_layout)?, // st_birthtime_nsec
            immty_from_uint_checked(metadata.size, off_t_layout)?, // st_size
            immty_from_uint_checked(0u128, blkcnt_t_layout)?,   // st_blocks
        ];

        Ok(0)
    }
}

@xedrac
Copy link

xedrac commented Feb 11, 2022

Would fixing this mean that trailing comments would remain unchanged? Several people on my team dislike using rustfmt because it visually changes their carefully aligned comments in most cases. For example:

match foo {
    A => "  ",            // Comment
    B => "         ",     // Comment
}

becomes:

match foo {
    A => "  ", // Comment
    B => "         ", // Comment
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants