Try to solve issue3456.

[xiongmao86]

Close #3456.
Close #3860.

[xiongmao86]

I read code and find this snippet relevant:

rustfmt/src/expr.rs

Lines 1251 to 1277 in cc97eaf

	if !context.config.format_strings() {
	if string_lit
	.lines()
	.dropping_back(1)
	.all(|line| line.ends_with('\\'))
	{
	let new_indent = shape.visual_indent(1).indent;
	let indented_string_lit = String::from(
	string_lit
	.lines()
	.map(|line| {
	format!(
	"{}{}",
	new_indent.to_string(context.config),
	line.trim_start()
	)
	})
	.collect::<Vec<_>>()
	.join("\n")
	.trim_start(),
	);
	return if context.config.version() == Version::Two {
	Some(indented_string_lit)
	} else {
	wrap_str(indented_string_lit, context.config.max_width(), shape)
	};
	} else {

This branch of code indents strings even when config.format_strings() is false.

I git log and find #1724 is the pull request introduce this condition branch, and #1661 is the issues that #1724 solve. Unfortunately, the code reference is not working, so I can't see why that change is make. What can I do? Any suggestion?

[xiongmao86]

Oh, multi-line string literal end with \, and '\\' is character \ in a escape form.

[scampi]

@xiongmao86 exactly. It checks that every line ends with \ and if they do, it aligns them. In effect, it doesn't change the string's layout (which is good), but I don't know why it was added based on the issue you shared...
@topecongiro do you remember the reason for this ?

[xiongmao86]

There are 4 tests failed in current code. 1 self test, and 3 system tests.

The self test indent the second line of a string lit that was re-indented by the code before change. So the second line changed by some function before the rewrite_string_lit call was changed back by the origin code.

The system tests pertain to the argument processing of function and macro invocation. The code possibly rely on this changing snippet to make sure the invocation never pass the line limit.

[xiongmao86]

Identify snippet that make system tests to failed, but don't know how wrapping string lit would make that happen:

rustfmt/src/overflow.rs

Lines 470 to 484 in cc97eaf

	match self.last_item() {
	Some(OverflowableItem::Expr(expr))
	if !combine_arg_with_callee && is_method_call(expr) =>
	{
	self.context.force_one_line_chain.replace(true);
	}
	Some(OverflowableItem::MacroArg(MacroArg::Expr(expr)))
	if !combine_arg_with_callee
	&& is_method_call(expr)
	&& self.context.config.version() == Version::Two =>
	{
	self.context.force_one_line_chain.replace(true);
	}
	_ => (),
	}

[xiongmao86]

It's so hard to trace. What mistake did I make?

[scampi]

In the part of the code you changed, there was a gated change. Keeping it resolve the issues:

--- a/src/expr.rs
+++ b/src/expr.rs
@@ -1250,7 +1250,11 @@ fn rewrite_string_lit(context: &RewriteContext<'_>, span: Span, shape: Shape) ->
     let string_lit = context.snippet(span);

     if !context.config.format_strings() {
-        return valid_str(string_lit.to_owned(), context.config.max_width(), shape);
+        return if context.config.version() == Version::Two {
+            Some(string_lit.to_owned())
+        } else {
+            valid_str(string_lit.to_owned(), context.config.max_width(), shape)
+        };
     }

However, since your fix modifies a the formatting of a default option, you need to gate what you did as well. See https://github.com/rust-lang/rustfmt/blob/master/Contributing.md#version-gate-formatting-changes for extra infos.

[xiongmao86]

@scampi, I think I have tried that option before, and it came up with new issues. I still don't know why changing in this way and what cause changing the layout of the string. I don't know how to narrow down to the location of relevant code.

[xiongmao86]

The lines that are ended with '\' except the last line will be processed and return early, otherwise the strings will be processed by rewrite_string, which indent the next line.

When format_strings is false, the next line is indented. Just opposite to what I thought.

[scampi]

What issues did you get with the proposed #3556 (comment) ?
One thing that needs to be fixed for sure is the versioning of the formatting, gated in https://github.com/rust-lang/rustfmt/pull/3556/files#diff-180bb3ad456cc10517d878fb7795341cL1272 but which got removed in this PR. That gating is necessary for the stability guarantee.

[xiongmao86]

Well, I try your suggestion and find the second line of the multiline string returned at the version two condition is indented with 4 more space, I was thinking that additional indent is because that second line was process by rewrite_string, but after experiment I realize that the string is return early, and hence is not process by the rewrite_string function as opposite to my origin assumption.

I can not find out since the string is return early as what it is and not changed by the rewrite_string, what cause the second line to indent with 4 extra space?

[xiongmao86]

Formatting in cargo-fmt/main.rs is fixed in recent version.

[xiongmao86]

@scampi, I take a look at the log of stdsimd build, and had some doubts. is this line in this snippet in right indentation? Line

                           (left: `{:?}`, right: `{:?}`, expect diff: `{:?}`, real diff: `{:?}`)",

In:

macro_rules! assert_approx_eq {
    ($a:expr, $b:expr, $eps:expr) => ({
        let (a, b) = (&$a, &$b);
        assert!((*a - *b).abs() < $eps,
                "assertion failed: `(left !== right)` \
                           (left: `{:?}`, right: `{:?}`, expect diff: `{:?}`, real diff: `{:?}`)",
                 *a, *b, $eps, (*a - *b).abs());
    })
}

---

Update: looking at the wrong crate.

[xiongmao86]

Oh, that failed test probably ended up like the old self test in cargo-fmt/main.rs, may be something wrong in the macro rewriting.

[xiongmao86]

Currently when processing the macro definition, the macro branch block is wrap into a main function by pretending function declaration and brace and by appending a close brace before using format_snippet() to format the that main function. After that the formatted function is unwrapped, and the content of the block is chopped back one unit of indention(determine by config).

I found the second line is not affected when I add more "block_indent()` to the block shape. So I think the problem could probably be missing to chop the second line of the multi-line string for one unit of indention. I will try to narrow down in the following days.

[xiongmao86]

Test passed. @scampi can you review my commit?

[xiongmao86]

If @scampi is not available, are there anyone have time to review my pr?

[topecongiro]

@xiongmao86 My apologies for the late review, and thank you for the multiple updates!

The code looks good to me.

[topecongiro]

Nit: if the line is not used, then we can remove it from the parameters.