Removing empty type parameter list results in invalid Rust code

[steffahn]

pub fn foo() {
    let x: u32 = 100;
    if x as i32<> < 0 {
        // ...
    }
}

(playground)

gets formatted into

pub fn foo() {
    let x: u32 = 100;
    if x as i32 < 0 {
        // ...
    }
}

   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: `(x as i32)`

error: aborting due to previous error

[ytmimi]

I spent a little bit of time looking into this. The issue can be tracked down to types::rewrite_generic_args because we don't handle the case where the ast::GenericArgs::AngleBracketed args are empty. The current behavior just falls back to returning an empty string.

rustfmt/src/types.rs

Lines 468 to 501 in 17003ce

	fn rewrite_generic_args(
	gen_args: &ast::GenericArgs,
	context: &RewriteContext<'_>,
	shape: Shape,
	span: Span,
	) -> Option<String> {
	match gen_args {
	ast::GenericArgs::AngleBracketed(ref data) if !data.args.is_empty() => {
	let args = data
	.args
	.iter()
	.map(|x| match x {
	ast::AngleBracketedArg::Arg(generic_arg) => {
	SegmentParam::from_generic_arg(generic_arg)
	}
	ast::AngleBracketedArg::Constraint(constraint) => {
	SegmentParam::Binding(constraint)
	}
	})
	.collect::<Vec<_>>();
	overflow::rewrite_with_angle_brackets(context, "", args.iter(), shape, span)
	}
	ast::GenericArgs::Parenthesized(ref data) => format_function_type(
	data.inputs.iter().map(|x| &**x),
	&data.output,
	false,
	data.span,
	context,
	shape,
	),
	_ => Some("".to_owned()),
	}
	}

[sec65]

@ytmimi I'm new to this project and just had a look at this issue. I understand, what's happening, but I'm not so sure about the expected result. I could remove the if-condition, so the match-block is executed whether the args are empty or not.

That leads to the result that the line

if x as i32<> < 0 {

is not changed (what I thought was the expected result). But that fails the test case of #3639, since it looks to me, that the expectation there is, that the <> is removed.

[calebcartwright]

@sec65 - i believe there's probably at least a couple callsites of the function Yacin referenced above, so you may want to see if you can drive differentiating behavior in that function by incorporating a new parameter that the calling functions can provide (e.g. don't remove for paths/segments, but do remove for empty associated constraints)

and thanks for working on this!

[sec65]

Thanks for your reply. I had a look into it, but unfortunately the distinction is not that easy (at least for me ;-):

The x as i32<> as well as impl Foo<> for Bar<> is rewritten via rewrite_path/rewrite_segment. A possible distinction is above, e.g.

x as i32<> is handled via ExprKind::Cast in format_expr_inner in expr.rs
Foo<> is handled via the Rewrite-implementation for TraitRef in types.rs
Bar<> is handled via format_impl_ref_and_type in items.rs

But it seems to me, that I don't have this information down in the rewrite-methods (?), so I can't distinguish there. And I'm not sure, if it's good to pass this information through all the methods in between. Or is there already any possibility to know where it comes from?

And, what excactly is the condition, that it should be removed or not?

E.g. for the aboved mentioned term if x as i32<> < 0 it shouldn't be removed because of the error, but for the term if x as i32<> > 0 it could be removed (because rust understands if x as i32 > 0). So is it only for a cast in combination with <, in which case it shouldn't be removed or for all casts?

Or maybe, the best solution would be to add paranthesis as proposed by rust, since from my point of view if (x as i32) < 0 is easier to understand than if x as i32<> < 0?

[calebcartwright]

And, what excactly is the condition, that it should be removed or not?

It's definitely a problem that needs to be solved. We never want to have rustfmt accept an input that's valid, compilable code and produce an output which isn't. I suspect this probably isn't a terribly common occurrence (certainly not a widespread, every day encounter), and the available workarounds play into priority, but it's still something that needs to be addressed.

I've not really had a chance to look at the code but based on what you're saying it sounds like it may not be feasible to easily pass along context through call args.

Or maybe, the best solution would be to add paranthesis as proposed by rust, since from my point of view if (x as i32) < 0 is

Perhaps. One of the benefits of the formatting model rustfmt employs is that it's possible and relatively straightforward to make such transformations. Are you thinking for cast expressions to just look at the type and if the type kind is path or whichever variant(s) with empty args to wrap in parens?

[calebcartwright]

Also I acknowledge the easy route would be to simple change the way we handle empty arg lists, but that's desired behavior in most other contexts and is how rustfmt has behaved forever. I'd be highly hesitant to change that behavior just for this case

[calebcartwright]

Are you thinking for cast expressions to just look at the type and if the type kind is path or whichever variant(s) with empty args to wrap in parens?

Sorry, I realized this would actually be significantly more complicated as it really has to be considered in the broader context, particularly some right-most path ending the lhs operand in the presence of the < operator.

Really appreciate you looking at this one @sec65 and you're absolutely free to continue working on it, but I'm going to remove the good first issue label to reflect that underlying complexity.

If you're interested in contributing to the project, I'll note there's a number of other issues with the good first issue label that are probably much more straightforward if you wanted to look at one of those (regardless of whether or not you want to continue working on this one too)

[sec65]

@calebcartwright Thanks for your response and help. In that case I will look for some other issue.

[ytmimi]

@sec65 Just wanted to apologize for the misleading good first issue. I assumed it would be easy when I labeled it that, but alas it was definitely harder to go down the route that I initially thought we could use. I've gone ahead and implemented your suggestion to wrap the cast in parentheses! Feel free to take a look at the PR if you've got time.

I know you're on the lookout for other issues. If you find one you'd like to work on you can comment @rustbot claim to assign yourself to the issue, and just know you can always reach out for advice or help with any implementation issues you run into.