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

Trait aliases should not bring supertraits into scope #107747

Closed
eggyal opened this issue Feb 7, 2023 · 2 comments · Fixed by #107803
Closed

Trait aliases should not bring supertraits into scope #107747

eggyal opened this issue Feb 7, 2023 · 2 comments · Fixed by #107803
Assignees
Labels
A-resolve Area: Name resolution A-trait-system Area: Trait system C-bug Category: This is a bug. F-trait_alias `#![feature(trait_alias)]`

Comments

@eggyal
Copy link
Contributor

eggyal commented Feb 7, 2023

In-scope trait aliases bring into scope (methods of) the aliased trait and all of its supertraits. I can understand the rationale for bringing (methods of) the aliased trait into scope (namely to fix #56485), but also bringing into scope (methods of) that trait's supertraits is both inconsistent (compared with bringing the aliased trait itself into scope) and surprising:

#![feature(trait_alias)]

use std::fmt;

trait Foo: fmt::Debug {}

// uncommenting the following line brings Debug into scope, which
// in turn causes the (unrelated) Display impl below to become ambiguous

// trait Bar = Foo;

#[derive(Debug)]
struct Qux(bool);

impl fmt::Display for Qux {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        self.0.fmt(f)
    }
}

Playground.

(For the avoidance of doubt, only the methods are brought into scope—not the trait itself: thus, in the above example with the Bar alias uncommented, one cannot use an unqualified Debug).

cc #41517

@rustbot label A-resolve A-traits C-bug F-trait_alias

@rustbot rustbot added A-resolve Area: Name resolution A-trait-system Area: Trait system C-bug Category: This is a bug. F-trait_alias `#![feature(trait_alias)]` labels Feb 7, 2023
@eggyal
Copy link
Contributor Author

eggyal commented Feb 7, 2023

From cursory inspection, the following (originally from #59166, cc @seanmonstar) looks suspect:

// For trait aliases, assume all supertraits are relevant.
let bounds = iter::once(ty::Binder::dummy(trait_ref));
self.elaborate_bounds(bounds, |this, new_trait_ref, item| {
let new_trait_ref = this.erase_late_bound_regions(new_trait_ref);
let (xform_self_ty, xform_ret_ty) =
this.xform_self_ty(&item, new_trait_ref.self_ty(), new_trait_ref.substs);
this.push_candidate(
Candidate {
xform_self_ty,
xform_ret_ty,
item,
import_ids: import_ids.clone(),
kind: TraitCandidate(new_trait_ref),
},
false,
);
});

@eggyal
Copy link
Contributor Author

eggyal commented Feb 8, 2023

@rustbot claim

@bors bors closed this as completed in 16a4138 Feb 9, 2023
eggyal added a commit to eggyal/rust that referenced this issue Feb 13, 2023
Only required until fix rust-lang#107803 is merged into stage0 compiler, expected
when beta 1.69.0 is released on 2023-03-09, then this commit can be
reverted.
bors added a commit to rust-lang-ci/rust that referenced this issue 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
bors added a commit to rust-lang-ci/rust that referenced this issue 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 issue 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 issue 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 issue 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 issue 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
A-resolve Area: Name resolution A-trait-system Area: Trait system C-bug Category: This is a bug. F-trait_alias `#![feature(trait_alias)]`
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants