Let's talk macros

[piegamesde]

To bridge the time until we have proc macros (if ever), let's make some small macros that make matching arguments more convenient.

macro_rules! first_arg {
    ($signal:expr, $var:ident: $type:ty) => {
        let signal: woab::Signal<_> = $signal;
        let $var: $type = signal.param(0)?;
    };
}

macro_rules! all_args {
    ($signal:expr, $($var:ident: $type:ty),+) => {
        let signal: woab::Signal<_> = $signal;
        /* This is not hygienic and will put index into scope */
        let mut index = 0;
        $(
            let $var: $type = signal.param(index)?;
            index += 1;
        )+
        /* Make sure we at least can't accidentally use it, creating subtly wrong behavior */
        std::mem::drop(index);
    };
}

I'm working on a signal! macro right now that generates the whole match statement, but it's a bit more difficult. Sadly, we can't generate match arms without generating the whole match statement around it. Here's my rough WIP:

macro_rules! signal {
    ($signal:expr => {
        $(($handler:expr$(, $var:ident: $type:ty)*) => $content:tt),*
        $(,)?
    }) => {
        let signal: woab::Signal<_> = $signal;
        match signal.name() {
            $($handler => {
                $(let $var: $type = signal.param(0)?;)*
                $content
            }),*
        }
    };
}

Apart from quality of life improvements (optional trailing comma, optional braces and brackets, better hygiene), the most important thing missing is to check the number of arguments. If we don't match all args, we better be explicit about this.

Btw the index variable could be avoided if woab::Signal exposed a slice of the parameters to iterate upon. Also, we need a method that returns self.parameters.len().

[idanarye]

Naively adding the option to iterate on all parameters is easy enough - I mean, we do have that data - but param() puts the signal name and the parameter index into the error, which I would like to preserve in our custom iterator. Still very doable - but we'll need to design the semantics. As for adding the len() - that's also easy, but if the goal is to ensure we got all the parameters we can always just check that the iterator has no more items.

Is first_arg! really necessary? Is this

first_arg!(signal, widget: gtk::Button);

Really more ergonomic than this?

widget: gtk::Button = signal.param(0)?;

I see the benefit of all_args!, but I don't think first_arg! justifies a macro.

Also - proc macros are not really that hard. They are harder than basic macro_rules! macros, but once the macro gets complicated it can be easier to do things straightforwardly in a proc macro than trying to hack weird macro_rules! patterns.

[piegamesde]

I see the benefit of all_args!, but I don't think first_arg! justifies a macro.

Yeah, I was experimenting with things. I also thought about special-casing the "all arguments except the first one", because that's a common pattern as well.

[idanarye]

Instead of having special syntax for skipping the first argument, I want to support _. So, using your syntax:

impl actix::Handler<woab::Signal> for SomeActor {
    type Result = woab::SignalResult;

    fn handle(&mut self, msg: woab::Signal, _ctx: &mut Self::Context) -> Self::Result {
        Ok(signal!(msg => {
            // When we don't care about first argument:
            ("move_cursor", _, movement_step: gtk::MovementStep, count: i32, extend_selection: bool) => {
                // ...
                None
            }
            // When we care about first argument:
            ("insert_at_cursor", entry: gtk::Entry, text: String) => {
                // ...
                None
            }
        }))
    }
}

[idanarye]

I think it should be possible, and not that hard, to have something like this:

impl actix::Handler<woab::Signal> for SomeActor {
    type Result = woab::SignalResult;

    fn handle(&mut self, msg: woab::Signal, _ctx: &mut Self::Context) -> Self::Result {
        #[woab::handler]
        match msg {
            "signal1" => |_, foo: Type1, bar: Type2| {
                // ...
                // No need for return value
            },
            "signal2" => |_, baz: Type3| {
                // ...
                gtk::Inhibit(false)
            },
            "signal3" => |_, qux: Type4| {
                // ...
                Ok(()) // so that we can raise errors inside the signal
            },
        }
    }
}

The main benefits of this style is that even without the macro it's a perfectly valid Rust code with only one problem - it doesn't typecheck. Moreso - the only thing that fails the typecheck is the closures' results which means (I suspect - I haven't actually checked) that completion should work perfectly.

I even think this style could be our final (as anything is final in software development) style, instead of method-per-signal. Our current method-per-signal concept looks something like this:

#[woab::handler]
impl SomeActor {
    fn signal1(&mut self, _ctx: &mut actix::Context<Self>, _: (), foo: Type1, bar: Type2) {
        // ...
    }

    fn signal2(&mut self, _ctx: &mut actix::Context<Self>, _: (), baz: Type3) -> gtk::Inhibit {
        // ...
        gtk::Inhibit(false)
    },

    fn signal3(&mut self, _ctx: &mut actix::Context<Self>, _: (), qux: Type4) -> Result<(), woab::Error> {
        // ...
        Ok(())
    }
}

I honestly think my new suggestion is superior, for the following reasons:

1. With macro-on-match, most of the boilerplate can be filled by rust-analyzer. All you have to do is type:

   impl actix::Handler<woab::Signal> for SomeActor {
   }

   And ask rust-analyzer to fill the missing impl items, and it'll give you:

   impl actix::Handler<woab::Signal> for SomeActor {
       type Result;
   
       fn handle(&mut self, msg: woab::Signal, ctx: &mut Self::Context) -> Self::Result {
           todo!()
       }
   }

   At which point you just have to add the = woab::SignalResult after the type Result and replace the todo!() with the match skeleton:

   impl actix::Handler<woab::Signal> for SomeActor {
       type Result = woab::SignalResult;
   
       fn handle(&mut self, msg: woab::Signal, ctx: &mut Self::Context) -> Self::Result {
           #[woab::handler]
           match msg {
           }
       }
   }

   And that's it - you are ready to add the signals.

   With macro-on-impl, the boilerplate is on each and every individual signal, and rust-analyzer can't do it for you. You'll have to manually add the _ctx: &mut actix::Context<Self> argument to each method, and if there is a tag - you'd have to add it too. The macro cannot access the types (behind reading their lexical tokens) and must rely on the argument's positions.

2. We could, technically, use _: () like we do with the first parameter - but that can get real ugly real fast:

   #[woab::handler(tag = usize)]
   impl SomeActor {
       // Needs the context
       fn signal1(&mut self, ctx: &mut actix::Context<Self>, _: (), _: (), foo: Type1, bar: Type2) {
           // ...
       }
   
       // Needs the tag
       fn signal2(&mut self, _: (), tag: usize, _: (), baz: Type3) -> gtk::Inhibit {
           // ...
           gtk::Inhibit(false)
       },
   
       // Needs the widget
       fn signal3(&mut self, _: (), _: (), widget: gtk::RadioButton, qux: Type4) -> Result<(), woab::Error> {
           // ...
           Ok(())
       }
   }

3. Speaking of which - I've yet to confirm it, but I'm pretty sure we are going to need _: () for the macro-on-impl because typeless methods are invalid and attribute macros only work on perfectly valid Rust code (it just doesn't have to typecheck). With macro-on-match we can use _ because typeless arguments are perfectly valid for closures.

4. The return type too, will need to be added to each method separately. This can get quite horrible when we need ResponseActFuture:

   #[woab::handler]
   impl SomeActor {
       #[handler(async)] // we need to somehow mark them, because the macro treats them differently
       fn signal1(&mut self, ctx: &mut actix::Context<Self>, _: (), _: (), foo: Type1, bar: Type2) -> ResponseActFuture<Self, Result<(), ()>> {
           // ...
           Box::pin(async move {
               // ...
           }
           .into_actor(self))
       }
   }

   Compared to

   impl actix::Handler<woab::Signal> for SomeActor {
       type Result = ResponseActFuture<Self, woab::SignalResult>;
   
       fn handle(&mut self, msg: woab::Signal, ctx: &mut Self::Context) -> Self::Result {
           #[woab::handler]
           match msg {
               "signal1" => |_, foo: Type1, bar: Type2| {
                   // ...
                   async move { // no need to Box::pin - it can happen after the closure
                       // ...
                   }.into_actor(self)
               },
   
               // here we'll have to mark the non-async signals - though I do hope we can avoid
               // this once specialization gets in.
               #[handler(!async)]
               "signal2" => |_, _| {
                   // ...
               },
           }
       }
   }

[piegamesde]

You make some pretty good points. I really like the closure syntax, but I haven't managed to get it working using plain macros. It always complains that the parsing is ambiguous.

I think one could reduce the biggest pain points of the function derive macro approach by only specifying required arguments, and somehow annotate their index using attributes.

EDIT I just remembered that the code needs only be syntactically correct when using an attribute derive macro. One can as well use a macro! {} style approach that contains arbitrary tokens to generate an fn item or even an impl block.

[idanarye]

Yes - we can have any syntax we want with expression macros - but that doesn't mean we should. By sticking to proper Rust syntax as much as we can we can both let users work within the familiar realm and allow IDE/LSP tools do their job.

I'm not convinced the advantages (what are they, even? Other than less nesting?) of having a method per signal justify breaking conventional Rust syntax that much with the macro.

[idanarye]

I was thinking a bit more about supporting async signal handling, and realized something - we shouldn't support them.

I'm not talking about ctx.spawn() - this is totally fine. But we should not support ResponseActFuture, and even though we can't prevent it with the non-macro solution - the macro should not allow it, and therefore does not need special syntax to support it.

ResponseActFuture means that the message that was sent to the actor still needs more handling before it can return to the Future you got from address.send(). But where is this Future? In the signal handler! The signal blocks on that Future, waiting for it to return!

So, if you use ResponseActFuture you are blocking the GTK loop until you are done doing whatever you are doing. The GUI will freeze! There should be no reason to do this, and if someone wants to do this I am perfectly fine with preventing them from doing it with the macro, forcing them to use the non-macro style for all signals handled by that actor. Not that the non-macro style is that bad, but maybe having to do this will make them think twice if blocking the GTK loop is really the best way to do what they want to do.

[piegamesde]

I totally agree; I never even thought about asynchronous signal handling. GTK signals are synchronous, and thus actix::Handler<woab::Signal> is too. You can still use the actix features to spawn asynchronous background tasks and that's totally fine.