Improve compile errors for async commands without Result return type

[Pascal-So]

What kind of change does this PR introduce?

• Bugfix
• Feature
• Docs
• New Binding issue #___
• Code style update
• Refactor
• Build-related changes
• Other, please describe:

Does this PR introduce a breaking change?

• Yes, and the changes were approved in issue #___
• No

Checklist

• When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
• A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
• I have added a convincing reason for adding this feature, if necessary

Other information

Issue #2533 points out that async commands currently have to return a Result, but this is not very obvious from the compile error that is shown to the user.

#[tauri::command]
async fn myAsyncCommand(name: &str) {
    tokio::time::sleep(Duration::from_secs_f32(1.)).await;
}

The above code resulted in the following error, which was not comprehensible to me when I encountered this while writing my first tauri app:

error[E0597]: `__tauri_message__` does not live long enough
  --> src/main.rs:15:1
   |
15 | #[tauri::command]
   | ^^^^^^^^^^^^^^^^-
   | |               |
   | |               `__tauri_message__` dropped here while still borrowed
   | borrowed value does not live long enough
   | argument requires that `__tauri_message__` is borrowed for `'static`
...
29 |         .invoke_handler(tauri::generate_handler![myAsyncCommand])
   |                         ---------------------------------------- in this macro invocation
   |
   = note: this error originates in the macro `__cmd__myAsyncCommand` which comes from the expansion of the macro `tauri::generate_handler` (in Nightly builds, run with -Z macro-backtrace for more info)

With the change introduced in this PR, the error now appears as follows:

error: async commands that contain references as inputs must return a `Result`
  --> src/main.rs:16:25
   |
16 | async fn myAsyncCommand(name: &str) {
   |                         ^^^^

error: cannot find macro `__cmd__myAsyncCommand` in this scope
  --> src/main.rs:27:50
   |
27 |         .invoke_handler(tauri::generate_handler![myAsyncCommand]);
   |                                                  ^^^^^^^^^^^^^^

This is what the user sees in their editor:

The problem with State described in the issue is also caught:

Errors are also generated on &'static references, because they also generate the same __tauri_message__ does not live long enough when tested without this change.

This change probably doesn't catch all possible errors, but it should catch the most common ones. It looks for reference types in the arguments, as well as for types that contain lifetime arguments. Can anyone think of a situation where this check would be too strict? The intention here is to rather catch too little than too much, because we don't want this to be a breaking change.

This PR should be reverted as soon as #2533 is resolved.

[Pascal-So]

ah, looks like I missed something, I'll check it out tomorrow

[Pascal-So]

In my original version of the PR I made the mistake of returning an error irrespective of whether or not any of the arguments contained any references, should be fixed now. The (non-ignored)tests and clippy now run without errors on my linux.

Can anyone see a situation that would cause this error to occur when it shouldn't?

[Pascal-So]

I just thought of a potential problem: do you think anyone would rename their result type and have a command returning MyResult? This currently wouldn't work with this PR

[amrbashir]

do you think anyone would rename their result type and have a command returning MyResult

I think it is somewhat common that projects tend to define their own Result type combined with an Error type like this:

enum Error {
}

type Result<T> = std::result::Result<T, Error>;

[Pascal-So]

type Result<T> = std::result::Result<T, Error>;

this wouldn't be a problem since we only check if the type is called Result, and that's about all we really can do at the token level. This common pattern wouldn't pose a problem, problems would only occur when the type is renamed to something like FooResult which I don't remember seeing in practice anywhere.

[amrbashir]

which I don't remember seeing in practice anywhere.

true but still there is a chance someone uses that.

[Pascal-So]

Yeah you're right, the risk of this being a breaking change for someone is too high. I thought about it some more and I think the simplest solution would be to just show the error when no return type is used.

This again restricts the usefulness to the point where we might ask if we should just drop the idea completely, but at least for the case that I encountered while first using tauri, it would have saved me some digging around in the tauri issues to find out what's wrong, because I had an async function taking &str that doesn't return anything.

What do you think, should we go ahead and add it only for the trivial case, plus maybe some primitive return types or return types containing references?

[amrbashir]

A workaround would be to allow users to specify their result type name, for example:

type MyResult<T> = Result<T, MyError>;

#[tauri::command(result = MyResult)]
async fn command(arg: &str) -> MyResult<()> {
	Ok(())
}

[Pascal-So]

#[tauri::command(result = MyResult)]

This would still be a breaking change since users would have to update their attributes when upgrading tauri, otherwise their code might not compile anymore.

In the meantime I've been thinking about this a bit more and questioned one of my above statements:

[...] and that's about all we really can do at the token level.

While it's true that we can't do more at the token level, I thought of some other fun tricks to detect if the user is returning a Result in their command. Given this input code:

type MyRenamedResult<T> = Result<T, String>;

#[tauri::command]
async fn command_name(a: &str) -> MyRenamedResult<()> {

the macro now generates an additional snippet that looks roughly like this:

const _: () = {
  trait AsyncCommandMustReturnResult {}
  impl<A, B> AsyncCommandMustReturnResult for Result<A, B> {};
  const fn check<T: AsyncCommandMustReturnResult>() {}
  const _: () = {
    check::<MyRenamedResult<()>>();
  };
};

This should be 100% reliable in detecting if the command returns a Result. This snippet is only generated when the command is an async functijon and takes a reference or a type with generic lifetime argument as input.

If the user tries to return a different type, they see this compiler error:

error[E0277]: the trait bound `Vec<()>: AsyncCommandMustReturnResult` is not satisfied
  --> src/main.rs:48:35
   |
48 | async fn command_name(a: &str) -> Vec<()> {
   |                                   ^^^^^^^ the trait `AsyncCommandMustReturnResult` is not implemented for `Vec<()>`
   |
   = help: the trait `AsyncCommandMustReturnResult` is implemented for `Result<A, B>`

This is not perfect but in my opinion much more helpful than some lifetime errors.

For the case where the command doesn't return anything we can still show the complete message I added previously.

error: async commands that contain references as inputs must return a `Result`
  --> src/main.rs:48:23
   |
48 | async fn command_name(a: &str) {
   |                       ^

With these changes I believe there should be zero false positives, i.e. this is not a breaking change, and I can't think of any way to make the error messages more readable. Unfortunately we can't use the rustc_on_unimplemented attribute outside of the compiler, that would be the only way to improve this that I could think of.

[JonasKruckenberg]

That trait idea is smart as hell! Though this whole ordeal kinda reinforces why I want to get rid of the command macro entirely 😅
great work though

[Pascal-So]

Didn't realize that the trick I used above doesn't match tauri's MSRV 1.59, I now changed it to use a different method to check the trait bound. The generated check now looks like this:

#[allow(unreachable_code)]
const _: () = if false {
  trait AsyncCommandMustReturnResult {}
  impl<A, B> AsyncCommandMustReturnResult for Result<A, B> {}
  let _check: Vec<()> = unreachable!();
  let _: &dyn AsyncCommandMustReturnResult = &_check;
};

The generated error now looks like this:

error[E0277]: the trait bound `Vec<()>: AsyncCommandMustReturnResult` is not satisfied
  --> src/main.rs:38:35
   |
38 | async fn command_name(a: &str) -> Vec<()> {
   |                                   ^^^ the trait `AsyncCommandMustReturnResult` is not implemented for `Vec<()>`
   |
   = help: the trait `AsyncCommandMustReturnResult` is implemented for `Result<A, B>`
   = note: required for the cast from `Vec<()>` to the object type `dyn AsyncCommandMustReturnResult`

[Pascal-So]

@JonasKruckenberg Thanks for the kind words!

this whole ordeal kinda reinforces why I want to get rid of the command macro entirely

let me know if help is needed on this process to get rid of the macro, I'm looking forward to the moment where my hack in this PR here becomes obsolete :D

[Pascal-So]

hmm looks like the new miniz_oxide release is the problem, if I delete my Cargo.lock and add miniz_oxide = "=0.6.2" to a Cargo.toml then the tests pass on 1.59 on my machine..

[FabianLars]

it's fine to lock it in Cargo.toml and commit it. not the first dependency we have to lock because of the 1.59 requirement :/

[Pascal-So]

looks like the issue is already fixed, miniz_oxide 0.6.4 has been yanked. I'll remove my last commit again where I fixed the version to 0.6.2

[Pascal-So]

this time around I remembered to enable all the relevant features when testing locally 😅 I think with this os_info fix the 1.59 builds should now all work, unless it's something that I can't test locally on linux :D

[Pascal-So]

🤦 didn't realize that not all of the fails on the last run were from 1.59 compatibility.. sorry for the back and forth on this

[IntegralPilot]

Yes!! This is so amazing, wish this was in the current version of Tauri as it would have saved lots of time for me.

[Pascal-So]

Hi all, I completely forgot that this PR was still open. Since some of the things I had to change in Cargo.toml now also happened on the dev branch, I rebased my work on to the current state of the dev branch.

Everything else is exactly as we left it last time, from my side this is ready to merge (assuming I didn't mess anything up again according to CI 😄)

[Pascal-So]

aaaand of course I forgot to sign the commit before, now it's actually ready 😄

[lucasfernog]

This is awesome, thanks!