Fix panic in 'remove semicolon' when types are not local

[osa1]

It's not possible to check if removing a semicolon fixes the type error
when checking match arms and one or both of the last arm's and the
current arm's return types are imported "opaque" types. In these cases
we don't generate a "consider removing semicolon" suggestions.

Fixes #81839

---

I'm not sure how to add a test for this. I think the test would need at least two crates. Do we have any existing tests that do this so that I can take a look?

[rust-highfive]

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

[osa1]

I'm not sure if this is the best "fix" for #81839. This is the error message with this patch: (without the patch the repro panics)

  --> src/transitions.rs:26:14
   |
24 | /     match num {
25 | |         1 => { cx.answer_str("hi"); }
   | |                -------------------- this is found to be of type `()`
26 | |         _ => cx.answer_str("hi")
   | |              ^^^^^^^^^^^^^^^^^^^ expected `()`, found opaque type
27 | |     }
   | |_____- `match` arms have incompatible types
   |
  ::: /home/omer/rust/teloxide/src/dispatching/update_with_cx.rs:36:51
   |
36 |       pub async fn answer_str<T>(&self, text: T) -> ResponseResult<Message>
   |                                                     ----------------------- the `Output` of this `async fn`'s found opaque type
   |
   = note:     expected type `()`
           found opaque type `impl futures::Future`

There should be some way of comparing two non-local opaque types for type checking. We should be able to use the same method to compare types of these two branches and generate the suggestion. I couldn't find how to do the comparison though.. See also my question in Zulip.

[osa1]

Last commit brings back the "consider removing this semicolon" help. Current error message:

error[E0308]: `match` arms have incompatible types
  --> src/transitions.rs:26:14
   |
24 | /     match num {
25 | |         1 => { cx.answer_str("hi"); }
   | |                --------------------
   | |                |                  |
   | |                |                  help: consider removing this semicolon
   | |                this is found to be of type `()`
26 | |         _ => cx.answer_str("hi")
   | |              ^^^^^^^^^^^^^^^^^^^ expected `()`, found opaque type
27 | |     }
   | |_____- `match` arms have incompatible types
   |
  ::: /home/omer/rust/teloxide/src/dispatching/update_with_cx.rs:36:51
   |
36 |       pub async fn answer_str<T>(&self, text: T) -> ResponseResult<Message>
   |                                                     ----------------------- the `Output` of this `async fn`'s found opaque type
   |
   = note:     expected type `()`
           found opaque type `impl futures::Future`

error: aborting due to previous error

This help is not great IMO, but I think improving it is outside of the scope of #81839.

[osa1]

Should I add a 'run_make' test for this? Does anyone know a better way to test this?

[osa1]

Hm, I think the new output of the failing test is actually an improvement. Relevant parts of the test:

async fn async_dummy() {}

async fn async_extra_semicolon_same() {
    let _ = match true {
        true => {
            async_dummy();
        }
        false => async_dummy(),
    };
}

Error message in master branch:

error[E0308]: `match` arms have incompatible types
  --> test.rs:10:18
   |
3  |   async fn async_dummy() {}
   |                          - the `Output` of this `async fn`'s found opaque type
...
6  |       let _ = match true {
   |  _____________-
7  | |         true => {
8  | |             async_dummy();
   | |             -------------- this is found to be of type `()`
9  | |         }
10 | |         false => async_dummy(),
   | |                  ^^^^^^^^^^^^^ expected `()`, found opaque type
11 | |     };
   | |_____- `match` arms have incompatible types
   |
   = note:     expected type `()`
           found opaque type `impl Future`
help: consider `await`ing on the `Future`
   |
10 |         false => async_dummy().await,
   |                               ^^^^^^
help: consider removing this semicolon and boxing the expressions
   |
8  |             Box::new(async_dummy())
9  |         }
10 |         false => Box::new(async_dummy()),
   |

error: aborting due to previous error

Error message with this PR:

error[E0308]: `match` arms have incompatible types
  --> test.rs:10:18
   |
3  |   async fn async_dummy() {}
   |                          - the `Output` of this `async fn`'s found opaque type
...
6  |       let _ = match true {
   |  _____________-
7  | |         true => {
8  | |             async_dummy();
   | |             -------------- this is found to be of type `()`
9  | |         }
10 | |         false => async_dummy(),
   | |                  ^^^^^^^^^^^^^ expected `()`, found opaque type
11 | |     };
   | |_____- `match` arms have incompatible types
   |
   = note:     expected type `()`
           found opaque type `impl Future`
help: consider `await`ing on the `Future`
   |
10 |         false => async_dummy().await,
   |                               ^^^^^^
help: consider removing this semicolon
   |
8  |             async_dummy()
   |                         --

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.

So we no longer suggest boxing, which is a good thing because just deleting the semicolon is enough to make this program type check.

[osa1]

Ping @lcnr

[lcnr]

yes, you can have multicrate tests by adding // aux-build:file.rs, see https://github.com/rust-lang/rust/blob/master/src/test/ui/inner-static.rs for example

[osa1]

Thanks @lcnr , I added a regression test now and rebased the branch.

Let me know if it looks good and I'll squash the commits.

[lcnr]

this method seems a bit weird to me, even when ignoring your changes so I want to look at it in a bit more depth. Don't have a lot of time rn so I might not get to it during the next two weeks.

The added tests looks good 👍

@estebank would be interested in taking this over?

[estebank]

r? @estebank

[estebank]

@bors r+ rollup

It's great to get the review by the time all my potential nits have been addressed :)

Looks great!

[bors]

📌 Commit 9ef67e0 has been approved by estebank