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

Switch pretty printer to block-based indentation #93155

Merged
merged 4 commits into from
Jan 31, 2022

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Jan 21, 2022

This PR backports dtolnay/prettyplease@401d60c from the prettyplease crate into rustc_ast_pretty.

A before and after:

- let res =
-     ((::alloc::fmt::format as
-          for<'r> fn(Arguments<'r>) -> String {format})(((::core::fmt::Arguments::new_v1
-                                                             as
-                                                             fn(&[&'static str], &[ArgumentV1]) -> Arguments {Arguments::new_v1})((&([("test"
-                                                                                                                                          as
-                                                                                                                                          &str)]
-                                                                                                                                        as
-                                                                                                                                        [&str; 1])
-                                                                                                                                      as
-                                                                                                                                      &[&str; 1]),
-                                                                                                                                  (&([]
-                                                                                                                                        as
-                                                                                                                                        [ArgumentV1; 0])
-                                                                                                                                      as
-                                                                                                                                      &[ArgumentV1; 0]))
-                                                            as
-                                                            Arguments))
-         as String);
+ let res =
+     ((::alloc::fmt::format as
+             for<'r> fn(Arguments<'r>) -> String {format})(((::core::fmt::Arguments::new_v1
+                 as
+                 fn(&[&'static str], &[ArgumentV1]) -> Arguments {Arguments::new_v1})((&([("test"
+                             as &str)] as [&str; 1]) as
+                 &[&str; 1]),
+             (&([] as [ArgumentV1; 0]) as &[ArgumentV1; 0])) as
+             Arguments)) as String);

Previously the pretty printer would compute indentation always relative to whatever column a block begins at, like this:

fn demo(arg1: usize,
        arg2: usize);

This is never the thing to do in the dominant contemporary Rust style. Rustfmt's default and the style used by the vast majority of Rust codebases is block indentation:

fn demo(
    arg1: usize,
    arg2: usize,
);

where every indentation level is a multiple of 4 spaces and each level is indented relative to the indentation of the previous line, not the position that the block starts in.

By itself this PR doesn't get perfect formatting in all cases, but it is the smallest possible step in clearly the right direction. More backports from prettyplease to tune the ibox/cbox indent levels around various AST node types are upcoming.

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

r? @nagisa

(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 21, 2022
Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

Some of examples do indeed look much more conventional, nice.

I don't actually know enough about ast_pretty (and I bet the sentiment will be shared across T-compiler in general) to have much idea what some of the minute changes to the code affect, but I'm quite comfortable r+ing this regardless given that all these changes can affect is whitespace.

My question however more broadly is – is there a reason why we're looking to backport the changes from prettyplease to ast_pretty vs just migrating rustc to use prettyplease in the first place?

///
/// fn demo(arg1: usize,
/// arg2: usize);
Visual,
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason behind retaining both modes? Seems like potentially unnecessary maintenance burden.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only used by trailing block comments, which align visually under the /*. I would be very happy to delete that test and revert 269b5a4.

fn f() {} /*
The next line should not be indented.

@bjorn3
Copy link
Member

bjorn3 commented Jan 22, 2022

My question however more broadly is – is there a reason why we're looking to backport the changes from prettyplease to ast_pretty vs just migrating rustc to use prettyplease in the first place?

Because prettyplease uses syn and thus proc_macro TokenStream, while rustc directly uses the pretty printer on the AST and HIR I would guess.

@dtolnay
Copy link
Member Author

dtolnay commented Jan 22, 2022

Yeah there's no way that prettyplease would be useful for printing a rustc AST.

@dtolnay
Copy link
Member Author

dtolnay commented Jan 30, 2022

To clarify from #93155 (comment): @nagisa would you prefer if 269b5a4 were removed from this PR? I don't expect it to be a large maintenance cost based on the other pretty printer changes I've written after this one and the small rate of change in the pretty printer in general, but it's also not a large benefit because block comments are pretty rare in modern Rust, so it's tough to gauge the right tradeoff.

@nagisa
Copy link
Member

nagisa commented Jan 30, 2022

Nah, I'm fine with it being in, personally. I guess I forgot to give r=me after reviewing, despite approving the PR…

@bors r+

@bors
Copy link
Contributor

bors commented Jan 30, 2022

📌 Commit 269b5a4 has been approved by nagisa

@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 30, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 30, 2022
Switch pretty printer to block-based indentation

This PR backports dtolnay/prettyplease@401d60c from the `prettyplease` crate into `rustc_ast_pretty`.

A before and after:

```diff
- let res =
-     ((::alloc::fmt::format as
-          for<'r> fn(Arguments<'r>) -> String {format})(((::core::fmt::Arguments::new_v1
-                                                             as
-                                                             fn(&[&'static str], &[ArgumentV1]) -> Arguments {Arguments::new_v1})((&([("test"
-                                                                                                                                          as
-                                                                                                                                          &str)]
-                                                                                                                                        as
-                                                                                                                                        [&str; 1])
-                                                                                                                                      as
-                                                                                                                                      &[&str; 1]),
-                                                                                                                                  (&([]
-                                                                                                                                        as
-                                                                                                                                        [ArgumentV1; 0])
-                                                                                                                                      as
-                                                                                                                                      &[ArgumentV1; 0]))
-                                                            as
-                                                            Arguments))
-         as String);
+ let res =
+     ((::alloc::fmt::format as
+             for<'r> fn(Arguments<'r>) -> String {format})(((::core::fmt::Arguments::new_v1
+                 as
+                 fn(&[&'static str], &[ArgumentV1]) -> Arguments {Arguments::new_v1})((&([("test"
+                             as &str)] as [&str; 1]) as
+                 &[&str; 1]),
+             (&([] as [ArgumentV1; 0]) as &[ArgumentV1; 0])) as
+             Arguments)) as String);
```

Previously the pretty printer would compute indentation always relative to whatever column a block begins at, like this:

```rust
fn demo(arg1: usize,
        arg2: usize);
```

This is never the thing to do in the dominant contemporary Rust style. Rustfmt's default and the style used by the vast majority of Rust codebases is block indentation:

```rust
fn demo(
    arg1: usize,
    arg2: usize,
);
```

where every indentation level is a multiple of 4 spaces and each level is indented relative to the indentation of the previous line, not the position that the block starts in.

By itself this PR doesn't get perfect formatting in all cases, but it is the smallest possible step in clearly the right direction. More backports from `prettyplease` to tune the ibox/cbox indent levels around various AST node types are upcoming.
@bors
Copy link
Contributor

bors commented Jan 31, 2022

☔ The latest upstream changes (presumably #90891) 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 Jan 31, 2022
Previously the pretty printer would compute indentation always relative
to whatever column a block begins at, like this:

    fn demo(arg1: usize,
            arg2: usize);

This is never the thing to do in the dominant contemporary Rust style.
Rustfmt's default and the style used by the vast majority of Rust
codebases is block indentation:

    fn demo(
        arg1: usize,
        arg2: usize,
    );

where every indentation level is a multiple of 4 spaces and each level
is indented relative to the indentation of the previous line, not the
position that the block starts in.
The `print_expr` method already places an `ibox(INDENT_UNIT)` around
every expr that gets printed. Some exprs were then using `self.head`
inside of that, which does its own `cbox(INDENT_UNIT)`, resulting in two
levels of indentation:

    while true {
            stuff;
        }

This commit fixes those cases to produce the expected single level of
indentation within every expression containing a block.

    while true {
        stuff;
    }
@dtolnay
Copy link
Member Author

dtolnay commented Jan 31, 2022

@dtolnay
Copy link
Member Author

dtolnay commented Jan 31, 2022

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Jan 31, 2022

📌 Commit 125c729 has been approved by nagisa

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 31, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2022
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#90277 (Improve terminology around "after typeck")
 - rust-lang#92918 (Allow eliding GATs in expression position)
 - rust-lang#93039 (Don't suggest inaccessible fields)
 - rust-lang#93155 (Switch pretty printer to block-based indentation)
 - rust-lang#93214 (Respect doc(hidden) when suggesting available fields)
 - rust-lang#93347 (Make `char::DecodeUtf16::size_hist` more precise)
 - rust-lang#93392 (Clarify documentation on char::MAX)
 - rust-lang#93444 (Fix some CSS warnings and errors from VS Code)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1cb22e4 into rust-lang:master Jan 31, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 31, 2022
@dtolnay dtolnay deleted the blockindent branch January 31, 2022 17:54
ehuss added a commit to ehuss/rust that referenced this pull request Feb 1, 2022
Allow any pretty printed line to have at least 60 chars

Follow-up to rust-lang#93155. The rustc AST pretty printer has a tendency to get stuck in "vertical smear mode" when formatting highly nested code, where it puts a linebreak at *every possible* linebreak opportunity once the indentation goes beyond the pretty printer's target line width:

```rust
...
                                                              ((&([("test"
                                                                       as
                                                                       &str)]
                                                                     as
                                                                     [&str; 1])
                                                                   as
                                                                   &[&str; 1]),
                                                               (&([]
                                                                     as
                                                                     [ArgumentV1; 0])
                                                                   as
                                                                   &[ArgumentV1; 0]))
...
```

```rust
...
                                                                          [(1
                                                                               as
                                                                               i32),
                                                                           (2
                                                                               as
                                                                               i32),
                                                                           (3
                                                                               as
                                                                               i32)]
                                                                             as
                                                                             [i32; 3]
...
```

This is less common after rust-lang#93155 because that PR greatly reduced the total amount of indentation, but the "vertical smear mode" failure mode is still just as present when you have deeply nested modules, functions, or trait impls, such as in the case of macro-expanded code from `-Zunpretty=expanded`.

Vertical smear mode is never the best way to format highly indented code though. It does not prevent the target line width from being exceeded, and it produces output that is less readable than just a longer line.

This PR makes the pretty printing algorithm allow a minimum of 60 chars on every line independent of indentation. So as code gets more indented, the right margin eventually recedes to make room for formatting without vertical smear.

```console
├─────────────────────────────────────┤
├─────────────────────────────────────┤
├─────────────────────────────────────┤
  ├───────────────────────────────────┤
    ├─────────────────────────────────┤
      ├───────────────────────────────┤
        ├─────────────────────────────┤
          ├───────────────────────────┤
            ├───────────────────────────┤
              ├───────────────────────────┤
            ├───────────────────────────┤
          ├───────────────────────────┤
        ├─────────────────────────────┤
      ├───────────────────────────────┤
    ├─────────────────────────────────┤
  ├───────────────────────────────────┤
├─────────────────────────────────────┤
```
@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.

6 participants