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

Improve output of impl Trait errors #68110

Closed
estebank opened this issue Jan 11, 2020 · 5 comments · Fixed by #68195
Closed

Improve output of impl Trait errors #68110

estebank opened this issue Jan 11, 2020 · 5 comments · Fixed by #68195
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-enhancement Category: An issue proposing an enhancement or a PR with one. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@estebank
Copy link
Contributor

Given the following code:

struct S;
struct Y;

trait Trait {}

impl Trait for S {}
impl Trait for Y {}

fn foo() -> impl Trait {
    if true {
        return S;
    }
    Y
}

fn bar() -> impl Trait {
    unimplemented!()
}

we currently emit:

error[E0308]: mismatched types
  --> src/main.rs:13:5
   |
9  | fn foo() -> impl Trait {
   |             ---------- expected because this return type...
10 |     if true {
11 |         return S;
   |                - ...is found to be `S` here
12 |     }
13 |     Y
   |     ^ expected struct `S`, found struct `Y`
   |
   = note: expected type `S`
              found type `Y`

error[E0277]: the trait bound `(): Trait` is not satisfied
  --> src/main.rs:16:13
   |
16 | fn bar() -> impl Trait {
   |             ^^^^^^^^^^ the trait `Trait` is not implemented for `()`
   |
   = note: the return type of a function must have a statically known size

The first case should suggest boxing everything instead of using impl Trait:

fn foo() -> Box<dyn Trait> {
    if true {
        return Box::new(S);
    }
    Box::new(Y)
}

The second case should point at the source of the (): Trait bound:

error[E0277]: the trait bound `(): Trait` is not satisfied
  --> src/main.rs:16:13
   |
16 | fn bar() -> impl Trait {
   |             ^^^^^^^^^^ the trait `Trait` is not implemented for `()`
17 |     unreachable!()
   |     -------------- this doesn't implement trait `Trait`
@estebank estebank 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. A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. labels Jan 11, 2020
@Centril
Copy link
Contributor

Centril commented Jan 11, 2020

The first case should suggest boxing everything instead of using impl Trait:

It's not clear that this is unambiguously the right solution for the user in this case. First, the user may not want to pay the overhead of dynamic dispatch, and second the trait may not be object safe. Another solution would be to use an enum for the various cases, and we should suggest that as well.

Equally important, I think it would be to tell the user what the problem is (that impl Trait requires a canonical type and that S and Y are not the same type). We should add a note here.

| -------------- this doesn't implement trait Trait

Only types implement traits, and unimplemented!() is an expression macro. I think this should instead say:

this is of type ()

@estebank
Copy link
Contributor Author

the user may not want to pay the overhead of dynamic dispatch

We can certainly qualify the suggestion more strongly and mention other alternatives, but it seems to me that dynamic dispatch is indeed the path of least resistance for what a new developer is trying to accomplish. Using dynamic dispatch is never wrong just may be suboptimal.

the trait may not be object safe

This is something that we can check as a precondition for structured suggestions, and if unfulfilled it is also a good time to teach about it.

Equally important, I think it would be to tell the user what the problem is

I completely agree. I'm playing around with some different approaches. This is what I've got so far (wording very much WIP):

error[E0277]: the size for values of type `(dyn Trait + 'static)` cannot be known at compilation time
  --> file18.rs:16:13
   |
16 | fn ban() -> dyn Trait { Struct } //~ ERROR E0277
   |             ^^^^^^^^^   ------ return place
   |             |
   |             doesn't have a size known at compile-time
   |             help: use `impl Trait`: `impl Trait`
   |
   = help: the trait `std::marker::Sized` is not implemented for `(dyn Trait + 'static)`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: the return type of a function must have a statically known size
error[E0308]: mismatched types
  --> file18.rs:20:16
   |
18 | fn bal() -> dyn Trait { //~ ERROR E0277
   |             --------- expected `(dyn Trait + 'static)` because of return type
19 |     if true {
20 |         return Struct;
   |                ^^^^^^ expected trait `Trait`, found struct `Struct`
   |
   = note: expected trait object `(dyn Trait + 'static)`
                    found struct `Struct`

error[E0308]: mismatched types
  --> file18.rs:22:5
   |
18 | fn bal() -> dyn Trait { //~ ERROR E0277
   |             --------- expected `(dyn Trait + 'static)` because of return type
...
22 |     Other
   |     ^^^^^ expected trait `Trait`, found struct `Other`
   |
   = note: expected trait object `(dyn Trait + 'static)`
                    found struct `Other`

error[E0277]: the size for values of type `(dyn Trait + 'static)` cannot be known at compilation time
  --> file18.rs:18:13
   |
18 | fn bal() -> dyn Trait { //~ ERROR E0277
   |             ^^^^^^^^^ doesn't have a size known at compile-time
19 |     if true {
20 |         return Struct;
   |                ------ return place
21 |     }
22 |     Other
   |     ----- return place
   |
   = help: the trait `std::marker::Sized` is not implemented for `(dyn Trait + 'static)`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: the return type of a function must have a statically known size
help: use boxed traits
   |
18 | fn bal() -> Box<dyn Trait> { //~ ERROR E0277
19 |     if true {
20 |         return Box::new(Struct);
21 |     }
22 |     Box::new(Other)
   |

@Centril
Copy link
Contributor

Centril commented Jan 11, 2020

Using dynamic dispatch is never wrong just may be suboptimal.

This is something that we can check as a precondition for structured suggestions, and if unfulfilled it is also a good time to teach about it.

Sounds great. Make sure to also mention enums though. It's also worth noting that the user might also want Rc<dyn Trait> or Arc<dyn Trait>, though Box<dyn Trait> would be the common case.

This is what I've got so far (wording very much WIP):

Wait, all of these cases are using -> dyn Trait as the input... but we are discussing suggestions and notes for -> impl Trait...?

@estebank
Copy link
Contributor Author

estebank commented Jan 12, 2020

Long message incoming

Make sure to also mention enums though.

👍

It's also worth noting that the user might also want Rc<dyn Trait> or Arc<dyn Trait>, though Box<dyn Trait> would be the common case.

I'd like to keep the verbosity a bit lower than enumerating all of them, but if we give the error a new error code we could list them in the index.

Wait, all of these cases are using -> dyn Trait as the input... but we are discussing suggestions and notes for -> impl Trait...?

I'm trying to look at this from the point of view of someone trying things out, and "leading the horse to water". The discussion around impl Trait appears in these cases when impl Trait has been suggested in the same error.

The test file I'm working on in this unpublished branch is:

struct Struct;
struct Other;
trait Trait {}
impl Trait for Struct {}
impl Trait for Other {}
// Commented out because they cause earlier errors that stop the world, but I will cater to them later.
//fn foo() -> Foo<dyn Trait> { Struct } //~ ERROR E0277
#[allow(bare_trait_objects)]
fn fuz() -> (usize, Trait) { (42, Struct) } //~ ERROR E0277
fn bar() -> (usize, dyn Trait) { (42, Struct) } //~ ERROR E0277
//#[allow(bare_trait_objects)]
//fn baz() -> (Iterator<Item = u32>, usize) { Struct } //~ ERROR E0277
//fn bat() -> (dyn Iterator<Item = u32>, usize) { Struct } //~ ERROR E0277
#[allow(bare_trait_objects)]
fn bap() -> Trait { Struct } //~ ERROR E0277
fn ban() -> dyn Trait { Struct } //~ ERROR E0277
fn bak() -> dyn Trait { unimplemented!() } //~ ERROR E0277
fn bal() -> dyn Trait { //~ ERROR E0277
    if true {
        return Struct;
    }
    Other
}
fn main() {}

and this is the current full output for it

error[E0308]: mismatched types
 --> file18.rs:9:35
  |
9 | fn fuz() -> (usize, Trait) { (42, Struct) } //~ ERROR E0277
  |                                   ^^^^^^ expected trait `Trait`, found struct `Struct`
  |
  = note: expected trait object `(dyn Trait + 'static)`
                   found struct `Struct`

error[E0277]: the size for values of type `(dyn Trait + 'static)` cannot be known at compilation time
 --> file18.rs:9:13
  |
9 | fn fuz() -> (usize, Trait) { (42, Struct) } //~ ERROR E0277
  |             ^^^^^^^^^^^^^^   ------------ return place
  |             |
  |             doesn't have a size known at compile-time
  |
  = help: within `(usize, (dyn Trait + 'static))`, the trait `std::marker::Sized` is not implemented for `(dyn Trait + 'static)`
  = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
  = note: required because it appears within the type `(usize, (dyn Trait + 'static))`
  = note: the return type of a function must have a statically known size

error[E0308]: mismatched types
  --> file18.rs:10:39
   |
10 | fn bar() -> (usize, dyn Trait) { (42, Struct) } //~ ERROR E0277
   |                                       ^^^^^^ expected trait `Trait`, found struct `Struct`
   |
   = note: expected trait object `(dyn Trait + 'static)`
                    found struct `Struct`

error[E0277]: the size for values of type `(dyn Trait + 'static)` cannot be known at compilation time
  --> file18.rs:10:13
   |
10 | fn bar() -> (usize, dyn Trait) { (42, Struct) } //~ ERROR E0277
   |             ^^^^^^^^^^^^^^^^^^   ------------ return place
   |             |
   |             doesn't have a size known at compile-time
   |
   = help: within `(usize, (dyn Trait + 'static))`, the trait `std::marker::Sized` is not implemented for `(dyn Trait + 'static)`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: required because it appears within the type `(usize, (dyn Trait + 'static))`
   = note: the return type of a function must have a statically known size

error[E0308]: mismatched types
  --> file18.rs:15:21
   |
15 | fn bap() -> Trait { Struct } //~ ERROR E0277
   |             -----   ^^^^^^ expected trait `Trait`, found struct `Struct`
   |             |
   |             expected `(dyn Trait + 'static)` because of return type
   |
   = note: expected trait object `(dyn Trait + 'static)`
                    found struct `Struct`

error[E0277]: the size for values of type `(dyn Trait + 'static)` cannot be known at compilation time
  --> file18.rs:15:13
   |
15 | fn bap() -> Trait { Struct } //~ ERROR E0277
   |             ^^^^^   ------ return place
   |             |
   |             doesn't have a size known at compile-time
   |             help: use `impl Trait`: `impl Trait`
   |
   = help: the trait `std::marker::Sized` is not implemented for `(dyn Trait + 'static)`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: the return type of a function must have a statically known size

error[E0308]: mismatched types
  --> file18.rs:16:25
   |
16 | fn ban() -> dyn Trait { Struct } //~ ERROR E0277
   |             ---------   ^^^^^^ expected trait `Trait`, found struct `Struct`
   |             |
   |             expected `(dyn Trait + 'static)` because of return type
   |
   = note: expected trait object `(dyn Trait + 'static)`
                    found struct `Struct`

error[E0277]: the size for values of type `(dyn Trait + 'static)` cannot be known at compilation time
  --> file18.rs:16:13
   |
16 | fn ban() -> dyn Trait { Struct } //~ ERROR E0277
   |             ^^^^^^^^^   ------ return place
   |             |
   |             doesn't have a size known at compile-time
   |             help: use `impl Trait`: `impl Trait`
   |
   = help: the trait `std::marker::Sized` is not implemented for `(dyn Trait + 'static)`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: the return type of a function must have a statically known size

error[E0277]: the size for values of type `(dyn Trait + 'static)` cannot be known at compilation time
  --> file18.rs:17:13
   |
17 | fn bak() -> dyn Trait { unimplemented!() } //~ ERROR E0277
   |             ^^^^^^^^^   ---------------- return place
   |             |
   |             doesn't have a size known at compile-time
   |             help: use `impl Trait`: `impl Trait`
   |
   = help: the trait `std::marker::Sized` is not implemented for `(dyn Trait + 'static)`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: the return type of a function must have a statically known size
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0308]: mismatched types
  --> file18.rs:20:16
   |
18 | fn bal() -> dyn Trait { //~ ERROR E0277
   |             --------- expected `(dyn Trait + 'static)` because of return type
19 |     if true {
20 |         return Struct;
   |                ^^^^^^ expected trait `Trait`, found struct `Struct`
   |
   = note: expected trait object `(dyn Trait + 'static)`
                    found struct `Struct`

error[E0308]: mismatched types
  --> file18.rs:22:5
   |
18 | fn bal() -> dyn Trait { //~ ERROR E0277
   |             --------- expected `(dyn Trait + 'static)` because of return type
...
22 |     Other
   |     ^^^^^ expected trait `Trait`, found struct `Other`
   |
   = note: expected trait object `(dyn Trait + 'static)`
                    found struct `Other`

error[E0277]: the size for values of type `(dyn Trait + 'static)` cannot be known at compilation time
  --> file18.rs:18:13
   |
18 | fn bal() -> dyn Trait { //~ ERROR E0277
   |             ^^^^^^^^^ doesn't have a size known at compile-time
19 |     if true {
20 |         return Struct;
   |                ------ return place
21 |     }
22 |     Other
   |     ----- return place
   |
   = help: the trait `std::marker::Sized` is not implemented for `(dyn Trait + 'static)`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: the return type of a function must have a statically known size
help: use boxed traits
   |
18 | fn bal() -> Box<dyn Trait> { //~ ERROR E0277
19 |     if true {
20 |         return Box::new(Struct);
21 |     }
22 |     Box::new(Other)
   |

but my ideal end state is

error[E0308]: mismatched types
 --> file18.rs:9:35
  |
9 | fn fuz() -> (usize, Trait) { (42, Struct) } //~ ERROR E0277
  |                     -----         ^^^^^^ expected trait `Trait`, found struct `Struct`
  |                     |
  |                     help: `Struct` implements `dyn Trait`, consider using: `impl Trait`
  |
  = note: expected trait object `(dyn Trait + 'static)`
                   found struct `Struct`

error[E0308]: mismatched types
  --> file18.rs:10:39
   |
10 | fn bar() -> (usize, dyn Trait) { (42, Struct) } //~ ERROR E0277
   |                     ---------         ^^^^^^ expected trait `Trait`, found struct `Struct`
   |                     |
   |                     help: `Struct` implements `dyn Trait`, consider using: `impl Trait`
   |
   = note: expected trait object `(dyn Trait + 'static)`
                    found struct `Struct`

error[E0XX1]: the size for values of type `(dyn Trait + 'static)` cannot be known at compilation time
  --> file18.rs:17:13
   |
17 | fn bak() -> dyn Trait { unimplemented!() } //~ ERROR E0277
   |             ^^^^^^^^^   ---------------- if this implemented `Trait`, you could use `impl Trait` as the return type
   |             |
   |             doesn't have a size known at compile-time
   |             help: because there's a single return in this function, you could use `impl Trait`: `impl Trait`
   |
   = help: the trait `std::marker::Sized` is not implemented for `(dyn Trait + 'static)`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: the return type of a function must have a statically known size
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0XX2]: the size for values of type `(dyn Trait + 'static)` cannot be known at compilation time
  --> file18.rs:18:13
   |
18 | fn bal() -> dyn Trait { //~ ERROR E0277
   |             ^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `std::marker::Sized` is not implemented for `(dyn Trait + 'static)`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: the return type of a function must have a statically known size
note: because this function has multiple returns, it wouldn't be possible to use `impl Trait`
   |
19 |     if true {
20 |         return Struct;
   |                ^^^^^^ `bal` returns here
21 |     }
22 |     Other
   |     ^^^^^ `bal` returns here
   |
   = note: for more information on `impl Trait` and trait objects, visit <FOO>
help: consider using trait objects by using `Box`
   |
18 | fn bal() -> Box<dyn Trait> { //~ ERROR E0277
19 |     if true {
20 |         return Box::new(Struct);
21 |     }
22 |     Box::new(Other)
   |

For the case where the user is already using impl Trait with multiple return types, we currently emit:

error[E0XX3]: mismatched types
  --> src/main.rs:13:5
   |
9  | fn foo() -> impl Trait {
   |             ---------- expected because this return type...
10 |     if true {
11 |         return S;
   |                - ...is found to be `S` here
12 |     }
13 |     Y
   |     ^ expected struct `S`, found struct `Y`

but I'd like us to have a structured suggestion as well

error[E0308]: mismatched types
  --> src/main.rs:13:5
   |
9  | fn foo() -> impl Trait {
   |             ---------- expected because this return type...
10 |     if true {
11 |         return S;
   |                - ...is found to be `S` here
12 |     }
13 |     Y
   |     ^ expected struct `S`, found struct `Y`
   = note: `impl Trait` can only be used in methods that have a single `return`
   = note: for more information on `impl Trait` and trait objects, visit <FOO>
help: consider using trait objects instead of `impl Trait`

9  | fn foo() -> Box<dyn Trait> {
10 |     if true {
11 |         return Box::new(S);
12 |     }
13 |     Box::new(Y)

Some extra clarification might be warranted: I filed this ticket while looking at the rustc output of the generated code from my first few emitted suggestions, where I noticed that they also needed some extra love. I'm sorry I'm conflating all of these cases, but looking at this whole thing as a single project is helping me understand the possible avenues of improvement.

@JohnTitor JohnTitor added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jan 12, 2020
@Centril
Copy link
Contributor

Centril commented Jan 12, 2020

I'd like to keep the verbosity a bit lower than enumerating all of them, but if we give the error a new error code we could list them in the index.

This was just a note on my part, no need to mention them.

For the case where the user is already using impl Trait with multiple return types, we currently emit:

I'm sorry I'm conflating all of these cases, but looking at this whole thing as a single project is helping me understand the possible avenues of improvement.

I would prefer having different tickets for each sub-problem though so that solving each subproblem is more actionable.

= note: impl Trait can only be used in methods that have a single return

This suggests that this is invalid:

fn foo() -> impl Ord {
    if 0 == 1 { return 0 }
    else if 0 == 2 { return 1 }
    else { return 2 }
}

= note: for more information on impl Trait and trait objects, visit

I'd split this into two links, one for impl Trait, and one for trait objects (which should be put after the trait objects suggestion). Finally, after the trait object suggestion, let's also suggest an enum.

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 A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-enhancement Category: An issue proposing an enhancement or a PR with one. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants