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

Select macro incorrectly binds expression result as shared reference #4076

Closed
tobz opened this issue Aug 27, 2021 · 6 comments · Fixed by #4211
Closed

Select macro incorrectly binds expression result as shared reference #4076

tobz opened this issue Aug 27, 2021 · 6 comments · Fixed by #4211
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Experience needed to fix: Medium / intermediate M-macros Module: macros in the main Tokio crate

Comments

@tobz
Copy link
Member

tobz commented Aug 27, 2021

Version
tokio 1.10.1

Description
When using select! and taking advantage of the built-in pattern matching i.e. Some(foo) = fut_here(), the macro incorrectly binds the result as a shared reference, which turns into confusing errors like this:

192 | /         select! {
193 | |             // TODO: why the hell is this coming out as &S3Request?!?!
194 | |             Some(mut req) = rx.recv() => {
    | |                  -------
    | |                  |
    | |                  data moved here
    | |                  move occurs because `req` has type `S3Request`, which does not implement the `Copy` trait
195 | |                 let seqno = seq_head;
...   |
236 | |             else => break
237 | |         }
    | |_________^

The full code can be seen here, but rx is Receiver<S3Request>, so clearly the returned value is/should be owned. @Darksonn pointed out that the code in the macro responsible for this issue is here.

Note to whoever is interested in fixing it: @Darksonn also pointed out that changing from match &out { to match &mut out { would likely be a suitable fix.

@tobz tobz added C-bug Category: This is a bug. A-tokio Area: The main tokio crate labels Aug 27, 2021
@Darksonn Darksonn added the M-macros Module: macros in the main Tokio crate label Aug 27, 2021
@blasrodri
Copy link
Contributor

This update

diff --git a/tokio/src/macros/select.rs b/tokio/src/macros/select.rs
index a90ee9eb..88c26adb 100644
--- a/tokio/src/macros/select.rs
+++ b/tokio/src/macros/select.rs
@@ -502,7 +502,7 @@ macro_rules! select {
                                 let mut fut = unsafe { Pin::new_unchecked(fut) };
 
                                 // Try polling it
-                                let out = match fut.poll(cx) {
+                                let mut out = match fut.poll(cx) {
                                     Ready(out) => out,
                                     Pending => {
                                         // Track that at least one future is
@@ -519,8 +519,8 @@ macro_rules! select {
                                 // the specified pattern.
                                 #[allow(unused_variables)]
                                 #[allow(unused_mut)]
-                                match &out {
-                                    $bind => {}
+                                match &mut out {
+                                    $bind => {},
                                     _ => continue,
                                 }
 

allows me to do the following:

    Some(ref mut req) = rx.recv() => {...}

However, I still cannot get the following pattern:

    Some(mut req) = rx.recv() => {...}

@Darksonn
Copy link
Contributor

Darksonn commented Sep 2, 2021

Try using the matches! macro instead.

@blasrodri
Copy link
Contributor

Try using the matches! macro instead.

Do you mean

if !matches!(&mut out, $bind) {continue}

same result

@Darksonn
Copy link
Contributor

Darksonn commented Sep 2, 2021

Hmm, yeah I tried a few things. It doesn't seem like we can get it to work. We probably have to change the macro to not require this check in the first place.

@blasrodri
Copy link
Contributor

Hmm, yeah I tried a few things. It doesn't seem like we can get it to work. We probably have to change the macro to not require this check in the first place.

Any suggestions? I tried playing around and couldn't find an easy way.

@Darksonn
Copy link
Contributor

Darksonn commented Sep 6, 2021

I don't think there's an easy way.

@Darksonn Darksonn added E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Experience needed to fix: Medium / intermediate labels Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Experience needed to fix: Medium / intermediate M-macros Module: macros in the main Tokio crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants