-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Pretty printer algorithm revamp step 2 #93065
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
label
Jan 19, 2022
r? @lcnr (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
label
Jan 19, 2022
that is a very clean commit history, thanks ❤️ would have taken me far longer to review this otherwise @bors r+ rollup |
📌 Commit 4d3faae has been approved by |
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
Jan 19, 2022
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 19, 2022
…askrgr Rollup of 6 pull requests Successful merges: - rust-lang#92316 (mangling_v0: Skip extern blocks during mangling) - rust-lang#92630 (Change PhantomData type for `BuildHasherDefault` (and more)) - rust-lang#92800 (Add manifest docs fallback.) - rust-lang#93005 (Move back templates into html folder) - rust-lang#93065 (Pretty printer algorithm revamp step 2) - rust-lang#93077 (remove `List::is_noop`) Failed merges: - rust-lang#93068 (Fix spacing for `·` between stability and source) r? `@ghost` `@rustbot` modify labels: rollup
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 20, 2022
Pretty printer algorithm revamp step 3 This PR follows rust-lang#93065 as a third chunk of minor modernizations backported from /~https://github.com/dtolnay/prettyplease into rustc_ast_pretty. I've broken this up into atomic commits that hopefully are sensible in isolation. At every commit, the pretty printer is compilable and has runtime behavior that is identical to before and after the PR. None of the refactoring so far changes behavior. This PR is the last chunk of non-behavior-changing cleanup. After this the **next PR** will begin backporting behavior changes from `prettyplease`, starting with block indentation: ```rust macro_rules! print_expr { ($expr:expr) => { println!("{}", stringify!($expr)); }; } fn main() { print_expr!(Struct { x: 0, y: 0 }); print_expr!(Structtttttttttttttttttttttttttttttttttttttttttttttttttt { xxxxxxxxx: 0, yyyyyyyyy: 0 }); } ``` Output currently on master (nowhere near modern Rust style): ```console Struct{x: 0, y: 0,} Structtttttttttttttttttttttttttttttttttttttttttttttttttt{xxxxxxxxx: 0, yyyyyyyyy: 0,} ``` After the upcoming PR for block indentation (based on dtolnay/prettyplease@401d60c): ```console Struct { x: 0, y: 0, } Structtttttttttttttttttttttttttttttttttttttttttttttttttt { xxxxxxxxx: 0, yyyyyyyyy: 0, } ``` And the PR after that, for intelligent trailing commas (based on dtolnay/prettyplease@e2a0297): ```console Struct { x: 0, y: 0 } Structtttttttttttttttttttttttttttttttttttttttttttttttttt { xxxxxxxxx: 0, yyyyyyyyy: 0, } ```
dtolnay
removed
the
S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
label
Jan 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
A-pretty
Area: Pretty printing (including `-Z unpretty`)
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR follows #92923 as a second chunk of modernizations backported from /~https://github.com/dtolnay/prettyplease into rustc_ast_pretty.
I've broken this up into atomic commits that hopefully are sensible in isolation. At every commit, the pretty printer is compilable and has runtime behavior that is identical to before and after the PR. None of the refactoring so far changes behavior.
The general theme of this chunk of commits is: the logic in the old pretty printer is doing some very basic things (pushing and popping tokens on a ring buffer) but expressed in a too-low-level way that I found makes it quite complicated/subtle to reason about. There are a number of obvious invariants that are "almost true" -- things like
self.left == self.buf.offset
andself.right == self.buf.offset + self.buf.data.len()
andself.right_total == self.left_total + self.buf.data.sum()
. The reason these things are "almost true" is the implementation tends to put updating one side of the invariant unreasonably far apart from updating the other side, leaving the invariant broken while unrelated stuff happens in between. The following code from master is an example of this:rust/compiler/rustc_ast_pretty/src/pp.rs
Lines 314 to 317 in e5e2b0b
In this code the
advance_right
is reserving an entry into which to write a next token on the right side of the ring buffer, thecheck_stack
is doing something totally unrelated to the right boundary of the ring buffer, and thescan_push
is actually writing the token we previously reserved space for. Much of what this PR is doing is rearranging code to shrink the amount of stuff in between when an invariant is broken to when it is restored, until the whole thing can be factored out into one indivisible method call on the RingBuffer type.The end state of the PR is that we can entirely eliminate
self.left
(because it's now just equal toself.buf.offset
always) andself.right
(because it's equal toself.buf.offset + self.buf.data.len()
always) and the wholeToken::Eof
state which used to be the value of tokens that have been reserved space for but not yet written.I found without these changes the pretty printer implementation to be hard to reason about and I wasn't able to confidently introduce improvements like trailing commas in
prettyplease
until after this refactor. The logic here is 43 years old at this point (Graydon translated it as directly as possible from the 1979 pretty printing paper) and while there are advantages to following the paper as closely as possible, inprettyplease
I decided if we're going to adapt the algorithm to work better for Rust syntax, it was worthwhile making it easier to follow than the original.