Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String literals with unescaped \ in the end of line is not formatted #4321

Closed
tesuji opened this issue Jul 11, 2020 · 11 comments
Closed

String literals with unescaped \ in the end of line is not formatted #4321

tesuji opened this issue Jul 11, 2020 · 11 comments
Labels
bug Panic, non-idempotency, invalid code, etc.

Comments

@tesuji
Copy link
Contributor

tesuji commented Jul 11, 2020

Describe the bug

The Rust reference has an exception rule about unescaped \ at the end of line
of string literals, quoted here:

when an unescaped U+005C character (\) occurs immediately before the line-break,
then the U+005C character, the line-break, and all whitespace at the beginning of
the next line are ignored.

It would be nice if rustfmt format this snippet:

pub fn foo() -> &'static str {
    "use of std::thread::current() is not \
                                          possible after the thread's local \
                                          data has been destroyed"
}

into

pub fn foo() -> &'static str {
    "use of std::thread::current() is not \
     possible after the thread's local \
     data has been destroyed"
}

Meta

@tesuji tesuji added the bug Panic, non-idempotency, invalid code, etc. label Jul 11, 2020
@ayazhafiz
Copy link
Contributor

I think the issues are different; in 2876, the whitespace is significant and it's more about formatting in the context of a call

@ayazhafiz
Copy link
Contributor

ayazhafiz commented Jul 11, 2020

You can already do this with the format_strings option, which does a little more:

pub fn foo() -> &'static str {
    "use of std::thread::current() is not \
                                          possible after the thread's local \
                                          data has been destroyed"
}

would go to

pub fn foo() -> &'static str {
    "use of std::thread::current() is not possible after the thread's local data has been \
     destroyed";
}

Would that work for your use case?

@tesuji
Copy link
Contributor Author

tesuji commented Jul 11, 2020

Thanks. Never know about this configuration.
Are there any reasons this is off by default?

@ayazhafiz
Copy link
Contributor

It just was never turned on as a stable option, there is a tracking issue here: #4036

@tesuji
Copy link
Contributor Author

tesuji commented Jul 12, 2020

I try that configuration a bit. It is useful and nice. But I would like to preserve the ending \
in each line, which mean that the configuration would not join multiline together.
For example, this diff is harder to read:

             rustflags.arg("-Zsave-analysis");
             cargo.env(
                 "RUST_SAVE_ANALYSIS_CONFIG",
-                "{\"output_file\": null,\"full_docs\": false,\
-                       \"pub_only\": true,\"reachable_only\": false,\
-                       \"distro_crate\": true,\"signatures\": false,\"borrow_data\": false}",
+                "{\"output_file\": null,\"full_docs\": false,\"pub_only\": \
+                 true,\"reachable_only\": false,\"distro_crate\": true,\"signatures\": \
+                 false,\"borrow_data\": false}",
             );
         }
         if !pass_sanity_check {
             println!("{}\n", subcommand_help);
             println!(
-                "Sorry, I couldn't figure out which subcommand you were trying to specify.\n\
-                 You may need to move some options to after the subcommand.\n"
+                "Sorry, I couldn't figure out which subcommand you were trying to specify.\nYou \
+                 may need to move some options to after the subcommand.\n"
             );
             process::exit(1);
         }
     if !output.status.success() {
         println!(
-            "\n\ncommand did not execute successfully: {:?}\n\
-             expected success, got: {}\n\n\
-             stdout ----\n{}\n\
-             stderr ----\n{}\n\n",
+            "\n\ncommand did not execute successfully: {:?}\nexpected success, got: {}\n\nstdout \
+             ----\n{}\nstderr ----\n{}\n\n",
             cmd,
             output.status,
             String::from_utf8_lossy(&output.stdout),

@tesuji
Copy link
Contributor Author

tesuji commented Jul 12, 2020

Also, is this a bug?

 #[bench]
 fn bench_contains_short_long(b: &mut Bencher) {
-    let haystack = "\
+    let haystack =
+        "\
 Lorem ipsum dolor sit amet, consectetur adipiscing elit. Suspendisse quis lorem sit amet dolor \
-ultricies condimentum. Praesent iaculis purus elit, ac malesuada quam malesuada in. Duis sed orci \
-eros. Suspendisse sit amet magna mollis, mollis nunc luctus, imperdiet mi. Integer fringilla non \
-sem ut lacinia. Fusce varius tortor a risus porttitor hendrerit. Morbi mauris dui, ultricies nec \
-tempus vel, gravida nec quam.
+         ultricies condimentum. Praesent iaculis purus elit, ac malesuada quam malesuada in. Duis \
+         sed orci eros. Suspendisse sit amet magna mollis, mollis nunc luctus, imperdiet mi. \
+         Integer fringilla non sem ut lacinia. Fusce varius tortor a risus porttitor hendrerit. \
+         Morbi mauris dui, ultricies nec tempus vel, gravida nec quam.
 

@ayazhafiz
Copy link
Contributor

But I would like to preserve the ending \
in each line, which mean that the configuration would not join multiline together.
For example, this diff is harder to read:

Yeah that makes sense. I agree the formatted version is harder to read. But I think it is unlikely that an option to only format the alignment of a string without formatting other parts of the string would be accepted. In some sense this is a custom formatting.

Also, is this a bug?

Yes, it looks like it. Can you share the entire bench function where that happened?

@tesuji
Copy link
Contributor Author

tesuji commented Jul 12, 2020

Yes, it here: https://github.com/rust-lang/rust/blob/4007d4ef26eab44bdabc2b7574d032152264d3ad/src/liballoc/benches/str.rs#L131

I was trying to use that formatting configuration on Rust repo. But I'm afraid it couldn't be accepted
as the result is not nicer to read.

@ayazhafiz
Copy link
Contributor

ayazhafiz commented Jul 12, 2020 via email

@ayazhafiz
Copy link
Contributor

@lzutao is there any further action we can take here, aside #4323? If not, is it worth closing this issue?

@tesuji
Copy link
Contributor Author

tesuji commented Jul 21, 2020

I still don't like that strings are merged and formatted. But that's probably a separate issue/RFC for fmts.
But I don't have strong desire for that.
Thanks for your helpful feedback. Closing!

@tesuji tesuji closed this as completed Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Panic, non-idempotency, invalid code, etc.
Projects
None yet
Development

No branches or pull requests

2 participants