Erroneous compiler error message for extra semicolon with try operator

[ik1ne]

Version
Cargo.toml: tokio = { version = "1", features = ["full"] }

 % cargo tree | grep tokio     
└── tokio v1.35.1
    └── tokio-macros v2.2.0 (proc-macro)

Platform

 % uname -a
Darwin (my hostname) 23.2.0 Darwin Kernel Version 23.2.0: Wed Nov 15 21:53:18 PST 2023; root:xnu-10002.61.3~2/RELEASE_ARM64_T6000 arm64
 % rustc --version
rustc 1.75.0 (82e1608df 2023-12-21)

Description

[short summary of the bug]
Compiler error message for an extra semicolon with ?(try operator) is confusing.

[code sample that causes the bug]

#[tokio::main]
async fn extra_semicolon_with_try_operator() -> Result<(), ()> {
    Ok(())?;
    Ok(());
}

fn main() {}

I expected to see this compiler error message with or without #[tokio::main] attribute:

error[E0308]: mismatched types
 --> hello-world/src/main.rs:2:70
  |
2 |   async fn extra_semicolon_with_try_operator_tokio() -> Result<(), ()> {
  |  ______________________________________________________________________^
3 | |     Ok(())?;
4 | |     Ok(());
  | |           - help: remove this semicolon to return this value
5 | | }
  | |_^ expected `Result<(), ()>`, found `()`
  |
  = note:   expected enum `Result<(), ()>`
          found unit type `()`

For more information about this error, try `rustc --explain E0308`.
error: could not compile `hello-world` (bin "hello-world") due to previous error

The compiler error message clearly states that line number 4's extra semicolon should be fixed, and there's no error at line 3.

Instead, this happened:

error[E0277]: the `?` operator can only be used in an async block that returns `Result` or `Option` (or another type that implements `FromResidual`)
 --> hello-world/src/main.rs:3:11
  |
1 | #[tokio::main]
  | -------------- this function should return `Result` or `Option` to accept `?`
2 | async fn extra_semicolon_with_try_operator_tokio() -> Result<(), ()> {
3 |     Ok(())?;
  |           ^ cannot use the `?` operator in an async block that returns `()`
  |
  = help: the trait `FromResidual<Result<Infallible, _>>` is not implemented for `()`

error[E0308]: mismatched types
 --> hello-world/src/main.rs:4:5
  |
2 | async fn extra_semicolon_with_try_operator_tokio() -> Result<(), ()> {
  |                                                       -------------- expected `Result<(), ()>` because of return type
3 |     Ok(())?;
4 |     Ok(());
  |     ^^^^^^^ expected `Result<(), ()>`, found `()`
  |
  = note:   expected enum `Result<(), ()>`
          found unit type `()`
help: try adding an expression at the end of the block
  |
4 ~     Ok(());;
5 +     Ok(())
  |

Some errors have detailed explanations: E0277, E0308.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `hello-world` (bin "hello-world") due to 2 previous errors

The compiler incorrectly states that line number 3 is problematic, and the compiler error at line number 4 is slightly different.

[just-an-engineer]

Adding a semi-colon to the final statement in a function turns it from an expression, to a statement, which does not return the value (from https://doc.rust-lang.org/book/ch03-03-how-functions-work.html, at the end of Statements and Expressions, just before Functions with Returns Values). My suspicion is that adding the semi-colon makes it return None, which doesn't conform to the return constraints, and thus is not an issue with the Rust compiler or Tokio.
Let us know if the issue persists after removing the ; from the last statement (Ok(()))

[ik1ne]

@just-an-engineer The compile failure is expected - what is unexpected is the message it generates.
The proc macro expansion converts the async body into async closure which alters the compiler message.

[Darksonn]

I don't know how easy it would be to fix this, but I would be happy to receive a PR for it.

[just-an-engineer]

@ik1ne Ah, my apologies. I'll take a closer look tonight.

[richardpringle]

Tough to figure out the proper way to do this.
But here's an attempt: #6438

I left it as a draft because if I'm way off, I'm not going to dive further into it. I can't get as nice of an error as the compiler would give without the macro though... Or at least I don't quite know how. I don't think the syn::Error::into_compile_error isn't that flexible (yet).

[taiki-e]

I have reviewed many PRs/suggestions for improving error messages in macros, and those that were rejected or not implemented basically adopted one of the following two approaches:

• Annotate the return type by using the return type of the function: Improve diagnostics for mismatched type in async main #3039, macros: improve diagnostics on type mismatch #3766 (comment), Make processing of entrypoints more efficient #4513 (comment), feat: generate function declaration in macro for better type check compile error message #6307, etc.
• Annotate the return type by using type annotations in let: Improve diagnostics for mismatched type in async main #3039 (comment), fix: better spans in #[tokio::main] output #5619, Handle return type properly #6438, etc.

Roughly summarizing the problems and solutions of the problems of these approaches:

• The former can reuse the error messages of async fn as they are, and can produce accurate errors in most cases, but will not work correctly if self or Self is included anywhere, so we must check all inputs, and if contained, we need to use the current approach or other as a fallback.
• The latter does not give as accurate an error message as the former, but there is no need to worry about the presence of self/Self. However, since never type and impl traits are unstable in type annotations in let, we need to check the return type to handle them.
  • Also, there seems to be a pattern of using let outside the async block and a pattern of using let with an additional block inside the async block, which may affect the accuracy of the error message and ease of implementation.

Some complexity is unavoidable with either, but my understanding is that the former has more benefits, but may be more complex in terms of both implementation and testing, as the 2 approaches will need to be retained internally.

If we don't care about the increase in compile-time, the latter implementation in a pattern that uses a let outside the async block is probably the easiest to implement.

[richardpringle]

Thanks for the detailed response @taiki-e!

I'm happy to do the latter if we don't care about compile times, but if we do, then I will close my PR as it isn't taking the right approach.

It's such a catch-22. If the author of a dependent crate cares about compile times, they can easily opt out of using the macro; it's really easy to use the builder to build a runtime. But in the same vein, it's really easy to figure out what's wrong with your code here. If that's the error message that stumps you, I think you'll have a really hard time with some other things.

Is there CI that benchmarks compile times? If so, how can I check the difference from using the visit feature of syn? I think it would make sense to add to the docs that the main macro might hurt compile times, optimizing said macro for beginners to get the best (as good as possible) error messages, getting them off the ground running as quickly as possible.

You (we?) could even add another macro with better compile performance if you (we?) wanted to.

If one of you (the maintainers) wants to make a judgment call, I'm happy to proceed by either finishing this up or closing it. I really just stumbled upon it and have been enjoying a little metaprogramming lately, so I figured I would make an attempt.

[Darksonn]

Compile-time regressions for users of the macro is one thing, but if we also hurt the compile-times of people who just enable the macros feature flag, then it's a bigger problem. And if the cause of a compile-time regression is enabling a feature on syn, well, then it sounds like it would affect anyone who enables the feature flag.

Are those extra syn features really so critical?

We don't have benchmarks for the macro compile-times, unfortunately.

[richardpringle]

@Darksonn, I don't think the PartialEq is critical, we can always compare strings... the visit features is a bit of a different story. I can tell you now that I don't have the time to re-implement walking the token-tree. I can try a couple things to see if I can avoid it and suppress compiler warnings when I have a () in place of a !, but I'm not hopeful.

I might have to drop the attempt at solving this issue... 😞