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

Erroneous compiler error message for extra semicolon with try operator #6306

Open
ik1ne opened this issue Jan 26, 2024 · 9 comments
Open

Erroneous compiler error message for extra semicolon with try operator #6306

ik1ne opened this issue Jan 26, 2024 · 9 comments
Labels
A-tokio-macros Area: The tokio-macros crate C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. M-macros Module: macros in the main Tokio crate

Comments

@ik1ne
Copy link
Contributor

ik1ne commented Jan 26, 2024

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.

@ik1ne ik1ne added A-tokio Area: The main tokio crate C-bug Category: This is a bug. labels Jan 26, 2024
@Darksonn Darksonn added A-tokio-macros Area: The tokio-macros crate M-macros Module: macros in the main Tokio crate and removed A-tokio Area: The main tokio crate labels Jan 26, 2024
@ghost
Copy link

ghost commented Feb 12, 2024

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
Copy link
Contributor Author

ik1ne commented Feb 12, 2024

@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
Copy link
Contributor

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

@Darksonn Darksonn added the E-help-wanted Call for participation: Help is requested to fix this issue. label Feb 12, 2024
@ghost
Copy link

ghost commented Feb 12, 2024

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

@richardpringle
Copy link

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
Copy link
Member

taiki-e commented Mar 29, 2024

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:

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
Copy link

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
Copy link
Contributor

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
Copy link

@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... 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-macros Area: The tokio-macros crate C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. M-macros Module: macros in the main Tokio crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants