Extra whitespace in nested pub extern

[rillian]

The following snippet formats with an extra space between pub and extern "C" if the arguments continue across multiple lines. i.e. rustfmt produces pub␣␣extern "C" instead of pub␣extern "C".

pub struct SomeCallback(
    pub extern "C" fn( 
        long_argument_name_to_avoid_wrap: u32,
        second_long_argument_name: u32,
        third_long_argument_name: u32,
    ),
);

Checked with

• rustfmt 1.5.1-stable (4b91a6e 2022-08-08) from 1.63.0 stable
• rustfmt 1.5.1-nightly (c07a8b4 2022-08-26) from a recent nightly

[ytmimi]

For anyone interested in working on this here's a little bit of what I found:

The Rewrite impl for ast::FieldDef is implemented by calling rewrite_struct_field.

To calculate the field_str, rewrite_struct_field calls rewrite_assign_rhs and from what I can tell that's where the error is.

rustfmt/src/items.rs

Line 1784 in 38659ec

let field_str = rewrite_assign_rhs(context, prefix, &*field.ty, &RhsAssignKind::Ty, shape)?;

Following the calls made after rewrite_assign_rhs you'll get to rewrite_assign_rhs_with, which just concatenates the lhs and rhs. In this case the lhs is 'pub ' and the rhs is ' extern "C" ...'

rustfmt/src/expr.rs

Lines 1977 to 1988 in 38659ec

	pub(crate) fn rewrite_assign_rhs_with<S: Into<String>, R: Rewrite>(
	context: &RewriteContext<'_>,
	lhs: S,
	ex: &R,
	shape: Shape,
	rhs_kind: &RhsAssignKind<'_>,
	rhs_tactics: RhsTactics,
	) -> Option<String> {
	let lhs = lhs.into();
	let rhs = rewrite_assign_rhs_expr(context, &lhs, ex, shape, rhs_kind, rhs_tactics)?;
	Some(lhs + &rhs)
	}

[jmj0502]

@ytmimi I would like to work on this issue! ✌️

[jmj0502]

@rustbot claim

[jmj0502]

@ytmimi in regard to this issue, I noticed that there are several tests with the following format:

 struct MyTuple(
    #[cfg(unix)] // some comment
    pub  u64,
    #[cfg(not(unix))] /*block comment */ pub(crate)  u32,
 );

Is that alright? Or should I correct such tests?

[ytmimi]

@jmj0502 Normally changing existing tests isn't something we want to do, but in this case it seems like the bug slipped into those tests. I think it should be fine to update the affected tests, but there's a chance we need to version gate the fix.

[jmj0502]

@ytmimi I understand 👍! I asked because the issue truly seems like a bug and there are only two affected tests. However, I think the version gate fix is the way to go, since that won't "break" the format of any existing project.

[daprilik]

Just ran into this myself.

type ThisIsAVeryLongTypeNameThatWillCauseWrappingToKickIn = usize;

#[derive(serde_derive::Serialize)]
pub struct Foo(
    #[serde(rename = "ThisIsAVeryLongTypeNameThatWillCauseWrappingToKickIn")]
    pub  ThisIsAVeryLongTypeNameThatWillCauseWrappingToKickIn,
);

Is this something that'll get fixed via #5529 or #5708?

[ytmimi]

@daprilik I just checked and #5708 should resolve your issue. At the moment the team is focused on higher priority issues like let-else formatting, so unfortunately I can't give a clear answer for when a fix will be released.

The linked PR is currently waiting on a review. Although maintainers are the only ones who can merge approved PRs anyone in the community is welcome to review and provide feedback on any PR 😁