-
Notifications
You must be signed in to change notification settings - Fork 28
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
Support multiple guarded transitions triggered by the same event #72
Conversation
We have opted explicitly to deviate with the return type of guards because other languages (namely C++ that this was based on) had no notion of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please add a CHANGELOG.md entry for this change
- Please run this through
cargo fmt
- I think I saw some places without proper spacing? - Please address failing CI tests: https://github.com/korken89/smlang-rs/actions/runs/9408744685/job/26521140660?pr=72
Overall, the changes look sound and solid, but I'm confused by the decision to migrate from Result
to bool
. Is there a reason you've made this choice instead of using Result
combinatorics to combine multiple Result
types?
I see your point, and I agree that with only one transition per (state,event) pair enabled, this logic makes sense. One detail to note is that in the original implementation, Result<(),()> is used. The second unit type () does not provide additional information on what you refer to as a guard failure. In fact, as per SML, the guard is intended to enable or disable a transition. This is indeed somewhat similar to bool (true/false), rather than to an Ok/Error result. When we switch to multiple transitions, this difference becomes more important. For example, consider the following:
Say
Other things to mention: this is more compatible with the SM UML spec as well as with the BoostSML. I hope this will convince to support this change. |
added unit tests for guard expressions parser |
…le_state_event, add wildcard_after_input_state
Hmm you make a good point that using
I think that it's important that guards are able to return a result. If you want a functional example of a state machine using fallible guards, take a look at https://github.com/quartiq/minimq/blob/master/src/mqtt_client.rs#L181 There is some complexity with this design about guard execution - what happens if one passes and we don't need to evaluate other guards? What if a guard fails after we've already determined some acceptable condition? I'd personally say:
|
The three state approach (enabled/disabled/ failed with an error) to the guards makes sense. I also thought about this option, and I had prepared the change to switch to Result<bool,_> . |
This approach seems fine to me :) Thanks for the discussion! I'll take a look at all the changes now. |
Agree on 2. On 1: Evaluate guard expressions for all such transitions, ensuring there are no conflicts (i.e., only one expression returns true). If no conflicts are detected, select the transition. Evaluate guard expressions in the order they appear in the state machine definition until either a successful transition is found or an error is thrown (errors will be delivered with the ? operator). The first approach has a slight advantage in that it can detect guard conflicts at runtime. However, its usefulness is debatable. Conflict detection at runtime does not guarantee that the logic is conflict-free; the conflict may still exist but never occur with the given data. On the other hand, this approach comes with a cost, as evaluating guard expressions for all transitions (for a given state and event) might be expensive. The second approach does not detect conflicts but seems more practical, as it does not carry the overhead. I would vote for the second approach as a reasonable tradeoff. |
Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks mostly good, just a few minor nitpicks. It would also be good if we could update doucmentation in the README around this.
You'll also need to run |
@ryan-summers thanks for the review! I've made the changes accordingly |
Please check the clippy output: https://github.com/korken89/smlang-rs/actions/runs/9682709221/job/26716714317?pr=72 - other than that, the changes look good. |
oops, sorry , will check clippy |
@ryan-summers : please see clippy errors fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on this!
This PR adds two new features:
Originally, only a single guard function was allowed, i.e., expressions like [guard1], where guard1 was a function returning Result<(), Error>. In fact, these are not exactly guards. Guards are normally defined as boolean functions/expressions that enable/disable transitions rather than allow the transitions to fail or not fail.
Therefore, this PR changes the syntax of a guard function's return type to bool and allows multiple transitions for a triggering event, so that it triggers the transition whose guard is enabled. Additionally, the extended syntax allows the use of guard expressions, enabling the combination of different guard functions in these expressions. Example from a unit test:
I hope these changes will add value to this project. Also, as this project was originally inspired by the BoostSML library, I should mention that the proposed changes will make the syntax closer to compatibility with BoostSML.