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 inheritance gives unexpected compile error when inherited traits use associated types defined in trait #35237

Closed
Osspial opened this issue Aug 3, 2016 · 11 comments · Fixed by #79209
Labels
A-trait-system Area: Trait system C-bug Category: This is a bug. I-cycle Issue: A query cycle occurred while none was expected T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Osspial
Copy link

Osspial commented Aug 3, 2016

If one defines a trait such that inherited traits involve one of the trait's associated types, the compiler throws a "cyclic reference" error when one does not need to be thrown. Take the following code:

trait Foo: AsRef<Self::Bar> {
    type Bar;
}

which throws this error:

error: unsupported cyclic reference between types/traits detected [--explain E0391]
 --> <anon>:1:18
1 |> trait Foo: AsRef<Self::Bar> {
  |>                  ^^^^^^^^^
note: the cycle begins when computing the supertraits of `Foo`...
note: ...which then again requires computing the supertraits of `Foo`, completing the cycle.

error: aborting due to previous error

This error, while it accurately describes what is happening inside of the compiler, does not actually provide any way to fix the error. Even worse, the situation where the error arises isn't consistent with how associated types work in other areas of the trait syntax.

Making the above code work is fairly simple, although it does unnecessarily clutter the trait definition. The following compiles:

trait Foo: AsRef<<Self as Foo>::Bar> {
    type Bar;
}

However, this is not an immediately obvious fix due to how referencing associated types work in other areas of the trait syntax. In all other areas, the as syntax is unnecessary if the associated type is a member of the trait being implemented, which is true of the Bar type, only requiring the usage of as if the associated type is part of a supertrait. For example, take the following trait Baz with the associated type A:

trait Baz {
    type A;
    fn do_something(&self) -> Self::A;
}

This compiles, as expected. The compiler correctly infers that, because of the lack of as syntax, the programmer is referring to a type belonging to the Baz trait. The programmer would also expect using Self::A to work when defining trait inheritance, although this is currently not the case. However, this should be the case - the compiler should, before throwing the "cyclic reference" error, scan the current trait for the given associated type and only throw the error if a type is not detected, as it does in other areas of trait definitions. The error should also outline the solution to the problem, suggesting that the programmer use the as syntax to specify the supertrait that the associated type is coming from.

Meta

Rust version:

rustc 1.10.0 (cfcb716cf 2016-07-03)
binary: rustc
commit-hash: cfcb716cf0961a7e3a4eceac828d94805cf8140b
commit-date: 2016-07-03
host: x86_64-unknown-linux-gnu
release: 1.10.0

cc @jonathandturner @arielb1

@clarfonthey
Copy link
Contributor

Bumping this to say that I just ran into this problem too. I think that we should at least improve the error to recommend the change because that's not clear at all from the error.

@Mark-Simulacrum Mark-Simulacrum added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 23, 2017
@vitalyd
Copy link

vitalyd commented Jul 12, 2017

@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 25, 2017
@SillyFreak
Copy link

I just ran into this as well. I guess the problem with Self::Bar is that the compiler thinks about what Self could be in a concrete instance, instead of just looking at the trait being defined?

I think this warrants at least a changed error message, if not some cleaner way to "quote" the use of Self in this situation.

@oli-obk oli-obk added the A-diagnostics Area: Messages for errors, warnings, and lints label Jul 3, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jul 3, 2020

cc @rust-lang/wg-diagnostics not sure on the false positive rate on this, but maybe we could just preemptively lint on Self::Foo within supertraits before we even get to HIR.

Alternatively we can try hooking into the cycle error. It already knows it's in the supertrait computation. If y'all agree on doing stuff in cycle errors is the right place to do it, I can extend the query system to allow us to add more notes to the cycle error on a per-query basis.

@estebank
Copy link
Contributor

estebank commented Jul 3, 2020

It feels to me that it should be accepted code, but I think that decision falls under @rust-lang/lang's purview. If they decide against it, then either proposed solution seems fine, with the former looking easiest to implement. Even if we decide to fix this so the code compiles it'd be nice to have a better message with a suggestion in the meantime.

@joshtriplett
Copy link
Member

If we can straightforwardly accept this, I think we should.

@estebank
Copy link
Contributor

estebank commented Jul 5, 2020

I think I just found out a trivial way of making this work for associated types on the base trait. What do we want to do for super traits, like the following case?

trait Foo: AsRef<Self::Bar> {
    type Bar;
}
trait Bar: Foo + AsRef<Self::Bar> {}

I can easily make the first one work, but for the later 1) I don't know how easy it will be to make it work, but probably doable, 2) don't know if we want to make it work when there's a single unambiguous assoc type that can be used from a super trait 3) I would imagine if more than one assoc type that could match we would want to error like we do for assoc fns:

error[E0034]: multiple applicable items in scope
  --> fil12.rs:16:25
   |
16 |     fn bar(&self) {self.foo()}
   |                         ^^^ multiple `foo` found
   |
note: candidate #1 is defined in the trait `Foo`
  --> fil12.rs:9:5
   |
9  |     fn foo(&self);
   |     ^^^^^^^^^^^^^^
note: candidate #2 is defined in the trait `Baz`
  --> fil12.rs:12:5
   |
12 |     fn foo(&self);
   |     ^^^^^^^^^^^^^^
help: disambiguate the associated function for candidate #1
   |
16 |     fn bar(&self) {Foo::foo(&self)}
   |                    ^^^^^^^^^^^^^^^
help: disambiguate the associated function for candidate #2
   |
16 |     fn bar(&self) {Baz::foo(&self)}
   |                    ^^^^^^^^^^^^^^^

@oli-obk
Copy link
Contributor

oli-obk commented Jul 6, 2020

I think it's fine if it just works for the Foo trait declaration but not for the Bar trait declaration. Currently if you use <Self as Bar> in the Bar trait declaration

trait Foo {
    type Bar;
}
trait Bar: Foo + AsRef<<Self as Bar>::Bar> {}

then we get

error[E0576]: cannot find associated type `Bar` in trait `Bar`
 --> src/lib.rs:4:39
  |
4 | trait Bar: Foo + AsRef<<Self as Bar>::Bar> {}
  |                                       ^^^ not found in `Bar`

and keeping this behaviour in a first PR seems totally reasonable to me

@estebank
Copy link
Contributor

estebank commented Jul 7, 2020

I got the behavior I was aiming at last night and just published a cleaned up PR with it.

It works in exactly the way you would expect. The following builds:

trait Foo {
    type Bar;
}
trait Qux: Foo + AsRef<Self::Bar> {}
trait Foo2 {}

trait Qux2: Foo2 + AsRef<Self::Bar> {
    type Bar;
}

and the following fails because of ambiguity, providing a good suggestion:

trait Foo {
    type Bar;
}
trait Qux: Foo + AsRef<Self::Bar> { //~ ERROR ambiguous associated type `Bar` in bounds of `Self`
    type Bar;
}
error[E0221]: ambiguous associated type `Bar` in bounds of `Self`
  --> $DIR/super-trait-referencing-self-name-clash.rs:4:24
   |
LL |     type Bar;
   |     --------- ambiguous `Bar` from `Foo`
LL | }
LL | trait Qux: Foo + AsRef<Self::Bar> {
   |                        ^^^^^^^^^ ambiguous associated type `Bar`
LL |     type Bar;
   |     --------- ambiguous `Bar` from `Qux`
   |
help: use fully qualified syntax to disambiguate
   |
LL | trait Qux: Foo + AsRef<<Self as Qux>::Bar> {
   |                        ^^^^^^^^^^^^^^^^^^
help: use fully qualified syntax to disambiguate
   |
LL | trait Qux: Foo + AsRef<<Self as Foo>::Bar> {
   |                        ^^^^^^^^^^^^^^^^^^

The code has a special case when it recognizes it is converting an hir path written like Self::Foo and uses an evaluation mode that then skips all super traits that reference Self. When doing so, we won't expand those, making the cycle error go away. The only thing that wouldn't work is trying to reference Self::Assoc in a super-trait that has Assoc, that will continue (correctly) emitting a cycle error (although its output could be made a bit nicer).

@joshtriplett I don't know if that PR qualifies as straight-forward :)

@estebank estebank added A-trait-system Area: Trait system and removed A-diagnostics Area: Messages for errors, warnings, and lints labels Jul 7, 2020
estebank added a commit to estebank/rust that referenced this issue Aug 31, 2020
Skip the recursive evaluation of super-traits that reference associated
types on `Self` directly (without using a fully-qualified path) in order
to allow its use.

The following code would previously emit a cycle evaluation error and
will now successfuly compile:

```rust
trait Bar<T> {
    fn foo() -> T;
}
trait Foo: Bar<Self::Qux> {
    type Qux;
}
```

The following will also work:

```rust
trait Bar<T> {
    fn foo() -> T;
}
trait Baz {
    type Qux;
}
trait Foo: Bar<Self::Qux> + Baz {}
```

Fix rust-lang#35237.
@bors bors closed this as completed in 349b3b3 Nov 29, 2020
@tshepang
Copy link
Member

tshepang commented Dec 3, 2020

should this be re-opened, given that the fix got a revert

@oli-obk oli-obk reopened this Dec 3, 2020
@jackh726
Copy link
Member

jackh726 commented Mar 7, 2021

Should have been closed by #80732

@jackh726 jackh726 closed this as completed Mar 7, 2021
@fmease fmease added the I-cycle Issue: A query cycle occurred while none was expected label Jan 27, 2024
@fmease fmease added A-trait-system Area: Trait system and removed A-trait-system Area: Trait system labels Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system C-bug Category: This is a bug. I-cycle Issue: A query cycle occurred while none was expected T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet