Trailing commas handling in macro calls

[vincentisambart]

fn main() {
    foo("abcdefghijklmnopqrstuvwxyz".to_string(), "abcdefghijklmnopqrstuvwxyz".to_string());
}

gets, as expected, formatted to:

fn main() {
    foo(
        "abcdefghijklmnopqrstuvwxyz".to_string(),
        "abcdefghijklmnopqrstuvwxyz".to_string(),
    );
}

However,

fn main() {
    assert_eq!("abcdefghijklmnopqrstuvwxyz".to_string(), "abcdefghijklmnopqrstuvwxyz".to_string());
}

gets formatted as:

fn main() {
    assert_eq!(
        "abcdefghijklmnopqrstuvwxyz".to_string(),
        "abcdefghijklmnopqrstuvwxyz".to_string()
    );
}

No trailing comma after the second parameter.

And even worse, function calls in one of the assert_eq! parameters get the same treatment:

fn main() {
    assert_eq!(foo("abcdefghijklmnopqrstuvwxyz".to_string(), "abcdefghijklmnopqrstuvwxyz".to_string()), "abcdefghijklmnopqrstuvwxyz".to_string());
}

gets formatted as:

fn main() {
    assert_eq!(
        foo(
            "abcdefghijklmnopqrstuvwxyz".to_string(),
            "abcdefghijklmnopqrstuvwxyz".to_string()
        ),
        "abcdefghijklmnopqrstuvwxyz".to_string()
    );
}

No trailing comma after the second parameter of foo.

And if you have a trailing comma and a parameter gets shorter, the trailing comma stays.

fn main() {
    assert_eq!(
        foo(
            "abcdef".to_string(),
            "abcdef".to_string(),
        ),
        "abcdefghijklmnopqrstuvwxyz".to_string()
    );
}

gets formatted as:

fn main() {
    assert_eq!(
        foo("abcdef".to_string(), "abcdef".to_string(),),
        "abcdefghijklmnopqrstuvwxyz".to_string()
    );
}

I understand that trailings commas can change the meaning of macros, but it seems assert_eq! is already special cased.

Even more than the trailing comma after assert_eq!'s second parameter, the handling of trailing commas in function calls in one of assert_eq!'s parameter seems to me pretty inconvenient.

Thanks for the hard work on such a useful tool.

[otavio]

I investigated this issue and it is indeed doing the "right thing" as designed. Looking at https://github.com/rust-lang-nursery/rustfmt/blob/master/src/macros.rs#L298-L318 we see:

        DelimToken::Paren => {
            // Format macro invocation as function call, preserve the trailing
            // comma because not all macros support them.
            overflow::rewrite_with_parens(
                context,
                &macro_name,
                arg_vec.iter(),
                shape,
                mac.span,
                context.config.width_heuristics().fn_call_width,
                if trailing_comma {
                    Some(SeparatorTactic::Always)
                } else {
                    Some(SeparatorTactic::Never)
                },
            )

so it is avoiding adding the comma at end. The default is SeparatorTactic::Vertical and the code sets it to SeparatorTactic::Never.

This is required because some macros do not support the trailing comma and fail to parse, causing parsing errors. The only way to address this would be adding a "whitelist" for macros which would be good to keep the default separator tectic.

What people thing?

[nrc]

the handling of trailing commas in function calls in one of assert_eq!'s parameter seems to me pretty inconvenient

Note that changing commas in nested expressions can still change the semantics of the macro expansion, so changing this only is not an option

What people think?

I think the proposal is that where we already special case macros, we should further special case them if we know that trailing commas do not affect semantics. This seems like a reasonable thing to do, but I am not in a rush to do it - formatting of macros is far from perfect and I think one of the big things for 2.0 will be doing better in that area in a more generic, elegant manner. As such I'd rather not add too many special cases now.

[otavio]

Good, I tend to agree with this vision as well. That said, I think creating a 2.0 milestone would be a good way to mark those issues also to allow new contributors to know where to focus their effort.