Skip to content

Accept "{:}<" in format macro parser #112773

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 2 commits into from

Conversation

TaKO8Ki
Copy link
Member

@TaKO8Ki TaKO8Ki commented Jun 18, 2023

@rustbot
Copy link
Collaborator

rustbot commented Jun 18, 2023

r? @jackh726

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 18, 2023
@TaKO8Ki TaKO8Ki changed the title Accept "{:}<" in format macro parser Accept "{:}<" in format macro parser Jun 18, 2023
@compiler-errors
Copy link
Member

compiler-errors commented Jun 18, 2023

For the record, this code would now behave differently, right?

fn main() {
    println!("{:}<3}", 0);
}

According to the linked issue, the format spec allows any fill character, so shouldn't we be preserving this old behavior?

@TaKO8Ki
Copy link
Member Author

TaKO8Ki commented Jun 19, 2023

@compiler-errors Yes. After this change is applied, this case causes an error like the following, In the old parser, it is compiled successfully. I think this might be wrong behavior and new error looks better.

According to the linked issue, the format spec allows any fill character, so shouldn't we be preserving this old behavior?

I guess we don't have detailed specification for formatting and the current behavior is regarded as a specification? Should I avoid changing the behavior of the format macro parse and just add a diagnostic for this case?

Given the following code:

fn main() {
    println!("{:}<3}", 0);
}

Before this change is applied: Compiled successfully

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=53f9ec33ce4fc8293d41b0fb061e0427

After the change is applied:

error: invalid format string: unmatched `}` found
 --> src/main.rs:2:20
  |
2 |     println!("{:}<3}", 0);
  |                    ^ unmatched `}` in format string
  |
  = note: if you intended to print `}`, you can escape it using `}}`

error: could not compile `debug_playground` (bin "debug_playground") due to previous error

@compiler-errors compiler-errors added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 19, 2023
@compiler-errors
Copy link
Member

Nominating for T-libs-api I think, because this has to do with the specification of the format macro, and it's not strictly a fix.

@compiler-errors compiler-errors added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jun 19, 2023
@@ -626,6 +626,13 @@ impl<'a> Parser<'a> {

// fill character
if let Some(&(_, c)) = self.cur.peek() {
// The following case should be compiled successfully.
// ```
// format!("foo {:}<", 2);
Copy link
Member

Choose a reason for hiding this comment

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

There's a missing close-brace here.

Copy link
Member Author

@TaKO8Ki TaKO8Ki Jun 20, 2023

Choose a reason for hiding this comment

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

You mean the following brace is not a close-brace?

format!("foo {:}<", 2);

But the current rustc can compile this successfully.

format!("{:}", 2)

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=2b950ebb99306f3c8f6d491aab8f84bd

Copy link
Member Author

@TaKO8Ki TaKO8Ki Jun 20, 2023

Choose a reason for hiding this comment

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

// check-pass

fn main() {
println!("Hello, world! {0:}<3", 2);
Copy link
Member

Choose a reason for hiding this comment

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

There's a missing close-brace here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as comments above.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 20, 2023

We discussed this in the libs-api meeting today. We're not sure if we can go forward with this, since this would be a breaking change: you could no longer set } as the fill character. ("{:}>}" would stop working.)

I mentioned that {:} is a bit of an anti-pattern. Clippy used to warn about that and suggest replacing it with {}, but I believe I broke that when I refactored the format arguments internals. We should probably add that lint back in rustc, and that logic can then also help with giving a better diagnostic for "{:}>".

@TaKO8Ki
Copy link
Member Author

TaKO8Ki commented Jun 20, 2023

@m-ou-se

Thank you for discussing it. I see. The example you provided would cause an error like the following after this change is applied. Should } be available as the fill character?

error: invalid format string: unmatched `}` found
 --> src/main.rs:2:18
  |
2 |     format!("{:}>}", 2);
  |                  ^ unmatched `}` in format string
  |
  = note: if you intended to print `}`, you can escape it using `}}`

error: could not compile `debug_playground` (bin "debug_playground") due to previous error

We should probably add that lint back in rustc, and that logic can then also help with giving a better diagnostic for "{:}>".

Ok. I'll create an issue in clippy and work on it when I have time.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 20, 2023

Should } be available as the fill character?

Today, every single character is available as fill character. "Every single character except }" doesn't seem like a big improvement. (What about '{' or other potentially confusing ones?)

I agree {:}>} is confusing, but {:} is uncommon enough (and equivalent to {}) that I don't think it's worth coming up with an alternative syntax for specifying } as fill character.

@TaKO8Ki
Copy link
Member Author

TaKO8Ki commented Jun 20, 2023

Today, every single character is available as fill character. "Every single character except }" doesn't seem like a big improvement. (What about '{' or other potentially confusing ones?)

Understood. You're right. I'll close this pull request.

@TaKO8Ki TaKO8Ki closed this Jun 20, 2023
@TaKO8Ki TaKO8Ki deleted the fix-format-macro-bug branch June 20, 2023 18:14
@dtolnay dtolnay removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Format interprets closing brace as fill and suggesting to add another closing brace for no apperant reason.
7 participants