Formatting adds erroneous comma at end of struct update syntax when missing comma before ..

[alice-i-cecile]

Following up on rust-lang/rust#104373, when using functional record update syntax, if a comma is missed before the ..Default::default(), rustfmt will then "helpfully" add a comma after the ..Default::default().

When you fix the error with the missing comma, you now have a different error ("no trailing commas after...") that the user did not make, as automatic formatting added a comma in its confusion.

I'm new to this project, but the new error enum variant proposed in rust-lang/rust#104504 may be helpful to identify these cases.

[ytmimi]

Thanks for reaching out. Using the latest master rustfmt 1.5.1-nightly (ee2bed96 2022-11-08) this is what I'm getting.

Input

fn main(){
    Outer {
        inner: Inner {
            a: 1,
            b: 2,
        }
        ..Default::default()
    };
}

Output

fn main() {
    Outer {
        inner: Inner { a: 1, b: 2 }..Default::default(),
    };
}

From my understanding the expected output is something like:

fn main() {
    Outer {
        inner: Inner { a: 1, b: 2 },
        ..Default::default(),
    };
}

[alice-i-cecile]

If we want to stick to pure formatting, the input shouldn't change at all. But if you want to automatically fix this class of mistake (which would be super slick, but maybe too implicit?), the expected output is actually

fn main() {
    Outer {
        inner: Inner { a: 1, b: 2 },
        ..Default::default()
    };
}

[TehPers]

Is it possible that the issue is that the input looks like range syntax?

[ytmimi]

@TehPers your hunch is correct. I just dug into this and from the perspective of the AST, which rustfmt operates on, Inner { a: 1, b: 2 }..Default::default(), gets parsed as an ast::Expr with a kind of ExprKind::Range.

Sadly, I don't think there's an easy fix for this on the rustfmt side 😕

[TehPers]

I agree. I think there's some ambiguity that's preventing this. For example, how is the following supposed to be formatted?

#[derive(Default)]
struct Inner;

struct Outer {
    inner: Range<Inner>,
}

fn main() {
    let outer = Outer {
        // is this supposed to be Range<Inner> or Inner? compiles as Inner
        inner: Default::default()
        ..Default::default()
    };
}

[ytmimi]

Although I'm not sure that rustfmt is the right place to implement a fix for this, if anyone's interested in coming up with a solution this might be a good place to start since it's where we handle rewriting ranges:

rustfmt/src/expr.rs

Lines 269 to 342 in ee2bed9

	ast::ExprKind::Range(ref lhs, ref rhs, limits) => {
	let delim = match limits {
	ast::RangeLimits::HalfOpen => "..",
	ast::RangeLimits::Closed => "..=",
	};
	fn needs_space_before_range(context: &RewriteContext<'_>, lhs: &ast::Expr) -> bool {
	match lhs.kind {
	ast::ExprKind::Lit(ref lit) => match lit.kind {
	ast::LitKind::Float(_, ast::LitFloatType::Unsuffixed) => {
	context.snippet(lit.span).ends_with('.')
	}
	_ => false,
	},
	ast::ExprKind::Unary(_, ref expr) => needs_space_before_range(context, expr),
	_ => false,
	}
	}
	fn needs_space_after_range(rhs: &ast::Expr) -> bool {
	// Don't format `.. ..` into `....`, which is invalid.
	//
	// This check is unnecessary for `lhs`, because a range
	// starting from another range needs parentheses as `(x ..) ..`
	// (`x .. ..` is a range from `x` to `..`).
	matches!(rhs.kind, ast::ExprKind::Range(None, _, _))
	}
	let default_sp_delim = |lhs: Option<&ast::Expr>, rhs: Option<&ast::Expr>| {
	let space_if = |b: bool| if b { " " } else { "" };
	format!(
	"{}{}{}",
	lhs.map_or("", |lhs| space_if(needs_space_before_range(context, lhs))),
	delim,
	rhs.map_or("", |rhs| space_if(needs_space_after_range(rhs))),
	)
	};
	match (lhs.as_ref().map(|x| &**x), rhs.as_ref().map(|x| &**x)) {
	(Some(lhs), Some(rhs)) => {
	let sp_delim = if context.config.spaces_around_ranges() {
	format!(" {} ", delim)
	} else {
	default_sp_delim(Some(lhs), Some(rhs))
	};
	rewrite_pair(
	&*lhs,
	&*rhs,
	PairParts::infix(&sp_delim),
	context,
	shape,
	context.config.binop_separator(),
	)
	}
	(None, Some(rhs)) => {
	let sp_delim = if context.config.spaces_around_ranges() {
	format!("{} ", delim)
	} else {
	default_sp_delim(None, Some(rhs))
	};
	rewrite_unary_prefix(context, &sp_delim, &*rhs, shape)
	}
	(Some(lhs), None) => {
	let sp_delim = if context.config.spaces_around_ranges() {
	format!(" {}", delim)
	} else {
	default_sp_delim(Some(lhs), None)
	};
	rewrite_unary_suffix(context, &sp_delim, &*lhs, shape)
	}
	(None, None) => Some(delim.to_owned()),
	}
	}

[ytmimi]

@alice-i-cecile given that rust-lang/rust#104373 is closed can this be closed too? As mentioned above, the reason why rustfmt produces the formatting that it does is a result of how the rustc_parser parses expr..Default::defaut() as a range when the comma is missing. I'm not as familiar with the parser, but I'm assuming a proper fix for this would be to ensure that the rustc_parser parses ..Default::default() as ast::StructRest::Base even when the trailing comma is missing.

[ytmimi]

If we do want to try to special case the formatting on the rustfmt side it might be better to look into rewrite_struct_lit instead of looking into the code that's responsible for rewriting ranges.