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

Suggest converting a type to trait object when it's possible and the method expects one #42499

Open
golddranks opened this issue Jun 7, 2017 · 6 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@golddranks
Copy link
Contributor

golddranks commented Jun 7, 2017

If the method explicitly expects a trait object, such as &postgres::types::ToSql, (ToSql is a trait), and the user is trying to pass it a type that implements the trait, such as String, it's likely that the user either doesn't understand the distinction between trait object and trait bounds in generics, or has accidentally missed that the function isn't expecting a generic type but a trait object.

In this case, it would be helpful to show a more specific error message than the standard "type mismatch". The standard error message is like this:

error[E0308]: mismatched types
  --> src/main.rs:75:39
   |
75 |     for row in &conn.query(get_table, schemas.as_slice()).unwrap() {
   |                                       ^^^^^^^^^^^^^^^^^^ expected trait postgres::types::ToSql, found struct `std::string::String`
   |
   = note: expected type `&[&postgres::types::ToSql]`
              found type `&[&std::string::String]`

First of all, "expected trait" is downright misleading, I think it should be "expected trait object". Secondly, it would be helpful to show a hint that says that:

  1. String implements trait ToSql, but this method is expecting a trait object, not any type that implements the trait.
  2. You can create a trait object from a value of type &String with value as &postgres::types::ToSql.
@oli-obk
Copy link
Contributor

oli-obk commented Jun 7, 2017

It's unnecessary to cast. A simple coercion will do the job if no generics are involved. The problem in your case is that there's no way to convert &[&T] to &[&Trait] without an allocation. Maybe your api should be generic or take an iterator?

@golddranks
Copy link
Contributor Author

golddranks commented Jun 8, 2017

First of all, if the API is not mine, so I can hardly do anything for it. Secondly, maybe @sfackler who is the author of the API in this example (the postgres crate), has something to say, but I think taking trait objects may be unavoidable in cases like heterogenous lists, so diagnostics in such cases are not a non-issue.

Indeed, as you say, there's no way to convert &[&T] to &[&Trait] without an allocation. I don't see that as an argument why a hint wouldn't be great to have here – precisely because the conversion can't be automatic in this case.

@sfackler
Copy link
Member

sfackler commented Jun 8, 2017

The API takes a slice because it was defined quite a long time ago - probably before IntoIterator existed. Making it generic in an appropriate fashion definitely seems reasonable. I've filed sfackler/rust-postgres#265 to track the change.

@Mark-Simulacrum Mark-Simulacrum added the A-diagnostics Area: Messages for errors, warnings, and lints label Jun 23, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 26, 2017
@steveklabnik
Copy link
Member

So, what's the state of this issue? Is there an actual change to be made here?

@oli-obk
Copy link
Contributor

oli-obk commented Nov 17, 2018

We should do two things as suggested in the original post:

  • report trait object instead of trait
  • add a note if T: Trait and the mismatch is expected Trait, found T that informs the user that such a conversion would require an allocation

@crlf0710 crlf0710 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 11, 2020
@estebank
Copy link
Contributor

@oli-obk we now suggest the constraining of type params with a trait bound, but there are a couple of cases that aren't totally handled:

error[E0277]: the trait bound `T: Trait` is not satisfied
 --> src/main.rs:5:9
  |
5 |     bar(&t);
  |         ^^ the trait `Trait` is not implemented for `T`
  |
  = note: required for the cast to the object type `dyn Trait`
help: consider restricting type parameter `T`
  |
4 | fn foo<T: Trait>(t: T) {
  |         ^^^^^^^

error[E0277]: the trait bound `&T: Trait` is not satisfied
  --> src/main.rs:11:10
   |
11 |     bar2(&t);
   |          ^^ the trait `Trait` is not implemented for `&T`
...
14 | fn bar2<T: Trait>(t: T) {}
   |            ----- required by this bound in `bar2`

error[E0277]: the trait bound `T: Trait` is not satisfied
  --> src/main.rs:17:10
   |
14 | fn bar2<T: Trait>(t: T) {}
   |            ----- required by this bound in `bar2`
...
17 |     bar2(t);
   |          ^ the trait `Trait` is not implemented for `T`
   |
help: consider restricting type parameter `T`
   |
16 | fn foo3<T: Trait>(t: T) {
   |          ^^^^^^^

error[E0308]: mismatched types
  --> src/main.rs:21:9
   |
20 | fn foo4<T>(t: T) {
   |         - this type parameter
21 |     bar(t);
   |         ^
   |         |
   |         expected `&dyn Trait`, found type parameter `T`
   |         help: consider borrowing here: `&t`
   |
   = note:   expected reference `&dyn Trait`
           found type parameter `T`

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 C-enhancement Category: An issue proposing an enhancement or a PR with one. 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

7 participants