Rollup merge of rust-lang#66134 - estebank:unknown-formatting-trait, r=nikomatsakis

Point at formatting descriptor string when it is invalid

When a formatting string contains an invalid descriptor, point at it
instead of the argument:

```
error: unknown format trait `foo`
  --> $DIR/ifmt-bad-arg.rs:86:17
   |
LL |     println!("{:foo}", 1);
   |                 ^^^
   |
   = note: the only appropriate formatting traits are:
           - ``, which uses the `Display` trait
           - `?`, which uses the `Debug` trait
           - `e`, which uses the `LowerExp` trait
           - `E`, which uses the `UpperExp` trait
           - `o`, which uses the `Octal` trait
           - `p`, which uses the `Pointer` trait
           - `b`, which uses the `Binary` trait
           - `x`, which uses the `LowerHex` trait
           - `X`, which uses the `UpperHex` trait
```

Author: Centril

src/libfmt_macros/lib.rs

@@ -35,7 +35,7 @@ impl InnerOffset {
 
 /// A piece is a portion of the format string which represents the next part
 /// to emit. These are emitted as a stream by the `Parser` class.
-#[derive(Copy, Clone, PartialEq)]
+#[derive(Copy, Clone, Debug, PartialEq)]
 pub enum Piece<'a> {
     /// A literal string which should directly be emitted
     String(&'a str),
@@ -45,7 +45,7 @@ pub enum Piece<'a> {
 }
 
 /// Representation of an argument specification.
-#[derive(Copy, Clone, PartialEq)]
+#[derive(Copy, Clone, Debug, PartialEq)]
 pub struct Argument<'a> {
     /// Where to find this argument
     pub position: Position,
@@ -54,7 +54,7 @@ pub struct Argument<'a> {
 }
 
 /// Specification for the formatting of an argument in the format string.
-#[derive(Copy, Clone, PartialEq)]
+#[derive(Copy, Clone, Debug, PartialEq)]
 pub struct FormatSpec<'a> {
     /// Optionally specified character to fill alignment with.
     pub fill: Option<char>,
@@ -74,10 +74,12 @@ pub struct FormatSpec<'a> {
     /// this argument, this can be empty or any number of characters, although
     /// it is required to be one word.
     pub ty: &'a str,
+    /// The span of the descriptor string (for diagnostics).
+    pub ty_span: Option<InnerSpan>,
 }
 
 /// Enum describing where an argument for a format can be located.
-#[derive(Copy, Clone, PartialEq)]
+#[derive(Copy, Clone, Debug, PartialEq)]
 pub enum Position {
     /// The argument is implied to be located at an index
     ArgumentImplicitlyIs(usize),
@@ -97,7 +99,7 @@ impl Position {
 }
 
 /// Enum of alignments which are supported.
-#[derive(Copy, Clone, PartialEq)]
+#[derive(Copy, Clone, Debug, PartialEq)]
 pub enum Alignment {
     /// The value will be aligned to the left.
     AlignLeft,
@@ -111,7 +113,7 @@ pub enum Alignment {
 
 /// Various flags which can be applied to format strings. The meaning of these
 /// flags is defined by the formatters themselves.
-#[derive(Copy, Clone, PartialEq)]
+#[derive(Copy, Clone, Debug, PartialEq)]
 pub enum Flag {
     /// A `+` will be used to denote positive numbers.
     FlagSignPlus,
@@ -131,7 +133,7 @@ pub enum Flag {
 
 /// A count is used for the precision and width parameters of an integer, and
 /// can reference either an argument or a literal integer.
-#[derive(Copy, Clone, PartialEq)]
+#[derive(Copy, Clone, Debug, PartialEq)]
 pub enum Count {
     /// The count is specified explicitly.
     CountIs(usize),
@@ -475,6 +477,7 @@ impl<'a> Parser<'a> {
             width: CountImplied,
             width_span: None,
             ty: &self.input[..0],
+            ty_span: None,
         };
         if !self.consume(':') {
             return spec;
@@ -548,6 +551,7 @@ impl<'a> Parser<'a> {
                 spec.precision_span = sp;
             }
         }
+        let ty_span_start = self.cur.peek().map(|(pos, _)| *pos);
         // Optional radix followed by the actual format specifier
         if self.consume('x') {
             if self.consume('?') {
@@ -567,6 +571,12 @@ impl<'a> Parser<'a> {
             spec.ty = "?";
         } else {
             spec.ty = self.word();
+            let ty_span_end = self.cur.peek().map(|(pos, _)| *pos);
+            if !spec.ty.is_empty() {
+                spec.ty_span = ty_span_start
+                    .and_then(|s| ty_span_end.map(|e| (s, e)))
+                    .map(|(start, end)| self.to_span_index(start).to(self.to_span_index(end)));
+            }
         }
         spec
     }

src/libfmt_macros/tests.rs

@@ -2,7 +2,7 @@ use super::*;
 
 fn same(fmt: &'static str, p: &[Piece<'static>]) {
     let parser = Parser::new(fmt, None, vec![], false);
-    assert!(parser.collect::<Vec<Piece<'static>>>() == p);
+    assert_eq!(parser.collect::<Vec<Piece<'static>>>(), p);
 }
 
 fn fmtdflt() -> FormatSpec<'static> {
@@ -15,6 +15,7 @@ fn fmtdflt() -> FormatSpec<'static> {
         precision_span: None,
         width_span: None,
         ty: "",
+        ty_span: None,
     };
 }
 
@@ -82,7 +83,7 @@ fn format_position_nothing_else() {
 #[test]
 fn format_type() {
     same(
-        "{3:a}",
+        "{3:x}",
         &[NextArgument(Argument {
             position: ArgumentIs(3),
             format: FormatSpec {
@@ -93,7 +94,8 @@ fn format_type() {
                 width: CountImplied,
                 precision_span: None,
                 width_span: None,
-                ty: "a",
+                ty: "x",
+                ty_span: None,
             },
         })]);
 }
@@ -112,6 +114,7 @@ fn format_align_fill() {
                 precision_span: None,
                 width_span: None,
                 ty: "",
+                ty_span: None,
             },
         })]);
     same(
@@ -127,6 +130,7 @@ fn format_align_fill() {
                 precision_span: None,
                 width_span: None,
                 ty: "",
+                ty_span: None,
             },
         })]);
     same(
@@ -142,6 +146,7 @@ fn format_align_fill() {
                 precision_span: None,
                 width_span: None,
                 ty: "abcd",
+                ty_span: Some(InnerSpan::new(6, 10)),
             },
         })]);
 }
@@ -150,7 +155,7 @@ fn format_counts() {
     use syntax_pos::{GLOBALS, Globals, edition};
     GLOBALS.set(&Globals::new(edition::DEFAULT_EDITION), || {
     same(
-        "{:10s}",
+        "{:10x}",
         &[NextArgument(Argument {
             position: ArgumentImplicitlyIs(0),
             format: FormatSpec {
@@ -161,11 +166,12 @@ fn format_counts() {
                 width: CountIs(10),
                 precision_span: None,
                 width_span: None,
-                ty: "s",
+                ty: "x",
+                ty_span: None,
             },
         })]);
     same(
-        "{:10$.10s}",
+        "{:10$.10x}",
         &[NextArgument(Argument {
             position: ArgumentImplicitlyIs(0),
             format: FormatSpec {
@@ -176,11 +182,12 @@ fn format_counts() {
                 width: CountIsParam(10),
                 precision_span: None,
                 width_span: Some(InnerSpan::new(3, 6)),
-                ty: "s",
+                ty: "x",
+                ty_span: None,
             },
         })]);
     same(
-        "{:.*s}",
+        "{:.*x}",
         &[NextArgument(Argument {
             position: ArgumentImplicitlyIs(1),
             format: FormatSpec {
@@ -191,11 +198,12 @@ fn format_counts() {
                 width: CountImplied,
                 precision_span: Some(InnerSpan::new(3, 5)),
                 width_span: None,
-                ty: "s",
+                ty: "x",
+                ty_span: None,
             },
         })]);
     same(
-        "{:.10$s}",
+        "{:.10$x}",
         &[NextArgument(Argument {
             position: ArgumentImplicitlyIs(0),
             format: FormatSpec {
@@ -206,11 +214,12 @@ fn format_counts() {
                 width: CountImplied,
                 precision_span: Some(InnerSpan::new(3, 7)),
                 width_span: None,
-                ty: "s",
+                ty: "x",
+                ty_span: None,
             },
         })]);
     same(
-        "{:a$.b$s}",
+        "{:a$.b$?}",
         &[NextArgument(Argument {
             position: ArgumentImplicitlyIs(0),
             format: FormatSpec {
@@ -221,7 +230,8 @@ fn format_counts() {
                 width: CountIsName(Symbol::intern("a")),
                 precision_span: None,
                 width_span: None,
-                ty: "s",
+                ty: "?",
+                ty_span: None,
             },
         })]);
     });
@@ -241,6 +251,7 @@ fn format_flags() {
                 precision_span: None,
                 width_span: None,
                 ty: "",
+                ty_span: None,
             },
         })]);
     same(
@@ -256,13 +267,14 @@ fn format_flags() {
                 precision_span: None,
                 width_span: None,
                 ty: "",
+                ty_span: None,
             },
         })]);
 }
 #[test]
 fn format_mixture() {
     same(
-        "abcd {3:a} efg",
+        "abcd {3:x} efg",
         &[
             String("abcd "),
             NextArgument(Argument {
@@ -275,7 +287,8 @@ fn format_mixture() {
                     width: CountImplied,
                     precision_span: None,
                     width_span: None,
-                    ty: "a",
+                    ty: "x",
+                    ty_span: None,
                 },
             }),
             String(" efg"),

src/libsyntax_ext/format.rs

@@ -21,7 +21,7 @@ use std::collections::hash_map::Entry;
 
 #[derive(PartialEq)]
 enum ArgumentType {
-    Placeholder(String),
+    Placeholder(&'static str),
     Count,
 }
 
@@ -244,7 +244,57 @@ impl<'a, 'b> Context<'a, 'b> {
                     parse::ArgumentNamed(s) => Named(s),
                 };
 
-                let ty = Placeholder(arg.format.ty.to_string());
+                let ty = Placeholder(match &arg.format.ty[..] {
+                    "" => "Display",
+                    "?" => "Debug",
+                    "e" => "LowerExp",
+                    "E" => "UpperExp",
+                    "o" => "Octal",
+                    "p" => "Pointer",
+                    "b" => "Binary",
+                    "x" => "LowerHex",
+                    "X" => "UpperHex",
+                    _ => {
+                        let fmtsp = self.fmtsp;
+                        let sp = arg.format.ty_span.map(|sp| fmtsp.from_inner(sp));
+                        let mut err = self.ecx.struct_span_err(
+                            sp.unwrap_or(fmtsp),
+                            &format!("unknown format trait `{}`", arg.format.ty),
+                        );
+                        err.note("the only appropriate formatting traits are:\n\
+                                - ``, which uses the `Display` trait\n\
+                                - `?`, which uses the `Debug` trait\n\
+                                - `e`, which uses the `LowerExp` trait\n\
+                                - `E`, which uses the `UpperExp` trait\n\
+                                - `o`, which uses the `Octal` trait\n\
+                                - `p`, which uses the `Pointer` trait\n\
+                                - `b`, which uses the `Binary` trait\n\
+                                - `x`, which uses the `LowerHex` trait\n\
+                                - `X`, which uses the `UpperHex` trait");
+                        if let Some(sp) = sp {
+                            for (fmt, name) in &[
+                                ("", "Display"),
+                                ("?", "Debug"),
+                                ("e", "LowerExp"),
+                                ("E", "UpperExp"),
+                                ("o", "Octal"),
+                                ("p", "Pointer"),
+                                ("b", "Binary"),
+                                ("x", "LowerHex"),
+                                ("X", "UpperHex"),
+                            ] {
+                                err.tool_only_span_suggestion(
+                                    sp,
+                                    &format!("use the `{}` trait", name),
+                                    fmt.to_string(),
+                                    Applicability::MaybeIncorrect,
+                                );
+                            }
+                        }
+                        err.emit();
+                        "<invalid>"
+                    }
+                });
                 self.verify_arg_type(pos, ty);
                 self.curpiece += 1;
             }
@@ -590,6 +640,7 @@ impl<'a, 'b> Context<'a, 'b> {
                         width: parse::CountImplied,
                         width_span: None,
                         ty: arg.format.ty,
+                        ty_span: arg.format.ty_span,
                     },
                 };
 
@@ -761,37 +812,8 @@ impl<'a, 'b> Context<'a, 'b> {
         sp = ecx.with_def_site_ctxt(sp);
         let arg = ecx.expr_ident(sp, arg);
         let trait_ = match *ty {
-            Placeholder(ref tyname) => {
-                match &tyname[..] {
-                    "" => "Display",
-                    "?" => "Debug",
-                    "e" => "LowerExp",
-                    "E" => "UpperExp",
-                    "o" => "Octal",
-                    "p" => "Pointer",
-                    "b" => "Binary",
-                    "x" => "LowerHex",
-                    "X" => "UpperHex",
-                    _ => {
-                        let mut err = ecx.struct_span_err(
-                            sp,
-                            &format!("unknown format trait `{}`", *tyname),
-                        );
-                        err.note("the only appropriate formatting traits are:\n\
-                                  - ``, which uses the `Display` trait\n\
-                                  - `?`, which uses the `Debug` trait\n\
-                                  - `e`, which uses the `LowerExp` trait\n\
-                                  - `E`, which uses the `UpperExp` trait\n\
-                                  - `o`, which uses the `Octal` trait\n\
-                                  - `p`, which uses the `Pointer` trait\n\
-                                  - `b`, which uses the `Binary` trait\n\
-                                  - `x`, which uses the `LowerHex` trait\n\
-                                  - `X`, which uses the `UpperHex` trait");
-                        err.emit();
-                        return DummyResult::raw_expr(sp, true);
-                    }
-                }
-            }
+            Placeholder(trait_) if trait_ == "<invalid>" => return DummyResult::raw_expr(sp, true),
+            Placeholder(trait_) => trait_,
             Count => {
                 let path = ecx.std_path(&[sym::fmt, sym::ArgumentV1, sym::from_usize]);
                 return ecx.expr_call_global(macsp, path, vec![arg]);

src/test/ui/if/ifmt-bad-arg.stderr

@@ -257,10 +257,10 @@ LL |     println!("{} {:07$} {}", 1, 3.2, 4);
    = note: for information about formatting flags, visit https://doc.rust-lang.org/std/fmt/index.html
 
 error: unknown format trait `foo`
-  --> $DIR/ifmt-bad-arg.rs:86:24
+  --> $DIR/ifmt-bad-arg.rs:86:17
    |
 LL |     println!("{:foo}", 1);
-   |                        ^
+   |                 ^^^
    |
    = note: the only appropriate formatting traits are:
            - ``, which uses the `Display` trait