Rustfmt deletes polarity of negative impl

[dtolnay]

As of current master branch (fd9e7d3):

$ echo 'impl!Trait{}' | CFG_RELEASE_CHANNEL=dev CFG_RELEASE=1.48.0 cargo +nightly run --bin rustfmt --features rustfmt

Expected:

impl !Trait {}

Actual:

impl Trait {}

This syntax is accepted syntactically by rustc since 1.25.0. Playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a0af137c2df7c45fe1571573ec6a8ad3

[calebcartwright]

That's an odd miss considering rustfmt wasn't "officially" released til 1.31.0 🤔

I'm a bit behind on rustfmt PRs and issues but will try to take a look at this after work today

[calebcartwright]

Ah I see, this only occurs in the absence of a trait_ref (compare with impl ! Trait for Foo{}) as rustfmt has an internal assumption that there wouldn't be a negative polarity if there wasn't a trait_ref present.

While I'm not sure what the use case is for a negative impl without a for type (in fairness I'm very unfamiliar with negative impls), it's nevertheless valid syntax and there's no reason rustfmt should drop a negative polarity on an impl item.

Going to mark this as a good-first-issue in case others want to work on it, but please let me know if this is a blocker/higher urgency for you and I'll go ahead and resolve myself.

For anyone interested in working on this, you'll want to take a look at the format_impl_ref_and_type function in items.rs (src/formatting/items.rs).

Things are already taken care of when there is a trait ref:

rustfmt/src/formatting/items.rs

Lines 967 to 976 in fd9e7d3

	if let Some(ref trait_ref) = *trait_ref {
	let result_len = last_line_width(&result);
	result.push_str(&rewrite_trait_ref(
	context,
	trait_ref,
	offset,
	polarity_str,
	result_len,
	)?);
	}

But in cases where there isn't then rustfmt is silently tossing the polarity string. In order to resolve this, we should probably incorporate a new check that looks for the combination of a negative polarity and the absence of a trait ref, and if true then ensure the polarity string (!) is included immediately preceding the type string.

Be sure to include the extra character in the budget computations to prevent any off-by-one issues with edgier cases that run up around the max_width limit, and then incorporate the check both for the case where the type string can fit on one line:

rustfmt/src/formatting/items.rs

Lines 991 to 1004 in fd9e7d3

	let used_space = last_line_width(&result) + trait_ref_overhead + curly_brace_overhead;
	// 1 = space before the type.
	let budget = context.budget(used_space + 1);
	if let Some(self_ty_str) = self_ty.rewrite(context, Shape::legacy(budget, offset)) {
	if !self_ty_str.contains('\n') {
	if trait_ref.is_some() {
	result.push_str(" for ");
	} else {
	result.push(' ');
	}
	result.push_str(&self_ty_str);
	return Some(result);
	}
	}

and the case where it has to be multilined

rustfmt/src/formatting/items.rs

Lines 1006 to 1019 in fd9e7d3

	// Couldn't fit the self type on a single line, put it on a new line.
	result.push('\n');
	// Add indentation of one additional tab.
	let new_line_offset = offset.block_indent(context.config);
	result.push_str(&new_line_offset.to_string(context.config));
	if trait_ref.is_some() {
	result.push_str("for ");
	}
	let budget = context.budget(last_line_width(&result));
	let type_offset = match context.config.indent_style() {
	IndentStyle::Visual => new_line_offset + trait_ref_overhead,
	IndentStyle::Block => new_line_offset,
	};
	result.push_str(&*self_ty.rewrite(context, Shape::legacy(budget, type_offset))?);