korken89 / smlang-rs

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

Reusing actions with states containing inconsistent data doesn't work #36

Closed ryan-summers closed 2 years ago

ryan-summers commented 2 years ago

When an action is used for multiple transitions, but the initial state does not contain consistent data, the codegen is incorrect. The same action will be called with a different number of input arguments.

E.g.:

transitions: {
    *Start + Event1 / action = State1(u32),
    State1(u32) + Event1 / action = State2(u32),
}
ryan-summers commented 2 years ago
error[E0050]: method `action` has 1 parameter but the declaration in trait `StateMachineContext::action` has 2
  --> examples\reuse_action.rs:20:15
   |
9  | / statemachine! {
10 | |     transitions: {
11 | |         *State1 + Event1 / action = State2(u32),
12 | |         State2(u32) + Event1 / action = State3(u32)
13 | |     }
14 | | }
   | |_- trait requires 2 parameters
...
20 |       fn action(&mut self) -> u32 {
   |                 ^^^^^^^^^ expected 2 parameters, found 1

error[E0061]: this function takes 1 argument but 0 arguments were supplied
  --> examples\reuse_action.rs:11:28
   |
11 |         *State1 + Event1 / action = State2(u32),
   |                            ^^^^^^- supplied 0 arguments
   |                            |
   |                            expected 1 argument
   |
note: associated function defined here
  --> examples\reuse_action.rs:12:32
   |
12 |         State2(u32) + Event1 / action = State3(u32)
   |                                ^^^^^^

Some errors have detailed explanations: E0050, E0061.
For more information about an error, try `rustc --explain E0050`.
error: could not compile `smlang` due to 2 previous errors
ryan-summers commented 2 years ago

My best guess here is that the action is being supplied state data from State1 in the second call, but since Start has no data, the first call doesn't get any state dat, which implies codegen is generating different signatures for action depending on which initial state it's called from.

ryan-summers commented 2 years ago

This is particularly annoying for a wildcard state + action, since this results in the wildcard not being usable if you have any states with data.

pleepleus commented 2 years ago

Hey @ryan-summers,

Thanks for the issue report. I have an idea for supporting state data for the initial state by generating different function signature for ::new() that accepts initial data for the initial state if you define it to contain some. Would this work for you? If so, I will prioritize its inclusion in a release as it is a pretty simple change that will be reverse compatible with all compiling projects.

ryan-summers commented 2 years ago

No, that's not what I'm referring to. I have an issue where some of my states contain data, but some don't. Some states contain an Instant indicating when they're going to complete and transition to the next state, while other states (e.g. Fault(reason)) contain entirely different data (or no data at all). However, my goal was to use a single action for transitioning from these various states into some known state.

E.g: _ + Reset / reset_state = Init isn't possible if you have one state with no data, but one state with data.

pleepleus commented 2 years ago

@ryan-summers,

I see. The use of the "reset_state" action forces you to use consistent data in your states to feed the argument signature of your action. I see a few options here.

  1. You use "|" pattern matching and only use states which have the data that needs to feed into your reset_state action.
  2. We could redesign the wildcard to mean only states which match the action signature when an action is used in the transition.
  3. You wrap your state data in an Enum (states with no data would have None).

@ryan-summers, would any of these options work well in your application?

ryan-summers commented 2 years ago

Pattern matching would be one approach - I didn't realize that was possible with the API.

My main concern is that the error displayed by the compiler doesn't describe at all what's going wrong to the end user. Rather, the proc-macro just blindly generates multiple function signatures that conflict with each other, which is wrong and difficult to debug. I'm fine implementing multiple actions with different signatures to handle different initial state data, I just had no idea it was required at first and the compiler didn't help me at all.

My goal raising this issue wasn't to resolve the issue, but more to add actual warnings to the user that what they're trying to do is not possible instead of generating invalid proc-macro'd code.

dzimmanck commented 2 years ago

@ryan-summers,

Thanks for the pull request. It looks really good! I had a few minor housekeeping requests, but other than that I think it is good to go.