Prevent trailing whitespace in where clause bound predicate

[ytmimi]

Resolves #5012
Resolves #4850

This behavior was noticed when using the trailing_comma = "Never"
configuration option (5012).

This behavior was also noticed when using default configurations (4850).

rustfmt would add a trailing space to where clause bounds that had an
empty right hand side.

Now no trailing space is added to the end of these where clause bounds.

[calebcartwright]

This is on one of the hottest paths in the codebase, and while there wouldn't be any significant performance concerns with the proposed diff in this location, we should strive to avoid having to make fixes at this level due to the higher potential for unintended knock on effects.

Ideally, we'd be able to correct the issue upstream instead of having to try to retcon it here. Do you think that will be feasible in this case?

[ytmimi]

Ideally, we'd be able to correct the issue upstream instead of having to try to retcon it here. Do you think that will be feasible in this case?

I think we could address it a little further upstream. Before I pitch some ideas I want to lay out my understanding of the code flow.

Code Flow

In this case when we call rewrite on ast::WherePredicate, we hit the first match arm of ast::WherePredicate::BoundPredicate, which ultimately calls expr::rewrite_assign_rhs

rustfmt/src/types.rs

Lines 412 to 433 in 599b2fd

	impl Rewrite for ast::WherePredicate {
	fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option<String> {
	// FIXME: dead spans?
	let result = match *self {
	ast::WherePredicate::BoundPredicate(ast::WhereBoundPredicate {
	ref bound_generic_params,
	ref bounded_ty,
	ref bounds,
	..
	}) => {
	let type_str = bounded_ty.rewrite(context, shape)?;
	let colon = type_bound_colon(context).trim_end();
	let lhs = if let Some(lifetime_str) =
	rewrite_lifetime_param(context, shape, bound_generic_params)
	{
	format!("for<{}> {}{}", lifetime_str, type_str, colon)
	} else {
	format!("{}{}", type_str, colon)
	};
	rewrite_assign_rhs(context, lhs, bounds, shape)?
	}

As it's currently implemented, expr::rewrite_assign_rhs just calls expr::rewrite_assign_rhs_with, which is where the lhs and rhs get concatenated (linked below for reference).

rustfmt/src/expr.rs

Lines 1918 to 1928 in 599b2fd

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

Ideas

1. We could add a conditional check in expr::rewrite_assign_rhs_with to only concatenate the lhs and rhs if the rhs isn't just whitespace.

• This change is still on the hot path, and really only patches the underlying issue introduced by expr::choose_rhs returning " " when there is no rhs, but it does tackle the problem slightly further upstream.

2. Create a new function expr::rewrite_assign_potentially_empty_rhs, (open to name suggestion) that will be called instead of expr::rewrite_assign_rhs when rewriting ast::WherePredicate::BoundPredicate.

• We'll still leverage code that's on the hot path, but we'll only have to perform the relevant conditional checks in cases where we can't be sure that there will always be a rhs. (I think I'm leaning towards this option)

3. Go with the current solution that makes changes directly to expr::choose_rhs

---

Definitely open to your thoughts and if you have other suggestions for where to make this fix!

[calebcartwright]

Thanks for the excellent, detailed write up. After reading and reflecting I think you were on the right path to begin with. I was originally thinking we'd check if the bounds where empty at the original callsite and skip trying to join a lhs with a non-existent rhs to begin with. However, doing that would likely start to bleed through the encapsulation of the Rewrite impl for GenericBounds and/or require an upfront rewrite call of the bounds node to check for the empty string.

As such I think I'm leaning pretty heavily towards num 3/your original proposal. Realize this goes slightly past the literal issue reports but could you also add some cases with the snippet from 5012 that are configured to do comma inserts too?

[ytmimi]

option 3 it is! I'll add those additional test cases and request a follow up review when they've been added!

[calebcartwright]

Thanks!