Skip to content

MULTITRANS #6

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.

Sign up for GitHub

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

Open
KBWilliamP wants to merge 2 commits into
base: master
Choose a base branch
from
Open

MULTITRANS #6

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4fe49d8
[MULTITRANS] add new type of transition to be able to do a transition…
KBWilliamP Dec 15, 2023
98b48b0
[MULTITRANS] didn't had the time to finish, just replace new def "mul…
KBWilliamP Feb 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 61 additions & 5 deletions lib/exfsm.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ defmodule ExFSM do
quote do
import ExFSM
@fsm %{}
@multitrans %{}
@bypasses %{}
@docs %{}
@to nil
Expand All @@ -68,6 +69,7 @@ defmodule ExFSM do
defmacro __before_compile__(_env) do
quote do
def fsm, do: @fsm
def event_multitrans, do: @multitrans
def event_bypasses, do: @bypasses
def docs, do: @docs
end
Expand Down Expand Up @@ -107,6 +109,23 @@ defmodule ExFSM do
def unquote(signature), do: unquote(body_block[:do])
end
end

@doc """
Define a function of type `multitransition` describing a transition and its
different states or not. The function name is the transition name, the mode (:excluded or :included) is the
first argument with a list of the states linked to this mode. A state object can be modified and is the second argument.
defmultitrans refund({:included, [:payed, :shipped, :partially_shipped], _}, order) do
{:next_state, :refunded, order}
end
"""
defmacro defmultitrans({event,_meta,[{_, _, [mode, states, _params]}| _rest]}=signature,body_block) do
quote do
@multitrans Map.put(@multitrans,{unquote(event), unquote(states), unquote(mode)},__MODULE__)
doc = Module.get_attribute(__MODULE__, :doc)
@docs Map.put(@docs,{:event_doc,unquote(event)},doc)
def unquote(signature), do: unquote(body_block[:do])
end
end

@Shakadak Shakadak Dec 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you don't handle the to attribute ? like with deftrans

@KBWilliamP KBWilliamP Dec 28, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just took the defbypass as model because at the beginning it was more similar to the defbypass.
There isn't any particular reason.

end

defmodule ExFSM.Machine do
Expand Down Expand Up @@ -179,6 +198,11 @@ defmodule ExFSM.Machine do
def event_bypasses(state), do:
event_bypasses(State.handlers(state))

def event_multitrans(handlers) when is_list(handlers), do:
(handlers |> Enum.map(&(&1.event_multitrans)) |> Enum.concat |> Enum.into(%{}))

@Shakadak Shakadak Dec 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be compressed into

Suggested change
(handlers |> Enum.map(&(&1.event_multitrans)) |> Enum.concat |> Enum.into(%{}))
Map.new(Enum.flat_map(handlers, &(&1.event_multitrans)))

@KBWilliamP KBWilliamP Dec 28, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do it for the def fsm and def event_bypasses as well

def event_multitrans(state), do:
event_multitrans(State.handlers(state))

@doc "find the ExFSM Module from the list `handlers` implementing the event `action` from `state_name`"
@spec find_handler({state_name::atom,event_name::atom},[exfsm_module :: atom]) :: exfsm_module :: atom
def find_handler({state_name,action},handlers) when is_list(handlers) do
Expand All @@ -187,6 +211,17 @@ defmodule ExFSM.Machine do
_ -> nil
end
end

def find_multitrans({state_name,action},handlers) when is_list(handlers) do
case Enum.find(event_multitrans(handlers), fn
{{this_action, states, :included}, _} -> this_action == action and state_name in states
{{this_action, states, :excluded}, _} -> this_action == action and state_name not in states
end) do
{{_, states, mode},handler}-> {states, handler, mode}
_ -> nil
end
end

@doc "same as `find_handler/2` but using a 'meta' state implementing `ExFSM.Machine.State`"
def find_handler({state,action}), do:
find_handler({State.state_name(state),action},State.handlers(state))
Expand All @@ -195,6 +230,10 @@ defmodule ExFSM.Machine do
event_bypasses(handlers_or_state)[action]
end

def find_multitrans({state, action}) do
find_multitrans({State.state_name(state),action},State.handlers(state))
end

def infos(handlers,_action) when is_list(handlers), do:
(handlers |> Enum.map(&(&1.docs)) |> Enum.concat |> Enum.into(%{}))
def infos(state,action), do:
Expand All @@ -214,10 +253,21 @@ defmodule ExFSM.Machine do
@spec event(ExFSM.Machine.State.t,{event_name :: atom, event_params :: any}) :: meta_event_reply
def event(state,{action,params}) do
case find_handler({state,action}) do
nil ->
case find_bypass(state,action) do
nil-> {:error,:illegal_action}
handler-> case apply(handler,action,[params,state]) do
nil ->
case find_multitrans({state, action}) do
nil ->
case find_bypass(state, action) do
nil -> {:error,:illegal_action}
handler ->
case apply(handler,action,[params,state]) do
{:keep_state,state}->{:next_state,state}
{:next_state,state_name,state,timeout} -> {:next_state,State.set_state_name(state,state_name),timeout}
{:next_state,state_name,state} -> {:next_state,State.set_state_name(state,state_name)}
other -> other
end
end
{states, handler, mode} ->
case apply(handler,action,[{mode,states,params},state]) do
{:keep_state,state}->{:next_state,state}
{:next_state,state_name,state,timeout} -> {:next_state,State.set_state_name(state,state_name),timeout}
{:next_state,state_name,state} -> {:next_state,State.set_state_name(state,state_name)}
Comment on lines +305 to 308

@KBWilliamP KBWilliamP Dec 28, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is here.
In the case we want to create a defmultitrans that just create a lot of object in @fsm how can we know in the find_multitrans when we have to apply on a defmultitrans or on a deftrans and how to find back the right states it had at the beginning.
We need to make the @fsm more complex.

@half-shell half-shell Jan 2, 2024 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal brought up beforehand should mean that we do not need a find_multitrans/1 anymore, because we can rely on the regular find_handler/1. Here's a quick shot at what I think the macro would look like:

defmacro defmultitrans(
             {event, _meta, [{states, _params} | _rest]} = signature,
             body_block
           ) do
    quote do
      @fsm Map.merge(
             @fsm,
             Enum.map(
               unquote(states),
               fn state ->
                 {
                   {state, unquote(event)},
                   {__MODULE__, unquote(Enum.uniq(find_nextstates(body_block[:do])))}
                 }
               end
             )
             |> Map.new()
           )
      [...]
      def unquote(signature), do: unquote(body_block[:do])
      @to nil
    end
  end

You're also more than welcome to add a new instance in the ExFSM's doctest to keep it updated. The Door module works somewhat well to express the multitrans idea:

defmodule Elixir.Door do
  use ExFSM

  defmultitrans open({[:closed, :opened], _}, s), do: {:next_state, :opened, s}
end

I might try to setup github actions to run a simple mix test

@half-shell half-shell Jan 2, 2024 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To address my earlier misunderstanding regarding the fact that we'd still need a find_multitrans/1 in order for ExFSM.Machine.event/2 to apply the arguments to the right function:
I'd argue we'd be better off by just straight up generating the functions that'd be creating if we were using deftrans instead of multitrans:

defmacro defmultitrans(
             {event, _meta, [{states, _params} | _rest]} = signature,
             body_block
           ) do
  quote do
     [...]
     unquote(
       for state <- states do
         quote do
           def unquote({state, meta, [{trans, params} | rest]}), do: unquote(body_block[:do])
         end
       end
     )
      @to nil
    end
  end

What do you think?

@Shakadak Shakadak Jan 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, what we want internally is a way to transpose transition <-> state in the signature. If we move the code generation from deftrans to a dedicated function, the code can be shared between deftrans and defmultitrans, then it just becomes a matter of converting the input from defmultitrans into what would look like the inputs for multiple deftrans
Originaly I wanted to reuse deftrans directly in a quote to reduce the amount of code changed, but from experience, handling the ast in a function for reuse purpose has always been better than using quote and the macro.

But then the @to can't be accessed from inside the macro even via Module.get_attribute/3 because the expansion time of a macro happen before the evaluation time of the module's ast
So the logic to clean up the @to attribute needs to be separate from the logic to generate the transition ast

In the end I came up with:

  defmacro deftrans(signature, do: body) do
    transition_ast(signature, body)
    |> cleanup_to()
  end

  defmacro defmultitrans(signature, do: body) do
    Enum.map(transpose(signature), &transition_ast(&1, body))
    |> cleanup_to()
  end

where transpose, transition_ast, and cleanup_to are:

  @doc false
  # states to transitions
  def transpose(signature) do
    {transition, meta, params} = signature
    [{states, input}, object] = params
    Enum.map(states, fn state -> 
      {state, meta, [{transition, input}, object]}
    end)
  end

  @doc false
  # Define the function for the transition, as well as it's metadatas
  def transition_ast(signature, body) do
    {state, _meta, params} = signature
    [{transition, _param}, _object] = params
    quote do
      @fsm Map.put(@fsm, {unquote(state), unquote(transition)}, {__MODULE__, @to || unquote(Enum.uniq(find_nextstates(body)))})
      doc = Module.get_attribute(__MODULE__, :doc)
      # I don't really understand why we don't directly use @doc
      @docs Map.put(@docs, {:transition_doc, unquote(state), unquote(transition)}, doc)
      def unquote(signature), do: unquote(body)
    end 
  end

  @doc false
  # Remove the @to for the next transition definitions
  def cleanup_to(ast) do
    quote do
      unquote(ast)
      @to nil
    end
  end

I really think we should wait another time to do the difference operation on the set of transitions. As stated, it is a source of implicit behaviour dependent on the global definition of the fsm (which may be more than the current module), which is really easy to forget when changing stuff.

@KBWilliamP KBWilliamP Jan 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like your implementation that you describe here, I tried it but we have a problem here:

warning: clauses with the same name and arity (number of arguments) should be grouped together, "def to_be_orchestrated/2" was previously defined (lib/fsm/orders.ex:149)
  lib/fsm/orders.ex:241

warning: clauses with the same name and arity (number of arguments) should be grouped together, "def partially_shipped/2" was previously defined (lib/fsm/orders.ex:230)
  lib/fsm/orders.ex:241

warning: clauses with the same name and arity (number of arguments) should be grouped together, "def no_automatic_orchestration/2" was previously defined (lib/fsm/orders.ex:186)
  lib/fsm/orders.ex:241

warning: clauses with the same name and arity (number of arguments) should be grouped together, "def to_be_orchestrated/2" was previously defined (lib/fsm/orders.ex:149)
  lib/fsm/orders.ex:245

warning: clauses with the same name and arity (number of arguments) should be grouped together, "def partially_shipped/2" was previously defined (lib/fsm/orders.ex:230)
  lib/fsm/orders.ex:245

warning: clauses with the same name and arity (number of arguments) should be grouped together, "def no_automatic_orchestration/2" was previously defined (lib/fsm/orders.ex:186)
  lib/fsm/orders.ex:245

the definition of the new deftrans has they are already existing is a problem.
Right now I don't have any solution...

@Julien29121998 Julien29121998 Jan 12, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KBWilliamP actually, this is only a matter of warning:
You get this error because the clauses defined by deftrans and defmultitrans that are originating from the same state are not grouped together.
However, it still compiles properly.
Here, you have two choices, i guess:

  • Finding a way to desactivate the warning with something like Code.compiler_option during the compilation of the module
  • Storing all the AST of your transitions in a variable and then sorting it (based on the origin state) so that the clauses are grouped together, before unquoting it as functions.

Additional remarks:

  • The defmultitrans declaration is inconsistant: when we use deftrans, we write

deftrans [original state]({:[action], [params]} [object])

whereas when using defmultitrans, you put:

defmultitrans [action] ...

Starting with the action, which can be misleading, as junior developper already tend to get mixed up on the syntax.
I'll suggest that you find a different way to declare multitransition, as you cant start with the states (obviously) but can't start with the action either (not advised).

Finally I would suggest keeping the original guards in the deftrans macro:

defmacro deftrans({state,_meta,[{trans,_param}|_rest]}=signature, body_block) do

@half-shell half-shell Jan 18, 2024 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay @KBWilliamP

After talking a bit with @awetzel , it turns out that there might be a major API change some time this year for ExFSM.

Taking that into account, the changes we were willing to make for the defmultitrans feature to get implemented as we'd wish probably won't be worth it on a short term timeline.

That means that we agreed that in order to get this QOL feature out and usable quickly enough, the easiest things to do is just to mute the compilation warning, using the quote option generated: true (c.f. https://hexdocs.pm/elixir/Kernel.SpecialForms.html#quote/2-options)

Expand All @@ -239,7 +289,13 @@ defmodule ExFSM.Machine do
|> Enum.filter(fn {{from,_},_}->from==State.state_name(state) end)
|> Enum.map(fn {{_,action},_}->action end)
bypasses_actions = ExFSM.Machine.event_bypasses(state) |> Map.keys
Enum.uniq(fsm_actions ++ bypasses_actions)
multitrans_actions = ExFSM.Machine.event_multitrans(state)
|> Enum.filter(fn
{{_,from,:included},_}->State.state_name(state) in from
{{_,from,:excluded},_}->State.state_name(state) not in from
end)
|> Enum.map(fn {{action,_,_},_}->action end)
Enum.uniq(fsm_actions ++ bypasses_actions ++ multitrans_actions)
end

@spec action_available?(ExFSM.Machine.State.t,action_name :: atom) :: boolean
Expand Down