Rust tries to be too helpful with impl suggestions, sometimes being unhelpful

[sgrif]

This code example is a simplified extraction of part of the hierarchy that I'm working on for https://github.com/sgrif/yaqb. It's still a bit contrived, but I think it really gets the point across about how unhelpful/nonsensical the resulting error message is.

The type structure goes like this: AsExpression represents a type which can be converted to an Expression which would return a given type. Expression represents... well, an expression. AsExpression gets a blanket impl for any type which implements Expression. Column represents a concrete column (which in the real code has other things associated to it). Ultimately the column has a type, and can be used as an expression of that type. Any type that implements Column gets a blanket impl for Expression.

In main, we're trying to compare a column representing an i32, to a str. This is incorrect, and should fail to compile. However, the error message we get out of this is:

error: the trait Column is not implemented for the type &str [E0277]

In the context of a SQL database, and what Column means, this is a nonsense error, and would not be helpful to the user in seeing why their code is failing to compile. Rust is just being super clever and realizing that the blanket impls would apply if we just implement that instead. I'd like to propose changing the error message to something with a bit more information, such as:

error: the trait AsExpression<i32> is not implemented for the type &str [E0277]

A blanket implementation of AsExpression, located at : could apply if Column were implemented for the type &str

[arielb1]

That was supposed to work.

[sgrif]

Anyone have any opinions on my recommendation on how to improve this, or other possibilities?

[sgrif]

Is there anything I can do to get some attention on this? We're at the point where we're considering some sweeping/painful changes to our trait hierarchy to work around this, as there are cases that have been consistent sources of confusion in Diesel. (Our particular problem case, among others, is that rustc notices that we have impl<ST, T> AsExpression<ST> for T where T: Expression<SqlType=ST>, and never mentions AsExpression in the error message.)

I can't imagine that we have the only case where recommending the blanket impl without mentioning the actual trait it's trying to satisfy is problematic.

[jonas-schievink]

I think the obligation forest provides the functionality needed to fix this properly: #30976

Minified example:

trait Expression {}

trait Column {}

impl<C: Column> Expression for C {}

fn assert_expression<T: Expression>() {}

fn main() {
    assert_expression::<()>();
}

<anon>:11:5: 11:28 error: the trait `Column` is not implemented for the type `()` [E0277]
<anon>:11     assert_expression::<()>();
              ^~~~~~~~~~~~~~~~~~~~~~~
<anon>:11:5: 11:28 help: see the detailed explanation for E0277
<anon>:11:5: 11:28 note: required by `assert_expression`
error: aborting due to previous error

[sgrif]

It seems like your example demonstrates the problem, not that it's fixed? Expression isn't mentioned anywhere in that error message.

[jonas-schievink]

Yes, it's just a minified version of your code above. Sorry for not being clear about that.

[sgrif]

Had another user get bit by this today: https://github.com/diesel-rs/diesel/issues/323

[arielb1]

This is fixed on 1.10/nightly:

error: the trait bound `(): Column` is not satisfied [--explain E0277]
  --> <anon>:11:5
11 |>     assert_expression::<()>();
   |>     ^^^^^^^^^^^^^^^^^^^^^^^
note: required because of the requirements on the impl of `Expression` for `()`
note: required by `assert_expression`

error: aborting due to previous error
playpen: application terminated with error code 101

[sgrif]

I don't think this issue is fixed. This at least mentions the trait name, which is an improvement, but the error message is still confusing to me. In the example that you gave, it makes it sound like Column is a supertrait of Expression to me.

The case in https://github.com/diesel-rs/diesel/issues/323 is also giving incorrect information.

src/macros/insertable.rs:206:13: 207:32 error: the trait bound `i32: expression::Expression` is not satisfied [E0277]
src/macros/insertable.rs:206             AsExpression::<<$column as Expression>::SqlType>
                                         ^
src/macros/insertable.rs:206:13: 207:32 help: run `rustc --explain E0277` to see a detailed explanation
src/macros/insertable.rs:206:13: 207:32 note: required because of the requirements on the impl of `expression::Expression` for `&i32`
src/macros/insertable.rs:206:13: 207:32 note: required by `expression::AsExpression::as_expression`
src/macros/insertable.rs:206:13: 207:47 error: mismatched types [E0308]

&i32 does not implement Expression, and the type parameter to AsExpression isn't even mentioned.

[sgrif]

It's still fairly trivial to get a case where the trait that's failing to apply isn't even mentioned. https://is.gd/DjS6Uk

@arielb1 I think there's still some work to do on this one, can we reopen this issue?

[arielb1]

That looks like a different bug. Are investigating

pub struct Text;

pub trait Expression {
    type SqlType;
}

pub trait AsExpression<ST> {
    type Expression: Expression<SqlType=ST>;

    fn as_expression(self) -> Self::Expression;
}

impl<T, ST> AsExpression<ST> for T where
    T: Expression<SqlType=ST>,
{
    type Expression = Self;

    fn as_expression(self) -> Self::Expression {
        self
    }
}

fn main() {
    AsExpression::as_expression(Text);
}

[arielb1]

@sgrif

In your case, the reported-upon Text: Expression predicate is coming from your trait declaration, rather than from any impl. We should not register these bounds directly, but there is some room for improvement.

[arielb1]

I would like to refactor that code before we make any changes to it. cc @eddyb. (https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/mod.rs#L4364)

[eddyb]

@arielb1 I will... try not to touch any of that. Except if I do anything to Substs, I suppose.

Do you want to track the reason for those obligations more accurately? Although the code looks like it should do the right thing in terms of ObligationCause, but maybe that's not enough. I don't have any strong opinions about any of that function, really, as long as it works.

[arielb1]

The problem is that we add obligations for the trait whose def-id refers to the method. If we would just add the FnSpace obligations + the Self: Trait predicate, we would get a cause that refers to the impl.

[eddyb]

@arielb1 Ah, I agree then that a single predicate would be better for cause tracking (maybe even more efficient?).

[arielb1]

Do you have any plans for refactoring the above code, or can I try to hack on it myself?

[eddyb]

@arielb1 Go ahead, don't mind me.

[behnam]

I have faced a similar problem when trait A is required, and there's blanket impl B for A available, resulting in the error message to say trait B is required, which is not accurate.

More details: https://users.rust-lang.org/t/unexpected-behaviors-of-trait-bounds/12286

[steveklabnik]

Triage: other than the new format, the error message remains the same:

error[E0277]: the trait bound `&str: Column` is not satisfied
  --> src/main.rs:39:26
   |
39 |     let predicate = name.eq("Sean");
   |                          ^^ the trait `Column` is not implemented for `&str`
   |
   = note: required because of the requirements on the impl of `Expression` for `&str`
   = note: required because of the requirements on the impl of `AsExpression<std::string::String>` for `&str`

[estebank]

For nightly users, libraries can leverage rustc_on_unimplemented to provide better errors in these cases.

[sgrif]

#[rustc_on_unimplemented] doesn't really apply here, since Rust is showing a recommendation for the wrong trait, and won't show the custom message. https://play.rust-lang.org/?version=nightly&mode=debug&edition=2015&gist=963ede706e74095f9b99192b2ac0e5d7

[estebank]

@sgrif that's because you need to give the on_unimplemented to trait Column:

#[rustc_on_unimplemented(
    on(
        _Self="&str",
        message="found &str but we want a `Column`",
        label="not on a `Column`",
        note="you should be fruxolizing your thingamathings"
    ),
    message="default message"
)]
trait Column {
    type SqlType;
}

error[E0277]: found &str but we want a `Column`
  --> src/main.rs:50:26
   |
50 |     let predicate = name.eq("Sean");
   |                          ^^ not on a `Column`
   |
   = help: the trait `Column` is not implemented for `&str`
   = note: you should be fruxolizing your thingamathings
   = note: required because of the requirements on the impl of `Expression` for `&str`
   = note: required because of the requirements on the impl of `AsExpression<std::string::String>` for `&str`

[sgrif]

This issue is about the fact that Column is irrelevant, not useful to users when the missing trait is AsExpression, and there's no way to control this behavior as a library author.

On Tue, May 21, 2019 at 12:06 PM Esteban Kuber notifications@github.com wrote:

@sgrif https://github.com/sgrif that's because you need to give the on_unimplemented to trait Column https://play.rust-lang.org/?version=nightly&mode=debug&edition=2015&gist=f12573f27165332f884dfd1abafc11a2 :

[rustc_on_unimplemented(

on(
    _Self="&str",
    message="found &str but we want a `Column`",
    label="not on a `Column`",
    note="you should be fruxolizing your thingamathings"
),
message="default message"

)] trait Column { type SqlType; }

error[E0277]: found &str but we want a Column --> src/main.rs:50:26	50	let predicate = name.eq("Sean");	^^ not on a Column

= help: the trait Column is not implemented for &str = note: you should be fruxolizing your thingamathings = note: required because of the requirements on the impl of Expression for &str = note: required because of the requirements on the impl of AsExpression<std::string::String> for &str

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/28894?email_source=notifications&email_token=AALVMKYC3WA66OPJP7SENATPWQ22DA5CNFSM4BRLSFGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV4W4EA#issuecomment-494497296, or mute the thread https://github.com/notifications/unsubscribe-auth/AALVMK5PICBHX6LQGOU3EDDPWQ22DANCNFSM4BRLSFGA .

--

Thanks, Sean Griffin

[estebank]

Could you show us the "fixed" code for the example provided?

I believe that I understand where you're coming from, as this is a pretty bad limitation rustc has, but I also think that the subset of problems that are expected to happen (like this one) can receive specific error messages that point people in the right direction, even though they need to be added to the "wrong" trait. I'm not saying that this ticket should be closed, as rustc should behave in a more reasonable manner, but rather that diesel can improve its user friendliness today by judicious use of on_unimplemented for common problems.

[sgrif]

https://gist.github.com/sgrif/fb06eec01a38a8f7a8f0ff9c1f62be7d

For what it's worth, https://github.com/rust-lang/rust/issues/51992 is probably enough to address this

On Tue, May 21, 2019 at 12:55 PM Esteban Kuber notifications@github.com wrote:

Could you show us the "fixed" code for the example provided?

I believe that I understand where you're coming from, as this is a pretty bad limitation rustc has, but I also think that the subset of problems that are expected to happen (like this one) can receive specific error messages that point people in the right direction, even though they need to be added to the "wrong" trait. I'm not saying that this ticket should be closed, as rustc should behave in a more reasonable manner, but rather that diesel can improve its user friendliness today by judicious use of on_unimplemented for common problems.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/28894?email_source=notifications&email_token=AALVMK5BGJHDGW3DRJJNLITPWRATHA5CNFSM4BRLSFGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV43CVQ#issuecomment-494514518, or mute the thread https://github.com/notifications/unsubscribe-auth/AALVMK5INSIXFPW75NWHR5LPWRATHANCNFSM4BRLSFGA .

--

Thanks, Sean Griffin

[estebank]

Current output, minor change to the span:

error[E0277]: the trait bound `&str: Column` is not satisfied
  --> file12.rs:39:29
   |
39 |     let predicate = name.eq("Sean");
   |                             ^^^^^^ the trait `Column` is not implemented for `&str`
   |
   = note: required because of the requirements on the impl of `Expression` for `&str`
   = note: required because of the requirements on the impl of `AsExpression<std::string::String>` for `&str`

[estebank]

Current output, more context for the requirements, but no other changes:

error[E0277]: the trait bound `&str: Column` is not satisfied
  --> src/main.rs:42:29
   |
42 |     let predicate = name.eq("Sean");
   |                             ^^^^^^ the trait `Column` is not implemented for `&str`
   |
note: required because of the requirements on the impl of `Expression` for `&str`
  --> src/main.rs:31:17
   |
31 | impl<C: Column> Expression for C {
   |                 ^^^^^^^^^^     ^
note: required because of the requirements on the impl of `AsExpression<String>` for `&str`
  --> src/main.rs:19:21
   |
19 | impl<E: Expression> AsExpression<E::SqlType> for E {
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^     ^

Crazy idea: what if crates started adding comments next to places where spans are displayed, like 👀

error[E0277]: the trait bound `&str: Column` is not satisfied
  --> src/main.rs:43:29
   |
43 |     let predicate = name.eq("Sean");
   |                             ^^^^^^ the trait `Column` is not implemented for `&str`
   |
note: required because of the requirements on the impl of `Expression` for `&str`
  --> src/main.rs:29:1
   |
29 | Expression // Make sure that the type
   | ^^^^^^^^^^
30 | for        // of the value you're using
31 | C          // matches the table definition
   | ^
note: required because of the requirements on the impl of `AsExpression<String>` for `&str`
  --> src/main.rs:16:21
   |
16 | impl<E: Expression> AsExpression<E::SqlType> for E {
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^     ^

[estebank]

Current output:

error[E0277]: the trait bound `&str: Column` is not satisfied
  --> src/main.rs:39:29
   |
39 |     let predicate = name.eq("Sean");
   |                          -- ^^^^^^ the trait `Column` is not implemented for `&str`
   |                          |
   |                          required by a bound introduced by this call
   |
   = help: the trait `Column` is implemented for `name`
note: required for `&str` to implement `Expression`
  --> src/main.rs:28:17
   |
28 | impl<C: Column> Expression for C {
   |         ------  ^^^^^^^^^^     ^
   |         |
   |         unsatisfied trait bound introduced here
note: required for `&str` to implement `AsExpression<String>`
  --> src/main.rs:16:21
   |
16 | impl<E: Expression> AsExpression<E::SqlType> for E {
   |         ----------  ^^^^^^^^^^^^^^^^^^^^^^^^     ^
   |         |
   |         unsatisfied trait bound introduced here
note: required by a bound in `Expression::eq`
  --> src/main.rs:4:14
   |
4  |     fn eq<T: AsExpression<Self::SqlType>>(self, other: T) {
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Expression::eq`

[estebank]

Triage: no change.