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

Confusing error message associated with universe transition: "one type is more general than the other" #57362

Closed
dtolnay opened this issue Jan 5, 2019 · 5 comments · Fixed by #57901
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@dtolnay
Copy link
Member

dtolnay commented Jan 5, 2019

trait Trait {
    fn f(self);
}

impl<T> Trait for fn(&T) {
    fn f(self) {
        println!("f");
    }
}

fn main() {
    let a: fn(_) = |_: &u8| {};
    a.f();
}

Prior to #56105, the error message was:

error[E0599]: no method named `f` found for type `fn(&u8)` in the current scope
  --> src/main.rs:13:7
   |
13 |     a.f();
   |       ^
   |
   = note: a is a function, perhaps you wish to call it
   = help: items from traits can only be used if the trait is implemented and in scope
   = note: the following trait defines an item `f`, perhaps you need to implement it:
           candidate #1: `Trait`

i.e. the trait is implemented only for for<'a> fn(&'a u8) and not for a concrete fn(&'x u8).

The new error message is:

error[E0308]: mismatched types
  --> src/main.rs:13:7
   |
13 |     a.f();
   |       ^ one type is more general than the other
   |
   = note: expected type `Trait`
              found type `Trait`

I don't know what this message is trying to say.

Mentioning @nikomatsakis and @scalexm who worked on universes.

@estebank estebank added the A-diagnostics Area: Messages for errors, warnings, and lints label Jan 6, 2019
@nikomatsakis nikomatsakis added P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Jan 7, 2019
@nikomatsakis nikomatsakis self-assigned this Jan 7, 2019
@nikomatsakis
Copy link
Contributor

Neither message seems very good, but yeah the new one isn't working as intended. Assigning to myself since this is a diagnostic regression caused by my PR.

@alexcrichton alexcrichton added this to Triaged in 1.33 regressions via automation Jan 7, 2019
@nikomatsakis
Copy link
Contributor

OK so I spent some time investigating this, but I failed to leave some notes. Here goes.

First off, I believe the relevant part of this error is generated by the note_type_err function:

pub fn note_type_err(
&self,
diag: &mut DiagnosticBuilder<'tcx>,
cause: &ObligationCause<'tcx>,
secondary_span: Option<(Span, String)>,
mut values: Option<ValuePairs<'tcx>>,
terr: &TypeError<'tcx>,
) {

In this case, the values parameter (which stores the two values that were being equated) will be a ValuePairs::TraitRefs (or maybe PolyTraitRefs) instance. I think that means that, in terms of these variables here:

let (expected_found, exp_found, is_simple_error) = match values {

we will wind up with is_simple_err being false and exp_found being None. The rather similarly named expected_found, however, will be Some(("Trait", "Trait")) (it is initialized here).

Part of the problem here is that the "to-string" for a trait-reference excludes the self type, which is a sort of annoying thing. (I've refactored this like 3 times but never landed the branches, dang it.) This is partly because we tend to prefer to print things like "Foo implemented for Bar" instead of Bar: Foo.

Finally I think we wind up invoking note_expected_found on line 1005.

I think we want a message more like (not great, but similar to what we had before):

error[E0308]: mismatched types
  --> src/main.rs:13:7
   |
13 |     a.f();
   |       ^ one type is more general than the other
   |
   = note: expected `Trait` to be implemented for `for<'r> &'r u8`
              found `Trait` implemented for `&u8`

To get that, we probably need to remember what kind of thing was being compared in values (types, trait-references, etc) and thread that information into note_trait_ref, but we also need to include not just the display of the trait-ref, but the self-type too.

@lqd lqd self-assigned this Jan 16, 2019
@nikomatsakis
Copy link
Contributor

@lqd and I have been digging into this. We've made some changes so that this message at least goes through the new specific "placeholder error" mechanism, but it is still rather confusing:

// error: implementation of `Trait` is not general enough
//   --> src/main.rs:13:7
//    |
// 13 |     a.f();
//    |       ^
//    |
//    = note: `fn(&u8)` must implement `Trait`
//    = note: but `for<'r> fn(&'r _)` only implements `Trait`

So we are now working on improving the placeholder error message. The goal is something like this:

// error: implementation of `Trait` is not general enough
//   --> src/main.rs:13:7
//    |
// 13 |     a.f();
//    |       ^
//    |
//    = note: `Trait` would have to be implemented for the type `fn(&'0 u8)`, for some specific lifetime `'0`
//    = note: but `Trait` is actually implemented for the type `for<'r> fn(&'r u8)`

Zulip topic with discussion.

@pnkfelix
Copy link
Member

pnkfelix commented Jan 25, 2019

//    = note: but `for<'r> fn(&'r _)` only implements `Trait`

seems to me like even just this change would accomplish your goal:

//    = note: but only `for<'r> fn(&'r _)` implements `Trait`

Update: That wasn't really fair of me, since clearly part of your goal is to make explicit the elided lifetime in fn(&'0 u8). But I worry a little about focusing too much on that, especially with wording like "for some specific lifetime", since in practice such "specific lifetimes" are introduced as generic parameters, as in impl<'a> Trait for fn(&'a u8), right?

Another way for me to phrase that worry: the word "specific" might lead someone away from the point that an impl<'a> Trait for ... is very different from impl Trait for for<'a> ....

  • Quantification is hard, lets go shopping.

@pnkfelix
Copy link
Member

The unclear diagnostic has leaked into beta.

As a matter of process, I'm going to label this as a stable-to-beta regression and I'm going to beta-nominate PR #742563 so we can debate whether to backport it. (I suppose process also dictates that I reopen this bug, but I feel like our process for keeping beta-regression bugs open is pretty haphazard at the moment.)

@pnkfelix pnkfelix added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Jan 30, 2019
@jonas-schievink jonas-schievink moved this from Triaged to Backport in progress in 1.33 regressions Apr 13, 2019
@jonas-schievink jonas-schievink removed the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Apr 13, 2019
@jonas-schievink jonas-schievink moved this from Backport in progress to Fixed in 1.33 regressions Apr 13, 2019
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 P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants