Skip to content

Printer cleanups #144776

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Aug 5, 2025
Merged

Printer cleanups #144776

merged 8 commits into from
Aug 5, 2025

Conversation

nnethercote
Copy link
Contributor

The trait Printer is implemented by six types, and the sub-trait PrettyPrinter is implemented by three of those types. The traits and the impls are complex and a bit of a mess. This PR starts to clean them up.

r? @davidtwco

@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 Aug 1, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 1, 2025

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred in need_type_info.rs

cc @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

RalfJung
RalfJung reviewed Aug 1, 2025
View reviewed changes
RalfJung
RalfJung reviewed Aug 1, 2025
View reviewed changes
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nnethercote nnethercote force-pushed the Printer-cleanups branch 2 times, most recently from 968070d to 8941c3c Compare August 1, 2025 14:58
@bors
Copy link
Collaborator

bors commented Aug 2, 2025

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

@nnethercote
Mark Printer methods as unreachable where appropriate.
df52416 
This helps me understand the structure of the code a lot.

If any of these are actually reachable, we can put the old code back,
add a new test case, and we will have improved our test coverage.
@nnethercote
Inline and remove two FmtPrinter methods.
bd0a308 
They each have a single call site.
@nnethercote
Remove type_name::AbsolutePathPrinter::comma_sep.
e7d6a07 
It's equivalent to the default `PrettyPrinter::comma_sep`.
@nnethercote
Rename Printer variables.
1698c8e 
Currently they are mostly named `cx`, which is a terrible name for a
type that impls `Printer`/`PrettyPrinter`, and is easy to confuse with
other types like `TyCtxt`. This commit changes them to `p`. A couple of
existing `p` variables had to be renamed to make way.
@nnethercote
Simplify SymbolMangler::print_type.
03bc1be 
`Bound`/`Placeholder`/`Infer`/`Error` shouldn't occur, so we can handle
them in the second exhaustive `match`, and ignore them in the first
non-exhaustive `match`.
@nnethercote
Remove p!.
03dab50 
It's a cryptic macro that makes some things slightly more concise in
`PrettyPrinter`. E.g. if you declare `define_scope_printer!(p)` in a
scope you can then call `p! to get these transformations:
```
p!("foo");         -->  write!(p, "foo")?;
p!(print(ty));     -->  ty.print(p)?;
p!(method(args));  -->  p.method(args)?;
```
You can also chain calls, e.g.:
```
p!("foo", print(ty));  -->  write!(p, "foo")?; ty.print(p)?;
```
Ultimately this doesn't seem worth it. The macro definition is hard to
read, the call sites are hard to read, `define_scope_printer!` is pretty
gross, and the code size reductions are small. Tellingly, many normal
`write!` and `print` calls are sprinkled throughout the code, probably
because people have made modifications and didn't want to use or
understand how to use `p!`.

This commit removes it.
@nnethercote
Remove unused arg from path_append_impl.
2434d8c 
None of the impls use it.
@nnethercote
Copy link
Contributor Author

I rebased.

cjgillot
cjgillot reviewed Aug 3, 2025
View reviewed changes
@nnethercote
Avoid some code duplication.
cc62d55 
`print_binder` can call `wrap_binder`.
@cjgillot
Copy link
Contributor

cjgillot commented Aug 4, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 4, 2025

📌 Commit cc62d55 has been approved by cjgillot

It is now in the queue for this repository.

@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 Aug 4, 2025
samueltardieu added a commit to samueltardieu/rust that referenced this pull request Aug 4, 2025
@samueltardieu
Rollup merge of rust-lang#144776 - nnethercote:Printer-cleanups, r=cj…
2aecc7c 
…gillot

`Printer` cleanups

The trait `Printer` is implemented by six types, and the sub-trait `PrettyPrinter` is implemented by three of those types. The traits and the impls are complex and a bit of a mess. This PR starts to clean them up.

r? `@davidtwco`
@samueltardieu samueltardieu mentioned this pull request Aug 4, 2025
Closed
bors added a commit that referenced this pull request Aug 4, 2025
@bors
Auto merge of #144929 - samueltardieu:rollup-n2c14p7, r=samueltardieu
5bd6cac 
Rollup of 18 pull requests

Successful merges:

 - #144467 (rustdoc template font links only emit `crossorigin` when needed)
 - #144548 (Rehome 21 `tests/ui/issues/` tests to other subdirectories under `tests/ui/`)
 - #144596 (libtest: print the type of test being run)
 - #144741 (fix: Error on illegal `[const]`s inside blocks within legal positions)
 - #144776 (`Printer` cleanups)
 - #144779 (Implement debugging output of the bootstrap Step graph into a DOT file)
 - #144813 (Add a tidy check to prevent adding UI tests directly under `tests/ui/`)
 - #144817 (Properly reject tail calls to `&FnPtr` or `&FnDef`)
 - #144852 (Rename `rust_panic_without_hook` to `resume_unwind` )
 - #144866 (Remove `SHOULD_EMIT_LINTS` in favor of `should_emit`)
 - #144867 (Use `as_array` in PartialEq for arrays)
 - #144872 (Document Poisoning in `LazyCell` and `LazyLock`)
 - #144877 (coverage: Various small cleanups)
 - #144887 (`rust-analyzer` subtree update)
 - #144890 (Add `InterpCx::project_fields`)
 - #144894 (Delete `tests/ui/threads-sendsync/tcp-stress.rs`)
 - #144905 (rustc-dev-guide subtree update)
 - #144920 (Dont print arg span in MIR dump for tail call)

r? `@ghost`
`@rustbot` modify labels: rollup
@samueltardieu samueltardieu mentioned this pull request Aug 5, 2025
Merged
bors added a commit that referenced this pull request Aug 5, 2025
@bors
Auto merge of #144934 - samueltardieu:rollup-25jvb9g, r=samueltardieu
0f35336 
Rollup of 17 pull requests

Successful merges:

 - #144467 (rustdoc template font links only emit `crossorigin` when needed)
 - #144548 (Rehome 21 `tests/ui/issues/` tests to other subdirectories under `tests/ui/`)
 - #144741 (fix: Error on illegal `[const]`s inside blocks within legal positions)
 - #144776 (`Printer` cleanups)
 - #144779 (Implement debugging output of the bootstrap Step graph into a DOT file)
 - #144813 (Add a tidy check to prevent adding UI tests directly under `tests/ui/`)
 - #144817 (Properly reject tail calls to `&FnPtr` or `&FnDef`)
 - #144852 (Rename `rust_panic_without_hook` to `resume_unwind` )
 - #144866 (Remove `SHOULD_EMIT_LINTS` in favor of `should_emit`)
 - #144867 (Use `as_array` in PartialEq for arrays)
 - #144872 (Document Poisoning in `LazyCell` and `LazyLock`)
 - #144877 (coverage: Various small cleanups)
 - #144887 (`rust-analyzer` subtree update)
 - #144890 (Add `InterpCx::project_fields`)
 - #144894 (Delete `tests/ui/threads-sendsync/tcp-stress.rs`)
 - #144905 (rustc-dev-guide subtree update)
 - #144920 (Dont print arg span in MIR dump for tail call)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a1e41a0 into rust-lang:master Aug 5, 2025
10 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Aug 5, 2025
rust-timer added a commit that referenced this pull request Aug 5, 2025
@rust-timer
Unrolled build for #144776
aff7836 
Rollup merge of #144776 - nnethercote:Printer-cleanups, r=cjgillot

`Printer` cleanups

The trait `Printer` is implemented by six types, and the sub-trait `PrettyPrinter` is implemented by three of those types. The traits and the impls are complex and a bit of a mess. This PR starts to clean them up.

r? ``@davidtwco``
@nnethercote nnethercote deleted the Printer-cleanups branch August 5, 2025 10:55
@nnethercote nnethercote mentioned this pull request Aug 5, 2025
Merged
@cyrgani cyrgani mentioned this pull request Aug 6, 2025
Closed
nnethercote added a commit to nnethercote/rust that referenced this pull request Aug 7, 2025
@nnethercote
Reimplement print_region in type_name.rs.
8074e67 
Broken by rust-lang#144776; this is reachable after all.

Fixes rust-lang#144994.

The commit also adds a lot more cases to the `type-name-basic.rs`,
because it's currently very anaemic. This includes some cases where
region omission does very badly; these are marked with FIXME.
@nnethercote nnethercote mentioned this pull request Aug 7, 2025
Merged
github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Aug 7, 2025
@bors
Auto merge of #144934 - samueltardieu:rollup-25jvb9g, r=samueltardieu
811f376 
Rollup of 17 pull requests

Successful merges:

 - rust-lang/rust#144467 (rustdoc template font links only emit `crossorigin` when needed)
 - rust-lang/rust#144548 (Rehome 21 `tests/ui/issues/` tests to other subdirectories under `tests/ui/`)
 - rust-lang/rust#144741 (fix: Error on illegal `[const]`s inside blocks within legal positions)
 - rust-lang/rust#144776 (`Printer` cleanups)
 - rust-lang/rust#144779 (Implement debugging output of the bootstrap Step graph into a DOT file)
 - rust-lang/rust#144813 (Add a tidy check to prevent adding UI tests directly under `tests/ui/`)
 - rust-lang/rust#144817 (Properly reject tail calls to `&FnPtr` or `&FnDef`)
 - rust-lang/rust#144852 (Rename `rust_panic_without_hook` to `resume_unwind` )
 - rust-lang/rust#144866 (Remove `SHOULD_EMIT_LINTS` in favor of `should_emit`)
 - rust-lang/rust#144867 (Use `as_array` in PartialEq for arrays)
 - rust-lang/rust#144872 (Document Poisoning in `LazyCell` and `LazyLock`)
 - rust-lang/rust#144877 (coverage: Various small cleanups)
 - rust-lang/rust#144887 (`rust-analyzer` subtree update)
 - rust-lang/rust#144890 (Add `InterpCx::project_fields`)
 - rust-lang/rust#144894 (Delete `tests/ui/threads-sendsync/tcp-stress.rs`)
 - rust-lang/rust#144905 (rustc-dev-guide subtree update)
 - rust-lang/rust#144920 (Dont print arg span in MIR dump for tail call)

r? `@ghost`
`@rustbot` modify labels: rollup
Zalathar added a commit to Zalathar/rust that referenced this pull request Aug 7, 2025
@Zalathar
Rollup merge of rust-lang#145033 - nnethercote:fix-144994, r=fmease
33f1862 
Reimplement `print_region` in `type_name.rs`.

Broken by rust-lang#144776; this is reachable after all.

Fixes rust-lang#144994.

The commit also adds a lot more cases to the `type-name-basic.rs`, because it's currently very anaemic. This includes some cases where region omission does very badly; these are marked with FIXME.

r? `@fmease`
rust-timer added a commit that referenced this pull request Aug 7, 2025
@rust-timer
Unrolled build for #145033
9a58804 
Rollup merge of #145033 - nnethercote:fix-144994, r=fmease

Reimplement `print_region` in `type_name.rs`.

Broken by #144776; this is reachable after all.

Fixes #144994.

The commit also adds a lot more cases to the `type-name-basic.rs`, because it's currently very anaemic. This includes some cases where region omission does very badly; these are marked with FIXME.

r? `@fmease`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Aug 8, 2025
@Zalathar
Rollup merge of #145033 - nnethercote:fix-144994, r=fmease
cc0cf48 
Reimplement `print_region` in `type_name.rs`.

Broken by rust-lang/rust#144776; this is reachable after all.

Fixes rust-lang/rust#144994.

The commit also adds a lot more cases to the `type-name-basic.rs`, because it's currently very anaemic. This includes some cases where region omission does very badly; these are marked with FIXME.

r? `@fmease`
github-actions bot pushed a commit to rust-lang/rust-analyzer that referenced this pull request Aug 11, 2025
@bors
Auto merge of #144934 - samueltardieu:rollup-25jvb9g, r=samueltardieu
6e3ab74 
Rollup of 17 pull requests

Successful merges:

 - rust-lang/rust#144467 (rustdoc template font links only emit `crossorigin` when needed)
 - rust-lang/rust#144548 (Rehome 21 `tests/ui/issues/` tests to other subdirectories under `tests/ui/`)
 - rust-lang/rust#144741 (fix: Error on illegal `[const]`s inside blocks within legal positions)
 - rust-lang/rust#144776 (`Printer` cleanups)
 - rust-lang/rust#144779 (Implement debugging output of the bootstrap Step graph into a DOT file)
 - rust-lang/rust#144813 (Add a tidy check to prevent adding UI tests directly under `tests/ui/`)
 - rust-lang/rust#144817 (Properly reject tail calls to `&FnPtr` or `&FnDef`)
 - rust-lang/rust#144852 (Rename `rust_panic_without_hook` to `resume_unwind` )
 - rust-lang/rust#144866 (Remove `SHOULD_EMIT_LINTS` in favor of `should_emit`)
 - rust-lang/rust#144867 (Use `as_array` in PartialEq for arrays)
 - rust-lang/rust#144872 (Document Poisoning in `LazyCell` and `LazyLock`)
 - rust-lang/rust#144877 (coverage: Various small cleanups)
 - rust-lang/rust#144887 (`rust-analyzer` subtree update)
 - rust-lang/rust#144890 (Add `InterpCx::project_fields`)
 - rust-lang/rust#144894 (Delete `tests/ui/threads-sendsync/tcp-stress.rs`)
 - rust-lang/rust#144905 (rustc-dev-guide subtree update)
 - rust-lang/rust#144920 (Dont print arg span in MIR dump for tail call)

r? `@ghost`
`@rustbot` modify labels: rollup
github-actions bot pushed a commit to rust-lang/rust-analyzer that referenced this pull request Aug 11, 2025
@Zalathar
Rollup merge of #145033 - nnethercote:fix-144994, r=fmease
9fc3f7f 
Reimplement `print_region` in `type_name.rs`.

Broken by rust-lang/rust#144776; this is reachable after all.

Fixes rust-lang/rust#144994.

The commit also adds a lot more cases to the `type-name-basic.rs`, because it's currently very anaemic. This includes some cases where region omission does very badly; these are marked with FIXME.

r? `@fmease`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 13, 2025
@GuillaumeGomez
Rollup merge of rust-lang#144949 - nnethercote:more-Printer-cleanups,…
e8261fb 
… r=davidtwco

More `Printer` cleanups

A sequel to rust-lang#144776.

r? `@davidtwco`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 13, 2025
@GuillaumeGomez
Rollup merge of rust-lang#144949 - nnethercote:more-Printer-cleanups,…
8fa0870 
… r=davidtwco

More `Printer` cleanups

A sequel to rust-lang#144776.

r? ``@davidtwco``
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 13, 2025
@GuillaumeGomez
Rollup merge of rust-lang#144949 - nnethercote:more-Printer-cleanups,…
e7e3a37 
… r=davidtwco

More `Printer` cleanups

A sequel to rust-lang#144776.

r? ```@davidtwco```
rust-timer added a commit that referenced this pull request Aug 13, 2025
@rust-timer
Unrolled build for #144949
fb2fec8 
Rollup merge of #144949 - nnethercote:more-Printer-cleanups, r=davidtwco

More `Printer` cleanups

A sequel to #144776.

r? ```@davidtwco```
github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Aug 18, 2025
@Zalathar
Rollup merge of #145033 - nnethercote:fix-144994, r=fmease
0afc5e0 
Reimplement `print_region` in `type_name.rs`.

Broken by rust-lang/rust#144776; this is reachable after all.

Fixes rust-lang/rust#144994.

The commit also adds a lot more cases to the `type-name-basic.rs`, because it's currently very anaemic. This includes some cases where region omission does very badly; these are marked with FIXME.

r? `@fmease`
@oxalica oxalica mentioned this pull request Aug 21, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@cjgillot cjgillot cjgillot left review comments

Assignees

@davidtwco davidtwco

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
Milestone
1.91.0
Development

Successfully merging this pull request may close these issues.
7 participants
@nnethercote @rustbot @rust-log-analyzer @bors @cjgillot @RalfJung @davidtwco