korken89 / smlang-rs

A State Machine Language DSL procedual macro for Rust
Apache License 2.0
202 stars 28 forks source link

How-to? Mutable event data #13

Closed MathiasKoch closed 3 years ago

MathiasKoch commented 3 years ago

Hi,

I have an application where i would like to wrap a &mut [u8] in my event data, but if i do this currently it fails with expected &mut [u8], found &&mut [u8] due to the ref here: https://github.com/korken89/smlang-rs/blob/48c660df3ed0b8098d17e0e52e8c536f60e01b21/macros/src/codegen.rs#L90

Not sure what the proper way would be, or if it is me who is doing something i am not supposed to?

MathiasKoch commented 3 years ago

I have looked a bit more into this, and found some more lifetime stuff i don't think is optimal?

Currently both guards and actions receive arguments by reference, which makes it very hard to make a state transition on the form:

statemachine! {
    // Initialization states
    StateB(Context) + EventA [guard_fn] / action_fn = StateC(Context)
}

if Context is not Clone/Copy or just expensive to clone.

I would propose to change this, such that guards will take state_data & event_data by reference, and actions will always take state_data by value, and event_data depending on the definition in statemachine!: EventA(&'a EventData) will provide EventData by reference to both guard & action, while EventA(EventData) will provide EventData by reference to guard, and by value to action.

This change should also fix the OP issue of mutability. The only thing i can't quite figure out how to wrap, is the reference wrapper example with internal lifetime: pub struct MyReferenceWrapper<'a>(pub &'a u32);

korken89 commented 3 years ago

Indeed, one could choose to go with moving instead of references, the interface today does a lot of assumptions on usage to get it to go together as all the possible uses are a lot to handle.

If you have ideas I'm all open to improving the interface! :)

Yatekii commented 3 years ago

I am not sure Move is a performance improvement over Copy here. The compiler uses both interchangeably under the hood if it results in an effective Move anyways :)

For ergonomics its most certainly much better though, so taking it by value sounds good :)

MathiasKoch commented 3 years ago

I wouldn't want to force move semantics, but rather just leave it up to the user of the library instead of forcing by reference everywhere

Yatekii commented 3 years ago

Sounds good :)

MathiasKoch commented 3 years ago

Hmm, playing a bit with this it might not be possible.

Take this simple expanded state machine as example:

pub fn process_event(&mut self, event: Events) -> Result<&States, Error> {
    match self.state {
        States::State1 => match event {
            Events::Event1 => {
                let _data = self.context.action();
                self.state = States::State2(_data);
                Ok(&self.state)
            }
            _ => Err(Error::InvalidEvent),
        },
        States::State2(state_data) => match event {
            Events::Event2 => {
                self.state = States::State1;
                Ok(&self.state)
            }
            _ => Err(Error::InvalidEvent),
        },
        _ => Err(Error::InvalidEvent),
    }
}

Because self is borrowed, it will yield cannot move out of 'self.state.0' which is behind a mutable reference without changing States::State2(state_data) to States::State2(ref state_data)