Fixing the internal state management, reverting actions to take references to state data, cleaning up codegen

[ryan-summers]

Fixes #75 by reconfiguring the state if the guard fails. We were unintentionally bailing immediately with no internal state configured due to the ? operation.

[ryan-summers]

cc @korken89 / @dzimmanck / @pleepleus - I don't think I can review/approve/merge without one of you guys signing off

[korken89]

Nice catch! If you get the changelog fixed I'll merge it :)

[korken89]

Just for my understanding, why is in-place replacing the state is not used? It would catch these errors at compile-time.

[ryan-summers]

It appears that the in-place swap-over of the state to take() approach happened in https://github.com/korken89/smlang-rs/pull/49/files#diff-5b70e2aae1cde8f581e01e9a15ac4f570e28b80ca2fb59d2911fd1ac99a30bd9L529. Comments in that PR indicate it was necessary to avoid an unwrap() in the code, but I suspect this may not be the case. Let me review to see if we can update it to generate code with in-place management of the state.

With respect to the CHANGELOG, would you still prefer this existed there? Generally, in other projects, when I introduce a bug and then fix it before said bug is released, I haven't included it in the CHANGELOG to prevent noise. I'm happy to add it in, just wanted to clarify first

[korken89]

Ah sorry I thought the bug was existing between releases, no changelog needed :)

I generally use the following create to do in place state management. Though it was not added at the std lib for good reason. https://crates.io/crates/replace_with

Though this is not the correct PR to change it.

A closure that works on top of the option would also give the same guarantees and not tred on the limits of what Rust can do. :)

[ryan-summers]

Edit: The Option<State> change happened because actions were updated to take ownership of internal state data to allow the data to be transferred to a new state.

However, the more I'm thinking about that, the more I think that we should perhaps revert #49. If a user has a large, non-clonable state data, I imagine they could put that in some type of Box (or static storage location) and just pass a ref to it inside the various states instead.

Will take a look at the closure based approach though. I agree that #49 has made the internal state pretty fragile. It seems nonsensical that there can be None state.

[korken89]

Yeah, to me I like replace_with just because it makes the compiler enforce that all transitions are correct and you don't need any intermediate representation. Using an option is fine though as well with the closure support, it will give the same result without unsafe. :)

Here is an implementation example: https://github.com/rust-lang/rfcs/pull/1736#issuecomment-251673355

[ryan-summers]

There's actually some issues with #49 specifically w.r.t fallible actions. If the action is provided the internal state data (owned), but the action fails (see https://github.com/korken89/smlang-rs/pull/73), there's no way to revert the internal state back to the original (because the action owned the internal state data, it is now irretrievably lost).

This shows up when trying this closure-based approach, as you can't pass the starting_state to the closure and then use it in a self.state.replace(starting_state) after the closure executes because starting_state is moved.

I think the only valid approaches would be to revert #49 or to use replace_with

[korken89]

Yeah, you really need to be careful when you can move stuff out. Which way to go is not clear to me, I'm not sure how much work it works be to use replace_with. The revert is the easy option :)

[ryan-summers]

Looking at it further, replace_with actually has the same draw-back. You get ownership of the internal state data in the replace_with closure, but you still lose it if you pass it to action and action fails, so you can never provide the same state data outwards. replace_with doesn't let you simply not replace the value.

I think the only fix here is to remove ownership of internal state data in action. The mechanism necessary to move data (without cloning) between states would be to Box it (i.e. make an owned ref to it) and clone the Box type to the new state.

[korken89]

Alright, then the path is set! :)

[ryan-summers]

Alright, the reversion should be complete here. Thanks for the discussion @korken89 :)

[korken89]

Thanks for doing the analysis! :D