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 #107712

Closed

Conversation

eggyal
Copy link
Contributor

@eggyal eggyal commented Feb 6, 2023

@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 6, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 6, 2023

Some changes occurred in engine.rs, potentially modifying the public API of ObligationCtxt.

cc @lcnr, @compiler-errors

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured in abstract_const.rs

cc @BoxyUwU

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the core trait solver

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

Comment on lines +891 to +905
pub trait OuterExclusiveBinder {
fn outer_exclusive_binder(self) -> DebruijnIndex;
}

pub trait BoundAtOrAboveBinder {
fn bound_at_or_above_binder(self, index: DebruijnIndex) -> bool;
}

pub trait BoundIndex {
fn bound_index(&self) -> Option<DebruijnIndex>;
}

pub trait Flags {
fn flags(self) -> TypeFlags;
}
Copy link
Contributor Author

@eggyal eggyal Feb 6, 2023

Choose a reason for hiding this comment

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

One alternative to these traits (and the refactorings of HasEscapingVarsVisitor and HasTypeFlagsVisitor that they enable) would be to project those two visitors from the Interner in order that they remain defined/implemented solely for TyCtxt (and so continue using the existing inherent methods).

@@ -336,7 +336,7 @@ impl<'tcx> HasTargetSpec for FunctionCx<'_, '_, 'tcx> {
impl<'tcx> FunctionCx<'_, '_, 'tcx> {
pub(crate) fn monomorphize<T>(&self, value: T) -> T
where
T: TypeFoldable<'tcx> + Copy,
T: TypeFoldable<TyCtxt<'tcx>> + Copy,
Copy link
Member

Choose a reason for hiding this comment

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

Could there be a trait alias for TypeFoldable filling in TyCtxt in rustc_middle? That would significantly reduce the diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't aware of trait aliases! Good idea, will look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay so, whilst a trait alias can be used in bounds such as these, it can't be used to implement the trait: so we'll still need impl<'tcx> TypeFoldable<TyCtxt<'tcx>> for ... followed by ... where T: TypeFoldableAlias<'tcx>—and hence we'd still end up modifying each of these bounds to refer to the alias instead, unless the alias is named TypeFoldable (which I think would be very confusing unless the trait itself is renamed something else?). So the diff will likely remain similar, but the boilerplate would be slightly reduced.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to have a generic TypeFoldable that is almost never used and one that is used everywhere else. We do a similar thing for Ty after all. It's just that type aliases are stable.

Copy link
Contributor Author

@eggyal eggyal Feb 7, 2023

Choose a reason for hiding this comment

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

I created another branch with an alternative implementation using trait aliases, but as suspected the impact on the overall diff is not so significant: +1494-1309 across 99 files versus this PR's +1709-1434 across 112 files.

I'm happy to use/force-push that if it's preferred, but IMHO defining e.g. TypeFoldable to mean different things depending on whether the alias or the underlying trait are brought into scope is pretty confusing (and all the existing reexports certainly don't help to keep track of what each name means in a given context). In particular for each trait, that branch now has at very least the following:

rustc_type_ir::fold::TypeFoldable // (1): actual trait definition

rustc_middle::ty::fold::TypeFoldable // (2): re-export of (1)
rustc_middle::ty::ir::TypeFoldable // (3): re-export of (2)

rustc_middle::ty::fold::ctxt::TypeFoldable // (4): type alias definition
rustc_middle::ty::TypeFoldable // (5): re-export of (4)

And the codebase variously uses each of the above in different and inconsistent ways. Certainly this could (and probably should) be rationalised, but fundamentally I think the naming conflict is unhelpful.

Incidentally, you may note that branch also removes some previously required imports from scope: this is a direct result of using trait aliases, which erroneously bring supertraits into scope (reported #107747). Once that bug is resolved, many of those imports will need to be re-added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! ❤️

Copy link
Contributor Author

@eggyal eggyal Feb 8, 2023

Choose a reason for hiding this comment

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

On reflection, I'm not sure there's any benefit in blanket implementing each (existing) middle trait (parameterised by 'tcx) for all types implementing the respective ir trait (parameterised by TyCtxt<'tcx>), because generic methods in the middle traits (e.g. TypeVisitable::visit_with) are parameterised by a type that must implement some other middle trait (e.g. V: TypeVisitor<'tcx>) and, in delegating to the ir trait, the blanket implementation must pass a value of that generic type to its equivalent method (ir::TypeVisitable::visit_with), which requires that value also to implement the respective ir trait (ir::TypeVisitor<TyCtxt<'tcx>>). However this cannot be guaranteed unless the middle traits are subtraits of the ir traits, thus always requiring that the ir traits be implemented and negating much benefit of retaining the middle traits; any remaining ergonomic benefit is then negated by ambiguities that such supertraits introduce when resolving associated items.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I see. I'm assuming trait aliases won't allow impls TraitAlias for Type, thus leading to the branch you linked that still references TypeVisitable<TyCtxt<'tcx>> almost as often as before.

I'm convinced to reopen this PR, even if it's a bit annoying to have to do that all over the place.

Copy link
Contributor

@oli-obk oli-obk Feb 8, 2023

Choose a reason for hiding this comment

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

Thanks for doing the experiment. Please add this information to the PR so that it's clear why this design was chosen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, you cannot (currently?) impl TraitAlias for ....

Will open a new PR (am looking into some other changes anyway) and provide the detail as suggested. Thanks!

@eggyal
Copy link
Contributor Author

eggyal commented Feb 6, 2023

Having only just read the types team announcement, I'm not sure whether this PR is even desirable anymore? Does it fit into the new plan?

@oli-obk oli-obk added the I-types-nominated Nominated for discussion during a types team meeting. label Feb 6, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Feb 6, 2023

I believe it is orthogonal to any other projects we have. Making the compiler's crates depend less on the TyCtxt directly is always a good thing for reusability and testability of these datastructures. I still nominated it for T-types discussion to give you a definite answer

@bjorn3
Copy link
Member

bjorn3 commented Feb 6, 2023

And even if chalk is gone, rust-analyzer still needs a trait solver. It would be nice if it can reuse the same one rustc will use in the future.

@eggyal eggyal force-pushed the move_fold_visit_traits_to_type_lib branch from 3e5b6f3 to ff87b7a Compare February 7, 2023 11:21
@eggyal
Copy link
Contributor Author

eggyal commented Feb 7, 2023

Force-push to ff87b7a modified some of the intermediate commits, but no change to the final result.

@eggyal eggyal closed this Feb 7, 2023
@eggyal eggyal deleted the move_fold_visit_traits_to_type_lib branch February 7, 2023 13:32
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 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
…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
@jackh726 jackh726 removed the I-types-nominated Nominated for discussion during a types team meeting. label May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants