"left behind trailing whitespace" internal error if second consecutive line comment starts with certain characters

[dtolnay]

Rustfmt fails to format the following file.

fn main() {
    let x = 0; // X
    //.Y
}

• The failure is sensitive to the contents of the second comment. For example // Y or //Y does not hit this bug. //.Y or //~Y or //^Y do.
• The failure does not appear to be sensitive to the contents of the first comment, just that there is a comment.
• The failure is not sensitive to the second comment being the last thing in a block; it occurs even if there are more statements inside the block after the second comment. I left that out above because it is unnecessary for a minimal repro.

Repro against current master (3de1a09):

$ echo -e 'fn main() {\n    let x = 0; // X\n    //.Y\n}' | cargo run --bin rustfmt -- --edition=2021

fn main() {
    let x = 0; // X
    
    //.Y
}
error[internal]: left behind trailing whitespace
 --> <stdin>:3:3:1
  |
3 |     
  | ^^^^
  |

warning: rustfmt has failed to format. See previous 1 errors.

I looked through all the open issues matching a "left behind trailing whitespace" search in this repo (of which there are many) and all the closed ones that mention "comment" in the title, and I did not find one that matches this situation. They mostly seemed to involve comments placed in weird places inside an expression or statement, like let x =⏎// comment⏎value; or map(|_|⏎//comment⏎value). In contrast, this issue involves comments in a location where a conscientious person might reasonably put a comment (particularly if there is more stuff in the block after the second comment, as mentioned above). In fact the compiletest_rs crate requires such comments (//~^ ERROR) which is how Miri's test suite ran into this bug.

[ytmimi]

Thanks for the report!

I quickly took a look at this. At least in the case that you've described above it looks like we need version=Two to be set. Setting version=One produces different formatting. Using rustfmt 1.5.0-nightly (3de1a095 2022-06-17) with version=One this is what I'm getting for your input snippet:

fn main() {
    let x = 0; // X
               //.Y
}

At least in the case where the comment is the last item in a block we hit the following code path:

rustfmt/src/visitor.rs

Lines 311 to 325 in 3de1a09

	Some(offset) => {
	let first_line = &sub_slice[..offset];
	self.push_str(first_line);
	self.push_str(&self.block_indent.to_string_with_newline(config));
	// put the other lines below it, shaping it as needed
	let other_lines = &sub_slice[offset + 1..];
	let comment_str =
	rewrite_comment(other_lines, false, comment_shape, config);
	match comment_str {
	Some(ref s) => self.push_str(s),
	None => self.push_str(other_lines),
	}
	}
	}

I think the issue here is that we push an indented newline on Line 315 with the intention that the next thing we push will be the comment, but rewrite_comment is returning Some("\n //.Y") (there are 4 spaces that aren't being rendered so well), so that explains the extra whitespace error we're seeing here. Here's the code path we hit in rewrite_comment:

rustfmt/src/comment.rs

Lines 385 to 408 in 3de1a09

	} else {
	identify_comment(
	rest.trim_start(),
	block_style,
	shape,
	config,
	is_doc_comment,
	)
	.map(|rest_str| {
	format!(
	"{}\n{}{}{}",
	rewritten_first_group,
	// insert back the blank line
	if has_bare_lines && style.is_line_comment() {
	"\n"
	} else {
	""
	},
	shape.indent.to_string(config),
	rest_str
	)
	})
	}
	}

Still need to do some digging into the case where the comment isn't the last statement in the block, but I'd assume that something similar is happening. Also a little confused that the content of the 2nd comment would influence this, but I'll see if I can figure that out as well.

[ytmimi]

Okay after some more digging its seems like the //.Y and the //~Y cases are being treated as CommentStyle::Custom since that's what comments::comment_style is returning.

rustfmt/src/comment.rs

Lines 41 to 50 in 3de1a09

	#[derive(Copy, Clone, PartialEq, Eq)]
	pub(crate) enum CommentStyle<'a> {
	DoubleSlash,
	TripleSlash,
	Doc,
	SingleBullet,
	DoubleBullet,
	Exclamation,
	Custom(&'a str),
	}

Placing a space between the leading // and the ~ or . helps rustfmt correctly classify these comments as CommentStyle::DoubleSlash. @dtolnay would adding the leading space be an acceptable workaround for now?

@calebcartwright I'm wondering if you recall what CommentStyle::Custom is used for. I'm guessing that it might be used to implement itemized blocks but I'm not totally sure where it gets used. For what it's worth, I did some testing and hard coded the comment_style to CommentStyle::DoubleSlash and that seems to have resolved the issue we're seeing here.

I think I have a fix for this, but I wanted to get some more context on CommentStyle::Custom to make sure what I'm thinking makes sense.

[dtolnay]

@dtolnay would adding the leading space be an acceptable workaround for now?

No. compiletest_rs requires no space. See https://rustc-dev-guide.rust-lang.org/tests/ui.html#error-annotations.

[ytmimi]

Got it, just wanted to see. It looks like the CommentStyle::Custom variant was added in #1607 to handle cases with multiple forward slashes