Cannot use `select!` with Receiver contained in a struct #12902

Open
quux00 opened this Issue Mar 15, 2014 · 13 comments

Projects

None yet

9 participants

@quux00
quux00 commented Mar 15, 2014

The select! macro doesn't work if you try to reference a Receiver (Port) in a struct. huon reviewed this in the IRC room and agreed I should file an issue on it.

Here are two attempts to do this and both fail at compile time:

use std::io::Timer;

struct A {
    c: Sender<~str>,
    p: Receiver<~str>
}

fn does_not_compile() {
    let (ch, pt): (Sender<~str>, Receiver<~str>) = channel();
    let a = A{c: ch, p: pt};
    let mut timer = Timer::new().unwrap();
    let timeout = timer.oneshot(1000);
    select! (
        s = a.p.recv() => println!("{:s}", s), // error: no rules expected the token `.`
        () = timeout.recv() => println!("time out!")
    );
}



fn also_does_not_compile() {
    let (ch, pt): (Sender<~str>, Receiver<~str>) = channel();
    let a = A{c: ch, p: pt};
    let mut timer = Timer::new().unwrap();
    let timeout = timer.oneshot(1000);
    let p = &a.p;
    select! (
        s = p.recv() => println!("{:s}", s), // see error below
        () = timeout.recv() => println!("time out!")
    );
}

/*
<std macros>:7:37: 7:41 error: mismatched types: expected `&std::comm::Receiver<<generic #45>>` but found `&&std::comm::Receiver<~str>` (expected struct std::comm::Receiver but found &-ptr)
<std macros>:7 $( let mut $rx = sel.handle(&$rx); )+
^~~~
<std macros>:1:1: 15:2 note: in expansion of select!
selfport.rs:21:5: 24:7 note: expansion site
selfport.rs:22:42: 22:43 error: cannot determine a type for this bounded type parameter: unconstrained type
selfport.rs:22 s = p.recv() => println!("{:s}", s), // error: no rules expected the token `.`
*/
@huonw
Member
huonw commented Mar 15, 2014

cc @alexcrichton

Maybe the syntax could be changed to something like

select! {
    s from a.p => ...,
    () from timeout => ...,
}

so that the port can be a full expression. (Although, those idents are used for new variable names... so this may be tricky.)

@alexcrichton
Member

The macro is what tripped me up when I wrote, this. Ideally you want something like:

$expr . $ident ()

But $expr will swallow the ident, so I had to force the left-hand side to be an ident. But this is all why this select! macro is experimental.

@quux00 quux00 referenced this issue in lenary/cs4414-project Apr 20, 2014
Merged

Log repair #4

@ghost
ghost commented Jun 14, 2014

I've hit this a few times myself. Would it be OK for the select! macro to be implemented as a syntax extension? It'd then be possible to allow an arbitrary expression on the LHS of => and extract the method name at the AST level. It could also provide better error messages on errors such as leaving out the method name.

@apoelstra
Contributor

I've hit this and it seems I can't move forward until it's resolved. (Well, "can't move forward" is strong; for now I'll just unwrap the macro manually.)

Is there any consensus on how this should be resolved? I'm happy to cut code but it's seems pretty open what direction to go in.

@apoelstra
Contributor

Here is my suggestion:

select!{
  obj1 from rx1 => {
    // Do stuff with obj1
  },
  obj2 from rx2 => {
    // Do stuff with obj2
  }
}

If people want to use something other than .recv() they can, using the alternate syntax

select!{
  obj1_opt from rx1 using recv_opt => {
    // Do stuff with obj1_opt
  },
  obj2_opt from rx2 using recv_opt => {
    // Do stuff with obj2_opt
  }
}

Here is a sample implementation which makes no use of the Select structure or unsafe code or allocations. Thanks much to @eddyb on IRC for helping to clean this up.
https://gist.github.com/apoelstra/1b12e849f537485e970b

Note that this is "all or nothing" regarding the optional using syntax. Maybe someone with better macro-fu can come up with one where some options can have using and others not.

@mackwic
Contributor
mackwic commented Jul 30, 2014

I just hit this bug on my code. Had to expand all our use of select! because of ownership errors.

Maybe multiple macros for multiple uses will do the trick ?

@apoelstra
Contributor

Hi guys, I have been using the select! implementation in my own code for a month now and it seems to be working ok. What needs to be done to get it merged?

@thehydroimpulse
Contributor

@apoelstra Did you ever send a PR for it?

@apoelstra
Contributor

No, I'll do that now.

@apoelstra apoelstra added a commit to apoelstra/rust that referenced this issue Sep 27, 2014
@apoelstra apoelstra Remove `Select` structure; implement inline `select!` macro
The old `Select` structure was very inefficient and required the user
to use unsafe code (and manually maintain a "this variable will never
move" invariant). Searching through several popular repositories, the
only instance I found of it being used was in servo --- to avoid the
setup cost of putting `select!` in a loop, which would create a new
`Select` on every iteration.

This commit deletes the `Select` structure entirely and moves all of
its code into the `select!` macro. The benefit of this is that there
is no more need for allocations, no more need for unsafe code, and
no setup costs (so servo can now use `select!` directly).

This also changes the interface to select! to fix #12902.

Fixes #12902.

[breaking-change]
c15ac5c
@steveklabnik steveklabnik added the A-libs label Feb 14, 2015
@tripped
tripped commented Aug 19, 2015

I just ran into this issue as well. It's especially vexing since the struct that contains the receivers must be borrowed as a mutable reference inside the select! -- so this fails:

fn do_something(&mut self) {
    let r1 = &self.r1;
    let r2 = &self.r2;
    select! {
        a = r1.recv() => a.unwrap()(self), // ERROR! self.r1 is already borrowed!
        b = r2.recv() => (),
    }
}

It seems that the only reason select! only allows expressions of the form "ident.ident()" is so that the macro can use that first ident as a unique local variable name inside the expansion.

This could be solved if we could generate unique symbols in a macro expansion. Issue #9031 mentions this but was closed back when macro hygiene was implemented. Sadly, macro hygiene does not help here, since all the identifiers we need to distinguish have the same syntax context.

I've been poking and prodding at the macro syntax trying to figure out a way to do this that isn't horrible, special-cased, or just plain inconvenient, but I'm not sure it's reasonable to do without the ability to generate symbols. Maybe we do need a syntax extension after all?

@tripped
tripped commented Aug 19, 2015

Looking at it with a little less sleep deprivation, I think the current syntax of the select! macro just doesn't make sense. It uses a matcher for the method invoked on the receiver -- ostensibly supporting receivers whose receive method is spelled differently than recv()? However, it does not use that method name to invoke a member of the receiver named by $rx:ident, it invokes it on a Handle, which only supports recv(). (And Handle's implementation requires that the thing selected on be a Receiver<T> and calls its .recv()).

Removing the spurious .recv() from the required syntax does free up the macro to use an expr for the receiver. Then the only remaining difficulty is generating a unique variable name for each handle in the expansion. Perhaps a slight modification to the macro system would help here? I may just try to hack something together and see if it works.

@tripped
tripped commented Aug 19, 2015

Apparently I was wrong about not being able to do this with the current macro system. User talchas on #rust came up with this formulation that allows the use of an arbitrary expression as the receiver in a select! expansion: http://is.gd/F6Wy5D

A little bit hairier than the original, and it doesn't solve my borrowing problems, but I think it does make the macro much more generally useful. Any thoughts?

@jFransham
Contributor

@tripped just a simple change to your suggestion, to stop macro clutter (this style is used in a bunch of internal macros) http://is.gd/bMLJK9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment