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

Pretty printer algorithm revamp step 2 #93065

Merged
merged 15 commits into from
Jan 20, 2022
Merged

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Jan 19, 2022

This PR follows #92923 as a second chunk of modernizations backported from https:/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 and self.right == self.buf.offset + self.buf.data.len() and self.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:

self.advance_right();
}
self.check_stack(0);
self.scan_push(BufEntry { token: Token::Break(b), size: -self.right_total });

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, the check_stack is doing something totally unrelated to the right boundary of the ring buffer, and the scan_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 to self.buf.offset always) and self.right (because it's equal to self.buf.offset + self.buf.data.len() always) and the whole Token::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, in prettyplease 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.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 19, 2022
@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 19, 2022
@lcnr
Copy link
Contributor

lcnr commented Jan 19, 2022

that is a very clean commit history, thanks ❤️ would have taken me far longer to review this otherwise

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 19, 2022

📌 Commit 4d3faae has been approved by lcnr

@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 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
@bors bors merged commit fe93f08 into rust-lang:master Jan 20, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 20, 2022
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:/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 dtolnay added the A-pretty Area: Pretty printing (including `-Z unpretty`) label Dec 28, 2023
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`) 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.

5 participants