Streamline `rustc_driver_impl` pretty-printing. #116619

nnethercote · 2023-10-10T23:41:56Z

This PR simplifies a lot of unnecessary structure in
rustc_driver_impl/src/pretty.rs. It removes some traits and functions,
simplifies some structs, renames some things for increased consistency, and
eliminates some boilerplate code. Overall it cuts more than 150 lines of code.

r? @compiler-errors

nnethercote · 2023-10-10T23:42:22Z

This PR is probably best reviewed one commit at a time. Each commit is a self-contained change, but the progress through the commits is not entirely a straight line; I was feeling my way as I went. Some things in earlier commits are undone in later commits (and the log messages indicate some of these). If I did it again from scratch I would end up with slightly fewer commits, but I don't think that's worth the effort.

bors · 2023-10-11T09:17:51Z

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

nnethercote · 2023-10-11T21:10:22Z

I fixed the conflicts.

compiler-errors

r=me after nits or not

compiler/rustc_driver_impl/src/pretty.rs

- Rename `pprust` as `pprust_ast`, to align with `pprust_hir`. - Rename `PrinterSupport` as `AstPrinterSupport`, to align with `HirPrinterSupport`.

The callback is trivial and no pp support is actually needed. This makes the `HirTree` case more like the `AstTree` case above.

`phase_3_run_analysis_passes` no longer exists, and AFAICT this code has been refactored so much since this comment was written that it no longer has any useful meaning.

It's simpler to distinguish the two AST modes directly in `PpMode`.

First, both `AstPrinterSupport` and `HirPrinterSupport` have a `sess` method. This commit introduces a `Sess` trait and makes the support traits be subtraits of `Sess`, to avoid some duplication. Second, both support traits have a `pp_ann` method that isn't needed if we enable `trait_upcasting`. This commit removes those methods. (Both of these traits will be removed in a subsequent commit, as will the `trait_upcasting` use.)

The handling of the `PpMode` variants is currently spread across three functions: `print_after_parsing`, `print_after_hir_lowering`, and `print_with_analysis`. Each one handles some of the variants. This split is primarily because `print_after_parsing` has slightly different arguments to the other two. This commit changes the structure. It merges the three functions into a single `print` function, and encapsulates the different arguments in a new enum `PrintExtra`. Benefits: - The code is a little shorter. - All the `PpMode` variants are handled in a single `match`, with no need for `unreachable!` arms. - It enables the trait removal in the subsequent commit by reducing the number of `call_with_pp_support_ast` call sites from two to one.

`call_with_pp_support_ast` and `call_with_pp_support_hir` how each have a single call site. This commit inlines and removes them, which also removes the need for all the supporting traits: `Sess`, `AstPrinterSupport`, and `HirPrinterSupport`. The `sess` member is also removed from several structs.

Because they all end up within a `TyCtxt`.

This avoids the need for a bespoke `tcx.analysis()` call.

`NoAnn` and `IdentifiedAnnotation` impl both `pprust_ast::PpAnn` and `pprust_hir::PpAnn`, which is a bit confusing, because the optional `tcx` is only needed for the HIR cases. (Currently the `tcx` is unnecessarily provided in the `expanded` AST cases.) This commit splits each one into `Ast` and `Hir` versions, which makes things clear about where the `tcx` is needed. The commit also renames all the traits so they consistently end with `Ann`.

nnethercote · 2023-10-12T19:50:52Z

@bors r=compiler-errors

bors · 2023-10-12T19:50:54Z

📌 Commit 2b4c338 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-10-13T05:35:32Z

⌛ Testing commit 2b4c338 with merge 2763ca5...

bors · 2023-10-13T07:27:40Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 2763ca5 to master...

rust-timer · 2023-10-13T08:48:43Z

Finished benchmarking commit (2763ca5): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.5%	[4.5%, 4.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.1%, -3.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 628.344s -> 627.545s (-0.13%)
Artifact size: 271.27 MiB -> 271.33 MiB (0.02%)

50: Automated pull from upstream `master` r=Dajamante a=github-actions[bot] This PR pulls the following changes from the upstream repository: * rust-lang/rust#116619 * rust-lang/rust#115964 * rust-lang/rust#116391 * rust-lang/rust#116510 * rust-lang/rust#116671 * rust-lang/rust#116669 * rust-lang/rust#116654 * rust-lang/rust#116642 * rust-lang/rust#116625 * rust-lang/rust#116593 * rust-lang/rust#116649 * rust-lang/rust#116600 * rust-lang/rust#116628 Co-authored-by: Nadrieril <[email protected]> Co-authored-by: Scott McMurray <[email protected]> Co-authored-by: bjorn3 <[email protected]> Co-authored-by: Nicholas Nethercote <[email protected]> Co-authored-by: Trevor Gross <[email protected]> Co-authored-by: Georg Semmler <[email protected]> Co-authored-by: Guillaume Gomez <[email protected]> Co-authored-by: Gurinder Singh <[email protected]> Co-authored-by: bors <[email protected]>

rustbot assigned compiler-errors Oct 10, 2023

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 Oct 10, 2023

nnethercote force-pushed the rustc_driver_impl branch from c096758 to d1e3dc6 Compare October 11, 2023 21:10

compiler-errors approved these changes Oct 12, 2023

View reviewed changes

compiler/rustc_driver_impl/src/pretty.rs Show resolved Hide resolved

compiler/rustc_driver_impl/src/pretty.rs Outdated Show resolved Hide resolved

compiler-errors 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 12, 2023

nnethercote added 12 commits October 13, 2023 06:20

Rename some things.

ef8701a

- Rename `pprust` as `pprust_ast`, to align with `pprust_hir`. - Rename `PrinterSupport` as `AstPrinterSupport`, to align with `HirPrinterSupport`.

Remove unnecessary call to call_with_pp_support_hir.

c5cfcdc

The callback is trivial and no pp support is actually needed. This makes the `HirTree` case more like the `AstTree` case above.

Remove an outdated comment.

87090a9

`phase_3_run_analysis_passes` no longer exists, and AFAICT this code has been refactored so much since this comment was written that it no longer has any useful meaning.

Remove PpAstTreeMode.

1467ba0

It's simpler to distinguish the two AST modes directly in `PpMode`.

Remove unused PrinterSupport::hir_map method.

d5e7c5f

Rename some 'hir lifetimes as 'tcx.

ba58e32

Because they all end up within a `TyCtxt`.

Make needs_analysis true for PpHirMode::Typed.

b65227a

This avoids the need for a bespoke `tcx.analysis()` call.

Remove unneeded pubs.

2b4c338

nnethercote force-pushed the rustc_driver_impl branch from d1e3dc6 to 2b4c338 Compare October 12, 2023 19:49

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 Oct 12, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 13, 2023

bors merged commit 2763ca5 into rust-lang:master Oct 13, 2023
11 checks passed

rustbot added this to the 1.75.0 milestone Oct 13, 2023

github-actions bot mentioned this pull request Oct 13, 2023

Automated pull from upstream master ferrocene/ferrocene#50

Merged

nnethercote deleted the rustc_driver_impl branch October 15, 2023 21:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streamline `rustc_driver_impl` pretty-printing. #116619

Streamline `rustc_driver_impl` pretty-printing. #116619

nnethercote commented Oct 10, 2023

nnethercote commented Oct 10, 2023 •

edited

Loading

bors commented Oct 11, 2023

nnethercote commented Oct 11, 2023

compiler-errors left a comment

nnethercote commented Oct 12, 2023

bors commented Oct 12, 2023

bors commented Oct 13, 2023

bors commented Oct 13, 2023

rust-timer commented Oct 13, 2023

Streamline rustc_driver_impl pretty-printing. #116619

Streamline rustc_driver_impl pretty-printing. #116619

Conversation

nnethercote commented Oct 10, 2023

nnethercote commented Oct 10, 2023 • edited Loading

bors commented Oct 11, 2023

nnethercote commented Oct 11, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

nnethercote commented Oct 12, 2023

bors commented Oct 12, 2023

bors commented Oct 13, 2023

bors commented Oct 13, 2023

rust-timer commented Oct 13, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Streamline `rustc_driver_impl` pretty-printing. #116619

Streamline `rustc_driver_impl` pretty-printing. #116619

nnethercote commented Oct 10, 2023 •

edited

Loading