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

Account for common impl Trait/dyn Trait return type errors #68195

Merged
merged 15 commits into from
Jan 17, 2020

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Jan 14, 2020

  • When all return paths have the same type, suggest impl Trait.
  • When all return paths implement the expected trait, suggest Box<dyn Trait> and mention using an enum.
  • When multiple different types are returned and impl Trait is expected, extend the explanation.
  • When return type is impl Trait and the return paths do not implement Trait, point at the returned values.
  • Split src/librustc/traits/error_reporting.rs into multiple files to keep size under control.

Fix #68110, cc #66523.

@rust-highfive
Copy link
Collaborator

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 14, 2020
@estebank
Copy link
Contributor Author

The changes have reasonable single commit sizes, but the final diff is on the larger side.

CC @rust-lang/wg-diagnostics

@estebank
Copy link
Contributor Author

Screen Shot 2020-01-13 at 6 36 48 PM

Screen Shot 2020-01-13 at 6 37 11 PM

Screen Shot 2020-01-13 at 6 38 48 PM

Screen Shot 2020-01-13 at 6 38 23 PM

Note that suggestions always come last, which messes up the flow a bit (I'd prefer the last thing in the diagnostic to be the links, but alas I won't be changing DiagnosticBuilder today).

@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 14, 2020

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned eddyb Jan 14, 2020
src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/coercion.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/coercion.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

src/librustc_error_codes/error_codes/E0746.md Outdated Show resolved Hide resolved
src/librustc_error_codes/error_codes/E0746.md Outdated Show resolved Hide resolved
src/librustc_error_codes/error_codes/E0746.md Outdated Show resolved Hide resolved
src/librustc/traits/mod.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor

oli-obk commented Jan 15, 2020

r? @Centril (reflecting reality here)

@estebank
Copy link
Contributor Author

I'm looking at addressing the cases where T is not object safe to keep it out of the suggestion, but it is a hard error that doesn't even reach the unsized error:

error[E0038]: the trait `NotObjectSafe` cannot be made into an object
  --> src/test/ui/impl-trait/object-unsafe-trait-in-return-position-dyn-trait.rs:21:1
   |
3  |     fn foo() -> Self;
   |        --- associated function `foo` has no `self` parameter
...
21 | fn car() -> dyn NotObjectSafe {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `NotObjectSafe` cannot be made into an object

error[E0038]: the trait `NotObjectSafe` cannot be made into an object
  --> src/test/ui/impl-trait/object-unsafe-trait-in-return-position-dyn-trait.rs:28:1
   |
3  |     fn foo() -> Self;
   |        --- associated function `foo` has no `self` parameter
...
28 | fn cat() -> Box<dyn NotObjectSafe> {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `NotObjectSafe` cannot be made into an object

The impl T case doesn't account for object safety yet.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@Centril
Copy link
Contributor

Centril commented Jan 16, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jan 16, 2020

📌 Commit 029a9c6 has been approved by Centril

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 17, 2020
Account for common `impl Trait`/`dyn Trait` return type errors

- When all return paths have the same type, suggest `impl Trait`.
- When all return paths implement the expected `trait`, suggest `Box<dyn Trait>` and mention using an `enum`.
- When multiple different types are returned and `impl Trait` is expected, extend the explanation.
- When return type is `impl Trait` and the return paths do not implement `Trait`, point at the returned values.
- Split `src/librustc/traits/error_reporting.rs` into multiple files to keep size under control.

Fix rust-lang#68110, cc rust-lang#66523.
bors added a commit that referenced this pull request Jan 17, 2020
Rollup of 6 pull requests

Successful merges:

 - #67956 (Detail transitive containment in E0588 diagnostic)
 - #68153 (resolve: Point at the private item definitions in privacy errors)
 - #68195 (Account for common `impl Trait`/`dyn Trait` return type errors)
 - #68288 (Fix some of the rustfmt fallout in Miri)
 - #68292 (don't clone types that are copy)
 - #68301 (Don't propagate __RUST_TEST_INVOKE to subprocess)

Failed merges:

r? @ghost
@bors bors merged commit 029a9c6 into rust-lang:master Jan 17, 2020
@Ixrec
Copy link
Contributor

Ixrec commented Jan 20, 2020

Do we have an issue for changing the "return type cannot be a bare trait" language to something more dyn-aware? (I couldn't find one)

@estebank
Copy link
Contributor Author

It should be "unboxed trait object". Don't think there's a ticket.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 25, 2020
Further improve `impl Trait`/`dyn Trait` suggestions

After reading [_Returning Trait Objects_ by Bryce Fisher-Fleig](https://bryce.fisher-fleig.org/blog/returning-trait-objects/), [I noticed that](https://www.reddit.com/r/rust/comments/esueur/returning_trait_objects/ffczl4k/) rust-lang#68195 had a few bugs due to not ignoring `ty::Error`.

- Account for `ty::Error`.
- Account for `if`/`else` and `match` blocks when pointing at return types and referencing their types.
- Increase the multiline suggestion output from 6 lines to 20.
bors added a commit that referenced this pull request Jan 26, 2020
Further improve `impl Trait`/`dyn Trait` suggestions

After reading [_Returning Trait Objects_ by Bryce Fisher-Fleig](https://bryce.fisher-fleig.org/blog/returning-trait-objects/), [I noticed that](https://www.reddit.com/r/rust/comments/esueur/returning_trait_objects/ffczl4k/) #68195 had a few bugs due to not ignoring `ty::Error`.

- Account for `ty::Error`.
- Account for `if`/`else` and `match` blocks when pointing at return types and referencing their types.
- Increase the multiline suggestion output from 6 lines to 20.
@estebank estebank deleted the impl-trait-2000 branch November 9, 2023 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve output of impl Trait errors
8 participants