Skip to content

Suggest flipping reversed ?#/?x/?X in format specifiers #131004

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions compiler/rustc_builtin_macros/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,12 @@ builtin_macros_format_redundant_args = redundant {$n ->
}
.suggestion = this can be removed

builtin_macros_format_remove = consider removing the format specifier instead

builtin_macros_format_remove_raw_ident = remove the `r#`

builtin_macros_format_reorder = consider reordering the format specifier instead
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


builtin_macros_format_requires_string = requires at least a format string argument

builtin_macros_format_string_invalid = invalid format string: {$desc}
Expand Down
19 changes: 19 additions & 0 deletions compiler/rustc_builtin_macros/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,25 @@ pub(crate) enum InvalidFormatStringSuggestion {
#[primary_span]
span: Span,
},
#[multipart_suggestion(
builtin_macros_format_reorder,
style = "tool-only",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?}");

applicability = "machine-applicable"
)]
ReorderFormat {
#[suggestion_part(code = "{string}")]
span: Span,
string: String,
},
#[multipart_suggestion(
builtin_macros_format_remove,
style = "tool-only",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem [REV-TOOL 2/2]: ditto here.

applicability = "machine-applicable"
)]
RemoveCharacter {
#[suggestion_part(code = "")]
span: Span,
},
Comment on lines +569 to +572
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?}");

}

#[derive(Diagnostic)]
Expand Down
14 changes: 14 additions & 0 deletions compiler/rustc_builtin_macros/src/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,20 @@ fn make_format_args(
e.sugg_ = Some(errors::InvalidFormatStringSuggestion::RemoveRawIdent { span })
}
}
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,
});
}
}
Comment on lines +305 to +314
Copy link
Member

@jieyouxu jieyouxu Sep 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

parse::Suggestion::RemoveCharacter(span) => {
let span = fmt_span.from_inner(InnerSpan::new(span.start, span.end));
e.sugg_ = Some(errors::InvalidFormatStringSuggestion::RemoveCharacter { span })
}
}
let guar = ecx.dcx().emit_err(e);
return ExpandResult::Ready(Err(guar));
Expand Down
38 changes: 38 additions & 0 deletions compiler/rustc_parse_format/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ pub enum Suggestion {
/// Remove `r#` from identifier:
/// `format!("{r#foo}")` -> `format!("{foo}")`
RemoveRawIdent(InnerSpan),
// Reorder `?#` to `#?`.
ReorderFormat(InnerSpan),
// Remove possibly unnecessary character.
RemoveCharacter(InnerSpan),
}

/// The parser structure for interpreting the input format string. This is
Expand Down Expand Up @@ -731,6 +735,13 @@ impl<'a> Parser<'a> {
}
} else if self.consume('?') {
spec.ty = "?";
if let Some(&(_, char)) = self.cur.peek() {
match char {
'#' => self.suggest_format_reorder(char),
'x' | 'X' => self.suggest_format_remove(char),
Comment on lines +740 to +741
Copy link
Member

@jieyouxu jieyouxu Sep 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: we can pull out a maybe_suggest_format_parameter_reorder to make reorder suggestions for all three cases ?#, ?X and ?x. In it, we do something like

fn maybe_suggest_format_parameter_reorder(&mut self, c: char) {
    let Some(&(_, c)) = self.cur.peek() else {
        return;
    };
    let Some(pos) = self.consume_pos(c) else {
        return;
    };

    // We've seen `?`, but the user wrote `#`/`X`/`x` immediately after.
    let span = self.span(pos - 1, pos + 1);
    let replacement = match c {
        '#' => "#?",
        'x' => "x?",
        'X' => "X?",
        _ => return,
    };
    let sugg = Suggestion::ReorderFormat(span, replacement);

    self.errors.insert(0, ParseError {
        description: format!("unknown format identifier '?{}'", c),
        note: Some("if you intended to print `{`, you can escape it using `{{`".to_owned()),
        label: format!("help: the format parameter should be written  as `{replacement}`"),
        span: pos,
        secondary_label: None,
        suggestion: sugg,
    });
}

_ => {}
}
}
} else {
spec.ty = self.word();
if !spec.ty.is_empty() {
Expand Down Expand Up @@ -895,6 +906,33 @@ impl<'a> Parser<'a> {
}
}

fn suggest_format_reorder(&mut self, reorder: char) {
if let Some(pos) = self.consume_pos(reorder) {
let pos = self.span(pos - 1, pos + 1);
self.errors.insert(0, ParseError {
description: format!("unknown format identifier '?{}'", reorder),
note: Some("if you intended to print `{`, you can escape it using `{{`".to_owned()),
label: format!("help: the format parameter should be written `{}?`", reorder),
span: pos,
secondary_label: None,
suggestion: Suggestion::ReorderFormat(pos),
});
}
}
fn suggest_format_remove(&mut self, remove: char) {
if let Some(pos) = self.consume_pos(remove) {
let pos = self.span(pos, pos + 1);
self.errors.insert(0, ParseError {
description: format!("unknown format identifier '?{}'", remove),
note: Some("if you intended to print `{`, you can escape it using `{{`".to_owned()),
label: format!("help: consider removing `{}`", remove),
span: pos,
secondary_label: None,
suggestion: Suggestion::RemoveCharacter(pos),
});
}
}

fn suggest_positional_arg_instead_of_captured_arg(&mut self, arg: Argument<'a>) {
if let Some(end) = self.consume_pos('.') {
let byte_pos = self.to_span_index(end);
Expand Down
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?`?
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Test checks for the help messages in the error output/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

Suggested change
// Test checks for the help messages in the error output/
//! Check that we provide suggestions for some wrongly ordered format parameters.
//!
//! Issue: <https://github.com/rust-lang/rust/issues/129966>.

//@ check-run-results
Copy link
Member

@jieyouxu jieyouxu Sep 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem [REV-RUSTFIX 1/4]: we want to check the suggestion is machine-applicable, so we need to //@ run-rustfix which checks rustfix-applied suggestions, instead of check-run-results which checks output of test binary. See the file-level comment tagged with REV-RUSTFIX OVERALL about what I think the test should look like.


struct Foo(u8, u8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 '?#'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
//~^ ERROR invalid format string: unknown format identifier '?#'
//~^ ERROR invalid format string: unknown format identifier '?#'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?`?

format!("{f:?x}");
//~^ ERROR invalid format string: unknown format identifier '?x'
format!("{f:?X}");
//~^ ERROR invalid format string: unknown format identifier '?X'
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
error: invalid format string: unknown format identifier '?#'
--> $DIR/help-message-issue-129966-reorder-remove-format-specifier.rs:8:17
|
LL | format!("{f:?#}");
| ^^ help: the format parameter should be written `#?` in format string
|
= note: if you intended to print `{`, you can escape it using `{{`

error: invalid format string: unknown format identifier '?x'
--> $DIR/help-message-issue-129966-reorder-remove-format-specifier.rs:10:18
|
LL | format!("{f:?x}");
| ^ help: consider removing `x` in format string
|
= note: if you intended to print `{`, you can escape it using `{{`

error: invalid format string: unknown format identifier '?X'
--> $DIR/help-message-issue-129966-reorder-remove-format-specifier.rs:12:18
|
LL | format!("{f:?X}");
| ^ help: consider removing `X` in format string
|
= note: if you intended to print `{`, you can escape it using `{{`

error: aborting due to 3 previous errors

Loading