diff options
| author | Manish Goregaokar <manishsmail@gmail.com> | 2021-10-01 09:18:19 -0700 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2021-10-01 09:18:19 -0700 |
| commit | d388428aaad3915c65106a1aae2d09effcdbc034 (patch) | |
| tree | 3288270fef85c00c7d03864229fc69f4a676ec4d | |
| parent | 2f67063fbe7ab6bc0793046ecb2c0626531ad451 (diff) | |
| parent | 6490ed30e1025fbb85b3a2dd4849a249c072fb30 (diff) | |
| download | rust-d388428aaad3915c65106a1aae2d09effcdbc034.tar.gz rust-d388428aaad3915c65106a1aae2d09effcdbc034.zip | |
Rollup merge of #89340 - FabianWolff:issue-89173, r=petrochenkov
Improve error message for `printf`-style format strings
Fixes #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 #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```
| -rw-r--r-- | compiler/rustc_builtin_macros/src/format.rs | 26 | ||||
| -rw-r--r-- | compiler/rustc_builtin_macros/src/format_foreign.rs | 65 | ||||
| -rw-r--r-- | compiler/rustc_builtin_macros/src/format_foreign/printf/tests.rs | 4 | ||||
| -rw-r--r-- | compiler/rustc_builtin_macros/src/format_foreign/shell/tests.rs | 4 | ||||
| -rw-r--r-- | src/test/ui/fmt/issue-89173.rs | 14 | ||||
| -rw-r--r-- | src/test/ui/fmt/issue-89173.stderr | 18 |
6 files changed, 105 insertions, 26 deletions
diff --git a/compiler/rustc_builtin_macros/src/format.rs b/compiler/rustc_builtin_macros/src/format.rs index 1c9c6834c10..f0056cb7976 100644 --- 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 index 0cc520e5bd1..bfddd7073ff 100644 --- 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 index 33c54c9cee0..1336aab7316 100644 --- 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 index ed8fe81dfcd..f5f82732f20 100644 --- 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 new file mode 100644 index 00000000000..96277d4d0d9 --- /dev/null +++ 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 new file mode 100644 index 00000000000..7b21e0a4fc8 --- /dev/null +++ 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 + |
