struct_field_align_threshold breaks struct update syntax inside matches! macro

[jrose-signal]

Sending the following code through rustfmt --config struct_field_align_threshold=30 removes the .., changing the meaning of the code.

struct X { a: i32, b: i32 }

fn test(x: X) {
    let y = matches!(x, X {
        a: 1,
        ..
    });
}

becomes

struct X {
    a: i32,
    b: i32,
}

fn test(x: X) {
    let y = matches!(x, X { a: 1 });
}

[calebcartwright]

Thank you for the report. I wonder if this stems back to not-so-long-ago addition of being able to destructure slices and structs and the splitting of the base variants to differentiate between ..foo vs ... I think rustfmt wouldn't have seen the new base (..) in the expression context prior to that feature, so perhaps we need to update the logic that determines whether alignment is possible to account for both of those variants.

For anyone interested in working on this, the function linked below would probably be a decent place to start hunting for the cause:

rustfmt/src/expr.rs

Lines 1500 to 1507 in e81c393

	let has_base = match struct_rest {
	ast::StructRest::None if fields.is_empty() => return Some(format!("{} {{}}", path_str)),
	ast::StructRest::Rest(_) if fields.is_empty() => {
	return Some(format!("{} {{ .. }}", path_str));
	}
	ast::StructRest::Base(_) => true,
	_ => false,
	};

[ytmimi]

Hey! I saw this issue and I thought I'd see if I could figure something out. Also @calebcartwright thanks for pointing me in the direction of expr::rewrite_struct_lit. Would love some feedback on what I've figured out so far:

• The .. isn't removed if you don't supply the struct_field_align_threshold
• On line 1546 of expr.rs the following boolean expression is evaluated as true.

    let fields_str = if struct_lit_can_be_aligned(fields, has_base)
        && context.config.struct_field_align_threshold() > 0

• Because it evaluates to true expr::rewrite_struct_lit calls vertical::rewrite_with_alignment, but in the case where the expression evaluates to false the code is properly formatted so I think there's a hint to what should be happening in the else case (more on that below) .

• vertical::rewrite_with_alignment ends up calling vertical::rewrite_aligned_items_inner, which returns "a: 1". I assume it should really return "a:1, ..", so something is probably off in vertical::rewrite_aligned_items_inner.

• looking at the body of vertical::rewrite_aligned_items_inner it does a lot of the same work that the else case does back in expr::rewrite_struct_lit. Which is to say that it:

  • creates a ListItems object
  • figures out the DefinitiveListTactic
  • figures out the ListFormatting
  • and calls write_list

• The main difference that I can see between what's happening in expr::rewrite_struct_lit and vertical::rewrite_aligned_items_inner is that internally expr::rewrite_struct_lit defines an enum named StructLitField, and maps each field to a variant. Also the closure defined for get_lo when constructing the ListItems in expr::rewrite_struct_lit seems to be doing a lot more than the get_lo closure defined in vertical::rewrite_aligned_items_inner.

// get_lo in expr::rewrite_struct_lit
let span_lo = |item: &StructLitField<'_>| match *item {
    StructLitField::Regular(field) => field.span().lo(),
    StructLitField::Base(expr) => {
        let last_field_hi = fields.last().map_or(span.lo(), |field| field.span.hi());
        let snippet = context.snippet(mk_sp(last_field_hi, expr.span.lo()));
        let pos = snippet.find_uncommented("..").unwrap();
        last_field_hi + BytePos(pos as u32)
    }
    StructLitField::Rest(span) => span.lo(),
};

V.S

// get_lo in vertical::rewrite_aligned_items_inner
|field| field.get_span().lo()

• Based only on the name it seems like the call to snippet.find_uncommented("..") might be what we need to make this work? To be honest I have no idea what span.lo or span.hi are so it's a little hard for me to understand what's happening in the StructLitField::Base match arm.

[ytmimi]

So I kept digging and I actually think I've figured out what's going on. It's partially related to the things I figured out above, but there's a bit more too it. The main reason why the code is properly formatted in the else case of expr::rewrite_struct_lit is because of the StructLitField enum.

In the else case of expr::rewrite_struct_lit the iterator used to construct the ListItems object includes all the struct fields + the struct_rest: &ast::StructRest. In the case where vertical::rewrite_aligned_items_inner is called only the struct fields are used.

Above I said that the get_lo closure caught my attention, but it turns out the difference really comes down to the closure used for get_item_string. In the else case of expr::rewrite_struct_lit where the struct_rest is a ast::StructRest::Rest then Some("..") is returned. I figured out that this match arm was hit by adding a call to debug. We hit that match arm using the provided example and running rustfmt without providing the --config struct_field_align_threshold=30 option.

For reference here's the match arm.

let rewrite = |item: &StructLitField<'_>| match *item {
    StructLitField::Regular(field) => {
        // The 1 taken from the v_budget is for the comma.
        rewrite_field(context, field, v_shape.sub_width(1)?, 0)
    }
    StructLitField::Base(expr) => {
        // 2 = ..
        expr.rewrite(context, v_shape.offset_left(2)?)
            .map(|s| format!("..{}", s))
    }
    StructLitField::Rest(_) => {
        debug!("Writing the rest")
        Some("..".to_owned()), // <--Handle ".."
    }
};

I think I have a fix and I'd like to open a PR!

[calebcartwright]

Playing a bit of catch up, and while there's been some great discussion here I think there's a much simpler fix available which I'll note on the associated PR.

To be honest I have no idea what span.lo or span.hi are so it's a little hard for me to understand what's happening in the StructLitField::Base match arm.

I suspect you have probably already gained a better feel for this in the time that's past since that comment, but the short/grossly-simplified version is:

Nodes in the AST (most of them anyway) have a Span field which is essentially a pointer back to the raw contents in the input source file.

rustfmt uses the compiler's internal parser to process input files so that rustfmt sees the exact same AST that's produced in the early stages of rustc's compilation process. The primary motivation for the compiler to track these spans on the AST nodes is to be able to provide solid, contextual error messages and recommended fixes, whereas in rustfmt we often use them to peek back into the input source to look for comments (other than doc-style comments, comments are not explicitly represented in the AST).

The original input files and their contents are maintained in a parsing data structure called the SourceMap, and each individual span has a low and high boundary that the sourcemap uses to provide the exact original source contents associated with that AST node.