Improve error message for printf-style format strings

rust-lang · Sep 29, 2021 · 6490ed3 · 6490ed3
1 parent 1d71ba8
commit 6490ed3
Show file tree

Hide file tree

Showing 6 changed files with 105 additions and 26 deletions.
diff --git a/compiler/rustc_builtin_macros/src/format.rs b/compiler/rustc_builtin_macros/src/format.rs
@@ -1154,11 +1154,12 @@ pub fn expand_preparsed_format_args(
                     // account for `"` and account for raw strings `r#`
                     let padding = str_style.map(|i| i + 2).unwrap_or(1);
                     for sub in foreign::$kind::iter_subs(fmt_str, padding) {
-                        let trn = match sub.translate() {
-                            Some(trn) => trn,
+                        let (trn, success) = match sub.translate() {
+                            Ok(trn) => (trn, true),
+                            Err(Some(msg)) => (msg, false),
 
                             // If it has no translation, don't call it out specifically.
-                            None => continue,
+                            _ => continue,
                         };
 
                         let pos = sub.position();
@@ -1175,9 +1176,24 @@ pub fn expand_preparsed_format_args(
 
                         if let Some(inner_sp) = pos {
                             let sp = fmt_sp.from_inner(inner_sp);
-                            suggestions.push((sp, trn));
+
+                            if success {
+                                suggestions.push((sp, trn));
+                            } else {
+                                diag.span_note(
+                                    sp,
+                                    &format!("format specifiers use curly braces, and {}", trn),
+                                );
+                            }
                         } else {
-                            diag.help(&format!("`{}` should be written as `{}`", sub, trn));
+                            if success {
+                                diag.help(&format!("`{}` should be written as `{}`", sub, trn));
+                            } else {
+                                diag.note(&format!(
+                                    "`{}` should use curly braces, and {}",
+                                    sub, trn
+                                ));
+                            }
                         }
                     }
 

diff --git a/compiler/rustc_builtin_macros/src/format_foreign.rs b/compiler/rustc_builtin_macros/src/format_foreign.rs
@@ -1,4 +1,4 @@
-pub mod printf {
+pub(crate) mod printf {
     use super::strcursor::StrCursor as Cur;
     use rustc_span::InnerSpan;
 
@@ -36,10 +36,10 @@ pub mod printf {
         ///
         /// This ignores cases where the substitution does not have an exact equivalent, or where
         /// the substitution would be unnecessary.
-        pub fn translate(&self) -> Option<String> {
+        pub fn translate(&self) -> Result<String, Option<String>> {
             match *self {
                 Substitution::Format(ref fmt) => fmt.translate(),
-                Substitution::Escape => None,
+                Substitution::Escape => Err(None),
             }
         }
     }
@@ -68,9 +68,9 @@ pub mod printf {
     impl Format<'_> {
         /// Translate this directive into an equivalent Rust formatting directive.
         ///
-        /// Returns `None` in cases where the `printf` directive does not have an exact Rust
+        /// Returns `Err` in cases where the `printf` directive does not have an exact Rust
         /// equivalent, rather than guessing.
-        pub fn translate(&self) -> Option<String> {
+        pub fn translate(&self) -> Result<String, Option<String>> {
             use std::fmt::Write;
 
             let (c_alt, c_zero, c_left, c_plus) = {
@@ -84,7 +84,12 @@ pub mod printf {
                         '0' => c_zero = true,
                         '-' => c_left = true,
                         '+' => c_plus = true,
-                        _ => return None,
+                        _ => {
+                            return Err(Some(format!(
+                                "the flag `{}` is unknown or unsupported",
+                                c
+                            )));
+                        }
                     }
                 }
                 (c_alt, c_zero, c_left, c_plus)
@@ -104,7 +109,9 @@ pub mod printf {
             let width = match self.width {
                 Some(Num::Next) => {
                     // NOTE: Rust doesn't support this.
-                    return None;
+                    return Err(Some(
+                        "you have to use a positional or named parameter for the width".to_string(),
+                    ));
                 }
                 w @ Some(Num::Arg(_)) => w,
                 w @ Some(Num::Num(_)) => w,
@@ -125,13 +132,21 @@ pub mod printf {
                 "p" => (Some(self.type_), false, true),
                 "g" => (Some("e"), true, false),
                 "G" => (Some("E"), true, false),
-                _ => return None,
+                _ => {
+                    return Err(Some(format!(
+                        "the conversion specifier `{}` is unknown or unsupported",
+                        self.type_
+                    )));
+                }
             };
 
             let (fill, width, precision) = match (is_int, width, precision) {
                 (true, Some(_), Some(_)) => {
                     // Rust can't duplicate this insanity.
-                    return None;
+                    return Err(Some(
+                        "width and precision cannot both be specified for integer conversions"
+                            .to_string(),
+                    ));
                 }
                 (true, None, Some(p)) => (Some("0"), Some(p), None),
                 (true, w, None) => (fill, w, None),
@@ -169,7 +184,17 @@ pub mod printf {
             s.push('{');
 
             if let Some(arg) = self.parameter {
-                write!(s, "{}", arg.checked_sub(1)?).ok()?;
+                match write!(
+                    s,
+                    "{}",
+                    match arg.checked_sub(1) {
+                        Some(a) => a,
+                        None => return Err(None),
+                    }
+                ) {
+                    Err(_) => return Err(None),
+                    _ => {}
+                }
             }
 
             if has_options {
@@ -199,12 +224,18 @@ pub mod printf {
                 }
 
                 if let Some(width) = width {
-                    width.translate(&mut s).ok()?;
+                    match width.translate(&mut s) {
+                        Err(_) => return Err(None),
+                        _ => {}
+                    }
                 }
 
                 if let Some(precision) = precision {
                     s.push('.');
-                    precision.translate(&mut s).ok()?;
+                    match precision.translate(&mut s) {
+                        Err(_) => return Err(None),
+                        _ => {}
+                    }
                 }
 
                 if let Some(type_) = type_ {
@@ -213,7 +244,7 @@ pub mod printf {
             }
 
             s.push('}');
-            Some(s)
+            Ok(s)
         }
     }
 
@@ -623,11 +654,11 @@ pub mod shell {
             }
         }
 
-        pub fn translate(&self) -> Option<String> {
+        pub fn translate(&self) -> Result<String, Option<String>> {
             match *self {
-                Substitution::Ordinal(n, _) => Some(format!("{{{}}}", n)),
-                Substitution::Name(n, _) => Some(format!("{{{}}}", n)),
-                Substitution::Escape(_) => None,
+                Substitution::Ordinal(n, _) => Ok(format!("{{{}}}", n)),
+                Substitution::Name(n, _) => Ok(format!("{{{}}}", n)),
+                Substitution::Escape(_) => Err(None),
             }
         }
     }

diff --git a/compiler/rustc_builtin_macros/src/format_foreign/printf/tests.rs b/compiler/rustc_builtin_macros/src/format_foreign/printf/tests.rs
@@ -3,7 +3,7 @@ use super::{iter_subs, parse_next_substitution as pns, Format as F, Num as N, Su
 macro_rules! assert_eq_pnsat {
     ($lhs:expr, $rhs:expr) => {
         assert_eq!(
-            pns($lhs).and_then(|(s, _)| s.translate()),
+            pns($lhs).and_then(|(s, _)| s.translate().ok()),
             $rhs.map(<String as From<&str>>::from)
         )
     };
@@ -98,7 +98,7 @@ fn test_parse() {
 #[test]
 fn test_iter() {
     let s = "The %d'th word %% is: `%.*s` %!\n";
-    let subs: Vec<_> = iter_subs(s, 0).map(|sub| sub.translate()).collect();
+    let subs: Vec<_> = iter_subs(s, 0).map(|sub| sub.translate().ok()).collect();
     assert_eq!(
         subs.iter().map(|ms| ms.as_ref().map(|s| &s[..])).collect::<Vec<_>>(),
         vec![Some("{}"), None, Some("{:.*}"), None]

diff --git a/compiler/rustc_builtin_macros/src/format_foreign/shell/tests.rs b/compiler/rustc_builtin_macros/src/format_foreign/shell/tests.rs
@@ -3,7 +3,7 @@ use super::{parse_next_substitution as pns, Substitution as S};
 macro_rules! assert_eq_pnsat {
     ($lhs:expr, $rhs:expr) => {
         assert_eq!(
-            pns($lhs).and_then(|(f, _)| f.translate()),
+            pns($lhs).and_then(|(f, _)| f.translate().ok()),
             $rhs.map(<String as From<&str>>::from)
         )
     };
@@ -37,7 +37,7 @@ fn test_parse() {
 fn test_iter() {
     use super::iter_subs;
     let s = "The $0'th word $$ is: `$WORD` $!\n";
-    let subs: Vec<_> = iter_subs(s, 0).map(|sub| sub.translate()).collect();
+    let subs: Vec<_> = iter_subs(s, 0).map(|sub| sub.translate().ok()).collect();
     assert_eq!(
         subs.iter().map(|ms| ms.as_ref().map(|s| &s[..])).collect::<Vec<_>>(),
         vec![Some("{0}"), None, Some("{WORD}")]

diff --git a/src/test/ui/fmt/issue-89173.rs b/src/test/ui/fmt/issue-89173.rs
@@ -0,0 +1,14 @@
+// Regression test for #89173: Make sure a helpful note is issued for
+// printf-style format strings using `*` to specify the width.
+
+fn main() {
+    let num = 0x0abcde;
+    let width = 6;
+    print!("%0*x", width, num);
+    //~^ ERROR: multiple unused formatting arguments
+    //~| NOTE: multiple missing formatting specifiers
+    //~| NOTE: argument never used
+    //~| NOTE: argument never used
+    //~| NOTE: format specifiers use curly braces, and you have to use a positional or named parameter for the width
+    //~| NOTE: printf formatting not supported
+}
diff --git a/src/test/ui/fmt/issue-89173.stderr b/src/test/ui/fmt/issue-89173.stderr
@@ -0,0 +1,18 @@
+error: multiple unused formatting arguments
+  --> $DIR/issue-89173.rs:7:20
+   |
+LL |     print!("%0*x", width, num);
+   |            ------  ^^^^^  ^^^ argument never used
+   |            |       |
+   |            |       argument never used
+   |            multiple missing formatting specifiers
+   |
+note: format specifiers use curly braces, and you have to use a positional or named parameter for the width
+  --> $DIR/issue-89173.rs:7:13
+   |
+LL |     print!("%0*x", width, num);
+   |             ^^^^
+   = note: printf formatting not supported; see the documentation for `std::fmt`
+
+error: aborting due to previous error
+