Double Commas in #derive eats entire #derive

[sanbox-irl]

Input:

#[derive(Debug, ,Clone)]
pub enum Test {
    One,
    Two
}

(note the double commas after Debug above. The space is irrelevant in my testing)

Outputs:

#[derive()]
pub enum Test {
    One,
    Two,
}

and a :( from me.

Thanks guys

[topecongiro]

Marking this p-low as the input does not compile in the first place, though rustfmt should be able to handle this somehow as it is parsable.

[lusen82]

I will try to take a look at this as my very first open-source issue at RustFest impl-days..

[calebcartwright]

That's great @lusen82! I don't know offhand specifically where the issue may be, but I'd probably start with looking at some of the below functions in src/attr.rs

rustfmt/src/attr.rs

Lines 60 to 66 in 7ecd467

	fn get_derive_spans<'a>(attr: &'a ast::Attribute) -> Option<impl Iterator<Item = Span> + 'a> {
	attr.meta_item_list().map(|meta_item_list| {
	meta_item_list
	.into_iter()
	.map(|nested_meta_item| nested_meta_item.span())
	})
	}

I'm guessing that meta_item_list there is returning None with double commas, and that ends up resulting in derive_args being empty for formatting:

rustfmt/src/attr.rs

Lines 95 to 127 in 7ecd467

	fn format_derive(
	derive_args: &[Span],
	prefix: &str,
	shape: Shape,
	context: &RewriteContext<'_>,
	) -> Option<String> {
	let mut result = String::with_capacity(128);
	result.push_str(prefix);
	result.push_str("[derive(");
	let argument_shape = argument_shape(10 + prefix.len(), 2, false, shape, context)?;
	let item_str = format_arg_list(
	derive_args.iter(),
	|_| DUMMY_SP.lo(),
	|_| DUMMY_SP.hi(),
	|sp| Some(context.snippet(**sp).to_owned()),
	DUMMY_SP,
	context,
	argument_shape,
	// 10 = "[derive()]", 3 = "()" and "]"
	shape.offset_left(10 + prefix.len())?.sub_width(3)?,
	None,
	false,
	)?;
	result.push_str(&item_str);
	if item_str.starts_with('\n') {
	result.push(',');
	result.push_str(&shape.indent.to_string_with_newline(context.config));
	}
	result.push_str(")]");
	Some(result)
	}

[lusen82]

Hi Calebcartwright, sorry, it seems I came back to reality (and with that, my day-job) so I haven't seen your message.
Thank you very much for the help. I had also arrived into that part and debug-printed a note which seems to show that for this case, the derives vec is not empty, but the derive_spans is. To me, it seems that meta_item_list is empty for that "derive"-attribute. So, the making in "get_derive_spans" is mapping over an empty list (and return as you already mentioned) derive_args empty.
Then, I tried to see how the derive attribute actually was constructed, but it seems that it led me to the rustc and I didn't get any deeper.
To be able to debug (I figured that maybe that would help me), I then focused on trying to install a VisualSourceCode (instead of Intellij) and apply a rust debugger, however, unfortunately with no success.
Could it be that the attribute at a much earlier state is sort of already "empty"?

[calebcartwright]

Thanks for digging into it @lusen82! Were you able to determine if meta_item_list is returning None or Some with an empty vec for the malformed derive?

I could see this issue being frustrating in instances where folks have automatic format-on-save enabled in their editors, and a simple extra comma typo results in that entire derive getting wiped on save.

I think to address this we'd need to be able to detect this malformed derivce condition and if possible, remove the erroneous comma, or just leave the malformed derive as-is in the worst case scenario.

rustfmt would also need to be able to differentiate between this scenario (a malformed derive) and an empty derive #[derive()] so that rustfmt would still format this:

#[derive(Debug)]
#[derive()]
struct Foo {
    bar: u32,
}

to

#[derive(Debug)]
struct Foo {
    bar: u32,
}

[lusen82]

Thanks for your reply! To me, it really seems that meta_item_list in this case is returning None (and the derives are thus already "lost"). I tried to trace back to where the attribute was kind of created to see if the check could com in at an earlier state, but I some what ended up in the rustc and didn't really find anywhere to identify the typo..

[calebcartwright]

I tried to trace back to where the attribute was kind of created to see if the check could com in at an earlier state, but I some what ended up in the rustc and didn't really find anywhere to identify the typo

rustfmt uses the rustc parser to generate the AST, and the AST is then used to generate the desired formatting. It's this piece that'll need to be updated to handle the malformed derive (rustfmt takes whatever the parser gives us).

Even though the derive is malformed, the associated AST node still has everything needed to get the original derive attribute contents (via context.snippet(span)) that could be used to emit the original (malformed) derive, or potentially, try to craft the "fixed" version that removes the extra ,.

The problem is that when merge_derives is enabled (and it is by default), the malformed derive attributes aren't properly handled. They're basically tossed because the meta info is unavailable:

rustfmt/src/attr.rs

Lines 418 to 431 in d5b6560

	if context.config.merge_derives() && is_derive(&attrs[0]) {
	let derives = take_while_with_pred(context, attrs, is_derive);
	let derive_spans: Vec<_> = derives
	.iter()
	.filter_map(get_derive_spans)
	.flatten()
	.collect();
	let derive_str =
	format_derive(&derive_spans, attr_prefix(&attrs[0]), shape, context)?;
	result.push_str(&derive_str);
	let missing_span = attrs
	.get(derives.len())
	.map(|next| mk_sp(attrs[derives.len() - 1].span.hi(), next.span.lo()));

For those derive attributes, the attributes that have Some(..) for meta()/meta_item_list() are valid/well-formed and can be merged via the format_derive function, but some changes/additions are needed to be able to handle the malformed derive attribute(s).

You'll notice that if you set merge_derives to false in a rustfmt config file with the snippet in the description that the malformed derive attribute is left as-is because that formats the attribute with:

rustfmt/src/attr.rs

Lines 459 to 460 in d5b6560

	let formatted_attr = attrs[0].rewrite(context, shape)?;
	result.push_str(&formatted_attr);

Which just emits the original snippet of the derive.

Perhaps one way to solve this would be to split the resulting collection of derive attributes returned from take_while_with_pred on line 419 into a collection of the valid/well-formed derives and another collection of the malformed derives, and then format those two sets of derived attributes separately.

[lusen82]

Thank you very much for your detailed input. I understand what you mean and also found the context's original text of derive. I have a small fix now which seems to work but it feels a little bit ugly and non-general. I will at least try to clean it up a little bit and test some more cases before proposing a first version :)