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

More structured suggestions for boxed trait objects instead of impl Trait on non-coerceable tail expressions #75608

Merged
merged 6 commits into from
Sep 14, 2020

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Aug 16, 2020

When encountering a match or if as a tail expression where the
different arms do not have the same type and the return type of that
fn is an impl Trait, check whether those arms can implement Trait
and if so, suggest using boxed trait objects.

Use structured suggestion for impl T to Box<dyn T>.

Fix #69107

@rust-highfive
Copy link
Collaborator

r? @lcnr

(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 Aug 16, 2020
@estebank

This comment has been minimized.

@rust-timer

This comment has been minimized.

@bors

This comment has been minimized.

@estebank

This comment has been minimized.

@bors

This comment has been minimized.

@rust-timer

This comment has been minimized.

@rust-timer

This comment has been minimized.

@estebank
Copy link
Contributor Author

🤯 I am amazed at how little this affected perf. Expect a further iteration cleaning this up.

@estebank
Copy link
Contributor Author

Screen Shot 2020-08-16 at 8 00 29 PM

@estebank

This comment has been minimized.

@rust-timer

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (de4f59cb4ed414ffb33a797ae2c0832cb68647a9): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@estebank estebank changed the title [WIP] suggest boxing in tail match expr More structured suggestions for boxed trait objects instead of impl Trait on non-coerceable tail expressions Aug 18, 2020
@@ -48,25 +54,38 @@ LL | 1u32
|
= note: to return `impl Trait`, all returned values must be of the same type
= note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
= help: you can instead return a boxed trait object using `Box<dyn std::fmt::Display>`
= note: for information on trait objects, see <https://doc.rust-lang.org/book/ch17-02-trait-objects.html#using-trait-objects-that-allow-for-values-of-different-types>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you move that note below the changed help message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions always come last. It's been in my backlog for a while to address that.

src/librustc_infer/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
src/librustc_infer/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/_match.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/_match.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/_match.rs Outdated Show resolved Hide resolved
@bors

This comment has been minimized.

@estebank estebank force-pushed the suggest-boxed-match-exprs branch 3 times, most recently from d47903f to 58678ce Compare August 25, 2020 01:08
@estebank
Copy link
Contributor Author

We still provide two errors for the case where a bare dyn Trait is the return type, but that "should be enough", at least for now:

fn qux() -> dyn std::fmt::Display {
    if false {
        0i32
    } else {
        1u32 //~ ERROR `if` and `else` have incompatible types
    }
}
error[E0308]: `if` and `else` have incompatible types
 --> fil3.rs:5:9
  |
2 | /     if false {
3 | |         0i32
  | |         ---- expected because of this
4 | |     } else {
5 | |         1u32 //~ ERROR `if` and `else` have incompatible types
  | |         ^^^^ expected `i32`, found `u32`
6 | |     }
  | |_____- `if` and `else` have incompatible types

error[E0746]: return type cannot have an unboxed trait object
 --> fil3.rs:1:13
  |
1 | fn qux() -> dyn std::fmt::Display {
  |             ^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
  |
  = note: for information on trait objects, see <https://doc.rust-lang.org/book/ch17-02-trait-objects.html#using-trait-objects-that-allow-for-values-of-different-types>
  = note: if all the returned values were of the same type you could use `impl std::fmt::Display` as the return type
  = note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
  = note: you can create a new `enum` with a variant for each returned type
help: return a boxed trait object instead
  |
1 | fn qux() -> Box<dyn std::fmt::Display> {
2 |     if false {
3 |         Box::new(0i32)
4 |     } else {
5 |         Box::new(1u32) //~ ERROR `if` and `else` have incompatible types
  |

error: aborting due to 2 previous errors

When encountering a `match` or `if` as a tail expression where the
different arms do not have the same type *and* the return type of that
`fn` is an `impl Trait`, check whether those arms can implement `Trait`
and if so, suggest using boxed trait objects.
@varkor
Copy link
Member

varkor commented Sep 12, 2020

@bors r=lcnr,varkor

@bors
Copy link
Contributor

bors commented Sep 12, 2020

📌 Commit 701bb7651762b229bf9d1c91549168dec037d667 has been approved by lcnr,varkor

@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 Sep 12, 2020
@bors

This comment has been minimized.

@bors

This comment has been minimized.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 12, 2020
@estebank estebank force-pushed the suggest-boxed-match-exprs branch 2 times, most recently from 44b4134 to c6f2ddf Compare September 14, 2020 19:51
@estebank
Copy link
Contributor Author

@bors r=lcnr,varkor

@bors
Copy link
Contributor

bors commented Sep 14, 2020

📌 Commit c6f2ddf has been approved by lcnr,varkor

@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 Sep 14, 2020
@bors
Copy link
Contributor

bors commented Sep 14, 2020

⌛ Testing commit c6f2ddf with merge 41dc394...

@bors
Copy link
Contributor

bors commented Sep 14, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: lcnr,varkor
Pushing 41dc394 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 14, 2020
@bors bors merged commit 41dc394 into rust-lang:master Sep 14, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 14, 2020
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 7, 2022
…s-instead-of-impl-trait, r=compiler-errors

Revive suggestions for boxed trait objects instead of impl Trait

The suggestion implemented in rust-lang#75608 was not working properly, so I fixed it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

Detect when if/else is tail expression has mismatched types that could be a boxed trait
7 participants