Wrap cast expr in parens when the ty ends with empty angle brackets

[ytmimi]

Fixes #4621

Previously rustfmt would remove empty angle brackets from cast expressions e.g. x as i32<> => x as i32. This lead to code that could not compile when the cast occurred in an if statement.

The advice from the compiler was to wrap the cast in parentheses, which is now the behavior rustfmt employs.

[calebcartwright]

To expand on what I was alluding to in #4621 (comment), I don't think we can necessarily solve this problem at the cast expression level. Even with casts, the issue only occurs when it is in the lhs operand position of a bin expression which also has the less than/less than equal to operators. For example, x as i32<> >= 0 isn't a problem.

Additionally, I'm not sure if there's other scenarios/expr variants in the lhs position where this could occur too

[ytmimi]

Additionally, I'm not sure if there's other scenarios/expr variants in the lhs position where this could occur too

From what I can tell there are only three binary operator that starts with a < and could be confused with the start of a generic args list (<, <=, <<).

less than (<)

The case that originally brought this issue to our attention. Here's the playground link, and Here's the parse error that we get after removing the <>.

Compiling playground v0.0.1 (/playground)
error: `<` is interpreted as a start of generic arguments for `i32`, not a comparison
 --> src/lib.rs:3:17
  |
3 |     if x as i32 < 0 {
  |                 ^ --- interpreted as generic arguments
  |                 |
  |                 not interpreted as comparison
  |
help: try comparing the cast value
  |
3 |     if (x as i32) < 0 {
  |        +        +

error: could not compile `playground` due to previous error

less than or equal (<=)

works just fine! here's the playground link.

left shift (<<)

This also leads to a parse error. Here's the playground link, and Here's the parse error that we get after removing the <>.

error: `<` is interpreted as a start of generic arguments for `i32`, not a shift
 --> /playground/src/main.rs:3:17
  |
3 |     if x as i32 << 1 < 0 {
  |                 ^^ - interpreted as generic arguments
  |                 |
  |                 not interpreted as shift
  |
help: try shifting the cast value
  |
3 |     if (x as i32) << 1 < 0 {
  |        +        +

I'm trying to think if there are any other contexts where this might come up, but can't think of any where a Ty would be followed by a <, where the < doesn't represents the start of a generic args list.

---

I don't think we can necessarily solve this problem at the cast expression level

I think solving this at the cast level has the smallest footprint on the codebase, but agree that it's a broad fix that encompasses scenarios that can parse just fine without the addition of the parens.

Solving this at the BinaryOp level should also be possible, but maybe a bit more involved.

Taking a look at ExprKind::Binary if feels like we'd need to handle this empty generic args + misinterpreted < case in ast::Expr::flatten (which is called by rewrite_all_pairs) and rewrite_pair to handle the single pair case.

rustfmt/src/expr.rs

Lines 144 to 156 in 33c6074

	ast::ExprKind::Binary(op, ref lhs, ref rhs) => {
	// FIXME: format comments between operands and operator
	rewrite_all_pairs(expr, shape, context).or_else(|| {
	rewrite_pair(
	&**lhs,
	&**rhs,
	PairParts::infix(&format!(" {} ", context.snippet(op.span))),
	context,
	shape,
	context.config.binop_separator(),
	)
	})
	}

rustfmt/src/pairs.rs

Lines 251 to 256 in 33c6074

	impl FlattenPair for ast::Expr {
	fn flatten(
	&self,
	context: &RewriteContext<'_>,
	shape: Shape,
	) -> Option<PairList<'_, '_, ast::Expr>> {

[calebcartwright]

The needle we have to try to thread at this point is solving for the case that's an issue without applying breaking formatting changes in unnecessary cases (and yes this would qualify as a breaking change in those unnecessary cases). If we could go back in time and establish this as the formatting approach from day 1 I'd definitely agree this would be the right way to go.

However, we've got a lot of historical context to account for. I'm not sure what the best path forward is, but given the rarity of the problematic scenario and the multiple viable workarounds, this isn't something we should try to push a broader change for in order to get an easier fix to the more narrow scenario

[ytmimi]

I totally understand that this would be a breaking change for those cases where things already work out just fine. I'll continue to explore what options we have to fix this!

[calebcartwright]

Sounds good. This isn't a terribly urgent item, so what would you think about us closing for now and we can revisit if you come up with a different solution?

[ytmimi]

I think I'd want to hack on this at least through the weekend, and if I can't make any meaningful progress then I'd be happy to close this and revisit it at a later time.

[ytmimi]

Alright, I went ahead and moved the fix up to the Binary expression level. I plan on adding a test to cover the left shift case and the greater than or equal case. I'll also add a few test cases with some longer binary expression chains just to be sure we handle those cases as well.