Hello everyone,
I know FSM is deprecated and not encouraged to use on new projects. But believe it or not some people are still using it.
While recently trying to update to scala 2.13 we realized concat on partial functions has changed behaviour. Basically if you andThen two partial functions, and you use isDefinedAt over the chain, you execute the first partial function twice. Here is a really simple example showing this behaviour:
scala> val pf1:PartialFunction[Int, String] = { case 3 => println("here in pf1"); "3"}
val pf1: PartialFunction[Int,String] = <function1>
scala> val pf2:PartialFunction[String, Unit] = { case "3" => println("here in pf2")}
val pf2: PartialFunction[String,Unit] = <function1>
scala> val chain = pf1 andThen pf2
val chain: PartialFunction[Int,Unit] = <function1>
scala> if(chain.isDefinedAt(3)) chain(3)
here in pf1
here in pf1
here in pf2
Please check this pr for more details:
scala/scala#7263
This breaks FSM's transform...using pattern.
final class TransformHelper(func: StateFunction) {
def using(andThen: PartialFunction[State, State]): StateFunction =
func.andThen(andThen.orElse { case x => x })
}
final def transform(func: StateFunction): TransformHelper = new TransformHelper(func)
You will see here it uses andThen on partial function. And if we check the implementation you will see there is actually a warning there:
/**
* Composes this partial function with another partial function that
* gets applied to results of this partial function.
*
* Note that calling [[isDefinedAt]] on the resulting partial function may apply the first
* partial function and execute its side effect. It is highly recommended to call [[applyOrElse]]
* instead of [[isDefinedAt]] / [[apply]] for efficiency.
*
* @param k the transformation function
* @tparam C the result type of the transformation function.
* @return a partial function with the domain of this partial function narrowed by
* other partial function, which maps arguments `x` to `k(this(x))`.
*/
def andThen[C](k: PartialFunction[B, C]): PartialFunction[A, C] =
new Combined[A, B, C](this, k)
But FSM uses isDefinedAt on the resulting PartialFunction. For Example:
private[akka] def processEvent(event: Event, @unused source: AnyRef): Unit = {
val stateFunc = stateFunctions(currentState.stateName)
val nextState = if (stateFunc.isDefinedAt(event)) {
stateFunc(event)
} else {
// handleEventDefault ensures that this is always defined
handleEvent(event)
}
applyState(nextState)
}
One solution comes to mind is to use applyOrElse as the documentation dictates. Something like:
private[akka] def processEvent(event: Event, @unused source: AnyRef): Unit = {
val stateFunc = stateFunctions(currentState.stateName)
val nextState = istateFunc.applyOrElse(event, handleEvent)
}
applyState(nextState)
}
But I am not sure if that's enough.
We are planning to move away from FSM as well, but in the meantime, it would be great to have a fix for this.
Please let me know your ideas. If you think this solution is valid and enough, I can open a pull request for it.
Thanks!
P.S. We are using 2.6.14 but I couldn't see any mentions of this in later releases.
I also couldn't find any mentions of this on other issues. Please let me know if this is a duplicate.
Hello everyone,
I know FSM is deprecated and not encouraged to use on new projects. But believe it or not some people are still using it.
While recently trying to update to scala
2.13we realized concat on partial functions has changed behaviour. Basically if youandThentwo partial functions, and you useisDefinedAtover the chain, you execute the first partial function twice. Here is a really simple example showing this behaviour:Please check this pr for more details:
scala/scala#7263
This breaks
FSM'stransform...usingpattern.You will see here it uses
andThenon partial function. And if we check the implementation you will see there is actually a warning there:But
FSMusesisDefinedAton the resulting PartialFunction. For Example:One solution comes to mind is to use
applyOrElseas the documentation dictates. Something like:But I am not sure if that's enough.
We are planning to move away from FSM as well, but in the meantime, it would be great to have a fix for this.
Please let me know your ideas. If you think this solution is valid and enough, I can open a pull request for it.
Thanks!
P.S. We are using 2.6.14 but I couldn't see any mentions of this in later releases.
I also couldn't find any mentions of this on other issues. Please let me know if this is a duplicate.