Exclude block-style comments in wrap_str/filter_normal_code check

[calebcartwright]

There's a utility function, wrap_str, used in various locations throughout the rustfmt codebase which does some checks against a string of formatted code, including whether each line in the string will fit within the width constraints.

The width check is supposed to exclude comments, and does so successfully for line-style comments (e.g. // foo bar). However, there is an issue where it does not exclude block-style comments (e.g. /* foo bar */).

The code that handles sorting out comments can be found in the comments.rs file:

rustfmt/src/formatting/comment.rs

Lines 1613 to 1629 in 79c3696

	pub(crate) fn filter_normal_code(code: &str) -> String {
	let mut buffer = String::with_capacity(code.len());
	LineClasses::new(code).for_each(|(kind, line)| match kind {
	FullCodeCharKind::Normal
	| FullCodeCharKind::StartString
	| FullCodeCharKind::InString
	| FullCodeCharKind::EndString => {
	buffer.push_str(&line);
	buffer.push('\n');
	}
	_ => {}
	});
	if !code.ends_with('\n') && buffer.ends_with('\n') {
	buffer.pop();
	}
	buffer
	}

and this would need to be extended to exclude block-style comments as well. Remember that multi-line block style comments are a possibility, so that needs to be accounted for as well, though happy to accept improvements that cover single-line block style comments.

The changes should include tests, for example additional unit tests like the existing ones for filter_normal_code:

rustfmt/src/formatting/comment.rs

Lines 1984 to 1999 in 79c3696

	fn test_filter_normal_code() {
	let s = r#"
	fn main() {
	println!("hello, world");
	}
	"#;
	assert_eq!(s, filter_normal_code(s));
	let s_with_comment = r#"
	fn main() {
	// hello, world
	println!("hello, world");
	}
	"#;
	assert_eq!(s, filter_normal_code(s_with_comment));
	}
	}

Additionally, it would be beneficial to include the standard system/idempotence tests with an input file (tests/source/...) that contains snippets that result in the above code paths getting hit, such as a chain that contains a block-style comment that exceeds max_width, along with a target/output file (tests/target/...) that shows the resultant formatting.

[ChinYing-Li]

Looking at this :

rustfmt/tests/source/comment.rs

Lines 47 to 51 in 257311c

	fn chains() {
	foo.bar(|| {
	let x = 10;
	/* comment */ x })
	}

I am wondering what should be the desirable formatted result of the snippet below:

test().map(|| {
        let x = 11;
        /* Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec a diam lectus. Sed sit amet ipsum mauris. Maecenas congue
         * ligula ac quam */ x
    });

I personally think that

• since it's impossible to put the entire comment string in one line given the width constraint, the block style of the comment should not be substituted by the line style.

The following is the formatted code that I would expect:

test().map(|| {
        let x = 11;
        /* Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec a diam lectus. 
         * Sed sit amet ipsum mauris. Maecenas congue ligula ac quam */ 
        x
    });

But currently, rustfmt gives

test().map(|| {
        let x = 11;
        // Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec a diam lectus.
        // Sed sit amet ipsum mauris. Maecenas congue ligula ac quam
        x
    });

[calebcartwright]

But currently, rustfmt gives

That shouldn't be happening by default, and I'm unable to reproduce this. Are you formatting that snippet with normalize_comments set to true?

[ChinYing-Li]

That shouldn't be happening by default, and I'm unable to reproduce this. Are you formatting that snippet with normalize_comments set to true?

You are right!
I put the snippet in the idempotent test right after this test case, and the configuration for tests/source/comment.rs is

rustfmt/tests/source/comment.rs

Lines 1 to 3 in 257311c

	// rustfmt-normalize_comments: true
	// rustfmt-wrap_comments: true

In order to test wrap_str, I think I should put my snippet as an idempotent test case (PR #4705) in tests/source/comment2.rs, which only has wrap_comments set to true.

[calebcartwright]

Excellent thanks!

[ChinYing-Li]

@calebcartwright Can this issue be closed, now that #4720 has been merged?

[calebcartwright]

@calebcartwright Can this issue be closed, now that #4720 has been merged?

Thank you for the question, but no I don't want to close it until #4758 is merged