Auto merge of #52394 - estebank:println, r=oli-obk

Improve suggestion for missing fmt str in println

Avoid using `concat!(fmt, "\n")` to improve the diagnostics being
emitted when the first `println!()` argument isn't a formatting string
literal.

Fix #52347.

src/libfmt_macros/lib.rs

@@ -150,12 +150,17 @@ pub struct Parser<'a> {
     pub errors: Vec<ParseError>,
     /// Current position of implicit positional argument pointer
     curarg: usize,
+    /// `Some(raw count)` when the string is "raw", used to position spans correctly
+    style: Option<usize>,
+    /// How many newlines have been seen in the string so far, to adjust the error spans
+    seen_newlines: usize,
 }
 
 impl<'a> Iterator for Parser<'a> {
     type Item = Piece<'a>;
 
     fn next(&mut self) -> Option<Piece<'a>> {
+        let raw = self.style.map(|raw| raw + self.seen_newlines).unwrap_or(0);
         if let Some(&(pos, c)) = self.cur.peek() {
             match c {
                 '{' => {
@@ -170,20 +175,24 @@ impl<'a> Iterator for Parser<'a> {
                 }
                 '}' => {
                     self.cur.next();
-                    let pos = pos + 1;
                     if self.consume('}') {
-                        Some(String(self.string(pos)))
+                        Some(String(self.string(pos + 1)))
                     } else {
+                        let err_pos = pos + raw + 1;
                         self.err_with_note(
                             "unmatched `}` found",
                             "unmatched `}`",
                             "if you intended to print `}`, you can escape it using `}}`",
-                            pos,
-                            pos,
+                            err_pos,
+                            err_pos,
                         );
                         None
                     }
                 }
+                '\n' => {
+                    self.seen_newlines += 1;
+                    Some(String(self.string(pos)))
+                }
                 _ => Some(String(self.string(pos))),
             }
         } else {
@@ -194,12 +203,14 @@ impl<'a> Iterator for Parser<'a> {
 
 impl<'a> Parser<'a> {
     /// Creates a new parser for the given format string
-    pub fn new(s: &'a str) -> Parser<'a> {
+    pub fn new(s: &'a str, style: Option<usize>) -> Parser<'a> {
         Parser {
             input: s,
             cur: s.char_indices().peekable(),
             errors: vec![],
             curarg: 0,
+            style,
+            seen_newlines: 0,
         }
     }
 
@@ -262,24 +273,32 @@ impl<'a> Parser<'a> {
     /// found, an error is emitted.
     fn must_consume(&mut self, c: char) {
         self.ws();
+        let raw = self.style.unwrap_or(0);
+
+        let padding = raw + self.seen_newlines;
         if let Some(&(pos, maybe)) = self.cur.peek() {
             if c == maybe {
                 self.cur.next();
             } else {
+                let pos = pos + padding + 1;
                 self.err(format!("expected `{:?}`, found `{:?}`", c, maybe),
                          format!("expected `{}`", c),
-                         pos + 1,
-                         pos + 1);
+                         pos,
+                         pos);
             }
         } else {
             let msg = format!("expected `{:?}` but string was terminated", c);
-            let pos = self.input.len() + 1; // point at closing `"`
+            // point at closing `"`, unless the last char is `\n` to account for `println`
+            let pos = match self.input.chars().last() {
+                Some('\n') => self.input.len(),
+                _ => self.input.len() + 1,
+            };
             if c == '}' {
                 self.err_with_note(msg,
                                    format!("expected `{:?}`", c),
                                    "if you intended to print `{`, you can escape it using `{{`",
-                                   pos,
-                                   pos);
+                                   pos + padding,
+                                   pos + padding);
             } else {
                 self.err(msg, format!("expected `{:?}`", c), pos, pos);
             }
@@ -536,7 +555,7 @@ mod tests {
     use super::*;
 
     fn same(fmt: &'static str, p: &[Piece<'static>]) {
-        let parser = Parser::new(fmt);
+        let parser = Parser::new(fmt, None);
         assert!(parser.collect::<Vec<Piece<'static>>>() == p);
     }
 
@@ -552,7 +571,7 @@ mod tests {
     }
 
     fn musterr(s: &str) {
-        let mut p = Parser::new(s);
+        let mut p = Parser::new(s, None);
         p.next();
         assert!(!p.errors.is_empty());
     }

src/librustc/traits/on_unimplemented.rs

@@ -242,7 +242,7 @@ impl<'a, 'gcx, 'tcx> OnUnimplementedFormatString {
     {
         let name = tcx.item_name(trait_def_id);
         let generics = tcx.generics_of(trait_def_id);
-        let parser = Parser::new(&self.0);
+        let parser = Parser::new(&self.0, None);
         let mut result = Ok(());
         for token in parser {
             match token {
@@ -298,7 +298,7 @@ impl<'a, 'gcx, 'tcx> OnUnimplementedFormatString {
             Some((name, value))
         }).collect::<FxHashMap<String, String>>();
 
-        let parser = Parser::new(&self.0);
+        let parser = Parser::new(&self.0, None);
         parser.map(|p| {
             match p {
                 Piece::String(s) => s,

src/libstd/macros.rs

@@ -153,10 +153,17 @@ macro_rules! print {
 /// ```
 #[macro_export]
 #[stable(feature = "rust1", since = "1.0.0")]
+#[allow_internal_unstable]
 macro_rules! println {
     () => (print!("\n"));
-    ($fmt:expr) => (print!(concat!($fmt, "\n")));
-    ($fmt:expr, $($arg:tt)*) => (print!(concat!($fmt, "\n"), $($arg)*));
+    ($($arg:tt)*) => ({
+        #[cfg(not(stage0))] {
+            ($crate::io::_print(format_args_nl!($($arg)*)));
+        }
+        #[cfg(stage0)] {
+            print!("{}\n", format_args!($($arg)*))
+        }
+    })
 }
 
 /// Macro for printing to the standard error.
@@ -210,10 +217,17 @@ macro_rules! eprint {
 /// ```
 #[macro_export]
 #[stable(feature = "eprint", since = "1.19.0")]
+#[allow_internal_unstable]
 macro_rules! eprintln {
     () => (eprint!("\n"));
-    ($fmt:expr) => (eprint!(concat!($fmt, "\n")));
-    ($fmt:expr, $($arg:tt)*) => (eprint!(concat!($fmt, "\n"), $($arg)*));
+    ($($arg:tt)*) => ({
+        #[cfg(all(not(stage0), not(stage1)))] {
+            ($crate::io::_eprint(format_args_nl!($($arg)*)));
+        }
+        #[cfg(any(stage0, stage1))] {
+            eprint!("{}\n", format_args!($($arg)*))
+        }
+    })
 }
 
 #[macro_export]

src/libsyntax/ext/base.rs

@@ -959,29 +959,34 @@ impl<'a> ExtCtxt<'a> {
 /// Extract a string literal from the macro expanded version of `expr`,
 /// emitting `err_msg` if `expr` is not a string literal. This does not stop
 /// compilation on error, merely emits a non-fatal error and returns None.
-pub fn expr_to_spanned_string(cx: &mut ExtCtxt, expr: P<ast::Expr>, err_msg: &str)
-                              -> Option<Spanned<(Symbol, ast::StrStyle)>> {
+pub fn expr_to_spanned_string<'a>(
+    cx: &'a mut ExtCtxt,
+    expr: P<ast::Expr>,
+    err_msg: &str,
+) -> Result<Spanned<(Symbol, ast::StrStyle)>, DiagnosticBuilder<'a>> {
     // Update `expr.span`'s ctxt now in case expr is an `include!` macro invocation.
     let expr = expr.map(|mut expr| {
         expr.span = expr.span.apply_mark(cx.current_expansion.mark);
         expr
     });
 
-    // we want to be able to handle e.g. concat("foo", "bar")
+    // we want to be able to handle e.g. `concat!("foo", "bar")`
     let expr = cx.expander().fold_expr(expr);
-    match expr.node {
+    Err(match expr.node {
         ast::ExprKind::Lit(ref l) => match l.node {
-            ast::LitKind::Str(s, style) => return Some(respan(expr.span, (s, style))),
-            _ => cx.span_err(l.span, err_msg)
+            ast::LitKind::Str(s, style) => return Ok(respan(expr.span, (s, style))),
+            _ => cx.struct_span_err(l.span, err_msg)
         },
-        _ => cx.span_err(expr.span, err_msg)
-    }
-    None
+        _ => cx.struct_span_err(expr.span, err_msg)
+    })
 }
 
 pub fn expr_to_string(cx: &mut ExtCtxt, expr: P<ast::Expr>, err_msg: &str)
                       -> Option<(Symbol, ast::StrStyle)> {
-    expr_to_spanned_string(cx, expr, err_msg).map(|s| s.node)
+    expr_to_spanned_string(cx, expr, err_msg)
+        .map_err(|mut err| err.emit())
+        .ok()
+        .map(|s| s.node)
 }
 
 /// Non-fatally assert that `tts` is empty. Note that this function

src/libsyntax/ext/expand.rs

@@ -1609,6 +1609,7 @@ impl<'feat> ExpansionConfig<'feat> {
         fn enable_trace_macros = trace_macros,
         fn enable_allow_internal_unstable = allow_internal_unstable,
         fn enable_custom_derive = custom_derive,
+        fn enable_format_args_nl = format_args_nl,
         fn use_extern_macros_enabled = use_extern_macros,
         fn macros_in_extern_enabled = macros_in_extern,
         fn proc_macro_mod = proc_macro_mod,

src/libsyntax/feature_gate.rs

@@ -129,6 +129,7 @@ declare_features! (
     // rustc internal, for now:
     (active, intrinsics, "1.0.0", None, None),
     (active, lang_items, "1.0.0", None, None),
+    (active, format_args_nl, "1.29.0", None, None),
 
     (active, link_llvm_intrinsics, "1.0.0", Some(29602), None),
     (active, linkage, "1.0.0", Some(29603), None),
@@ -1320,6 +1321,9 @@ pub const EXPLAIN_LOG_SYNTAX: &'static str =
 pub const EXPLAIN_CONCAT_IDENTS: &'static str =
     "`concat_idents` is not stable enough for use and is subject to change";
 
+pub const EXPLAIN_FORMAT_ARGS_NL: &'static str =
+    "`format_args_nl` is only for internal language use and is subject to change";
+
 pub const EXPLAIN_TRACE_MACROS: &'static str =
     "`trace_macros` is not stable enough for use and is subject to change";
 pub const EXPLAIN_ALLOW_INTERNAL_UNSTABLE: &'static str =

src/libsyntax_ext/concat.rs

@@ -27,6 +27,7 @@ pub fn expand_syntax_ext(
         None => return base::DummyResult::expr(sp),
     };
     let mut accumulator = String::new();
+    let mut missing_literal = vec![];
     for e in es {
         match e.node {
             ast::ExprKind::Lit(ref lit) => match lit.node {
@@ -51,17 +52,15 @@ pub fn expand_syntax_ext(
                 }
             },
             _ => {
-                let mut err = cx.struct_span_err(e.span, "expected a literal");
-                let snippet = cx.codemap().span_to_snippet(e.span).unwrap();
-                err.span_suggestion(
-                    e.span,
-                    "you might be missing a string literal to format with",
-                    format!("\"{{}}\", {}", snippet),
-                );
-                err.emit();
+                missing_literal.push(e.span);
             }
         }
     }
+    if missing_literal.len() > 0 {
+        let mut err = cx.struct_span_err(missing_literal, "expected a literal");
+        err.note("only literals (like `\"foo\"`, `42` and `3.14`) can be passed to `concat!()`");
+        err.emit();
+    }
     let sp = sp.apply_mark(cx.current_expansion.mark);
     base::MacEager::expr(cx.expr_str(sp, Symbol::intern(&accumulator)))
 }