dart-statemachine icon indicating copy to clipboard operation
dart-statemachine copied to clipboard

Feature: Illegal Transitions

Open sebastianhaberey opened this issue 2 years ago • 11 comments

Hi, I just tried out the plugin but I couldn't figure out how to define legal vs illegal transitions. I would say they are one of the main reasons to use a state machine. Did I just oversee the feature? Or if not, maybe there's a reason for their absence?

sebastianhaberey avatar Oct 01 '21 09:10 sebastianhaberey

State transitions are initiated programmatically through someState.enter(). If you want to condition this you could wrap such code into a method that performs additional checks?

renggli avatar Oct 01 '21 13:10 renggli

That's definitely possible. But it would mean having one method per state that checks the pre-conditions by looking at the previous state. That would be pretty generic boilerplate code that looks the same in every project that uses the plugin.

Thats why the other state machine plugin has state transition checks built in:

StateTransition turnOn = light.newStateTransition('turnOn', [isOff], isOn);

Sadly that plugin is not up to date (not null-safe). Anyways this is just a suggestion, I solved my problem by implementing a dumbed-down state machine, so I'm good. Feel free to close this!

sebastianhaberey avatar Oct 01 '21 13:10 sebastianhaberey

I am definitely open to expand this library, I just don't quite understand the need. Happy to see what can be done, if you could provide a minimally reproducible example?

renggli avatar Oct 01 '21 15:10 renggli

Hi,

I created a guard solution, that allows users to define guards for transitions. Effectively creating legal/illegal transitions.

Guards are implemented in the Guard class, and the conditions in the guard are evaluated before notifying listeners. It is hosted on my forked repo: repo.

Is this something you could see implemented in the main repository? Or did I overlook some functionality?

IvoSchols avatar Jun 24 '22 12:06 IvoSchols

Looks reasonable. What I don't like is the untyped context it introduces Map<String, dynamic> and that a very clear command like stateC.enter()is silently ignored in some cases. I still think it would be easier to track state in typed fields and enforce the guards through a clean API that wraps the StateMachine?

renggli avatar Jun 24 '22 13:06 renggli

I have implemented the guard decorator by my best understanding in this branch. However I run into two problems:

  1. Although the wrapper is nice for further expansion, it makes the code more convoluted than necessary.
  2. Because states are initialized with the calling machine, the tests rightfully fail. Please allow me to explain:

The standard machine is wrapped with a guard decorator. When a new state is added to the guard decorator, the underlying standard machine is instructed to add a new state. It creates a new state, and returns it to the decorator for guard initialization.

However, the issue arises when the standard machine creates a new state. In the standard newState method, states are constructed with: this. Initializing state.machine = this, referring to the standard machine and not the guard decorator. States therefore do not lookup the guard decorator to set current but rather use the standard machine. This can be circumvented in two not so clean ways:

  1. making machine non-final in state and setting it after construction in the guard decorator (cleanest of the two, I think).
  2. passing machine in the newState method

Side-notes:

  • I have moved the guards from state to guard decorator, so that the state object does not host code that might not be used. This is achieved with: Map<State, Map<State, Guard>> guards = {};. Not as clean as I hoped it to be. Do you think that there is a better way?

  • I do not think I totally understand what you meant by: tracking states in typed fields.

  • Additionally I do not see how context can be made typed, without losing the possibility of supporting all data types. I think context should have support for all possible data types, so that guards can be constructed in anyway that is needed.

IvoSchols avatar Jul 02 '22 16:07 IvoSchols

I agree, it is unfortunate that the State cannot be customized/wrapped. What about changing newState, newStartState, newStopState to something like the following?

S newState<S extends State<T>>(T identifier, {S Function(Machine<T>, T)? constructor}) {

With tracking states in typed fields, I mean that I don't like to have Map<String, dynamic>. I would rather create classes with strongly typed fields.

renggli avatar Jul 02 '22 21:07 renggli

This breaks the invalid machine check, target.machine != this Which is used to validate states:

if (target != null && target.machine != this) { throw ArgumentError.value(state, 'state', 'Invalid machine'); }

Changed it to: target.machine is! MachineInterface<T>,. All tests pass except: 'set state by unknown state' in group machine.

So context would accept something like varType. Which would be extended by intType, boolType, stringType etc?

IvoSchols avatar Jul 02 '22 21:07 IvoSchols

Hi,

I have integrated the commit into my decorator branch of the fork. However a lot of tests are now failing and my debugger cannot step into the failing tests. So I cannot debug as to why they fail.

I understand that you want to keep the core functionality intact and make the code extensible at the same time, but this commit broke so much that I do not know how to continue.

I just needed the guard and context functionality for my studies, and have now sunken too much time into adapting it to the decorator pattern.

I do not think that a clean implementation of the decorator pattern is easily doable without refactoring. Because the machine is not a single object, but actually two interwoven (machine&states).

IvoSchols avatar Jul 03 '22 12:07 IvoSchols

What about something along ...

import 'package:statemachine/statemachine.dart';

class GuardedMachine<T> extends Machine<T> {
  @override
  GuardedState<T> createState(T identifier) =>
      GuardedState<T>(this, identifier);

  @override
  set current(Object? state) {
    final target = state is State<T>
        ? state
        : state is T
            ? this[state]
            : state == null
                ? null
                : throw ArgumentError.value(state, 'state', 'Invalid state');
    // Should probably provide a template method or event to validate the
    // transition before executing it.
    if (current != null &&
        (current as GuardedState<T>).canExit(target as GuardedState<T>)) {
      throw UnsupportedError('Unable to exit to $target');
    }
    if (target != null &&
        (target as GuardedState<T>).canEnter(current as GuardedState<T>)) {
      throw UnsupportedError('Unable to enter to $target');
    }
    super.current = target;
  }

  @override
  GuardedState<T> newState(T identifier) =>
      super.newState(identifier) as GuardedState<T>;
}

class GuardedState<T> extends State<T> {
  GuardedState(super.machine, super.identifier);

  bool canExit(GuardedState<T>? to) => true;

  bool canEnter(GuardedState<T>? from) => true;
}

The casts are a bit ugly, but not sure how to avoid them ...

renggli avatar Jul 03 '22 14:07 renggli

Implemented your suggestion in: fork branch

Tests cases all pass. However they are probably not exhaustive.

Two questions

  • Why are GuardedStates nullable in the canExit and canEnter method?

With tracking states in typed fields, I mean that I don't like to have Map<String, dynamic>. I would rather create classes with strongly typed fields.

  • Does this mean wrapping context in its own class with helper functions to restrict access?

IvoSchols avatar Jul 03 '22 21:07 IvoSchols