Refactor AST trait bound modifiers #119163

fmease · 2023-12-20T14:24:43Z

Instead of having two types to represent trait bound modifiers in the parser / the AST (parser::ty::BoundModifiers & ast::TraitBoundModifier), only to map one to the other later, just use parser::ty::BoundModifiers (moved & renamed to ast::TraitBoundModifiers).

The struct type is more extensible and easier to deal with (see here and here for context) since it more closely models what it represents: A compound of two kinds of modifiers, constness and polarity. Modeling this as an enum (the now removed ast::TraitBoundModifier) meant one had to add a new variant per combination of modifier kind, which simply isn't scalable and which lead to a lot of explicit non-DRY matches.

NB: hir::TraitBoundModifier being an enum is fine since HIR doesn't need to worry representing invalid modifier kind combinations as those get rejected during AST validation thereby immensely cutting down the number of possibilities.

fmease · 2023-12-20T14:25:22Z

x64 size_of(ast::GenericBound) 64→72

@bors try @rust-timer queue

bors · 2023-12-20T14:26:32Z

⌛ Trying commit 724dc1d with merge af09d7b...

…fiers, r=<try> [perf] Refactor AST trait bound modifiers Context: https://github.com/rust-lang/rust/pull/119099/files#r1430749981, https://github.com/rust-lang/rust/pull/119099/files#r1430752116. r? `@ghost`

bors · 2023-12-20T15:53:26Z

☀️ Try build successful - checks-actions
Build commit: af09d7b (af09d7b63401dfa2221e508a51292b0b226f8dca)

rust-timer · 2023-12-20T17:14:05Z

Finished benchmarking commit (af09d7b): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -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.8%	[0.5%, 1.1%]	2
Regressions ❌ (secondary)	2.6%	[1.5%, 3.8%]	2
Improvements ✅ (primary)	-3.2%	[-3.2%, -3.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-3.2%, 1.1%]	3

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)	3.6%	[3.6%, 3.6%]	1
Improvements ✅ (primary)	-0.6%	[-0.8%, -0.4%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-0.8%, -0.4%]	2

Binary size

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.0%	[0.0%, 0.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 673.565s -> 673.39s (-0.03%)
Artifact size: 312.83 MiB -> 312.83 MiB (-0.00%)

fmease · 2023-12-20T17:25:51Z

Fixing some things up, making some matches exhaustive and writing some review comments, then this should be ready.

compiler/rustc_ast/src/ast.rs

fmease · 2023-12-20T18:45:18Z

compiler/rustc_ast_lowering/src/lib.rs

@@ -1425,19 +1425,16 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                        this.arena.alloc_from_iter(bounds.iter().filter_map(|bound| match bound {
                            GenericBound::Trait(
                                ty,
-                                modifier @ (TraitBoundModifier::None


Negligible behavior change in the error path: We now lower_poly_trait_ref for ~const ! (master: TBM::MaybeConstNegative) instead of mapping it to None. We now only map ? & ~const ? to None which seems more logical. In any case, ~const ! gets rejected in AST validation, shouldn't have any user-facing effect.

fmease · 2023-12-20T18:45:51Z

src/tools/rustfmt/src/types.rs

@@ -537,28 +537,19 @@ impl Rewrite for ast::Lifetime {
 impl Rewrite for ast::GenericBound {
    fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option<String> {
        match *self {
-            ast::GenericBound::Trait(ref poly_trait_ref, trait_bound_modifier) => {
+            ast::GenericBound::Trait(ref poly_trait_ref, modifiers) => {


The rustfmt changes might be a bit controversial…

@fmease I know this has already been merged, but please avoid making unnecessary changes to subtrees like rustfmt. It would have been more appropriate to open a PR for this directly in rustfmt. I would have asked for this to be reverted had I seen it earlier but given the holiday season this one slipped through.

cc: @compiler-errors @calebcartwright

Sorry, but I don't understand what you mean by "unnecessary changes" in this particular case. I'm not sure how this PR could've been done without modifying rustfmt -- that's just part of the reality of rustfmt using data structures from rustc_ast.

I'm certain @fmease or I will be happy to assist with a subtree sync if this ends up causing conflicts the next time you perform one.

Maybe I don't fully understand the AST changes, but could the diff have been minimized? It seems like a lot larger of a diff than most other AST modifications.

I don't believe this could be minimized in a way that doesn't affect code quality. This PR completely replaces the TraitBoundModifier enum.

What's the concern here? Synchronization conflicts? Do you expect this code to have been modified on the rustfmt side?

I appreciate you offering to help out with the sync if there are issue. I'm trying to get a subtree push done and this change jumped out as odd to me. It's not causing any issues, but it just looked like a much larger change than I would have expected. If this was the only way to rewrite the code then that's fair.

I'm glad it's not causing you too much trouble. While this diff might seem large, the point of this refactoring PR was to eliminate boilerplate like the one in rustfmt where one had to explicitly match on each kind to obtain a textual representation.

Thanks for reaching out! I won't make a habit of making such kind of changes to rustfmt from within the rust repo ;)

In case you do need any help with the sync or something similar, I'm definitely available and happy to help :)

Thanks for the clarification. I think I was quick to judge this as an unnecessary change. After taking another look the changes seem fine. Very much appreciate you offering to help in case there is an issue 🙏🏼

rustbot · 2023-12-20T18:57:03Z

Changes to the size of AST and/or HIR nodes.

cc @nnethercote

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

fmease · 2023-12-20T18:57:37Z

r? compiler-errors or compiler depending on your current workload.

compiler-errors

one comment

compiler-errors · 2023-12-20T19:18:46Z

compiler/rustc_ast_pretty/src/pprust/state/item.rs

-                for b in bounds.iter() {
-                    if let GenericBound::Trait(ptr, ast::TraitBoundModifier::Maybe) = b {
+                for bound in bounds.iter() {
+                    if let GenericBound::Trait(ptr, modifiers) = bound


why is this arm special?

it's old syntactic cruft, I'm already in the process of writing a PR for removing this & other occurrences of it in the HIR pretty-printer lol

compiler-errors · 2023-12-20T19:21:56Z

@bors r+

bors · 2023-12-20T19:22:00Z

📌 Commit 5e4f12b has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-12-20T20:02:59Z

⌛ Testing commit 5e4f12b with merge a4aae8c...

…fiers, r=compiler-errors Refactor AST trait bound modifiers Instead of having two types to represent trait bound modifiers in the parser & the AST (`parser::ty::BoundModifiers` & `ast::TraitBoundModifier`), only to map one to the other later, just use `parser::ty::BoundModifiers` (moved & renamed to `ast::TraitBoundModifiers`). The struct type is more extensible and easier to deal with (see [here](https://github.com/rust-lang/rust/pull/119099/files#r1430749981) and [here](https://github.com/rust-lang/rust/pull/119099/files#r1430752116) for context) since it more closely models what it represents: A compound of two kinds of modifiers, constness and polarity. Modeling this as an enum (the now removed `ast::TraitBoundModifier`) meant one had to add a new variant per *combination* of modifier kind, which simply isn't scalable and which lead to a lot of explicit non-DRY matches. NB: `hir::TraitBoundModifier` being an enum is fine since HIR doesn't need to worry representing invalid modifier combinations as those get rejected during AST validation thereby immensely cutting down the number of possibilities.

bors · 2023-12-21T00:18:00Z

💥 Test timed out

rust-log-analyzer · 2023-12-21T01:43:47Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

fmease · 2023-12-21T07:30:47Z

Huh?
@bors retry

bors · 2023-12-22T02:00:58Z

⌛ Testing commit 5e4f12b with merge aaef5fe...

bors · 2023-12-22T04:06:34Z

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

rust-timer · 2023-12-22T05:28:48Z

Finished benchmarking commit (aaef5fe): 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)	1.3%	[1.0%, 1.5%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-0.8%	[-0.8%, -0.8%]	1
All ❌✅ (primary)	0.5%	[-1.0%, 1.5%]	3

Cycles

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

Binary size

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.0%	[0.0%, 0.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 672.574s -> 673.306s (0.11%)
Artifact size: 312.78 MiB -> 312.81 MiB (0.01%)

…, r=compiler-errors Rid the AST & HIR pretty printer of cruft Found while working on rust-lang#119163. For `trait Trait: ?Sized {}` (semantically malformed), we currently output `trait Trait for ? Sized {}` (sic!) / `trait Trait for ? Sized { }` (sic!) if `-Zunpretty=expanded` / `-Zunpretty=hir` is passed. `trait Tr for Sized? {}` (rust-lang#15521) and later also `trait Tr for ?Sized {}` (I guess, rust-lang#20194) is former Rust syntax. Hence I'm removing these outdated branches. ~~This will conflict with rust-lang#119163, therefore marking this PR as blocked.~~ Rebased

Rollup merge of rust-lang#119169 - fmease:pretty-yeet-syntactic-cruft, r=compiler-errors Rid the AST & HIR pretty printer of cruft Found while working on rust-lang#119163. For `trait Trait: ?Sized {}` (semantically malformed), we currently output `trait Trait for ? Sized {}` (sic!) / `trait Trait for ? Sized { }` (sic!) if `-Zunpretty=expanded` / `-Zunpretty=hir` is passed. `trait Tr for Sized? {}` (rust-lang#15521) and later also `trait Tr for ?Sized {}` (I guess, rust-lang#20194) is former Rust syntax. Hence I'm removing these outdated branches. ~~This will conflict with rust-lang#119163, therefore marking this PR as blocked.~~ Rebased

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 Dec 20, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 20, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 20, 2023

Refactor AST trait bound modifiers

5e4f12b

fmease force-pushed the refactor-ast-trait-bound-modifiers branch from 724dc1d to 5e4f12b Compare December 20, 2023 18:40

fmease commented Dec 20, 2023

View reviewed changes

fmease changed the title ~~[perf] Refactor AST trait bound modifiers~~ Refactor AST trait bound modifiers Dec 20, 2023

This comment was marked as resolved.

Sign in to view

fmease marked this pull request as ready for review December 20, 2023 18:57

rustbot assigned compiler-errors Dec 20, 2023

fmease added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Dec 20, 2023

compiler-errors approved these changes Dec 20, 2023

View reviewed changes

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 Dec 20, 2023

fmease mentioned this pull request Dec 20, 2023

Rid the AST & HIR pretty printer of cruft #119169

Merged

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 21, 2023

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 Dec 21, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 22, 2023

bors merged commit aaef5fe into rust-lang:master Dec 22, 2023
12 checks passed

rustbot added this to the 1.77.0 milestone Dec 22, 2023

This was referenced Dec 22, 2023

Introduce const Trait (always-const trait bounds) #119099

Merged

Remove DiagCtxt API duplication #119146

Merged

fmease deleted the refactor-ast-trait-bound-modifiers branch December 22, 2023 08:00

ytmimi mentioned this pull request Dec 28, 2023

Subtree push with 1.77.0 nightly 2023-12-28 rust-lang/rustfmt#5994

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor AST trait bound modifiers #119163

Refactor AST trait bound modifiers #119163

fmease commented Dec 20, 2023 •

edited

fmease commented Dec 20, 2023

This comment has been minimized.

bors commented Dec 20, 2023

This comment has been minimized.

bors commented Dec 20, 2023

This comment has been minimized.

rust-timer commented Dec 20, 2023

fmease commented Dec 20, 2023

fmease Dec 20, 2023

fmease Dec 20, 2023

ytmimi Dec 28, 2023

compiler-errors Dec 28, 2023

compiler-errors Dec 28, 2023

ytmimi Dec 28, 2023

compiler-errors Dec 28, 2023

ytmimi Dec 28, 2023 •

edited

fmease Dec 28, 2023 •

edited

ytmimi Dec 29, 2023

This comment was marked as resolved.

rustbot commented Dec 20, 2023

fmease commented Dec 20, 2023

compiler-errors left a comment

compiler-errors Dec 20, 2023

fmease Dec 20, 2023 •

edited

compiler-errors commented Dec 20, 2023

bors commented Dec 20, 2023

bors commented Dec 20, 2023

bors commented Dec 21, 2023

rust-log-analyzer commented Dec 21, 2023

fmease commented Dec 21, 2023

bors commented Dec 22, 2023

bors commented Dec 22, 2023

rust-timer commented Dec 22, 2023

Refactor AST trait bound modifiers #119163

Refactor AST trait bound modifiers #119163

Conversation

fmease commented Dec 20, 2023 • edited

fmease commented Dec 20, 2023

This comment has been minimized.

bors commented Dec 20, 2023

This comment has been minimized.

bors commented Dec 20, 2023

This comment has been minimized.

rust-timer commented Dec 20, 2023

Overall result: no relevant changes - no action needed

fmease commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytmimi Dec 28, 2023 • edited

Choose a reason for hiding this comment

fmease Dec 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

rustbot commented Dec 20, 2023

fmease commented Dec 20, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease Dec 20, 2023 • edited

Choose a reason for hiding this comment

compiler-errors commented Dec 20, 2023

bors commented Dec 20, 2023

bors commented Dec 20, 2023

bors commented Dec 21, 2023

rust-log-analyzer commented Dec 21, 2023

fmease commented Dec 21, 2023

bors commented Dec 22, 2023

bors commented Dec 22, 2023

rust-timer commented Dec 22, 2023

Overall result: no relevant changes - no action needed

fmease commented Dec 20, 2023 •

edited

ytmimi Dec 28, 2023 •

edited

fmease Dec 28, 2023 •

edited

fmease Dec 20, 2023 •

edited