Skip to content
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

Use modern formatting for format! macros #93950

Merged

Conversation

mahmoud-moursy
Copy link
Contributor

@mahmoud-moursy mahmoud-moursy commented Feb 12, 2022

This updates the standard library's documentation to use the new format_args syntax.
The documentation is worthwhile to update as it should be more idiomatic
(particularly for features like this, which are nice for users to get acquainted
with). The general codebase is likely more hassle than benefit to update: it'll
hurt git blame, and generally updates can be done by folks updating the code if
(and when) that makes things more readable with the new format.

A few places in the compiler and library code are updated (mostly just due to
already having been done when this commit was first authored).

eprintln!("{}", e) becomes eprintln!("{e}"), but eprintln!("{}", e.kind()) remains untouched.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 12, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@mahmoud-moursy mahmoud-moursy marked this pull request as ready for review February 13, 2022 05:21
@klensy
Copy link
Contributor

klensy commented Feb 13, 2022

Mixing positional and named format args looks quite puzzling, maybe not change that ones?
There were some issue about it on github.

@mahmoud-moursy
Copy link
Contributor Author

@klensy good input, I think we should seek the maintainer's input on this (Mark-Simulacrum). If necessary, I'm willing to make the changes.

@Mark-Simulacrum
Copy link
Member

Yeah, I think in general if there are positional arguments (e.g., because they use a more complex expression) we should avoid the usage of inline arguments for now.

I'm not sure this is necessarily a good idea across the board, particularly in user-visible documentation, quite yet, either -- while we do only officially support 1 stable release, many Rust users are on somewhat older releases and identifiers just shipped in 1.58 not 6 weeks ago. I'll start a Zulip thread on this to see if there's opinions on this -- https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/format.20.7Bident.7D.20usage.20in.20docs

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 14, 2022
@bors
Copy link
Contributor

bors commented Feb 14, 2022

⌛ Trying commit 2b057cada2f01a2f710468823173f4dbcea75229 with merge 545b01588be441368ca92a109eec76f75737baf1...

@bors
Copy link
Contributor

bors commented Feb 14, 2022

☀️ Try build successful - checks-actions
Build commit: 545b01588be441368ca92a109eec76f75737baf1 (545b01588be441368ca92a109eec76f75737baf1)

@rust-timer
Copy link
Collaborator

Queued 545b01588be441368ca92a109eec76f75737baf1 with parent b321742, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (545b01588be441368ca92a109eec76f75737baf1): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 14, 2022
@rust-log-analyzer

This comment has been minimized.

@bstrie
Copy link
Contributor

bstrie commented Feb 15, 2022

@T-O-R-U-S I'm curious, while you were doing this, did you take note of what sorts of things couldn't be updated to the new syntax? What percentage of the format strings in the compiler could be successfully updated to use the new syntax? Would that percentage have been significantly increased if the format string syntax allowed trivial extensions like e.g. allowing struct field access?

@mahmoud-moursy
Copy link
Contributor Author

@T-O-R-U-S I'm curious, while you were doing this, did you take note of what sorts of things couldn't be updated to the new syntax? What percentage of the format strings in the compiler could be successfully updated to use the new syntax? Would that percentage have been significantly increased if the format string syntax allowed trivial extensions like e.g. allowing struct field access?

Unfortunately, no. I used a regex to quickly look for instances of old format strings in the codebase (of which there are far too many) and changed them by hand for fear of accidentally (and silently) breaking something. If I were to estimate, ~5% of inline format strings couldn't be done because struct field access wasn't possible.

@mahmoud-moursy
Copy link
Contributor Author

Should the documentation be left alone now, or should the rest of it be updated? @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

Hm, so given this would land at the earliest in 1.61, I think we can likely afford to update the documentation as well. Let's update the docs as well.

If you can squash the commits after doing so as well, that'll help speed along a potential approval -- this sort of PR doesn't (at least for me) really benefit from lots of individual commits.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 18, 2022
@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

Can you confirm that this updates all such cases across the compiler/standard library? I'd prefer to keep this to just 1-2 commits over time, to minimize noise in git blame and such.

@Mark-Simulacrum
Copy link
Member

Hm, so I think limiting it to just library/ (and documentation, at that) is probably fine -- updating to the new format throughout the compiler if there's that many cases is probably more noisy than helpful and just causes git history churn for relatively little benefit. It can be patched up as those parts of the codebase naturally change over time.

@mahmoud-moursy
Copy link
Contributor Author

On that topic, looks like I forgot to go through src/doc... I'll be on it tomorrow, or some time later today.

@mahmoud-moursy
Copy link
Contributor Author

mahmoud-moursy commented Feb 23, 2022

There was a flaw with my regex and it has now become apparent to me that /library/ is not completely done.

To elaborate, the regex didn't account for things like {:?} or {:x}.

@mahmoud-moursy
Copy link
Contributor Author

~500 un-updated format strings remain in src/doc

@mahmoud-moursy
Copy link
Contributor Author

I think The Book would need more delicate handling. It appears that Rust by Example is already partially updated to support the new syntax, but to use it in the Book might confuse users, and I also believe that extending the book to include a section about the new syntax is out of the scope of this PR.

@Mark-Simulacrum
Copy link
Member

Let's avoid bumping submodules in that PR, it'll increase the amount of conflicts and generally cause more pain -- they can get bumped on their normal schedules. I agree that most of the book-like documentation likely is not as easy to update and should be left out of scope.

@Mark-Simulacrum Mark-Simulacrum force-pushed the use-modern-formatting-for-format!-macros branch from 3f1e116 to 815d680 Compare March 8, 2022 02:31
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

Squashed commits down and rebased.

@bors
Copy link
Contributor

bors commented Mar 8, 2022

📌 Commit 815d680d736a87db4428b93a7f0d6fce937cc5f8 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2022
@bors
Copy link
Contributor

bors commented Mar 8, 2022

☔ The latest upstream changes (presumably #94734) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 8, 2022
@mahmoud-moursy mahmoud-moursy force-pushed the use-modern-formatting-for-format!-macros branch from fa5e5cb to 7730980 Compare March 9, 2022 13:20
@bors
Copy link
Contributor

bors commented Mar 10, 2022

☔ The latest upstream changes (presumably #94802) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 10, 2022
This updates the standard library's documentation to use the new syntax. The
documentation is worthwhile to update as it should be more idiomatic
(particularly for features like this, which are nice for users to get acquainted
with). The general codebase is likely more hassle than benefit to update: it'll
hurt git blame, and generally updates can be done by folks updating the code if
(and when) that makes things more readable with the new format.

A few places in the compiler and library code are updated (mostly just due to
already having been done when this commit was first authored).
@Mark-Simulacrum Mark-Simulacrum force-pushed the use-modern-formatting-for-format!-macros branch from 7730980 to 72a25d0 Compare March 10, 2022 15:24
@Mark-Simulacrum
Copy link
Member

@bors r+

Squashed commits down and rebased.

@bors
Copy link
Contributor

bors commented Mar 10, 2022

📌 Commit 72a25d0 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 10, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 10, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#93950 (Use modern formatting for format! macros)
 - rust-lang#94274 (Treat unstable lints as unknown)
 - rust-lang#94368 ([1/2] Implement macro meta-variable expressions)
 - rust-lang#94719 (Statically compile libstdc++ everywhere if asked)
 - rust-lang#94728 (Only emit pointer-like metadata for `Box<T, A>` when `A` is ZST)
 - rust-lang#94790 (enable portable-simd doctests in Miri)
 - rust-lang#94811 (Update browser-ui-test version)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5a7f09d into rust-lang:master Mar 11, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants