Rollup merge of rust-lang#89340 - FabianWolff:issue-89173, r=petroche…

…nkov

Improve error message for `printf`-style format strings

Fixes rust-lang#89173. The following is actually supported today:
```rust
fn main() {
    let num = 5;
    let width = 20;
    print!("%*2$x", num, width);
}
```
```
error: multiple unused formatting arguments
 --> src/main.rs:4:21
  |
4 |     print!("%*2$x", num, width);
  |            -------  ^^^  ^^^^^ argument never used
  |            ||       |
  |            ||       argument never used
  |            |help: format specifiers use curly braces: `{:1$x}`
  |            multiple missing formatting specifiers
  |
  = note: printf formatting not supported; see the documentation for `std::fmt`
```
However, as noted in rust-lang#89173, something like
```rust
    print!("%0*x", width, num);
```
does not give a helpful suggestion. I think this is partly intended, because there actually _is_ no Rust equivalent to this; you always have to use a positional or named argument to specify the width (instead of just using the "next" argument, as `printf` or even `.*` as a precision specifier in Rust would). Therefore, I have added a note:
```
[...]
note: format specifiers use curly braces, and you have to use a positional or named parameter for the width
 --> t2.rs:4:13
  |
4 |     print!("%0*x", width, num);
  |             ^^^^
  = note: printf formatting not supported; see the documentation for `std::fmt`
```
This is not perfect, but it should at least point the user in the right direction, instead of issuing no explanation at all.

cc `@lcnr`

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
+ ));
+ }
  }
  }
 

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

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]

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}")]

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
+}

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
+