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

improving error message when returning an opaque type inside an if else block #107899

Closed
vincenzopalazzo opened this issue Feb 10, 2023 · 10 comments
Assignees
Labels
A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@vincenzopalazzo
Copy link
Member

vincenzopalazzo commented Feb 10, 2023

Code

async fn async_dummy() {}

async fn async_dummy2() {}


fn main() {
    let _ = if true {
        async_dummy()
    } else {
        async_dummy2()
    };
}

Current output

Compiling playground v0.0.1 (/playground)
error[E0308]: `if` and `else` have incompatible types
  --> src/main.rs:10:9
   |
7  |       let _ = if true {
   |  _____________-
8  | |         async_dummy()
   | |         ------------- expected because of this
9  | |     } else {
10 | |         async_dummy2()
   | |         ^^^^^^^^^^^^^^ expected opaque type, found a different opaque type
11 | |     };
   | |_____- `if` and `else` have incompatible types

Desired output

No response

Rationale and extra context

We start a discussion on zulip regarding this that is available here where we noted that this suggestion is not helpful at all, but also we noted that it is difficult make a helpful suggestion in this case because there is the question that a user may ask to himself when reading the error message:

  1. What is it an opaque type?
  2. Future<Output = ()> is equal to Future<Output = ()>
  3. Why two futures are not equal?a

Other cases

No response

Anything else?

A possible solution for this can be:

  • Do not try to be too smart in some corner case like this, and in this case, I would like an error like You can not return two different futures inside an if-else block. rustc --explain ...
  • make a specific explanation on why we can not compare these two types
  • In case of the future generate a note that the user may want to call await on each branch
  • change the opaque type string into future string when applicable
@vincenzopalazzo vincenzopalazzo 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. labels Feb 10, 2023
@vincenzopalazzo
Copy link
Member Author

vincenzopalazzo commented Feb 10, 2023

I had a partial patch already for this, but I think this required a little bit of discussion

@rustbot label +A-async-await +I-async-nominated
@rustbot claim

@rustbot rustbot added A-async-await Area: Async & Await I-async-nominated Nominated for discussion during an async working group meeting. labels Feb 10, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2023

Error: Parsing assign command in comment failed: ...' assign' | error: specify user to assign to at >| ''...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@vincenzopalazzo vincenzopalazzo changed the title improving error message while we return a opaque type inside a if else improving error message when returning an opaque type inside an if else block Feb 10, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 13, 2023
…ggestion, r=compiler-errors

fix: improve the suggestion on future not awaited

Considering the following code

```rust
fn foo() -> u8 {
    async fn async_fn() -> u8 {  22 }

    async_fn()
}

fn main() {}
```

the error generated before this commit from the compiler is

```
➜  rust git:(macros/async_fn_suggestion) ✗ rustc test.rs --edition 2021
error[E0308]: mismatched types
 --> test.rs:4:5
  |
1 | fn foo() -> u8 {
  |             -- expected `u8` because of return type
...
4 |     async_fn()
  |     ^^^^^^^^^^ expected `u8`, found opaque type
  |
  = note:     expected type `u8`
          found opaque type `impl Future<Output = u8>`
help: consider `await`ing on the `Future`
  |
4 |     async_fn().await
  |               ++++++

error: aborting due to previous error
```

In this case the error is nor perfect, and can confuse the user that do not know that the opaque type is the future.

So this commit will propose (and conclude the work start in rust-lang#80658)
to change the string `opaque type` to `future` when applicable and also remove the Expected vs Received note by adding a more specific one regarding the async function that return a future type.

So the new error emitted by the compiler is

```
error[E0308]: mismatched types
 --> test.rs:4:5
  |
1 | fn foo() -> u8 {
  |             -- expected `u8` because of return type
...
4 |     async_fn()
  |     ^^^^^^^^^^ expected `u8`, found future
  |
note: calling an async function returns a future
 --> test.rs:4:5
  |
4 |     async_fn()
  |     ^^^^^^^^^^
help: consider `await`ing on the `Future`
  |
4 |     async_fn().await
  |               ++++++

error: aborting due to previous error
```

Fixes rust-lang#80658

It remains to rework the case described in the following issue rust-lang#107899 but I think this deserves its own PR after we discuss a little bit how to handle these kinds of cases.

r? `@eholk`

`@rustbot` label +I-async-nominated

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@vincenzopalazzo
Copy link
Member Author

@rustbot label +E-mentor

@rustbot rustbot added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Feb 18, 2023
@sladyn98
Copy link
Contributor

@rustbot claim

@rustbot rustbot assigned sladyn98 and unassigned vincenzopalazzo Feb 21, 2023
@eholk eholk added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Feb 27, 2023
@sladyn98
Copy link
Contributor

@vincenzopalazzo Where do I add the helpful error message, a good starting point would be helpful

@vincenzopalazzo
Copy link
Member Author

vincenzopalazzo commented Mar 15, 2023

@sladyn98 this PR #107902 is not helping you?

the full grep of the error is not working because the message is build dynamically

@sladyn98
Copy link
Contributor

@vincenzopalazzo This is my first contribution to the compiler so I am still getting used to a bit of the codebase, I can see some functions that construct messages about future in PR #107902, but I am not quite sure under which one would this new message come ? Sorry if the question does not make much sense, but I am still trying to understand the codebase

@sladyn98
Copy link
Contributor

sladyn98 commented Apr 8, 2023

https://github.com/rust-lang/rust/blob/HEAD/compiler/rustc_middle/src/ty/error.rs#L95-L96 I found out that the error is being built from here and the if else block is here https://github.com/rust-lang/rust/blob/HEAD/compiler/rustc_infer/src/infer/error_reporting/mod.rs#L801

I think I can understand the flow, the IfExpression cause is generated here, from where the error is constructed

self.cause(
error_sp,
ObligationCauseCode::IfExpression(Box::new(IfExpressionCause {
else_id,
then_id,
then_ty,
else_ty,
outer_span,
opt_suggest_box_span,
})),
)

I am wondering if the async type checks would be done just before this line so that the message could be changed

if let Some(sp) = outer_span {
err.span_label(sp, "if and else have incompatible types");
}

CC @vincenzopalazzo

@vincenzopalazzo
Copy link
Member Author

@rustbot claim

@rustbot rustbot assigned vincenzopalazzo and unassigned sladyn98 Apr 23, 2023
@vincenzopalazzo
Copy link
Member Author

When I was trying to run this with master I found that my #107902 now fixed also this?

So closing as resolved, this is the error message that we want

➜  rust git:(macros/opaque-and-if-else) ✗ ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc test.rs --edition 2021
error[E0308]: `if` and `else` have incompatible types
  --> test.rs:10:9
   |
7  |       let _ = if true {
   |  _____________-
8  | |         async_dummy()
   | |         ------------- expected because of this
9  | |     } else {
10 | |         async_dummy2()
   | |         ^^^^^^^^^^^^^^ expected future, found a different future
11 | |     };
   | |_____- `if` and `else` have incompatible types
   |
   = note: distinct uses of `impl Trait` result in different opaque types
help: consider `await`ing on both `Future`s
   |
8  ~         async_dummy().await
9  |     } else {
10 ~         async_dummy2().await
   |

error: aborting due to previous error

@tmandry tmandry removed the I-async-nominated Nominated for discussion during an async working group meeting. label Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Status: Done
Development

No branches or pull requests

5 participants