Suggest flipping reversed ?#/?x/?X in format specifiers
#131004
Conversation
…e '?#', '?x', '?X' is written in formatted string.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jieyouxu (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
|
(Note that the issue number in PR description is wrong, it's #129966 not 12996.) |
jieyouxu
changed the title
| @@ -551,6 +551,25 @@ pub(crate) enum InvalidFormatStringSuggestion { | |||
| #[primary_span] | |||
| span: Span, | |||
| }, | |||
| #[multipart_suggestion( | |||
| builtin_macros_format_reorder, | |||
| style = "tool-only", | |||
There was a problem hiding this comment.
Problem [REV-TOOL 1/2]: this should not be style = tool-only, the user will still want to see the suggestion in the absence of rustfix and such. Since it's hard to tell that #? got flipped to ?#, I suggest style = "verbose" instead so you get diagnostic like:
help: did you mean to write `x?`?
|
LL - let _ = format!("{f:?x}");
LL + let _ = format!("{f:x?}");| }, | ||
| #[multipart_suggestion( | ||
| builtin_macros_format_remove, | ||
| style = "tool-only", |
There was a problem hiding this comment.
Problem [REV-TOOL 2/2]: ditto here.
| RemoveCharacter { | ||
| #[suggestion_part(code = "")] | ||
| span: Span, | ||
| }, |
There was a problem hiding this comment.
Suggestion: This removal suggestion seems strange to me. Looking at the test, we currently suggest
help: consider removing the format specifier instead
|
LL - let _ = format!("{f:?X}");
LL + let _ = format!("{f:?}");Wouldn't we want to suggest reordering for this too? As in,
help: did you mean to write `x?`?
|
LL - let _ = format!("{f:?x}");
LL + let _ = format!("{f:x?}");| builtin_macros_format_remove_raw_ident = remove the `r#` | ||
|
|
||
| builtin_macros_format_reorder = consider reordering the format specifier instead |
There was a problem hiding this comment.
Suggestion: I think it's clearer if we outright spell out something like
did you mean to write
x??
or #? or X?. Format specifier I think refers to the whole {...} thing.
| parse::Suggestion::ReorderFormat(span) => { | ||
| let span = fmt_span.from_inner(InnerSpan::new(span.start, span.end)); | ||
| if let Ok(original_string) = ecx.source_map().span_to_snippet(span) { | ||
| let reversed_string: String = original_string.chars().rev().collect(); | ||
| e.sugg_ = Some(errors::InvalidFormatStringSuggestion::ReorderFormat { | ||
| span: span, | ||
| string: reversed_string, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Problem: we should not be generically reversing arbitrary sequence of formatting parameter fragments here when we already know what we saw. Consider just passing the span of e.g. ?X + a &'static str replacement e.g. X?: we should only be emitting the suggestion for ?X -> X?, ?x -> x?, ?# -> #? reorderings. Then, we can just replace the span with the "correct" ordered string.
| // Test checks for the help messages in the error output/ | ||
| //@ check-run-results | ||
|
|
||
| struct Foo(u8, u8); |
There was a problem hiding this comment.
Problem [REV-RUSTFIX 2/4]: if we use run-rustfix, some lints fire like dead_code and checks for missing debug impl on Foo.
#![allow(dead_code)]
#[derive(Debug)]
struct Foo(u8, u8);| fn main() { | ||
| let f = Foo(1, 2); | ||
| format!("{f:?#}"); | ||
| //~^ ERROR invalid format string: unknown format identifier '?#' |
There was a problem hiding this comment.
Nit: these UI test error annotations do not care about preceding indent whitespace, they match on a line-by-line basis. Usually we ident them in alignment with surrounding context as suitable.
| //~^ ERROR invalid format string: unknown format identifier '?#' | |
| //~^ ERROR invalid format string: unknown format identifier '?#' |
| fn main() { | ||
| let f = Foo(1, 2); | ||
| format!("{f:?#}"); | ||
| //~^ ERROR invalid format string: unknown format identifier '?#' |
There was a problem hiding this comment.
Problem [REV-RUSTFIX 3/4]: run-rustfix will reveal that format!() produces a value that is marked as #[must_use], so we need to
let _ = format!("{f:?x}");| fn main() { | ||
| let f = Foo(1, 2); | ||
| format!("{f:?#}"); | ||
| //~^ ERROR invalid format string: unknown format identifier '?#' |
There was a problem hiding this comment.
Problem [REV-RUSTFIX 4/4]: since we would like the help message to be present even in absence of tools like rustfix, we should also match help messages:
let _ = format!("{f:?x}");
//~^ ERROR invalid format string: unknown format identifier '?x'
//~| HELP did you mean to write `x?`?There was a problem hiding this comment.
Suggestion [REV-RUSTFIX OVERALL]: combining the remarks, we can write the test as something like
//! Check that we provide suggestions for wrongly ordered format parameters.
//!
//! Issue: <https://github.com/rust-lang/rust/issues/129966>.
//@ run-rustfix
#![allow(dead_code)]
#[derive(Debug)]
struct Foo(u8, u8);
fn main() {
let f = Foo(1, 2);
let _ = format!("{f:?#}");
//~^ ERROR invalid format string: unknown format identifier '?#'
//~| HELP did you mean to write `#?`?
let _ = format!("{f:?x}");
//~^ ERROR invalid format string: unknown format identifier '?x'
//~| HELP did you mean to write `x?`?
let _ = format!("{f:?X}");
//~^ ERROR invalid format string: unknown format identifier '?X'
//~| HELP did you mean to write `X?`?
}
rustbot
added
S-waiting-on-author
jieyouxu
changed the title
#?/?x/?X in format specifiers
jieyouxu
changed the title
#?/?x/?X in format specifiers?#/?x/?X in format specifiers
|
@deniskilseev |
|
@deniskilseev |
…p-message, r=estebank add suggestion for wrongly ordered format parameters Add suggestion for wrongly ordered format parameters like `?#`. Supersedes rust-lang#131004 Fix rust-lang#129966
…p-message, r=estebank add suggestion for wrongly ordered format parameters Add suggestion for wrongly ordered format parameters like `?#`. Supersedes rust-lang#131004 Fix rust-lang#129966
Rollup merge of rust-lang#134877 - DavisRayM:129966-format-string-help-message, r=estebank add suggestion for wrongly ordered format parameters Add suggestion for wrongly ordered format parameters like `?#`. Supersedes rust-lang#131004 Fix rust-lang#129966
Edits format parsing to accomodate for helpful messages in case '#?' is incorrectly ordered or '?x', '?X' is written.
Fixes #129966.