Handle ".." when rewriting struct with alignment

[ytmimi]

Summary

Fixes #4926

When rewriting a struct with alignment any ast::StructRest nodes are ignored. This behavior is inconsistent with how non aligned struct literals are rewritten. To match the behavior of non aligned struct literals when we have an ast::StructRest::Rest node we append ", .." to the rewritten struct fields.

[ytmimi]

Although the code passes for the test case in /test/source/issue_4926.rs, I still feel like I'm overlooking something. I'd definitely appreciate some guidance on whether my solution makes sense or if some additional checks need to be made before we can append ", .." to the formatted string.

[jrose-signal]

If the struct fields really do take multiple lines, ", .." probably isn't good enough! (I'd expect something like ",\n{indent}..".)

[ytmimi]

@jrose-signal you're right. I'd say it's a step in the right direction. I'll come up with some additional test cases, but also let me know if you can come up with any!

[jrose-signal]

Here's a file of ours that breaks when formatted: https://github.com/signalapp/ringrtc/blob/dca31fc984f7bd9809418b6e03716eb90f0537ae/src/rust/src/core/group_call.rs

with this configuration: https://github.com/signalapp/ringrtc/blob/dca31fc984f7bd9809418b6e03716eb90f0537ae/src/rust/rustfmt.toml

[jrose-signal]

It does look like the multi-line case currently works okay, though! So it's just important that this won't break it.

[ytmimi]

@jrose-signal thanks for the test file. It helped me find a bug in my implementation! See d12a324 if you're interested. After the additional changes I think your file can be properly formatted. Let me know if something's still off.

In general I feel like these changes address the original issue pretty well and rustfmt shouldn't change the meaning of the code by removing the "..".

I can only think of one edge case right now, where adding the ".." to the end of a line could go over the max_width configuration. With the current approach I took I'm not sure if there's a simple fix for that. That edge case is probably out of scope for this issue.

[calebcartwright]

@ytmimi - thanks for diving in on this one, and @jrose-signal thanks for reviewing and providing feedback!

Wanted to acknowledge seeing this and also let you know that I'm in the process of trying to pull the next release together so won't get around to reviewing this until afterwards.

[ytmimi]

@calebcartwright I had a lot of fun trying to figure this one out. Also, I totally understand that your priority is the release. Let me know if there's anything that needs to be added once you've got time to look this over.

[calebcartwright]

I probably should've mentioned this in the associated issue, but the real problem is that we shouldn't even be attempting to rewrite with alignment support in the first place. rustfmt should only support rewriting a struct in an expression position with alignment (non-default behavior) when there is no rest (with or without a base) and all of the fields are non-shorthand.

I think the easiest fix is simple to adjust this in the variable names and match:

rustfmt/src/expr.rs

Lines 1531 to 1538 in 365a2f8

	let has_base = match struct_rest {
	ast::StructRest::None if fields.is_empty() => return Some(format!("{} {{}}", path_str)),
	ast::StructRest::Rest(_) if fields.is_empty() => {
	return Some(format!("{} {{ .. }}", path_str));
	}
	ast::StructRest::Base(_) => true,
	_ => false,
	};

to update the pattern on the arm below, e.g.

    let has_base_or_rest = match struct_rest {
        ast::StructRest::None if fields.is_empty() => return Some(format!("{} {{}}", path_str)),
        ast::StructRest::Rest(_) if fields.is_empty() => {
            return Some(format!("{} {{ .. }}", path_str));
        }
        ast::StructRest::Rest(_) | ast::StructRest::Base(_) => true,
        _ => false,
    };

and updating the variable usage accordingly. we should also then be able to remove the extra line that ensures a trailing comma wasn't added in the event of a the rest variant:

rustfmt/src/expr.rs

Lines 1617 to 1620 in 365a2f8

	force_no_trailing_comma
	|| has_base
	|| !context.use_block_indent()
	|| matches!(struct_rest, ast::StructRest::Rest(_)),

[ytmimi]

Thanks for the feedback! I didn't realize that we shouldn't even attempt to rewrite the struct with alignment if it contained a "..". I went ahead and implemented your suggested changes and rebased the PR.

[calebcartwright]

Thanks for all the tests!