Auto merge of #52394 - estebank:println, r=<try>

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.

Author: bors

src/Cargo.lock

@@ -715,6 +715,9 @@ dependencies = [
 [[package]]
 name = "fmt_macros"
 version = "0.0.0"
+dependencies = [
+ "syntax 0.0.0",
+]
 
 [[package]]
 name = "fnv"

src/libfmt_macros/Cargo.toml

@@ -7,3 +7,6 @@ version = "0.0.0"
 name = "fmt_macros"
 path = "lib.rs"
 crate-type = ["dylib"]
+
+[dependencies]
+syntax = { path = "../libsyntax" }

src/libfmt_macros/lib.rs

@@ -28,6 +28,8 @@ pub use self::Alignment::*;
 pub use self::Flag::*;
 pub use self::Count::*;
 
+extern crate syntax;
+
 use std::str;
 use std::string;
 use std::iter;
@@ -150,12 +152,20 @@ pub struct Parser<'a> {
     pub errors: Vec<ParseError>,
     /// Current position of implicit positional argument pointer
     curarg: usize,
+    /// The style of the string (raw or not), used to position spans correctly
+    style: syntax::ast::StrStyle,
+    /// 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 = match self.style {
+            syntax::ast::StrStyle::Raw(raw) => raw as usize + self.seen_newlines,
+            _ => 0,
+        };
         if let Some(&(pos, c)) = self.cur.peek() {
             match c {
                 '{' => {
@@ -170,20 +180,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 +208,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: syntax::ast::StrStyle) -> Parser<'a> {
         Parser {
             input: s,
             cur: s.char_indices().peekable(),
             errors: vec![],
             curarg: 0,
+            style,
+            seen_newlines: 0,
         }
     }
 
@@ -262,24 +278,35 @@ impl<'a> Parser<'a> {
     /// found, an error is emitted.
     fn must_consume(&mut self, c: char) {
         self.ws();
+        let raw = match self.style {
+            syntax::ast::StrStyle::Raw(raw) => raw as usize,
+            _ => 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 +563,7 @@ mod tests {
     use super::*;
 
     fn same(fmt: &'static str, p: &[Piece<'static>]) {
-        let parser = Parser::new(fmt);
+        let parser = Parser::new(fmt, syntax::ast::StrStyle::Cooked);
         assert!(parser.collect::<Vec<Piece<'static>>>() == p);
     }
 
@@ -552,7 +579,7 @@ mod tests {
     }
 
     fn musterr(s: &str) {
-        let mut p = Parser::new(s);
+        let mut p = Parser::new(s, syntax::ast::StrStyle::Cooked);
         p.next();
         assert!(!p.errors.is_empty());
     }

src/librustc/traits/on_unimplemented.rs

@@ -15,7 +15,7 @@ use ty::{self, TyCtxt, GenericParamDefKind};
 use util::common::ErrorReported;
 use util::nodemap::FxHashMap;
 
-use syntax::ast::{MetaItem, NestedMetaItem};
+use syntax::ast::{self, MetaItem, NestedMetaItem};
 use syntax::attr;
 use syntax_pos::Span;
 use syntax_pos::symbol::LocalInternedString;
@@ -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, ast::StrStyle::Cooked);
         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, ast::StrStyle::Cooked);
         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.
@@ -399,6 +406,19 @@ pub mod builtin {
         ($fmt:expr, $($args:tt)*) => ({ /* compiler built-in */ });
     }
 
+    /// Internal version of [`format_args`].
+    ///
+    /// This macro differs from [`format_args`] in that it appends a newline to the format string
+    /// and nothing more. It is perma-unstable.
+    ///
+    /// [`format_args`]: ../std/macro.format_args.html
+    #[doc(hidden)]
+    #[unstable(feature = "println_format_args", issue="0")]
+    #[macro_export]
+    macro_rules! format_args_nl {
+        ($fmt:expr) => ({ /* compiler built-in */ });
+        ($fmt:expr, $($args:tt)*) => ({ /* compiler built-in */ });
+    }
     /// Inspect an environment variable at compile time.
     ///
     /// This macro will expand to the value of the named environment variable at

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/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)))
 }

src/libsyntax_ext/format.rs

@@ -683,7 +683,20 @@ pub fn expand_format_args<'cx>(ecx: &'cx mut ExtCtxt,
     sp = sp.apply_mark(ecx.current_expansion.mark);
     match parse_args(ecx, sp, tts) {
         Some((efmt, args, names)) => {
-            MacEager::expr(expand_preparsed_format_args(ecx, sp, efmt, args, names))
+            MacEager::expr(expand_preparsed_format_args(ecx, sp, efmt, args, names, false))
+        }
+        None => DummyResult::expr(sp),
+    }
+}
+
+pub fn expand_format_args_nl<'cx>(ecx: &'cx mut ExtCtxt,
+                                  mut sp: Span,
+                                  tts: &[tokenstream::TokenTree])
+                                  -> Box<dyn base::MacResult + 'cx> {
+    sp = sp.apply_mark(ecx.current_expansion.mark);
+    match parse_args(ecx, sp, tts) {
+        Some((efmt, args, names)) => {
+            MacEager::expr(expand_preparsed_format_args(ecx, sp, efmt, args, names, true))
         }
         None => DummyResult::expr(sp),
     }
@@ -695,18 +708,37 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt,
                                     sp: Span,
                                     efmt: P<ast::Expr>,
                                     args: Vec<P<ast::Expr>>,
-                                    names: HashMap<String, usize>)
+                                    names: HashMap<String, usize>,
+                                    append_newline: bool)
                                     -> P<ast::Expr> {
     // NOTE: this verbose way of initializing `Vec<Vec<ArgumentType>>` is because
     // `ArgumentType` does not derive `Clone`.
     let arg_types: Vec<_> = (0..args.len()).map(|_| Vec::new()).collect();
     let arg_unique_types: Vec<_> = (0..args.len()).map(|_| Vec::new()).collect();
     let mut macsp = ecx.call_site();
     macsp = macsp.apply_mark(ecx.current_expansion.mark);
-    let msg = "format argument must be a string literal.";
+    let msg = "format argument must be a string literal";
+    let fmt_sp = efmt.span;
     let fmt = match expr_to_spanned_string(ecx, efmt, msg) {
-        Some(fmt) => fmt,
-        None => return DummyResult::raw_expr(sp),
+        Ok(mut fmt) if append_newline => {
+            fmt.node.0 = Symbol::intern(&format!("{}\n", fmt.node.0));
+            fmt
+        }
+        Ok(fmt) => fmt,
+        Err(mut err) => {
+            let sugg_fmt = match args.len() {
+                0 => "{}".to_string(),
+                _ => format!("{}{{}}", "{}, ".repeat(args.len())),
+
+            };
+            err.span_suggestion(
+                fmt_sp.shrink_to_lo(),
+                "you might be missing a string literal to format with",
+                format!("\"{}\", ", sugg_fmt),
+            );
+            err.emit();
+            return DummyResult::raw_expr(sp);
+        },
     };
 
     let mut cx = Context {
@@ -731,7 +763,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt,
     };
 
     let fmt_str = &*fmt.node.0.as_str();
-    let mut parser = parse::Parser::new(fmt_str);
+    let mut parser = parse::Parser::new(fmt_str, fmt.node.1);
     let mut pieces = vec![];
 
     while let Some(mut piece) = parser.next() {
@@ -818,7 +850,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt,
                     errs.iter().map(|&(sp, _)| sp).collect::<Vec<Span>>(),
                     "multiple unused formatting arguments"
                 );
-                diag.span_label(cx.fmtsp, "multiple unused arguments in this statement");
+                diag.span_label(cx.fmtsp, "multiple missing formatting arguments");
                 diag
             }
         };
@@ -861,8 +893,10 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt,
                     }
 
                     if show_doc_note {
-                        diag.note(concat!(stringify!($kind), " formatting not supported; see \
-                                the documentation for `std::fmt`"));
+                        diag.note(concat!(
+                            stringify!($kind),
+                            " formatting not supported; see the documentation for `std::fmt`",
+                        ));
                     }
                 }};
             }