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

Move folding & visiting traits into type library #107924

Merged

Conversation

eggyal
Copy link
Contributor

@eggyal eggyal commented Feb 11, 2023

This is a rework of #107712, following feedback on that PR.

In particular, this version uses trait aliases to reduce the API churn for trait consumers. Doing so requires a workaround for #107747 until its fix in #107803 is merged into the stage0 compiler; this workaround, which uses conditional compilation based on the bootstrap configuration predicate, sits in dedicated commit b409329 for ease of reversion.

The possibility of the rustc_middle crate retaining its own distinct versions of each folding/visiting trait, blanket-implemented on all types that implement the respective trait in the type library, was also explored: however since this would necessitate making each rustc_middle trait a subtrait of the respective type library trait (so that such blanket implementations can delegate their generic methods), no benefit would be gained.

r? types

@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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Feb 11, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 11, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occured in abstract_const.rs

cc @BoxyUwU

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@eggyal
Copy link
Contributor Author

eggyal commented Feb 11, 2023

I should add that, in contrast to the previous PR, this version splits methods from the TypeVisitable trait into an extension trait that is retained solely in rustc_middle.

@eggyal eggyal force-pushed the move_fold_visit_traits_to_type_lib_with_trait_alias branch from 456ad1f to 3915c4e Compare February 11, 2023 11:17
@jackh726
Copy link
Member

r? @jackh726

@rustbot rustbot assigned jackh726 and unassigned spastorino Feb 11, 2023
@bors
Copy link
Contributor

bors commented Feb 13, 2023

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

@eggyal eggyal force-pushed the move_fold_visit_traits_to_type_lib_with_trait_alias branch from 3915c4e to 63ad5d0 Compare February 13, 2023 10:29
@oli-obk
Copy link
Contributor

oli-obk commented Feb 13, 2023

@bors r+ p=5 (extremely bitrotty)

@bors
Copy link
Contributor

bors commented Feb 13, 2023

📌 Commit 63ad5d0 has been approved by oli-obk

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 Feb 13, 2023
@bors
Copy link
Contributor

bors commented Feb 13, 2023

⌛ Testing commit 63ad5d0 with merge a3c9eed...

@bors
Copy link
Contributor

bors commented Feb 13, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing a3c9eed to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a3c9eed): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.5% [0.3%, 0.6%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

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

Cycles

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

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 22, 2023
…s, r=oli-obk

Remove type-traversal trait aliases

rust-lang#107924 moved the type traversal (folding and visiting) traits into the type library, but created trait aliases in `rustc_middle` to minimise both the API churn for trait consumers and the arising boilerplate.  As mentioned in that PR, an alternative approach of defining subtraits with blanket implementations of the respective supertraits was also considered at that time but was ruled out as not adding much value.

Unfortunately, it has since emerged that rust-analyzer has difficulty with these trait aliases at present, resulting in a degraded contributor experience (see the recent [r-a has become useless](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/r-a.20has.20become.20useless) topic on the #t-compiler/help Zulip stream).

This PR removes the trait aliases, and accordingly the underlying type library traits are now used directly; they are parameterised by `TyCtxt<'tcx>` rather than just the `'tcx` lifetime, and imports have been updated to reflect the fact that the trait aliases' explicitly named traits are no longer automatically brought into scope.  These changes also roll-back the (no-longer required) workarounds to rust-lang#107747 that were made in b409329.

Since this PR is just a find+replace together with the changes necessary for compilation & tidy to pass, it's currently just one mega-commit.  Let me know if you'd like it broken up.

r? `@oli-obk`
Jarcho pushed a commit to Jarcho/rust that referenced this pull request Feb 26, 2023
…e_lib_with_trait_alias, r=oli-obk

Move folding & visiting traits into type library

This is a rework of rust-lang#107712, following feedback on that PR.

In particular, this version uses trait aliases to reduce the API churn for trait consumers.  Doing so requires a workaround for rust-lang#107747 until its fix in rust-lang#107803 is merged into the stage0 compiler; this workaround, which uses conditional compilation based on the `bootstrap` configuration predicate, sits in dedicated commit b409329 for ease of reversion.

The possibility of the `rustc_middle` crate retaining its own distinct versions of each folding/visiting trait, blanket-implemented on all types that implement the respective trait in the type library, was also explored: however since this would necessitate making each `rustc_middle` trait a subtrait of the respective type library trait (so that such blanket implementations can delegate their generic methods), no benefit would be gained.

r? types
Jarcho pushed a commit to Jarcho/rust that referenced this pull request Feb 26, 2023
…s, r=oli-obk

Remove type-traversal trait aliases

rust-lang#107924 moved the type traversal (folding and visiting) traits into the type library, but created trait aliases in `rustc_middle` to minimise both the API churn for trait consumers and the arising boilerplate.  As mentioned in that PR, an alternative approach of defining subtraits with blanket implementations of the respective supertraits was also considered at that time but was ruled out as not adding much value.

Unfortunately, it has since emerged that rust-analyzer has difficulty with these trait aliases at present, resulting in a degraded contributor experience (see the recent [r-a has become useless](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/r-a.20has.20become.20useless) topic on the #t-compiler/help Zulip stream).

This PR removes the trait aliases, and accordingly the underlying type library traits are now used directly; they are parameterised by `TyCtxt<'tcx>` rather than just the `'tcx` lifetime, and imports have been updated to reflect the fact that the trait aliases' explicitly named traits are no longer automatically brought into scope.  These changes also roll-back the (no-longer required) workarounds to rust-lang#107747 that were made in b409329.

Since this PR is just a find+replace together with the changes necessary for compilation & tidy to pass, it's currently just one mega-commit.  Let me know if you'd like it broken up.

r? `@oli-obk`
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Mar 15, 2023
…s, r=oli-obk

Remove type-traversal trait aliases

rust-lang#107924 moved the type traversal (folding and visiting) traits into the type library, but created trait aliases in `rustc_middle` to minimise both the API churn for trait consumers and the arising boilerplate.  As mentioned in that PR, an alternative approach of defining subtraits with blanket implementations of the respective supertraits was also considered at that time but was ruled out as not adding much value.

Unfortunately, it has since emerged that rust-analyzer has difficulty with these trait aliases at present, resulting in a degraded contributor experience (see the recent [r-a has become useless](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/r-a.20has.20become.20useless) topic on the #t-compiler/help Zulip stream).

This PR removes the trait aliases, and accordingly the underlying type library traits are now used directly; they are parameterised by `TyCtxt<'tcx>` rather than just the `'tcx` lifetime, and imports have been updated to reflect the fact that the trait aliases' explicitly named traits are no longer automatically brought into scope.  These changes also roll-back the (no-longer required) workarounds to rust-lang#107747 that were made in b409329.

Since this PR is just a find+replace together with the changes necessary for compilation & tidy to pass, it's currently just one mega-commit.  Let me know if you'd like it broken up.

r? `@oli-obk`
antoyo pushed a commit to antoyo/rust that referenced this pull request Jun 19, 2023
…s, r=oli-obk

Remove type-traversal trait aliases

rust-lang#107924 moved the type traversal (folding and visiting) traits into the type library, but created trait aliases in `rustc_middle` to minimise both the API churn for trait consumers and the arising boilerplate.  As mentioned in that PR, an alternative approach of defining subtraits with blanket implementations of the respective supertraits was also considered at that time but was ruled out as not adding much value.

Unfortunately, it has since emerged that rust-analyzer has difficulty with these trait aliases at present, resulting in a degraded contributor experience (see the recent [r-a has become useless](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/r-a.20has.20become.20useless) topic on the #t-compiler/help Zulip stream).

This PR removes the trait aliases, and accordingly the underlying type library traits are now used directly; they are parameterised by `TyCtxt<'tcx>` rather than just the `'tcx` lifetime, and imports have been updated to reflect the fact that the trait aliases' explicitly named traits are no longer automatically brought into scope.  These changes also roll-back the (no-longer required) workarounds to rust-lang#107747 that were made in b409329.

Since this PR is just a find+replace together with the changes necessary for compilation & tidy to pass, it's currently just one mega-commit.  Let me know if you'd like it broken up.

r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants