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

Multiline string attr display #81438

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jan 27, 2021

Fixes #81482.

As you see here (or on the screenshot below):

Screenshot from 2021-01-27 14-23-49

The backslash is kept in the string literal in the end. This fix removes it, however I wonder if there isn't a function to do that already in the compiler (couldn't find anything so...). Maybe you might have some information about it @oli-obk ?

cc @CraftSpider
r? @camelid

@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 27, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 27, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Jan 27, 2021

We definitely have code for this somewhere already. I would actually have expected the parser to handle this for us so we don't ever see this.

@camelid
Copy link
Member

camelid commented Jan 28, 2021

I think the issue is that we get the attributes using:

        .filter_map(|attr| {
            if ALLOWED_ATTRIBUTES.contains(&attr.name_or_empty()) {
                Some(pprust::attribute_to_string(&attr))
            } else {
                None
            }
        })

which seems very hacky.

@camelid
Copy link
Member

camelid commented Jan 28, 2021

I think we want to use something like Attribute::value_str instead of the pretty-printer. Can you try that?

@rustbot label: -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 28, 2021
@GuillaumeGomez
Copy link
Member Author

The problem here is that we print not only the value but the whole attr. So here, we print #[must_use = "blablabla"], no just "blablabla".

@oli-obk
Copy link
Contributor

oli-obk commented Jan 28, 2021

maybe we can fix the pretty printer to use value_str?

@GuillaumeGomez
Copy link
Member Author

Sounds like a good idea! I'll go with this then.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jan 28, 2021

It seems that the pretty printer cannot really use value_str as we go through multiple steps and in the end just finish with a ast::Literal. We can still make it remove the backslash though (diff of what I did):

diff --git a/compiler/rustc_ast/src/util/literal.rs b/compiler/rustc_ast/src/util/literal.rs
index 2124f1efb99..cf0f6ef3b2b 100644
--- a/compiler/rustc_ast/src/util/literal.rs
+++ b/compiler/rustc_ast/src/util/literal.rs
@@ -23,7 +23,7 @@ pub enum LitError {
 
 impl LitKind {
     /// Converts literal token into a semantic literal.
-    fn from_lit_token(lit: token::Lit) -> Result<LitKind, LitError> {
+    pub fn from_lit_token(lit: token::Lit) -> Result<LitKind, LitError> {
         let token::Lit { kind, symbol, suffix } = lit;
         if suffix.is_some() && !kind.may_have_suffix() {
             return Err(LitError::InvalidSuffix);
diff --git a/compiler/rustc_ast_pretty/src/pprust/state.rs b/compiler/rustc_ast_pretty/src/pprust/state.rs
index 2c8caf68f00..b79398035c6 100644
--- a/compiler/rustc_ast_pretty/src/pprust/state.rs
+++ b/compiler/rustc_ast_pretty/src/pprust/state.rs
@@ -191,13 +191,49 @@ pub fn literal_to_string(lit: token::Lit) -> String {
     let mut out = match kind {
         token::Byte => format!("b'{}'", symbol),
         token::Char => format!("'{}'", symbol),
-        token::Str => format!("\"{}\"", symbol),
+        token::Str => {
+            if let Ok(ast::LitKind::Str(symbol, _)) = ast::LitKind::from_lit_token(lit) {
+                format!("\"{}\"", symbol)
+            } else {
+                format!("\"{}\"", symbol)
+            }
+        }
         token::StrRaw(n) => {
-            format!("r{delim}\"{string}\"{delim}", delim = "#".repeat(n as usize), string = symbol)
+            if let Ok(ast::LitKind::Str(symbol, _)) = ast::LitKind::from_lit_token(lit) {
+                format!(
+                    "r{delim}\"{string}\"{delim}",
+                    delim = "#".repeat(n as usize),
+                    string = symbol
+                )
+            } else {
+                format!(
+                    "r{delim}\"{string}\"{delim}",
+                    delim = "#".repeat(n as usize),
+                    string = symbol
+                )
+            }
+        }
+        token::ByteStr => {
+            if let Ok(l) = ast::LitKind::from_lit_token(lit) {
+                format!("b\"{}\"", l.to_lit_token().symbol)
+            } else {
+                format!("b\"{}\"", symbol)
+            }
         }
-        token::ByteStr => format!("b\"{}\"", symbol),
         token::ByteStrRaw(n) => {
-            format!("br{delim}\"{string}\"{delim}", delim = "#".repeat(n as usize), string = symbol)
+            if let Ok(l) = ast::LitKind::from_lit_token(lit) {
+                format!(
+                    "br{delim}\"{string}\"{delim}",
+                    delim = "#".repeat(n as usize),
+                    string = l.to_lit_token().symbol
+                )
+            } else {
+                format!(
+                    "br{delim}\"{string}\"{delim}",
+                    delim = "#".repeat(n as usize),
+                    string = symbol
+                )
+            }
         }
         token::Integer | token::Float | token::Bool | token::Err => symbol.to_string(),
     };

So in the meantime, I opened a discussion about it in zulip here.

One extra thing to be added here would be a test to ensure that the sources display still has the original content too.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 14, 2021
@GuillaumeGomez GuillaumeGomez added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 15, 2021
@camelid camelid added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Mar 12, 2021
@bors
Copy link
Contributor

bors commented Apr 5, 2021

☔ The latest upstream changes (presumably #83880) made this pull request unmergeable. Please resolve the merge conflicts.

@camelid camelid added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels May 28, 2021
@jsha
Copy link
Contributor

jsha commented Nov 28, 2021

This is not needed so badly anymore since we removed must_use from ALLOWED_ATTRIBUTES. Do you still want to get this done?

@camelid
Copy link
Member

camelid commented Dec 10, 2021

I'm going to close this for now since this PR isn't actionable.

@camelid camelid closed this Dec 10, 2021
@GuillaumeGomez GuillaumeGomez deleted the multiline-string-attr-display branch August 19, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc: attribute string literals render with backslashes
8 participants