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

Better error message for attempting to use an associated type as a TypeParamBound #91180

Open
not-my-profile opened this issue Nov 24, 2021 · 5 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@not-my-profile
Copy link
Contributor

not-my-profile commented Nov 24, 2021

Given the following code:

trait A: Self::X {
    type X;
}

The current output is:

error[E0405]: cannot find trait `X` in `Self`
 --> x.rs:1:16
  |
1 | trait A: Self::X {
  |                ^ not found in `Self`

Ideally the output should look like:

error: expected identifier, found keyword `Self`
 --> x.rs:1:16
  |
1 | trait A: Self::X {
  |          ^^^^ expected identifier, found keyword
@not-my-profile not-my-profile added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 24, 2021
@BGR360
Copy link
Contributor

BGR360 commented Nov 28, 2021

@rustbot claim

@BGR360
Copy link
Contributor

BGR360 commented Nov 29, 2021

I looked through the code for a bit, and it's not going to be as trivial a fix as I initially thought. I don't yet know enough to say whether it will require some larger refactor, but here's what I've learned.

The error is occurring at the resolve stage. When rustc_resolve visits the trait item and attempts to resolve the Self::X path, the only lexical scope it searches through is the whole module (i.e., it does not search through the contents of the trait A definition).

I think this is just due to the fact that LateResolutionVisitor is visiting items top-down, and only includes scopes that it has already seen in its search.

The good news is that this isn't some baked-in limitation of the language itself. The following example compiles, demonstrating that the language allows a trait to refer to its own associated types "before" they have been defined:

trait A: Iterator<Item = Self::X> {
    type X;
}

However, the only reason that works is because rustc_resolve is deferring the resolution of Self::X to the typeck stage:

Ok(Some(partial_res)) if source.defer_to_typeck() => {

And sadly, it looks like resolution cannot be deferred for trait paths:

fn defer_to_typeck(self) -> bool {
match self {
PathSource::Type
| PathSource::Expr(..)
| PathSource::Pat
| PathSource::Struct
| PathSource::TupleStruct(..) => true,
PathSource::Trait(_) | PathSource::TraitItem(..) => false,
}
}

So the only way forward that I can think of is to make the LateResolutionVisitor search the trait definition contents when resolving paths. I've no clue (yet) how much code needs to be rewritten to do that.

Would love to hear input from anybody that has any! I'm still relatively new to rustc contribution.

@not-my-profile
Copy link
Contributor Author

not-my-profile commented Nov 29, 2021

Hey Ben, thanks for looking into this!

I think that such a lookahead / deferral might not be necessary. The way I understand it is that there is currently no way that

trait A: Self::X { .. }

will ever compile. Because Self is a reserved keyword: mod Self {} fails with "expected identifier, found keyword Self"

While other keywords can be used as identifiers by using raw identifiers (e.g. mod r#mod {}), that apparently does not work for Self: mod r#Self {} fails to compile with "Self cannot be a raw identifier". I guess that is intentional because having a module called Self would be utterly confusing.

So I think when a trait is expected the resolver should be able to error out just on the basis that Self is used as the first path segment. However I am also not that well-versed with rustc internals, so it would probably be a good idea to have somebody that is comment on that.

@BGR360 BGR360 removed their assignment Nov 29, 2021
@BGR360
Copy link
Contributor

BGR360 commented Nov 29, 2021

So I think when a trait is expected the resolver should be able to error out just on the basis that Self is used as the first path segment

This is a good point, and I agree that it's true. However, it doesn't make the problem of deciding what to say in the error message any easier.

Suppose we make the compiler use the following logic: if the inherited trait path starts with Self::, then say expected trait, found associated type. You'd get the right message for your code example, but you'd get the wrong one for something like this:

trait A: Self::DoesNotExist {
    type X;
}

Well, maybe isn't the wrong error message, but it seems less good than cannot find trait DoesNotExist in A.

I'm now starting to doubt if the proposed improvement is enough of an improvement to justify the work that it would take to make it happen. As it stands, the diagnostic is mostly truthful, even if only by accident (there is indeed no trait named X in Self). If a beginner or some equally confused person gets themselves into this situation, do you think that the message is confusing enough to cause them trouble?

Idk. I've unassigned myself from the issue to leave it up for somebody else should they want to take it on.

@not-my-profile
Copy link
Contributor Author

not-my-profile commented Nov 29, 2021

Yes I think that:

cannot find trait DoesNotExist in Self

is a bad error message because there can be no traits in Self. This has potential to confuse beginners because saying that you couldn't find something implies that it could exist ... because why else would you be looking for it? The current error message makes you think the problem is that something is missing but the actual problem is a fundamental misunderstanding. But you are right that found associated type would also be misleading when there isn't actually such an associated type, so I now think that the better error message would be:

error: expected identifier, found keyword `Self`
 --> x.rs:1:16
  |
1 | trait A: Self::X {
  |          ^^^^ expected identifier, found keyword

(I updated the issue description accordingly)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants