Skip to content

Check if format argument is identifier to avoid error err-emit #140286

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+140 −45
Open

Check if format argument is identifier to avoid error err-emit #140286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44232a6
Add ui test parser/issues/invalid-parse-format-issue-139104.rs
xizheyin Apr 25, 2025
64867c6
Check if format argument is identifier to avoid error err-emit
xizheyin Apr 25, 2025
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
135 changes: 90 additions & 45 deletions compiler/rustc_parse_format/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,30 @@ pub struct Argument<'a> {
pub format: FormatSpec<'a>,
}

impl<'a> Argument<'a> {
pub fn is_identifier(&self) -> bool {
matches!(self.position, Position::ArgumentNamed(_))
&& matches!(
self.format,
FormatSpec {
fill: None,
fill_span: None,
align: AlignUnknown,
sign: None,
alternate: false,
zero_pad: false,
debug_hex: None,
precision: CountImplied,
precision_span: None,
width: CountImplied,
width_span: None,
ty: "",
ty_span: None,
},
)
}
}

/// Specification for the formatting of an argument in the format string.
#[derive(Copy, Clone, Debug, PartialEq)]
pub struct FormatSpec<'a> {
Expand Down Expand Up @@ -894,52 +918,73 @@ impl<'a> Parser<'a> {
}

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);
let start = InnerOffset(byte_pos.0 + 1);
let field = self.argument(start);
// We can only parse simple `foo.bar` field access or `foo.0` tuple index access, any
// deeper nesting, or another type of expression, like method calls, are not supported
if !self.consume('}') {
return;
}
if let ArgumentNamed(_) = arg.position {
match field.position {
ArgumentNamed(_) => {
self.errors.insert(
0,
ParseError {
description: "field access isn't supported".to_string(),
note: None,
label: "not supported".to_string(),
span: InnerSpan::new(
arg.position_span.start,
field.position_span.end,
),
secondary_label: None,
suggestion: Suggestion::UsePositional,
},
);
}
ArgumentIs(_) => {
self.errors.insert(
0,
ParseError {
description: "tuple index access isn't supported".to_string(),
note: None,
label: "not supported".to_string(),
span: InnerSpan::new(
arg.position_span.start,
field.position_span.end,
),
secondary_label: None,
suggestion: Suggestion::UsePositional,
},
);
}
_ => {}
};
// If the argument is an identifier, it may be a field access.
if arg.is_identifier() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I first determine if it's an identifier before considering if it's a field access, e.g. foo.0, foo.a

if let Some(end) = self.consume_pos('.') {
let byte_pos = self.to_span_index(end);
let start = InnerOffset(byte_pos.0 + 1);
let field = self.argument(start);
// We can only parse simple `foo.bar` field access or `foo.0` tuple index access, any
// deeper nesting, or another type of expression, like method calls, are not supported
if !self.consume('}') {
return;
}
if let ArgumentNamed(_) = arg.position {
match field.position {
ArgumentNamed(_) => {
self.errors.insert(
0,
ParseError {
description: "field access isn't supported".to_string(),
note: None,
label: "not supported".to_string(),
span: InnerSpan::new(
arg.position_span.start,
field.position_span.end,
),
secondary_label: None,
suggestion: Suggestion::UsePositional,
},
);
}
ArgumentIs(_) => {
self.errors.insert(
0,
ParseError {
description: "tuple index access isn't supported".to_string(),
note: None,
label: "not supported".to_string(),
span: InnerSpan::new(
arg.position_span.start,
field.position_span.end,
),
secondary_label: None,
suggestion: Suggestion::UsePositional,
},
);
}
_ => {}
};
}
}
} else if matches!(arg.position, ArgumentNamed(_) | ArgumentIs(_)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only trigger this error when there is an explicit explicit location or a named argument.

This is because the case of ArgumentImplicitlyIs is more complex, e.g. “{ x”, and I think it should be prioritized to output the missing } over this error. For arguments that have a name or an explicit position, it's more appropriate to output invalid format specifier instead of missing }.

Also, the original logic does not emit an ERROR in ArgumentImplicitlyIs case, so I'm not making radical changes for now. It can be left for later enhancements.

let arg_name = match arg.position {
ArgumentNamed(arg_name) => &format!("`{arg_name}`"),
ArgumentIs(arg_index) => &format!("at index `{arg_index}`"),
_ => unreachable!(),
};

self.errors.insert(
0,
ParseError {
description: format!("invalid format string for argument {}", arg_name),
note: None,
label: format!("invalid format specifier for this argument"),
span: InnerSpan::new(arg.position_span.start, arg.position_span.end),
secondary_label: None,
suggestion: Suggestion::None,
},
);
}
}

Expand Down
8 changes: 8 additions & 0 deletions tests/ui/parser/issues/invalid-parse-format-issue-139104.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
fn main() {
println!("{foo:_1.4}", foo = 3.14); //~ ERROR invalid format string: invalid format string for argument `foo`
println!("{foo:1.4_1.4}", foo = 3.14); //~ ERROR invalid format string: invalid format string for argument `foo`
println!("xxx{0:_1.4}", 1.11); //~ ERROR invalid format string: invalid format string for argument at index `0`
println!("{foo:_1.4", foo = 3.14); //~ ERROR invalid format string: invalid format string for argument `foo`
println!("xxx{0:_1.4", 1.11); //~ ERROR invalid format string: invalid format string for argument at index `0`
println!("xxx{ 0", 1.11); //~ ERROR invalid format string: expected `}`, found `0`
}
42 changes: 42 additions & 0 deletions tests/ui/parser/issues/invalid-parse-format-issue-139104.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
error: invalid format string: invalid format string for argument `foo`
--> $DIR/invalid-parse-format-issue-139104.rs:2:16
|
LL | println!("{foo:_1.4}", foo = 3.14);
| ^^^ invalid format specifier for this argument in format string

error: invalid format string: invalid format string for argument `foo`
--> $DIR/invalid-parse-format-issue-139104.rs:3:16
|
LL | println!("{foo:1.4_1.4}", foo = 3.14);
| ^^^ invalid format specifier for this argument in format string

error: invalid format string: invalid format string for argument at index `0`
--> $DIR/invalid-parse-format-issue-139104.rs:4:19
|
LL | println!("xxx{0:_1.4}", 1.11);
| ^ invalid format specifier for this argument in format string

error: invalid format string: invalid format string for argument `foo`
--> $DIR/invalid-parse-format-issue-139104.rs:5:16
|
LL | println!("{foo:_1.4", foo = 3.14);
| ^^^ invalid format specifier for this argument in format string

error: invalid format string: invalid format string for argument at index `0`
--> $DIR/invalid-parse-format-issue-139104.rs:6:19
|
LL | println!("xxx{0:_1.4", 1.11);
| ^ invalid format specifier for this argument in format string

error: invalid format string: expected `}`, found `0`
--> $DIR/invalid-parse-format-issue-139104.rs:7:21
|
LL | println!("xxx{ 0", 1.11);
| - ^ expected `}` in format string
| |
| because of this opening brace
|
= note: if you intended to print `{`, you can escape it using `{{`

error: aborting due to 6 previous errors

Loading